From e6c6de0848caab863f410bbf9a74312392db9dcd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 2 Feb 2020 13:10:21 +0000 Subject: [PATCH] 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 | 6 +-- .../Auth/ForgotPasswordController.php | 1 + app/Http/Controllers/Auth/LoginController.php | 14 ++--- .../Controllers/Auth/RegisterController.php | 3 +- .../Auth/ResetPasswordController.php | 1 + app/Http/Controllers/Auth/Saml2Controller.php | 11 +--- .../Controllers/Auth/UserInviteController.php | 16 ++---- app/Http/Kernel.php | 5 +- app/Http/Middleware/CheckGuard.php | 27 ++++++++++ app/Http/Middleware/PermissionMiddleware.php | 2 +- resources/lang/en/settings.php | 2 +- routes/web.php | 4 +- tests/Api/ApiAuthTest.php | 8 +-- tests/Auth/LdapTest.php | 33 ++++++++++++ tests/Auth/Saml2Test.php | 52 ++++++++++++++++--- tests/SharedTestHelpers.php | 15 ++++++ tests/TestCase.php | 13 ----- 17 files changed, 146 insertions(+), 67 deletions(-) create mode 100644 app/Http/Middleware/CheckGuard.php diff --git a/app/Config/auth.php b/app/Config/auth.php index 66793308b..51b152ff1 100644 --- a/app/Config/auth.php +++ b/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', ], diff --git a/app/Http/Controllers/Auth/ForgotPasswordController.php b/app/Http/Controllers/Auth/ForgotPasswordController.php index a3c0433a5..a60fcc1c9 100644 --- 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'); + $this->middleware('guard:standard'); parent::__construct(); } diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 8116288ad..4292ad6dd 100644 --- 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) { - $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('/'); - } } diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php index 8fb13d536..56ec66bff 100644 --- 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) { - $this->middleware('guest')->only(['getRegister', 'postRegister']); + $this->middleware('guest'); + $this->middleware('guard:standard'); $this->socialAuthService = $socialAuthService; $this->registrationService = $registrationService; diff --git a/app/Http/Controllers/Auth/ResetPasswordController.php b/app/Http/Controllers/Auth/ResetPasswordController.php index 4d98eca59..214647cd5 100644 --- 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'); + $this->middleware('guard:standard'); parent::__construct(); } diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php index 72cf0e019..8c0cb21d2 100644 --- 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; - - // 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 c361b0a9b..c61b1c42b 100644 --- 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 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 */ diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index c2016281a..4517deb90 100644 --- 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, - '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 index 000000000..cc73ea68d --- /dev/null +++ b/app/Http/Middleware/CheckGuard.php @@ -0,0 +1,27 @@ +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 28ffff643..d0bb4f79e 100644 --- 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)) { - Session::flash('error', trans('errors.permission')); + session()->flash('error', trans('errors.permission')); return redirect()->back(); } diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index b36fdda7a..8c9675f09 100755 --- 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_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', diff --git a/routes/web.php b/routes/web.php index 3aa95dd68..90261e1ac 100644 --- 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'); -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 diff --git a/tests/Api/ApiAuthTest.php b/tests/Api/ApiAuthTest.php index 2ab81814b..1f283753a 100644 --- 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); - $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)); diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 8bf9475cc..92ff4a829 100644 --- 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); } + + 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 2cecad8bf..b3e6bd41b 100644 --- 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); - $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); - $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); diff --git a/tests/SharedTestHelpers.php b/tests/SharedTestHelpers.php index 3433f3b83..f7b7d5edf 100644 --- 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}"); } + /** + * 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 diff --git a/tests/TestCase.php b/tests/TestCase.php index f20b20fd8..1f1d5ece7 100644 --- a/tests/TestCase.php +++ b/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