SAML2: Fixed non-spec point of logout, Improved redirect location

This changes the point-of-logout to be within the initial part of the
SAML logout flow, as per 5.3.2 of the SAML spec, processing step 2.
This also improves the logout redirect handling to use the global
redirect suggestion so that auto-login handling is properly taken into
account.

Added tests to cover.
Manual testing performed against keycloak.
For #4713

diff --git a/app/Access/Controllers/Saml2Controller.php b/app/Access/Controllers/Saml2Controller.php
index 2f16984464843fb0f1a87f55953fcca062a5196e..6f802370e9c50d84064b8187ed37ff4a1544e288 100644 (file)
--- 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 e7627f7e4c2d8a9ce4cedd5cbac357ba9b67420b..664b77aba4510289e3eccc98b59ced6c77205a9d 100644 (file)
--- 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 67d56eabe5acb9cf891ff6547b105a219b3c9f73..3de6238edc88dceef225bf05b58fff351c0dfcfa 100644 (file)
--- 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([