From 81d256aebd6e7bca7b86369abd5e79b3a6679f33 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 7 Dec 2023 17:45:17 +0000 Subject: [PATCH] OIDC RP Logout: Fixed issues during testing - Disabled by default due to strict rejection by auth systems. - Fixed issue when autoloading logout URL, but not provided in autodiscovery response. - Added proper handling for if the logout URL contains a query string already. - Added extra tests to cover. - Forced config endpoint to be used, if set as a string, instead of autodiscovery endpoint. --- app/Access/Oidc/OidcService.php | 9 ++++-- app/Config/oidc.php | 5 ++-- tests/Auth/OidcTest.php | 53 +++++++++++++++++++++++++++++++-- 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/app/Access/Oidc/OidcService.php b/app/Access/Oidc/OidcService.php index 3f9cd41b4..f1e5b25af 100644 --- a/app/Access/Oidc/OidcService.php +++ b/app/Access/Oidc/OidcService.php @@ -84,7 +84,7 @@ class OidcService 'redirectUri' => url('/oidc/callback'), 'authorizationEndpoint' => $config['authorization_endpoint'], 'tokenEndpoint' => $config['token_endpoint'], - 'endSessionEndpoint' => $config['end_session_endpoint'], + 'endSessionEndpoint' => is_string($config['end_session_endpoint']) ? $config['end_session_endpoint'] : null, ]); // Use keys if configured @@ -102,8 +102,11 @@ class OidcService } // Prevent use of RP-initiated logout if specifically disabled + // Or force use of a URL if specifically set. if ($config['end_session_endpoint'] === false) { $settings->endSessionEndpoint = null; + } else if (is_string($config['end_session_endpoint'])) { + $settings->endSessionEndpoint = $config['end_session_endpoint']; } $settings->validate(); @@ -314,6 +317,8 @@ class OidcService 'post_logout_redirect_uri' => $defaultLogoutUrl, ]; - return $oidcSettings->endSessionEndpoint . '?' . http_build_query($endpointParams); + $joiner = str_contains($oidcSettings->endSessionEndpoint, '?') ? '&' : '?'; + + return $oidcSettings->endSessionEndpoint . $joiner . http_build_query($endpointParams); } } diff --git a/app/Config/oidc.php b/app/Config/oidc.php index ed9302d10..7f8f40d41 100644 --- a/app/Config/oidc.php +++ b/app/Config/oidc.php @@ -37,9 +37,10 @@ return [ 'token_endpoint' => env('OIDC_TOKEN_ENDPOINT', null), // OIDC RP-Initiated Logout endpoint URL. - // A null value gets the URL from discovery, if active. // A false value force-disables RP-Initiated Logout. - 'end_session_endpoint' => env('OIDC_END_SESSION_ENDPOINT', null), + // A true value gets the URL from discovery, if active. + // A string value is used as the URL. + 'end_session_endpoint' => env('OIDC_END_SESSION_ENDPOINT', false), // Add extra scopes, upon those required, to the OIDC authentication request // Multiple values can be provided comma seperated. diff --git a/tests/Auth/OidcTest.php b/tests/Auth/OidcTest.php index d10582d8c..dbf26f1bd 100644 --- a/tests/Auth/OidcTest.php +++ b/tests/Auth/OidcTest.php @@ -44,7 +44,7 @@ class OidcTest extends TestCase 'oidc.groups_claim' => 'group', 'oidc.remove_from_groups' => false, 'oidc.external_id_claim' => 'sub', - 'oidc.end_session_endpoint' => null, + 'oidc.end_session_endpoint' => false, ]); } @@ -486,8 +486,9 @@ class OidcTest extends TestCase $resp = $this->get('/'); $this->withHtml($resp)->assertElementExists('header form[action$="/oidc/logout"] button'); } - public function test_logout_with_autodiscovery() + public function test_logout_with_autodiscovery_with_oidc_logout_enabled() { + config()->set(['oidc.end_session_endpoint' => true]); $this->withAutodiscovery(); $transactions = $this->mockHttpClient([ @@ -499,9 +500,10 @@ class OidcTest extends TestCase $resp->assertRedirect('https://auth.example.com/oidc/logout?post_logout_redirect_uri=' . urlencode(url('/'))); $this->assertEquals(2, $transactions->requestCount()); + $this->assertFalse(auth()->check()); } - public function test_logout_with_autodiscovery_but_oidc_logout_disabled() + public function test_logout_with_autodiscovery_with_oidc_logout_disabled() { $this->withAutodiscovery(); config()->set(['oidc.end_session_endpoint' => false]); @@ -513,6 +515,7 @@ class OidcTest extends TestCase $resp = $this->asEditor()->post('/oidc/logout'); $resp->assertRedirect('/'); + $this->assertFalse(auth()->check()); } public function test_logout_without_autodiscovery_but_with_endpoint_configured() @@ -521,6 +524,16 @@ class OidcTest extends TestCase $resp = $this->asEditor()->post('/oidc/logout'); $resp->assertRedirect('https://example.com/logout?post_logout_redirect_uri=' . urlencode(url('/'))); + $this->assertFalse(auth()->check()); + } + + public function test_logout_without_autodiscovery_with_configured_endpoint_adds_to_query_if_existing() + { + config()->set(['oidc.end_session_endpoint' => 'https://example.com/logout?a=b']); + + $resp = $this->asEditor()->post('/oidc/logout'); + $resp->assertRedirect('https://example.com/logout?a=b&post_logout_redirect_uri=' . urlencode(url('/'))); + $this->assertFalse(auth()->check()); } public function test_logout_with_autodiscovery_and_auto_initiate_returns_to_auto_prevented_login() @@ -530,6 +543,7 @@ class OidcTest extends TestCase 'auth.auto_initiate' => true, 'services.google.client_id' => false, 'services.github.client_id' => false, + 'oidc.end_session_endpoint' => true, ]); $this->mockHttpClient([ @@ -541,6 +555,39 @@ class OidcTest extends TestCase $redirectUrl = url('/login?prevent_auto_init=true'); $resp->assertRedirect('https://auth.example.com/oidc/logout?post_logout_redirect_uri=' . urlencode($redirectUrl)); + $this->assertFalse(auth()->check()); + } + + public function test_logout_endpoint_url_overrides_autodiscovery_endpoint() + { + config()->set(['oidc.end_session_endpoint' => 'https://a.example.com']); + $this->withAutodiscovery(); + + $transactions = $this->mockHttpClient([ + $this->getAutoDiscoveryResponse(), + $this->getJwksResponse(), + ]); + + $resp = $this->asEditor()->post('/oidc/logout'); + $resp->assertRedirect('https://a.example.com?post_logout_redirect_uri=' . urlencode(url('/'))); + + $this->assertEquals(2, $transactions->requestCount()); + $this->assertFalse(auth()->check()); + } + + public function test_logout_with_autodiscovery_does_not_use_rp_logout_if_no_url_via_autodiscovery() + { + config()->set(['oidc.end_session_endpoint' => true]); + $this->withAutodiscovery(); + + $this->mockHttpClient([ + $this->getAutoDiscoveryResponse(['end_session_endpoint' => null]), + $this->getJwksResponse(), + ]); + + $resp = $this->asEditor()->post('/oidc/logout'); + $resp->assertRedirect('/'); + $this->assertFalse(auth()->check()); } public function test_logout_redirect_contains_id_token_hint_if_existing()