diff --git a/app/Access/Controllers/Saml2Controller.php b/app/Access/Controllers/Saml2Controller.php index 2f1698446..6f802370e 100644 --- a/app/Access/Controllers/Saml2Controller.php +++ b/app/Access/Controllers/Saml2Controller.php @@ -9,14 +9,9 @@ use Illuminate\Support\Str; class Saml2Controller extends Controller { - protected Saml2Service $samlService; - - /** - * Saml2Controller constructor. - */ - public function __construct(Saml2Service $samlService) - { - $this->samlService = $samlService; + public function __construct( + protected Saml2Service $samlService + ) { $this->middleware('guard:saml2'); } @@ -36,7 +31,12 @@ class Saml2Controller extends Controller */ public function logout() { - $logoutDetails = $this->samlService->logout(auth()->user()); + $user = user(); + if ($user->isGuest()) { + return redirect('/login'); + } + + $logoutDetails = $this->samlService->logout($user); if ($logoutDetails['id']) { session()->flash('saml2_logout_request_id', $logoutDetails['id']); @@ -64,7 +64,7 @@ class Saml2Controller extends Controller public function sls() { $requestId = session()->pull('saml2_logout_request_id', null); - $redirect = $this->samlService->processSlsResponse($requestId) ?? '/'; + $redirect = $this->samlService->processSlsResponse($requestId); return redirect($redirect); } diff --git a/app/Access/Saml2Service.php b/app/Access/Saml2Service.php index e7627f7e4..664b77aba 100644 --- a/app/Access/Saml2Service.php +++ b/app/Access/Saml2Service.php @@ -48,20 +48,23 @@ class Saml2Service /** * Initiate a logout flow. + * Returns the SAML2 request ID, and the URL to redirect the user to. * * @throws Error + * @returns array{url: string, id: ?string} */ public function logout(User $user): array { $toolKit = $this->getToolkit(); - $returnRoute = url('/'); + $sessionIndex = session()->get('saml2_session_index'); + $returnUrl = url($this->loginService->logout()); try { $url = $toolKit->logout( - $returnRoute, + $returnUrl, [], $user->email, - session()->get('saml2_session_index'), + $sessionIndex, true, Constants::NAMEID_EMAIL_ADDRESS ); @@ -71,7 +74,7 @@ class Saml2Service throw $error; } - $url = $this->loginService->logout(); + $url = $returnUrl; $id = null; } @@ -121,7 +124,7 @@ class Saml2Service * * @throws Error */ - public function processSlsResponse(?string $requestId): ?string + public function processSlsResponse(?string $requestId): string { $toolkit = $this->getToolkit(); @@ -130,7 +133,7 @@ class Saml2Service // value so that the exact encoding format is matched when checking the signature. // This is primarily due to ADFS encoding query params with lowercase percent encoding while // PHP (And most other sensible providers) standardise on uppercase. - $redirect = $toolkit->processSLO(true, $requestId, true, null, true); + $samlRedirect = $toolkit->processSLO(true, $requestId, true, null, true); $errors = $toolkit->getErrors(); if (!empty($errors)) { @@ -139,9 +142,9 @@ class Saml2Service ); } - $this->loginService->logout(); + $defaultBookStackRedirect = $this->loginService->logout(); - return $redirect; + return $samlRedirect ?? $defaultBookStackRedirect; } /** diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php index 67d56eabe..3de6238ed 100644 --- a/tests/Auth/Saml2Test.php +++ b/tests/Auth/Saml2Test.php @@ -181,7 +181,7 @@ class Saml2Test extends TestCase ]); $handleLogoutResponse = function () { - $this->assertTrue($this->isAuthenticated()); + $this->assertFalse($this->isAuthenticated()); $req = $this->get('/saml2/sls'); $req->assertRedirect('/'); @@ -214,6 +214,55 @@ class Saml2Test extends TestCase $this->assertFalse($this->isAuthenticated()); } + public function test_logout_sls_flow_logs_user_out_before_redirect() + { + config()->set([ + 'saml2.onelogin.strict' => false, + ]); + + $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]); + $this->assertTrue($this->isAuthenticated()); + + $req = $this->post('/saml2/logout'); + $redirect = $req->headers->get('location'); + $this->assertStringStartsWith('http://saml.local/saml2/idp/SingleLogoutService.php', $redirect); + $this->assertFalse($this->isAuthenticated()); + } + + public function test_logout_sls_request_redirect_prevents_auto_login_when_enabled() + { + config()->set([ + 'saml2.onelogin.strict' => false, + 'auth.auto_initiate' => true, + 'services.google.client_id' => false, + 'services.github.client_id' => false, + ]); + + $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]); + + $req = $this->post('/saml2/logout'); + $redirect = $req->headers->get('location'); + $this->assertStringContainsString(urlencode(url('/login?prevent_auto_init=true')), $redirect); + } + + public function test_logout_sls_response_endpoint_redirect_prevents_auto_login_when_enabled() + { + config()->set([ + 'saml2.onelogin.strict' => false, + 'auth.auto_initiate' => true, + 'services.google.client_id' => false, + 'services.github.client_id' => false, + ]); + + $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]); + + $this->withGet(['SAMLResponse' => $this->sloResponseData], function () { + $req = $this->get('/saml2/sls'); + $redirect = $req->headers->get('location'); + $this->assertEquals(url('/login?prevent_auto_init=true'), $redirect); + }); + } + public function test_dump_user_details_option_works() { config()->set([