diff --git a/app/Access/Controllers/OidcController.php b/app/Access/Controllers/OidcController.php index 055d4c140..654ed6928 100644 --- a/app/Access/Controllers/OidcController.php +++ b/app/Access/Controllers/OidcController.php @@ -9,11 +9,9 @@ use Illuminate\Http\Request; class OidcController extends Controller { - protected OidcService $oidcService; - - public function __construct(OidcService $oidcService) - { - $this->oidcService = $oidcService; + public function __construct( + protected OidcService $oidcService + ) { $this->middleware('guard:oidc'); } @@ -30,7 +28,7 @@ class OidcController extends Controller return redirect('/login'); } - session()->flash('oidc_state', $loginDetails['state']); + session()->put('oidc_state', time() . ':' . $loginDetails['state']); return redirect($loginDetails['url']); } @@ -41,10 +39,16 @@ class OidcController extends Controller */ public function callback(Request $request) { - $storedState = session()->pull('oidc_state'); $responseState = $request->query('state'); + $splitState = explode(':', session()->pull('oidc_state', ':'), 2); + if (count($splitState) !== 2) { + $splitState = [null, null]; + } - if ($storedState !== $responseState) { + [$storedStateTime, $storedState] = $splitState; + $threeMinutesAgo = time() - 3 * 60; + + if (!$storedState || $storedState !== $responseState || intval($storedStateTime) < $threeMinutesAgo) { $this->showErrorNotification(trans('errors.oidc_fail_authed', ['system' => config('oidc.name')])); return redirect('/login'); @@ -62,7 +66,7 @@ class OidcController extends Controller } /** - * Log the user out then start the OIDC RP-initiated logout process. + * Log the user out, then start the OIDC RP-initiated logout process. */ public function logout() { diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index a0db1c2ba..710e53757 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -138,7 +138,7 @@ class OidcTest extends TestCase { // Start auth $this->post('/oidc/login'); - $state = session()->get('oidc_state'); + $state = explode(':', session()->get('oidc_state'), 2)[1]; $transactions = $this->mockHttpClient([$this->getMockAuthorizationResponse([ 'email' => 'benny@example.com', @@ -190,6 +190,35 @@ class OidcTest extends TestCase $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization'); } + public function test_callback_works_even_if_other_request_made_by_session() + { + $this->mockHttpClient([$this->getMockAuthorizationResponse([ + 'email' => 'benny@example.com', + 'sub' => 'benny1010101', + ])]); + + $this->post('/oidc/login'); + $state = explode(':', session()->get('oidc_state'), 2)[1]; + + $this->get('/'); + + $resp = $this->get("/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state={$state}"); + $resp->assertRedirect('/'); + } + + public function test_callback_fails_if_state_timestamp_is_too_old() + { + $this->post('/oidc/login'); + $state = explode(':', session()->get('oidc_state'), 2)[1]; + session()->put('oidc_state', (time() - 60 * 4) . ':' . $state); + + $this->get('/'); + + $resp = $this->get("/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state={$state}"); + $resp->assertRedirect('/login'); + $this->assertSessionError('Login using SingleSignOn-Testing failed, system did not provide successful authorization'); + } + public function test_dump_user_details_option_outputs_as_expected() { config()->set('oidc.dump_user_details', true); @@ -797,7 +826,7 @@ class OidcTest extends TestCase { // Start auth $resp = $this->post('/oidc/login'); - $state = session()->get('oidc_state'); + $state = explode(':', session()->get('oidc_state'), 2)[1]; $pkceCode = session()->get('oidc_pkce_code'); $this->assertGreaterThan(30, strlen($pkceCode)); @@ -825,7 +854,7 @@ class OidcTest extends TestCase { config()->set('oidc.display_name_claims', 'first_name|last_name'); $this->post('/oidc/login'); - $state = session()->get('oidc_state'); + $state = explode(':', session()->get('oidc_state'), 2)[1]; $client = $this->mockHttpClient([ $this->getMockAuthorizationResponse(['name' => null]), @@ -973,7 +1002,7 @@ class OidcTest extends TestCase ]); $this->post('/oidc/login'); - $state = session()->get('oidc_state'); + $state = explode(':', session()->get('oidc_state'), 2)[1]; $client = $this->mockHttpClient([$this->getMockAuthorizationResponse([ 'groups' => [], ])]); @@ -999,7 +1028,7 @@ class OidcTest extends TestCase protected function runLogin($claimOverrides = [], $additionalHttpResponses = []): TestResponse { $this->post('/oidc/login'); - $state = session()->get('oidc_state'); + $state = explode(':', session()->get('oidc_state'), 2)[1] ?? ''; $this->mockHttpClient([$this->getMockAuthorizationResponse($claimOverrides), ...$additionalHttpResponses]); return $this->get('/oidc/callback?code=SplxlOBeZQQYbYS6WxSbIA&state=' . $state);