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); }