Updated SAML ACS post to retain user session

Session was being lost due to the callback POST request cookies
not being provided due to samesite=lax. This instead adds an additional
hop in the flow to route the request via a GET request so the session is
retained. SAML POST data is stored encrypted in cache via a unique ID
then pulled out straight afterwards, and restored into POST for the SAML
toolkit to validate.

Updated testing to cover.

diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index 6d3915c4d5bc23f8b4bd00cf207e19d9b37341bc..9c208832aba1b43aafb4864dae60991865502f45 100644 (file)
--- a/app/Auth/Access/Saml2Service.php
+++ b/app/Auth/Access/Saml2Service.php
@@ -91,8 +91,11 @@ class Saml2Service
      * @throws JsonDebugException
      * @throws UserRegistrationException
      */
-    public function processAcsResponse(?string $requestId): ?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.
+        $_POST['SAMLResponse'] = $samlResponse;
         $toolkit = $this->getToolkit();
         $toolkit->processResponse($requestId);
         $errors = $toolkit->getErrors();

diff --git a/app/Http/Controllers/Auth/Saml2Controller.php b/app/Http/Controllers/Auth/Saml2Controller.php
index 14eb65b717de6076ecbd76f1d352b877146c95f7..6a9071f9801331146436189b9c9b8400dc2fea9d 100644 (file)
--- a/app/Http/Controllers/Auth/Saml2Controller.php
+++ b/app/Http/Controllers/Auth/Saml2Controller.php
@@ -4,6 +4,9 @@ namespace BookStack\Http\Controllers\Auth;
 
 use BookStack\Auth\Access\Saml2Service;
 use BookStack\Http\Controllers\Controller;
+use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Cache;
+use Str;
 
 class Saml2Controller extends Controller
 {
@@ -68,17 +71,56 @@ class Saml2Controller extends Controller
     }
 
     /**
-     * Assertion Consumer Service.
-     * Processes the SAML response from the IDP.
+     * Assertion Consumer Service start URL. Takes the SAMLResponse from the IDP.
+     * Due to being an external POST request, we likely won't have context of the
+     * current user session due to lax cookies. To work around this we store the
+     * SAMLResponse data and redirect to the processAcs endpoint for the actual
+     * processing of the request with proper context of the user session.
      */
