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

diff --git a/app/Config/auth.php b/app/Config/auth.php
index 66793308b6df4091ed5de58fbbc22dfa682c9f54..51b152ff184805e67e5addd1573a6e2258c51e9d 100644 (file)
--- a/app/Config/auth.php
+++ b/app/Config/auth.php
@@ -11,14 +11,14 @@
 return [
 
     // Method of authentication to use
 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' => [
     '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',
     ],
 
         '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' => [
     // 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',
         ],
             'driver' => 'session',
             'provider' => 'users',
         ],

diff --git a/app/Http/Controllers/Auth/ForgotPasswordController.php b/app/Http/Controllers/Auth/ForgotPasswordController.php
index a3c0433a56f9e4af9d574ae0446edac02dbd5c1f..a60fcc1c9a6142d450d318375a930e0fce294994 100644 (file)
--- a/app/Http/Controllers/Auth/ForgotPasswordController.php
+++ b/app/Http/Controllers/Auth/ForgotPasswordController.php
@@ -30,6 +30,7 @@ class ForgotPasswordController extends Controller
     public function __construct()
     {
         $this->middleware('guest');
     public function __construct()
     {
         $this->middleware('guest');

+        $this->middleware('guard:standard');

         parent::__construct();
     }
 
         parent::__construct();
     }
 

diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php
index 8116288ada445149f15a56d11f2f1c07a3cc0a3b..4292ad6dd97a4bd75a18599f14e11fba025a1e4e 100644 (file)
--- a/app/Http/Controllers/Auth/LoginController.php
+++ b/app/Http/Controllers/Auth/LoginController.php
@@ -38,7 +38,9 @@ class LoginController extends Controller
      */
     public function __construct(SocialAuthService $socialAuthService)
     {
      */
     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');
         $this->socialAuthService = $socialAuthService;
         $this->redirectPath = url('/');
         $this->redirectAfterLogout = url('/login');


@@ -159,14 +161,4 @@ class LoginController extends Controller
         return redirect('/login');
     }
 
         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('/');
-    }

 }
 }

diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php
index 8fb13d536c63901a3901ec78b8795f7a00996d7f..56ec66bfff3a65948ada1101cb4d161780fd56a7 100644 (file)
--- a/app/Http/Controllers/Auth/RegisterController.php
+++ b/app/Http/Controllers/Auth/RegisterController.php
@@ -43,7 +43,8 @@ class RegisterController extends Controller
      */
     public function __construct(SocialAuthService $socialAuthService, RegistrationService $registrationService)
     {
      */
     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;
 
         $this->socialAuthService = $socialAuthService;
         $this->registrationService = $registrationService;

diff --git a/app/Http/Controllers/Auth/ResetPasswordController.php b/app/Http/Controllers/Auth/ResetPasswordController.php
index 4d98eca597c3fffcece65fc79209bae646a49ab9..214647cd5746a0eff3798a1a9deb1d8803183787 100644 (file)
--- a/app/Http/Controllers/Auth/ResetPasswordController.php
+++ b/app/Http/Controllers/Auth/ResetPasswordController.php
@@ -31,6 +31,7 @@ class ResetPasswordController extends Controller
     public function __construct()
     {
         $this->middleware('guest');
     public function __construct()
     {
         $this->middleware('guest');

+        $this->middleware('guard:standard');

         parent::__construct();
     }
 
         parent::__construct();
     }
 

diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
index 72cf0e01970d023b06c05bc8cae07c7e20deb131..8c0cb21d2821f6377dacd07b2ff20c2625c2a450 100644 (file)
--- a/app/Http/Controllers/Auth/Saml2Controller.php
+++ b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -17,16 +17,7 @@ class Saml2Controller extends Controller
     {
         parent::__construct();
         $this->samlService = $samlService;
     {
         parent::__construct();
         $this->samlService = $samlService;

-
-        // SAML2 access middleware
-        $this->middleware(function ($request, $next) {
-
-            if (config('auth.method') !== 'saml2') {
-                $this->showPermissionError();
-            }
-
-            return $next($request);
-        });
+        $this->middleware('guard:saml2');

     }
 
     /**
     }
 
     /**

diff --git a/app/Http/Controllers/Auth/UserInviteController.php b/app/Http/Controllers/Auth/UserInviteController.php
index c361b0a9b43687101e577db4f3e46f55b86ba397..c61b1c42b688e58b8c6defd8c007f8db7a099009 100644 (file)
--- a/app/Http/Controllers/Auth/UserInviteController.php
+++ b/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 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\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Illuminate\Routing\Redirector;

-use Illuminate\View\View;

 
 class UserInviteController extends Controller
 {
 
 class UserInviteController extends Controller
 {


@@ -21,22 +19,20 @@ class UserInviteController extends Controller
 
     /**
      * Create a new controller instance.
 
     /**
      * Create a new controller instance.

-     *
-     * @param UserInviteService $inviteService
-     * @param UserRepo $userRepo

      */
     public function __construct(UserInviteService $inviteService, UserRepo $userRepo)
     {
      */
     public function __construct(UserInviteService $inviteService, UserRepo $userRepo)
     {

+        $this->middleware('guest');
+        $this->middleware('guard:standard');
+

         $this->inviteService = $inviteService;
         $this->userRepo = $userRepo;
         $this->inviteService = $inviteService;
         $this->userRepo = $userRepo;

-        $this->middleware('guest');
+

         parent::__construct();
     }
 
     /**
      * Show the page for the user to set the password for their account.
         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)
      * @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.
 
     /**
      * 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)
      * @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.
 
     /**
      * Check and validate the exception thrown when checking an invite token.

-     * @param Exception $exception

      * @return RedirectResponse|Redirector
      * @throws Exception
      */
      * @return RedirectResponse|Redirector
      * @throws Exception
      */

diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php
index c2016281a2908415a00f2b7e5e5a624903889b3e..4517deb903213aa06c8a9c95e7ca0cabfda6cf8c 100644 (file)
--- a/app/Http/Kernel.php
+++ b/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,
         'auth'       => \BookStack\Http\Middleware\Authenticate::class,
         'can'        => \Illuminate\Auth\Middleware\Authorize::class,
         'guest'      => \BookStack\Http\Middleware\RedirectIfAuthenticated::class,

-        'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class,
-        'perm'       => \BookStack\Http\Middleware\PermissionMiddleware::class
+        'throttle'   => \Illuminate\Routing\Middleware\ThrottleRequests::class,
+        'perm'       => \BookStack\Http\Middleware\PermissionMiddleware::class,
+        'guard'      => \BookStack\Http\Middleware\CheckGuard::class,

     ];
 }
     ];
 }

diff --git a/app/Http/Middleware/CheckGuard.php b/app/Http/Middleware/CheckGuard.php
new file mode 100644 (file)
index 0000000..cc73ea6
--- /dev/null
+++ b/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);
+    }
+}

