Merge pull request #2996 from BookStackApp/saml2_acs_session

Updated SAML ACS post to retain user session

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

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.
+     * Assertion Consumer Service start URL. Takes the SAMLResponse from the IDP.
-     * Processes the SAML response 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);
+        $samlResponse = $request->get('SAMLResponse', null);
-        if ($user === 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');
         }
 

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

tests/Auth/Saml2Test.php

@@ -68,9 +68,18 @@ 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', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->post('/saml2/acs');
+        $redirect = $acsPost->headers->get('Location');
-            $acsPost->assertRedirect('/');
+        $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'            => 'user@example.com',
@@ -78,7 +87,28 @@ class Saml2Test extends TestCase
             '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) {
+        $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->post('/saml2/acs');
         $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');
-        });
     }
 
     public function test_group_role_sync_removal_option_works_as_expected()
@@ -110,8 +138,7 @@ class Saml2Test extends TestCase
             'saml2.remove_from_groups' => true,
         ]);
 
-        $this->withPost(['SAMLResponse' => $this->acsPostData], function () {
+        $acsPost = $this->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->post('/saml2/acs');
         $user = User::query()->where('external_auth_id', '=', 'user')->first();
 
         $randomRole = factory(Role::class)->create(['external_auth_id' => 'random']);
@@ -119,9 +146,8 @@ class Saml2Test extends TestCase
         $this->assertContains($randomRole->id, $user->roles()->pluck('id'));
 
         auth()->logout();
-            $acsPost = $this->post('/saml2/acs');
+        $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->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $this->post('/saml2/acs');
 
         $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);
     }
 
     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->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $this->post('/saml2/acs');
+        $this->assertTrue($this->isAuthenticated());
 
         $req = $this->get('/saml2/logout');
         $req->assertRedirect('/');
         $this->assertFalse($this->isAuthenticated());
-        };
-
-        $this->withPost(['SAMLResponse' => $this->acsPostData], $loginAndStartLogout);
     }
 
     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->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->post('/saml2/acs');
         $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->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->post('/saml2/acs');
+        $acsPost->assertSeeText('That email domain does not have access to this application');
-            $acsPost->assertRedirect('/login');
+        $this->assertFalse(auth()->check());
-            $errorMessage = session()->get('error');
-            $this->assertStringContainsString('That email domain does not have access to this application', $errorMessage);
         $this->assertDatabaseMissing('users', ['email' => 'user@example.com']);
-        });
     }
 
     public function test_group_sync_functions_when_email_confirmation_required()
@@ -284,8 +298,7 @@ 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', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->followingRedirects()->post('/saml2/acs');
 
         $this->assertEquals('http://localhost/register/confirm', url()->current());
         $acsPost->assertSee('Please check your email and click the confirmation button to access BookStack.');
@@ -296,7 +309,6 @@ class Saml2Test extends TestCase
         $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->followingRedirects()->post('/saml2/acs', ['SAMLResponse' => $this->acsPostData]);
-            $acsPost = $this->post('/saml2/acs');
-            $acsPost->assertRedirect('/login');
         $this->assertFalse($this->isAuthenticated());
         $this->assertDatabaseHas('users', [
             'email'            => 'user@example.com',
             'external_auth_id' => 'old_system_user_id',
         ]);
 
-            $loginGet = $this->get('/login');
+        $acsPost->assertSee('A user with the email user@example.com already exists but with different credentials');
-            $loginGet->assertSee('A user with the email user@example.com 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 = [];