-    public function acs()
+    public function startAcs(Request $request)
     {
-        $requestId = session()->pull('saml2_request_id', null);
+        // 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');
 
-        $user = $this->samlService->processAcsResponse($requestId);
-        if ($user === null) {
+        $samlResponse = $request->get('SAMLResponse', null);
+
+        if (empty($samlResponse)) {
+            $this->showErrorNotification(trans('errors.saml_fail_authed', ['system' => config('saml2.name')]));
+            return redirect('/login');
+        }
+
+        $acsId = Str::random(16);
+        $cacheKey = 'saml2_acs:' . $acsId;
+        cache()->set($cacheKey, encrypt($samlResponse), 10);
+
+        return redirect()->guest('/saml2/acs?id=' . $acsId);
+    }
+
+    /**
+     * Assertion Consumer Service process endpoint.
+     * Processes the SAML response from the IDP with context of the current session.
+     * Takes the SAML request from the cache, added by the startAcs method above.
+     */
+    public function processAcs(Request $request)
+    {
+        $acsId = $request->get('id', null);
+        $cacheKey = 'saml2_acs:' . $acsId;
+        $samlResponse = null;
+        try {
+            $samlResponse = decrypt(cache()->pull($cacheKey));
+        } catch (\Exception $exception) {}
+        $requestId = session()->pull('saml2_request_id', 'unset');
+
+        if (empty($acsId) || empty($samlResponse)) {
             $this->showErrorNotification(trans('errors.saml_fail_authed', ['system' => config('saml2.name')]));
+            return redirect('/login');
+        }
 
+        $user = $this->samlService->processAcsResponse($requestId, $samlResponse);
+        if (is_null($user)) {
+            $this->showErrorNotification(trans('errors.saml_fail_authed', ['system' => config('saml2.name')]));
             return redirect('/login');
         }
 

diff --git a/routes/web.php b/routes/web.php
index 2540764512a7816a9818776d6a7a531cd33a9c1b..a5f35fb8af4a60f8f9e9540c2abe8b13a60f1a5d 100644 (file)
--- a/routes/web.php
+++ b/routes/web.php
@@ -265,7 +265,8 @@ Route::post('/saml2/login', 'Auth\Saml2Controller@login');
 Route::get('/saml2/logout', 'Auth\Saml2Controller@logout');
 Route::get('/saml2/metadata', 'Auth\Saml2Controller@metadata');
 Route::get('/saml2/sls', 'Auth\Saml2Controller@sls');
-Route::post('/saml2/acs', 'Auth\Saml2Controller@acs');
+Route::post('/saml2/acs', 'Auth\Saml2Controller@startAcs');
+Route::get('/saml2/acs', 'Auth\Saml2Controller@processAcs');
 
 // OIDC routes
 Route::post('/oidc/login', 'Auth\OidcController@login');

diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index 8ace3e2ee4f19dd9ea8fee11f606f2225f601277..7fb8d6ddbe4cd40fa8f82ca76ff41149f60ae46a 100644 (file)
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -68,17 +68,47 @@ class Saml2Test extends TestCase
         config()->set(['saml2.onelogin.strict' => false]);
         $this->assertFalse($this->isAuthenticated());
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
-            $acsPost = $this->post('/saml2/acs');
-            $acsPost->assertRedirect('/');
-            $this->assertTrue($this->isAuthenticated());
-            $this->assertDatabaseHas('users', [
-                'email'            => '[email protected]',
-                'external_auth_id' => 'user',
-                'email_confirmed'  => false,
-                'name'             => 'Barry Scott',
-            ]);
-        });
+        $acsPost = $this->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $redirect = $acsPost->headers->get('Location');
+        $acsId = explode('?id=', $redirect)[1];
+        $this->assertTrue(strlen($acsId) > 12);
+
+        $this->assertStringContainsString('/saml2/acs?id=', $redirect);
+        $this->assertTrue(cache()->has('saml2_acs:' . $acsId));
+
+        $acsGet = $this->get($redirect);
+        $acsGet->assertRedirect('/');
+        $this->assertFalse(cache()->has('saml2_acs:' . $acsId));
+
+        $this->assertTrue($this->isAuthenticated());
+        $this->assertDatabaseHas('users', [
+            'email'            => '[email protected]',
+            'external_auth_id' => 'user',
+            'email_confirmed'  => false,
+            'name'             => 'Barry Scott',
+        ]);
+    }
+
+    public function test_acs_process_id_randomly_generated()
+    {
+        $acsPost = $this->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $redirectA = $acsPost->headers->get('Location');
+
+        $acsPost = $this->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $redirectB = $acsPost->headers->get('Location');
+
+        $this->assertFalse($redirectA === $redirectB);
+    }
+
+    public function test_process_acs_endpoint_cant_be_called_with_invalid_id()
+    {
+        $resp = $this->get('/saml2/acs');
+        $resp->assertRedirect('/login');
+        $this->followRedirects($resp)->assertSeeText('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
+
+        $resp = $this->get('/saml2/acs?id=abc123');
+        $resp->assertRedirect('/login');
+        $this->followRedirects($resp)->assertSeeText('Login using SingleSignOn-Testing failed, system did not provide successful authorization');
     }
 
     public function test_group_role_sync_on_login()
@@ -92,14 +122,12 @@ class Saml2Test extends TestCase
         $memberRole = factory(Role::class)->create(['external_auth_id' => 'member']);
         $adminRole = Role::getSystemRole('admin');
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () use ($memberRole, $adminRole) {
-            $acsPost = $this->post('/saml2/acs');
-            $user = User::query()->where('external_auth_id', '=', 'user')->first();
+        $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $user = User::query()->where('external_auth_id', '=', 'user')->first();
 
-            $userRoleIds = $user->roles()->pluck('id');
-            $this->assertContains($memberRole->id, $userRoleIds, 'User was assigned to member role');
-            $this->assertContains($adminRole->id, $userRoleIds, 'User was assigned to admin role');
-        });
+        $userRoleIds = $user->roles()->pluck('id');
+        $this->assertContains($memberRole->id, $userRoleIds, 'User was assigned to member role');
+        $this->assertContains($adminRole->id, $userRoleIds, 'User was assigned to admin role');
     }
 
     public function test_group_role_sync_removal_option_works_as_expected()