diff --git a/app/Http/Middleware/PermissionMiddleware.php b/app/Http/Middleware/PermissionMiddleware.php
index 28ffff6438cbb3979aa692d985d24542e5453081..d0bb4f79ef852e9e4b19fdcbd149159a6a05adb4 100644 (file)
--- a/app/Http/Middleware/PermissionMiddleware.php
+++ b/app/Http/Middleware/PermissionMiddleware.php
@@ -19,7 +19,7 @@ class PermissionMiddleware
     {
 
         if (!$request->user() || !$request->user()->can($permission)) {
     {
 
         if (!$request->user() || !$request->user()->can($permission)) {

-            Session::flash('error', trans('errors.permission'));
+            session()->flash('error', trans('errors.permission'));

             return redirect()->back();
         }
 
             return redirect()->back();
         }
 

diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php
index b36fdda7a01dcce479db2bdc4b5f7d2a1a9b69a1..8c9675f095b6535f900176ed7bdc3b13f9d506b8 100755 (executable)
--- a/resources/lang/en/settings.php
+++ b/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_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',
     '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',

diff --git a/routes/web.php b/routes/web.php
index 3aa95dd6868d441e4b57ae53affc16847dd5a921..90261e1ac291c128b7df9442f230d7e004372063 100644 (file)
--- a/routes/web.php
+++ b/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');
 // 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
 Route::get('/register/service/{socialDriver}', 'Auth\SocialController@socialRegister');
 
 // Login/Logout routes

diff --git a/tests/Api/ApiAuthTest.php b/tests/Api/ApiAuthTest.php
index 2ab81814bcde21b1453ac8ec0a7692a7ded8d892..1f283753ae9dc02f83d2dddd0c4107d4aa4b7604 100644 (file)
--- a/tests/Api/ApiAuthTest.php
+++ b/tests/Api/ApiAuthTest.php
@@ -20,7 +20,7 @@ class ApiAuthTest extends TestCase
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(401);
 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(401);
 

-        $this->actingAs($viewer, 'web');
+        $this->actingAs($viewer, 'standard');

 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(200);
 
         $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();
     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);
 
         $resp = $this->get($this->endpoint);
         $resp->assertStatus(200);

-        auth('web')->logout();
+        auth('standard')->logout();

 
         $accessApiPermission = RolePermission::getByName('access-api');
         $editorRole = $this->getEditor()->roles()->first();
 
         $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();
 
 
         $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));
         $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));

diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index 8bf9475cc39106c99861b9751601102cd8589389..92ff4a829bb40e8ea3c8e65c28cf6d1d5637775b 100644 (file)
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -478,4 +478,37 @@ class LdapTest extends BrowserKitTest
     {
         $this->checkLdapReceivesCorrectDetails('ldap.bookstack.com', 'ldap.bookstack.com', 389);
     }
     {
         $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);
+    }

 }
 }

diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index 2cecad8bf087f839375b487b8670e70d3afe6578..b3e6bd41b94aae6a95d31447b7e03c20f86bd161 100644 (file)
--- a/tests/Auth/Saml2Test.php
+++ b/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);
         $getRoutes = ['/logout', '/metadata', '/sls'];
         foreach ($getRoutes as $route) {
             $req = $this->get('/saml2' . $route);

-            $req->assertRedirect('/');
-            $error = session()->get('error');
-            $this->assertStringStartsWith('You do not have permission to access', $error);
-            session()->flush();
+            $this->assertPermissionError($req);

         }
 
         $postRoutes = ['/login', '/acs'];
         foreach ($postRoutes as $route) {
             $req = $this->post('/saml2' . $route);
         }
 
         $postRoutes = ['/login', '/acs'];
         foreach ($postRoutes as $route) {
             $req = $this->post('/saml2' . $route);

-            $req->assertRedirect('/');
-            $error = session()->get('error');
-            $this->assertStringStartsWith('You do not have permission to access', $error);
-            session()->flush();
+            $this->assertPermissionError($req);

         }
     }
 
         }
     }
 

+    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);
     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);

diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php
index 3433f3b832f86d61411729ea23983905596d57d0..f7b7d5edf3ef23e4b3d59a0189f4707faa00f1dd 100644 (file)
--- a/tests/SharedTestHelpers.php
+++ b/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}");
     }
 
         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);
+    }
+

 }
\ No newline at end of file
 }
\ No newline at end of file

diff --git a/tests/TestCase.php b/tests/TestCase.php
index f20b20fd83146cc9d84fb0bb9facbce126a548c0..1f1d5ece7288e88575b49975848812bac5915173 100644 (file)
--- a/tests/TestCase.php
+++ b/tests/TestCase.php
@@ -16,19 +16,6 @@ abstract class TestCase extends BaseTestCase
      */
     protected $baseUrl = 'http://localhost';
 
      */
     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
     /**
      * Assert the session contains a specific entry.
      * @param string $key