From f910738a80f918e1fdcf9f1ae4e63bce4287bd83 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 14 Nov 2021 21:13:24 +0000 Subject: [PATCH] Changed logout routes to POST instead of GET As per #3047. Also made some SAML specific fixes: - IDP initiated login was broken due to forced default session value. Double checked against OneLogin lib docs that this reverted logic was fine. - Changed how the saml login flow works to use 'withoutMiddleware' on the route instead of hacking out the session driver. This was due to the array driver (previously used for the hack) no longer being considered non-persistent. --- app/Auth/Access/Saml2Service.php | 2 +- app/Http/Controllers/Auth/Saml2Controller.php | 10 ++-------- resources/views/common/header.blade.php | 12 +++++++----- routes/web.php | 10 +++++++--- tests/Auth/AuthTest.php | 4 ++-- tests/Auth/OidcTest.php | 2 +- tests/Auth/Saml2Test.php | 13 ++++++------- 7 files changed, 26 insertions(+), 27 deletions(-) diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php index ad889faf7c1..f5d0cd7ccf5 100644 --- a/app/Auth/Access/Saml2Service.php +++ b/app/Auth/Access/Saml2Service.php @@ -99,7 +99,7 @@ public function logout(User $user): array * @throws JsonDebugException * @throws UserRegistrationException */ - public function processAcsResponse(string $requestId, string $samlResponse): ?User + public function processAcsResponse(?string $requestId, string $samlResponse): ?User { // The SAML2 toolkit expects the response to be within the $_POST superglobal // so we need to manually put it back there at this point. diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php index bd3b25da770..b8448396112 100644 --- a/app/Http/Controllers/Auth/Saml2Controller.php +++ b/app/Http/Controllers/Auth/Saml2Controller.php @@ -5,8 +5,7 @@ use BookStack\Auth\Access\Saml2Service; use BookStack\Http\Controllers\Controller; use Illuminate\Http\Request; -use Illuminate\Support\Facades\Cache; -use Str; +use Illuminate\Support\Str; class Saml2Controller extends Controller { @@ -79,11 +78,6 @@ public function sls() */ public function startAcs(Request $request) { - // Note: This is a bit of a hack to prevent a session being stored - // on the response of this request. Within Laravel7+ this could instead - // be done via removing the StartSession middleware from the route. - config()->set('session.driver', 'array'); - $samlResponse = $request->get('SAMLResponse', null); if (empty($samlResponse)) { @@ -114,7 +108,7 @@ public function processAcs(Request $request) $samlResponse = decrypt(cache()->pull($cacheKey)); } catch (\Exception $exception) { } - $requestId = session()->pull('saml2_request_id', 'unset'); + $requestId = session()->pull('saml2_request_id', null); if (empty($acsId) || empty($samlResponse)) { $this->showErrorNotification(trans('errors.saml_fail_authed', ['system' => config('saml2.name')])); diff --git a/resources/views/common/header.blade.php b/resources/views/common/header.blade.php index 2311ce3e019..d55f3ae2dac 100644 --- a/resources/views/common/header.blade.php +++ b/resources/views/common/header.blade.php @@ -71,11 +71,13 @@ class="mobile-menu-toggle hide-over-l">@icon('more') @icon('edit'){{ trans('common.edit_profile') }}
  • - @if(config('auth.method') === 'saml2') - @icon('logout'){{ trans('auth.logout') }} - @else - @icon('logout'){{ trans('auth.logout') }} - @endif +
    + {{ csrf_field() }} + +
  • diff --git a/routes/web.php b/routes/web.php index 653b5c22711..c924ed68c9e 100644 --- a/routes/web.php +++ b/routes/web.php @@ -277,7 +277,7 @@ // Login/Logout routes Route::get('/login', [Auth\LoginController::class, 'getLogin']); Route::post('/login', [Auth\LoginController::class, 'login']); -Route::get('/logout', [Auth\LoginController::class, 'logout']); +Route::post('/logout', [Auth\LoginController::class, 'logout']); Route::get('/register', [Auth\RegisterController::class, 'getRegister']); Route::get('/register/confirm', [Auth\ConfirmEmailController::class, 'show']); Route::get('/register/confirm/awaiting', [Auth\ConfirmEmailController::class, 'showAwaiting']); @@ -287,10 +287,14 @@ // SAML routes Route::post('/saml2/login', [Auth\Saml2Controller::class, 'login']); -Route::get('/saml2/logout', [Auth\Saml2Controller::class, 'logout']); +Route::post('/saml2/logout', [Auth\Saml2Controller::class, 'logout']); Route::get('/saml2/metadata', [Auth\Saml2Controller::class, 'metadata']); Route::get('/saml2/sls', [Auth\Saml2Controller::class, 'sls']); -Route::post('/saml2/acs', [Auth\Saml2Controller::class, 'startAcs']); +Route::post('/saml2/acs', [Auth\Saml2Controller::class, 'startAcs'])->withoutMiddleware([ + \Illuminate\Session\Middleware\StartSession::class, + \Illuminate\View\Middleware\ShareErrorsFromSession::class, + \BookStack\Http\Middleware\VerifyCsrfToken::class, +]); Route::get('/saml2/acs', [Auth\Saml2Controller::class, 'processAcs']); // OIDC routes diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php index 66ab09d3c62..50f56bfb9c6 100644 --- a/tests/Auth/AuthTest.php +++ b/tests/Auth/AuthTest.php @@ -192,7 +192,7 @@ public function test_restricted_registration_with_confirmation_disabled() public function test_logout() { $this->asAdmin()->get('/')->assertOk(); - $this->get('/logout')->assertRedirect('/'); + $this->post('/logout')->assertRedirect('/'); $this->get('/')->assertRedirect('/login'); } @@ -204,7 +204,7 @@ public function test_mfa_session_cleared_on_logout() $mfaSession->markVerifiedForUser($user); $this->assertTrue($mfaSession->isVerifiedForUser($user)); - $this->asAdmin()->get('/logout'); + $this->asAdmin()->post('/logout'); $this->assertFalse($mfaSession->isVerifiedForUser($user)); } diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index e7665a679a4..0b033ea8125 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -90,7 +90,7 @@ public function test_standard_login_routes_inaccessible() public function test_logout_route_functions() { $this->actingAs($this->getEditor()); - $this->get('/logout'); + $this->post('/logout'); $this->assertFalse(auth()->check()); } diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php index aac2710a889..cb217585c5f 100644 --- a/tests/Auth/Saml2Test.php +++ b/tests/Auth/Saml2Test.php @@ -157,8 +157,7 @@ public function test_logout_link_directs_to_saml_path() ]); $resp = $this->actingAs($this->getEditor())->get('/'); - $resp->assertElementExists('a[href$="/saml2/logout"]'); - $resp->assertElementContains('a[href$="/saml2/logout"]', 'Logout'); + $resp->assertElementContains('form[action$="/saml2/logout"] button', 'Logout'); } public function test_logout_sls_flow() @@ -177,7 +176,7 @@ public function test_logout_sls_flow() $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]); - $req = $this->get('/saml2/logout'); + $req = $this->post('/saml2/logout'); $redirect = $req->headers->get('location'); $this->assertStringStartsWith('http://saml.local/saml2/idp/SingleLogoutService.php', $redirect); $this->withGet(['SAMLResponse' => $this->sloResponseData], $handleLogoutResponse); @@ -193,7 +192,7 @@ public function test_logout_sls_flow_when_sls_not_configured() $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]); $this->assertTrue($this->isAuthenticated()); - $req = $this->get('/saml2/logout'); + $req = $this->post('/saml2/logout'); $req->assertRedirect('/'); $this->assertFalse($this->isAuthenticated()); } @@ -216,13 +215,13 @@ public function test_dump_user_details_option_works() public function test_saml_routes_are_only_active_if_saml_enabled() { config()->set(['auth.method' => 'standard']); - $getRoutes = ['/logout', '/metadata', '/sls']; + $getRoutes = ['/metadata', '/sls']; foreach ($getRoutes as $route) { $req = $this->get('/saml2' . $route); $this->assertPermissionError($req); } - $postRoutes = ['/login', '/acs']; + $postRoutes = ['/login', '/acs', '/logout']; foreach ($postRoutes as $route) { $req = $this->post('/saml2' . $route); $this->assertPermissionError($req); @@ -249,7 +248,7 @@ public function test_standard_login_routes_inaccessible() $resp = $this->post('/login'); $this->assertPermissionError($resp); - $resp = $this->get('/logout'); + $resp = $this->post('/logout'); $this->assertPermissionError($resp); }