@@ -110,18 +138,16 @@ class Saml2Test extends TestCase
             'saml2.remove_from_groups' => true,
         ]);
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
-            $acsPost = $this->post('/saml2/acs');
-            $user = User::query()->where('external_auth_id', '=', 'user')->first();
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $user = User::query()->where('external_auth_id', '=', 'user')->first();
 
-            $randomRole = factory(Role::class)->create(['external_auth_id' => 'random']);
-            $user->attachRole($randomRole);
-            $this->assertContains($randomRole->id, $user->roles()->pluck('id'));
+        $randomRole = factory(Role::class)->create(['external_auth_id' => 'random']);
+        $user->attachRole($randomRole);
+        $this->assertContains($randomRole->id, $user->roles()->pluck('id'));
 
-            auth()->logout();
-            $acsPost = $this->post('/saml2/acs');
-            $this->assertNotContains($randomRole->id, $user->roles()->pluck('id'));
-        });
+        auth()->logout();
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $this->assertNotContains($randomRole->id, $user->roles()->pluck('id'));
     }
 
     public function test_logout_link_directs_to_saml_path()
@@ -149,16 +175,12 @@ class Saml2Test extends TestCase
             $this->assertFalse($this->isAuthenticated());
         };
 
-        $loginAndStartLogout = function () use ($handleLogoutResponse) {
-            $this->post('/saml2/acs');
+        $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
 
-            $req = $this->get('/saml2/logout');
-            $redirect = $req->headers->get('location');
-            $this->assertStringStartsWith('http://saml.local/saml2/idp/SingleLogoutService.php', $redirect);
-            $this->withGet(['SAMLResponse' => $this->sloResponseData], $handleLogoutResponse);
-        };
-
-        $this->withPost(['SAMLResponse' => $this->acsPostData], $loginAndStartLogout);
+        $req = $this->get('/saml2/logout');
+        $redirect = $req->headers->get('location');
+        $this->assertStringStartsWith('http://saml.local/saml2/idp/SingleLogoutService.php', $redirect);
+        $this->withGet(['SAMLResponse' => $this->sloResponseData], $handleLogoutResponse);
     }
 
     public function test_logout_sls_flow_when_sls_not_configured()
@@ -168,15 +190,12 @@ class Saml2Test extends TestCase
             'saml2.onelogin.idp.singleLogoutService.url' => null,
         ]);
 
-        $loginAndStartLogout = function () {
-            $this->post('/saml2/acs');
+        $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $this->assertTrue($this->isAuthenticated());
 
-            $req = $this->get('/saml2/logout');
-            $req->assertRedirect('/');
-            $this->assertFalse($this->isAuthenticated());
-        };
-
-        $this->withPost(['SAMLResponse' => $this->acsPostData], $loginAndStartLogout);
+        $req = $this->get('/saml2/logout');
+        $req->assertRedirect('/');
+        $this->assertFalse($this->isAuthenticated());
     }
 
     public function test_dump_user_details_option_works()
@@ -186,14 +205,12 @@ class Saml2Test extends TestCase
             'saml2.dump_user_details' => true,
         ]);
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
-            $acsPost = $this->post('/saml2/acs');
-            $acsPost->assertJsonStructure([
-                'id_from_idp',
-                'attrs_from_idp'      => [],
-                'attrs_after_parsing' => [],
-            ]);
-        });
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $acsPost->assertJsonStructure([
+            'id_from_idp',
+            'attrs_from_idp'      => [],
+            'attrs_after_parsing' => [],
+        ]);
     }
 
     public function test_saml_routes_are_only_active_if_saml_enabled()
