Simplified guard names and rolled out guard route checks

- Included tests to cover for LDAP and SAML
- Updated wording for external auth id option.
- Updated 'assertPermissionError' test case to be usable in BrowserKitTests

app/Config/auth.php

@@ -11,14 +11,14 @@
 return [
 
     // Method of authentication to use
-    // Options: standard, ldap
+    // Options: standard, ldap, saml2
     'method' => env('AUTH_METHOD', 'standard'),
 
     // Authentication Defaults
     // This option controls the default authentication "guard" and password
     // reset options for your application.
     'defaults' => [
-        'guard' => env('AUTH_METHOD', 'standard') === 'standard' ? 'web' : env('AUTH_METHOD'),
+        'guard' => env('AUTH_METHOD', 'standard'),
         'passwords' => 'users',
     ],
 
@@ -28,7 +28,7 @@ return [
     // mechanisms used by this application to persist your user's data.
     // Supported drivers: "session", "api-token", "ldap-session"
     'guards' => [
-        'web' => [
+        'standard' => [
             'driver' => 'session',
             'provider' => 'users',
         ],

app/Http/Controllers/Auth/ForgotPasswordController.php

@@ -30,6 +30,7 @@ class ForgotPasswordController extends Controller
     public function __construct()
     {
         $this->middleware('guest');
+        $this->middleware('guard:standard');
         parent::__construct();
     }
 

app/Http/Controllers/Auth/LoginController.php

@@ -38,7 +38,9 @@ class LoginController extends Controller
      */
     public function __construct(SocialAuthService $socialAuthService)
     {
-        $this->middleware('guest', ['only' => ['getLogin', 'postLogin']]);
+        $this->middleware('guest', ['only' => ['getLogin', 'login']]);
+        $this->middleware('guard:standard,ldap', ['only' => ['login', 'logout']]);
+
         $this->socialAuthService = $socialAuthService;
         $this->redirectPath = url('/');
         $this->redirectAfterLogout = url('/login');
@@ -159,14 +161,4 @@ class LoginController extends Controller
         return redirect('/login');
     }
 
-    /**
-     * Log the user out of the application.
-     */
-    public function logout(Request $request)
-    {
-        $this->guard()->logout();
-        $request->session()->invalidate();
-
-        return $this->loggedOut($request) ?: redirect('/');
-    }
 }

app/Http/Controllers/Auth/RegisterController.php

@@ -43,7 +43,8 @@ class RegisterController extends Controller
      */
     public function __construct(SocialAuthService $socialAuthService, RegistrationService $registrationService)
     {
-        $this->middleware('guest')->only(['getRegister', 'postRegister']);
+        $this->middleware('guest');
+        $this->middleware('guard:standard');
 
         $this->socialAuthService = $socialAuthService;
         $this->registrationService = $registrationService;

app/Http/Controllers/Auth/ResetPasswordController.php

@@ -31,6 +31,7 @@ class ResetPasswordController extends Controller
     public function __construct()
     {
         $this->middleware('guest');
+        $this->middleware('guard:standard');
         parent::__construct();
     }
 

app/Http/Controllers/Auth/Saml2Controller.php

@@ -17,16 +17,7 @@ class Saml2Controller extends Controller
     {
         parent::__construct();
         $this->samlService = $samlService;
-
+        $this->middleware('guard:saml2');
-        // SAML2 access middleware
-        $this->middleware(function ($request, $next) {
-
-            if (config('auth.method') !== 'saml2') {
-                $this->showPermissionError();
-            }
-
-            return $next($request);
-        });
     }
 
     /**

app/Http/Controllers/Auth/UserInviteController.php

@@ -8,11 +8,9 @@ use BookStack\Exceptions\UserTokenExpiredException;
 use BookStack\Exceptions\UserTokenNotFoundException;
 use BookStack\Http\Controllers\Controller;
 use Exception;
-use Illuminate\Contracts\View\Factory;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Routing\Redirector;
-use Illuminate\View\View;
 
 class UserInviteController extends Controller
 {
@@ -21,22 +19,20 @@ class UserInviteController extends Controller
 
     /**
      * Create a new controller instance.
-     *
-     * @param UserInviteService $inviteService
-     * @param UserRepo $userRepo
      */
     public function __construct(UserInviteService $inviteService, UserRepo $userRepo)
     {
+        $this->middleware('guest');
+        $this->middleware('guard:standard');
+
         $this->inviteService = $inviteService;
         $this->userRepo = $userRepo;
-        $this->middleware('guest');
+
         parent::__construct();
     }
 
     /**
      * Show the page for the user to set the password for their account.
-     * @param string $token
-     * @return Factory|View|RedirectResponse
      * @throws Exception
      */
     public function showSetPassword(string $token)
@@ -54,9 +50,6 @@ class UserInviteController extends Controller
 
     /**
      * Sets the password for an invited user and then grants them access.
-     * @param Request $request
-     * @param string $token
-     * @return RedirectResponse|Redirector
      * @throws Exception
      */
     public function setPassword(Request $request, string $token)
@@ -85,7 +78,6 @@ class UserInviteController extends Controller
 
     /**
      * Check and validate the exception thrown when checking an invite token.
-     * @param Exception $exception
      * @return RedirectResponse|Redirector
      * @throws Exception
      */

app/Http/Kernel.php

@@ -48,7 +48,8 @@ class Kernel extends HttpKernel
         'auth'       => \BookStack\Http\Middleware\Authenticate::class,
         'can'        => \Illuminate\Auth\Middleware\Authorize::class,
         'guest'      => \BookStack\Http\Middleware\RedirectIfAuthenticated::class,
-        'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class,
+        'throttle'   => \Illuminate\Routing\Middleware\ThrottleRequests::class,
-        'perm'       => \BookStack\Http\Middleware\PermissionMiddleware::class
+        'perm'       => \BookStack\Http\Middleware\PermissionMiddleware::class,
+        'guard'      => \BookStack\Http\Middleware\CheckGuard::class,
     ];
 }

app/Http/Middleware/CheckGuard.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace BookStack\Http\Middleware;
+
+use Closure;
+
+class CheckGuard
+{
+    /**
+     * Handle an incoming request.
+     *
+     * @param  \Illuminate\Http\Request  $request
+     * @param  \Closure  $next
+     * @param string $allowedGuards
+     * @return mixed
+     */
+    public function handle($request, Closure $next, ...$allowedGuards)
+    {
+        $activeGuard = config('auth.method');
+        if (!in_array($activeGuard, $allowedGuards)) {
+            session()->flash('error', trans('errors.permission'));
+            return redirect('/');
+        }
+
+        return $next($request);
+    }
+}

app/Http/Middleware/PermissionMiddleware.php

@@ -19,7 +19,7 @@ class PermissionMiddleware
     {
 
         if (!$request->user() || !$request->user()->can($permission)) {
-            Session::flash('error', trans('errors.permission'));
+            session()->flash('error', trans('errors.permission'));
             return redirect()->back();
         }
 

resources/lang/en/settings.php

@@ -131,7 +131,7 @@ return [
     'users_send_invite_text' => 'You can choose to send this user an invitation email which allows them to set their own password otherwise you can set their password yourself.',
     'users_send_invite_option' => 'Send user invite email',
     'users_external_auth_id' => 'External Authentication ID',
-    'users_external_auth_id_desc' => 'This is the ID used to match this user when communicating with your LDAP system.',
+    'users_external_auth_id_desc' => 'This is the ID used to match this user when communicating with your external authentication system.',
     'users_password_warning' => 'Only fill the below if you would like to change your password.',
     'users_system_public' => 'This user represents any guest users that visit your instance. It cannot be used to log in but is assigned automatically.',
     'users_delete' => 'Delete User',

routes/web.php

@@ -210,7 +210,9 @@ Route::group(['middleware' => 'auth'], function () {
 // Social auth routes
 Route::get('/login/service/{socialDriver}', 'Auth\SocialController@getSocialLogin');
 Route::get('/login/service/{socialDriver}/callback', 'Auth\SocialController@socialCallback');
-Route::get('/login/service/{socialDriver}/detach', 'Auth\SocialController@detachSocialAccount');
+Route::group(['middleware' => 'auth'], function () {
+    Route::get('/login/service/{socialDriver}/detach', 'Auth\SocialController@detachSocialAccount');
+});
 Route::get('/register/service/{socialDriver}', 'Auth\SocialController@socialRegister');
 
 // Login/Logout routes

tests/Api/ApiAuthTest.php

@@ -20,7 +20,7 @@ class ApiAuthTest extends TestCase
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(401);
 
-        $this->actingAs($viewer, 'web');
+        $this->actingAs($viewer, 'standard');
 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(200);
@@ -72,11 +72,11 @@ class ApiAuthTest extends TestCase
     public function test_api_access_permission_required_to_access_api_with_session_auth()
     {
         $editor = $this->getEditor();
-        $this->actingAs($editor, 'web');
+        $this->actingAs($editor, 'standard');
 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(200);
-        auth('web')->logout();
+        auth('standard')->logout();
 
         $accessApiPermission = RolePermission::getByName('access-api');
         $editorRole = $this->getEditor()->roles()->first();
@@ -84,7 +84,7 @@ class ApiAuthTest extends TestCase
 
         $editor = User::query()->where('id', '=', $editor->id)->first();
 
-        $this->actingAs($editor, 'web');
+        $this->actingAs($editor, 'standard');
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(403);
         $resp->assertJson($this->errorResponse("The owner of the used API token does not have permission to make API calls", 403));

tests/Auth/LdapTest.php

@@ -478,4 +478,37 @@ class LdapTest extends BrowserKitTest
     {
         $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com', 'ldap.bookstack.com', 389);
     }
+
+    public function test_forgot_password_routes_inaccessible()
+    {
+        $resp = $this->get('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->get('/password/reset/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/reset');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_invite_routes_inaccessible()
+    {
+        $resp = $this->get('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_register_routes_inaccessible()
+    {
+        $resp = $this->get('/register');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register');
+        $this->assertPermissionError($resp);
+    }
 }

tests/Auth/Saml2Test.php

@@ -219,22 +219,58 @@ class Saml2Test extends TestCase
         $getRoutes = ['/logout', '/metadata', '/sls'];
         foreach ($getRoutes as $route) {
             $req = $this->get('/saml2' . $route);
-            $req->assertRedirect('/');
+            $this->assertPermissionError($req);
-            $error = session()->get('error');
-            $this->assertStringStartsWith('You do not have permission to access', $error);
-            session()->flush();
         }
 
         $postRoutes = ['/login', '/acs'];
         foreach ($postRoutes as $route) {
             $req = $this->post('/saml2' . $route);
-            $req->assertRedirect('/');
+            $this->assertPermissionError($req);
-            $error = session()->get('error');
-            $this->assertStringStartsWith('You do not have permission to access', $error);
-            session()->flush();
         }
     }
 
+    public function test_forgot_password_routes_inaccessible()
+    {
+        $resp = $this->get('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/email');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->get('/password/reset/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/password/reset');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_standard_login_routes_inaccessible()
+    {
+        $resp = $this->post('/login');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->get('/logout');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_invite_routes_inaccessible()
+    {
+        $resp = $this->get('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register/invite/abc123');
+        $this->assertPermissionError($resp);
+    }
+
+    public function test_user_register_routes_inaccessible()
+    {
+        $resp = $this->get('/register');
+        $this->assertPermissionError($resp);
+
+        $resp = $this->post('/register');
+        $this->assertPermissionError($resp);
+    }
+
     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);

tests/SharedTestHelpers.php

@@ -262,4 +262,19 @@ trait SharedTestHelpers
         self::assertThat($passed, self::isTrue(), "Failed asserting that given map:\n\n{$toCheckStr}\n\nincludes:\n\n{$toIncludeStr}");
     }
 
+    /**
+     * Assert a permission error has occurred.
+     */
+    protected function assertPermissionError($response)
+    {
+        if ($response instanceof BrowserKitTest) {
+            $response = \Illuminate\Foundation\Testing\TestResponse::fromBaseResponse($response->response);
+        }
+
+        $response->assertRedirect('/');
+        $this->assertSessionHas('error');
+        $error = session()->pull('error');
+        $this->assertStringStartsWith('You do not have permission to access', $error);
+    }
+
 }

tests/TestCase.php

@@ -16,19 +16,6 @@ abstract class TestCase extends BaseTestCase
      */
     protected $baseUrl = 'http://localhost';
 
-    /**
-     * Assert a permission error has occurred.
-     * @param TestResponse $response
-     * @return TestCase
-     */
-    protected function assertPermissionError(TestResponse $response)
-    {
-        $response->assertRedirect('/');
-        $this->assertSessionHas('error');
-        session()->remove('error');
-        return $this;
-    }
-
     /**
      * Assert the session contains a specific entry.
      * @param string $key