@@ -263,13 +280,10 @@ class Saml2Test extends TestCase
             'saml2.onelogin.strict' => false,
         ]);
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
-            $acsPost = $this->post('/saml2/acs');
-            $acsPost->assertRedirect('/login');
-            $errorMessage = session()->get('error');
-            $this->assertStringContainsString('That email domain does not have access to this application', $errorMessage);
-            $this->assertDatabaseMissing('users', ['email' => '[email protected]']);
-        });
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $acsPost->assertSeeText('That email domain does not have access to this application');
+        $this->assertFalse(auth()->check());
+        $this->assertDatabaseMissing('users', ['email' => '[email protected]']);
     }
 
     public function test_group_sync_functions_when_email_confirmation_required()
@@ -284,19 +298,17 @@ class Saml2Test extends TestCase
         $memberRole = factory(Role::class)->create(['external_auth_id' => 'member']);
         $adminRole = Role::getSystemRole('admin');
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () use ($memberRole, $adminRole) {
-            $acsPost = $this->followingRedirects()->post('/saml2/acs');
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
 
-            $this->assertEquals('http://localhost/register/confirm', url()->current());
-            $acsPost->assertSee('Please check your email and click the confirmation button to access BookStack.');
-            /** @var User $user */
-            $user = User::query()->where('external_auth_id', '=', 'user')->first();
+        $this->assertEquals('http://localhost/register/confirm', url()->current());
+        $acsPost->assertSee('Please check your email and click the confirmation button to access BookStack.');
+        /** @var User $user */
+        $user = User::query()->where('external_auth_id', '=', 'user')->first();
 
-            $userRoleIds = $user->roles()->pluck('id');
-            $this->assertContains($memberRole->id, $userRoleIds, 'User was assigned to member role');
-            $this->assertContains($adminRole->id, $userRoleIds, 'User was assigned to admin role');
-            $this->assertFalse(boolval($user->email_confirmed), 'User email remains unconfirmed');
-        });
+        $userRoleIds = $user->roles()->pluck('id');
+        $this->assertContains($memberRole->id, $userRoleIds, 'User was assigned to member role');
+        $this->assertContains($adminRole->id, $userRoleIds, 'User was assigned to admin role');
+        $this->assertFalse(boolval($user->email_confirmed), 'User email remains unconfirmed');
 
         $this->assertNull(auth()->user());
         $homeGet = $this->get('/');
@@ -316,18 +328,14 @@ class Saml2Test extends TestCase
             'name'             => 'Barry Scott',
         ]);
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
-            $acsPost = $this->post('/saml2/acs');
-            $acsPost->assertRedirect('/login');
-            $this->assertFalse($this->isAuthenticated());
-            $this->assertDatabaseHas('users', [
-                'email'            => '[email protected]',
-                'external_auth_id' => 'old_system_user_id',
-            ]);
-
-            $loginGet = $this->get('/login');
-            $loginGet->assertSee('A user with the email [email protected] already exists but with different credentials');
-        });
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
+        $this->assertFalse($this->isAuthenticated());
+        $this->assertDatabaseHas('users', [
+            'email'            => '[email protected]',
+            'external_auth_id' => 'old_system_user_id',
+        ]);
+
+        $acsPost->assertSee('A user with the email [email protected] already exists but with different credentials');
     }
 
     public function test_login_request_contains_expected_default_authncontext()
@@ -370,11 +378,6 @@ class Saml2Test extends TestCase
         return $this->withGlobal($_GET, $options, $callback);
     }
 
-    protected function withPost(array $options, callable $callback)
-    {
-        return $this->withGlobal($_POST, $options, $callback);
-    }
-
     protected function withGlobal(array &$global, array $options, callable $callback)
     {
         $original = [];