From 4a9b9a095f50318a2113e28af37dd460fca4cf34 Mon Sep 17 00:00:00 2001 From: Pieter Fiers Date: Fri, 26 Aug 2022 10:41:04 +0200 Subject: [PATCH] Add client_id and id_token_hint to IdP logout This adds two parameters to the end_session_endpoint IdP URL which the user gets redirected to when singleLogout is triggered. These paramters are: - client_id: the client ID of the current session's provider. 'OPTIONAL' as per the relevant OpenID specification. - id_token_hint: the raw id_token that was obtained during the code callback of this session's login flow (set in session variable `oidc.id_token`). 'RECOMMENDED' by the relevant OpenID specification [1]. Some providers (e.g. node-oidc-provider[2] and Keycloak[3]) require this when using the code OAuth flow. Because passing id_token_hint reveals the id_token to the user agent, a app setting was also added to optionally turn this behaviour off (default is turned on). Builds upon PR #373 / issue #336 Fixes issue #449 [1]: https://openid.net/specs/openid-connect-rpinitiated-1_0.html#RPLogout [2]: https://github.com/panva/node-oidc-provider/blob/c243bf6b6663c41ff3e75c09b95fb978eba87381/lib/actions/end_session.js#L32 [3]: https://www.keycloak.org/docs/latest/release_notes/index.html#oidc-logout-changes Signed-off-by: Pieter Fiers --- lib/Command/UpsertProvider.php | 4 +++ lib/Controller/LoginController.php | 29 +++++++++++++++++----- lib/Service/ProviderService.php | 11 ++++++-- src/components/AdminSettings.vue | 1 + src/components/SettingsForm.vue | 6 +++++ tests/unit/Service/ProviderServiceTest.php | 13 ++++++++++ 6 files changed, 56 insertions(+), 8 deletions(-) diff --git a/lib/Command/UpsertProvider.php b/lib/Command/UpsertProvider.php index 4cee0a56..f164fdfb 100644 --- a/lib/Command/UpsertProvider.php +++ b/lib/Command/UpsertProvider.php @@ -59,6 +59,7 @@ protected function configure() { ->addOption('scope', 'o', InputOption::VALUE_OPTIONAL, 'OpenID requested value scopes, if not set defaults to "openid email profile"') ->addOption('unique-uid', null, InputOption::VALUE_OPTIONAL, 'Flag if unique user ids shall be used or not. 1 to enable (default), 0 to disable') ->addOption('check-bearer', null, InputOption::VALUE_OPTIONAL, 'Flag if Nextcloud API/WebDav calls should check the Bearer token against this provider or not. 1 to enable (default), 0 to disable') + ->addOption('send-id-token-hint', null, InputOption::VALUE_OPTIONAL, 'Flag if ID token should be included as a parameter to the end_session_endpoint URL when using unified logout. 1 to enable (default), 0 to disable') ->addOption('mapping-display-name', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the display name') ->addOption('mapping-email', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the email address') ->addOption('mapping-quota', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the quota') @@ -148,6 +149,9 @@ protected function execute(InputInterface $input, OutputInterface $output) { if (($checkBearer = $input->getOption('check-bearer')) !== null) { $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_CHECK_BEARER, (string)$checkBearer === '0' ? '0' : '1'); } + if (($sendIdTokenHint = $input->getOption('send-id-token-hint')) !== null) { + $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_SEND_ID_TOKEN_HINT, (string)$sendIdTokenHint === '0' ? '0' : '1'); + } if (($uniqueUid = $input->getOption('unique-uid')) !== null) { $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_UNIQUE_UID, (string)$uniqueUid === '0' ? '0' : '1'); } diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index e23220f1..e16ca520 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -69,6 +69,7 @@ class LoginController extends Controller { private const NONCE = 'oidc.nonce'; public const PROVIDERID = 'oidc.providerid'; private const REDIRECT_AFTER_LOGIN = 'oidc.redirect'; + private const ID_TOKEN = 'oidc.id_token'; /** @var ISecureRandom */ private $random; @@ -368,9 +369,10 @@ public function code(string $state = '', string $code = '', string $scope = '', $this->eventDispatcher->dispatchTyped(new TokenObtainedEvent($data, $provider, $discovery)); // TODO: proper error handling + $idTokenRaw = $data['id_token']; $jwks = $this->discoveryService->obtainJWK($provider); JWT::$leeway = 60; - $idTokenPayload = JWT::decode($data['id_token'], $jwks, array_keys(JWT::$supported_algs)); + $idTokenPayload = JWT::decode($idTokenRaw, $jwks, array_keys(JWT::$supported_algs)); $this->logger->debug('Parsed the JWT payload: ' . json_encode($idTokenPayload, JSON_THROW_ON_ERROR)); @@ -405,6 +407,8 @@ public function code(string $state = '', string $code = '', string $scope = '', return new JSONResponse(['Failed to provision user']); } + $this->session->set(self::ID_TOKEN, $idTokenRaw); + $this->logger->debug('Logging user in'); $this->userSession->setUser($user); @@ -539,13 +543,26 @@ public function singleLogoutService() { $oidcSystemConfig = $this->config->getSystemValue('user_oidc', []); $targetUrl = $this->urlGenerator->getAbsoluteURL('/'); if (!isset($oidcSystemConfig['single_logout']) || $oidcSystemConfig['single_logout']) { - $providerId = (int)$this->session->get(self::PROVIDERID); - $provider = $this->providerMapper->getProvider($providerId); - $targetUrl = $this->discoveryService->obtainDiscovery($provider)['end_session_endpoint'] ?? $this->urlGenerator->getAbsoluteURL('/'); - if ($targetUrl) { - $targetUrl .= '?post_logout_redirect_uri=' . $this->urlGenerator->getAbsoluteURL('/'); + $providerId = $this->session->get(self::PROVIDERID); + if ($providerId) { + $provider = $this->providerMapper->getProvider((int)$providerId); + $endSessionEndpoint = $this->discoveryService->obtainDiscovery($provider)['end_session_endpoint']; + if ($endSessionEndpoint) { + $endSessionEndpoint .= '?post_logout_redirect_uri=' . $targetUrl; + $endSessionEndpoint .= '&client_id=' . $provider->getClientId(); + $shouldSendIdToken = $this->providerService->getSetting( + $provider->getId(), + ProviderService::SETTING_SEND_ID_TOKEN_HINT, '0' + ) === '1'; + $idToken = $this->session->get(self::ID_TOKEN); + if ($shouldSendIdToken && $idToken) { + $endSessionEndpoint .= '&id_token_hint=' . $idToken; + } + $targetUrl = $endSessionEndpoint; + } } } + // cleanup related oidc session $this->sessionMapper->deleteFromNcSessionId($this->session->getId()); diff --git a/lib/Service/ProviderService.php b/lib/Service/ProviderService.php index d88265c3..16473e31 100644 --- a/lib/Service/ProviderService.php +++ b/lib/Service/ProviderService.php @@ -34,6 +34,7 @@ class ProviderService { public const SETTING_CHECK_BEARER = 'checkBearer'; + public const SETTING_SEND_ID_TOKEN_HINT = 'sendIdTokenHint'; public const SETTING_UNIQUE_UID = 'uniqueUid'; public const SETTING_MAPPING_UID = 'mappingUid'; public const SETTING_MAPPING_UID_DEFAULT = 'sub'; @@ -43,6 +44,11 @@ class ProviderService { public const SETTING_EXTRA_CLAIMS = 'extraClaims'; public const SETTING_JWKS_CACHE = 'jwksCache'; public const SETTING_JWKS_CACHE_TIMESTAMP = 'jwksCacheTimestamp'; + private const BOOLEAN_SETTINGS = array( + self::SETTING_UNIQUE_UID, + self::SETTING_CHECK_BEARER, + self::SETTING_SEND_ID_TOKEN_HINT + ); /** @var IConfig */ private $config; @@ -127,12 +133,13 @@ private function getSupportedSettings(): array { self::SETTING_MAPPING_UID, self::SETTING_UNIQUE_UID, self::SETTING_CHECK_BEARER, + self::SETTING_SEND_ID_TOKEN_HINT, self::SETTING_EXTRA_CLAIMS, ]; } private function convertFromJSON(string $key, $value): string { - if ($key === self::SETTING_UNIQUE_UID || $key === self::SETTING_CHECK_BEARER) { + if (in_array($key, self::BOOLEAN_SETTINGS)) { $value = $value ? '1' : '0'; } return (string)$value; @@ -140,7 +147,7 @@ private function convertFromJSON(string $key, $value): string { private function convertToJSON(string $key, $value) { // default is disabled (if not set) - if ($key === self::SETTING_UNIQUE_UID || $key === self::SETTING_CHECK_BEARER) { + if (in_array($key, self::BOOLEAN_SETTINGS)) { return $value === '1'; } return (string)$value; diff --git a/src/components/AdminSettings.vue b/src/components/AdminSettings.vue index 800f3489..31087cde 100644 --- a/src/components/AdminSettings.vue +++ b/src/components/AdminSettings.vue @@ -143,6 +143,7 @@ export default { settings: { uniqueUid: true, checkBearer: false, + sendIdTokenHint: true, }, }, showNewProvider: false, diff --git a/src/components/SettingsForm.vue b/src/components/SettingsForm.vue index f28a3a8e..785d2b96 100644 --- a/src/components/SettingsForm.vue +++ b/src/components/SettingsForm.vue @@ -108,6 +108,12 @@

{{ t('user_oidc', 'Do you want to allow API calls and WebDav request that are authenticated with an OIDC ID token or access token?') }}

+ + {{ t('user_oidc', 'Send ID token hint on logout') }} + +

+ {{ t('user_oidc', 'Should the ID token be included as the id_token_hint GET parameter in the OpenID logout URL? Users are redirected to this URL after logging out of Nextcloud. Enabling this setting exposes the OIDC ID token to the user agent, which may not be necessary depending on the OIDC provider.') }} +

diff --git a/tests/unit/Service/ProviderServiceTest.php b/tests/unit/Service/ProviderServiceTest.php index 6a6eec3d..21ec7993 100644 --- a/tests/unit/Service/ProviderServiceTest.php +++ b/tests/unit/Service/ProviderServiceTest.php @@ -82,6 +82,7 @@ public function testGetProvidersWithSettings() { 'mappingUid' => '1', 'uniqueUid' => true, 'checkBearer' => true, + 'sendIdTokenHint' => true, 'extraClaims' => '1', ], ], @@ -98,6 +99,7 @@ public function testGetProvidersWithSettings() { 'mappingUid' => '1', 'uniqueUid' => true, 'checkBearer' => true, + 'sendIdTokenHint' => true, 'extraClaims' => '1', ], ], @@ -112,6 +114,7 @@ public function testSetSettings() { 'mappingUid' => 'uid', 'uniqueUid' => true, 'checkBearer' => false, + 'sendIdTokenHint' => true, 'extraClaims' => 'claim1 claim2', ]; $this->config->expects(self::any()) @@ -123,6 +126,7 @@ public function testSetSettings() { [Application::APP_ID, 'provider-1-' . ProviderService::SETTING_MAPPING_UID, '', 'uid'], [Application::APP_ID, 'provider-1-' . ProviderService::SETTING_UNIQUE_UID, '', '1'], [Application::APP_ID, 'provider-1-' . ProviderService::SETTING_CHECK_BEARER, '', '0'], + [Application::APP_ID, 'provider-1-' . ProviderService::SETTING_SEND_ID_TOKEN_HINT, '', '1'], [Application::APP_ID, 'provider-1-' . ProviderService::SETTING_EXTRA_CLAIMS, '', 'claim1 claim2'], ]); @@ -140,6 +144,11 @@ public function testSetSettings() { array_merge($defaults, ['checkBearer' => '1']), $this->providerService->setSettings(1, [ProviderService::SETTING_CHECK_BEARER => '1']) ); + + Assert::assertEquals( + array_merge($defaults, ['sendIdTokenHint' => '0']), + $this->providerService->setSettings(1, [ProviderService::SETTING_SEND_ID_TOKEN_HINT => '0']) + ); } public function testDeleteSettings() { @@ -188,6 +197,10 @@ public function dataConvertJson() { [ProviderService::SETTING_CHECK_BEARER, true, '1', true], [ProviderService::SETTING_CHECK_BEARER, false, '0', false], [ProviderService::SETTING_CHECK_BEARER, 'test', '1', true], + // Setting sendIdTokenHint is a boolean + [ProviderService::SETTING_SEND_ID_TOKEN_HINT, true, '1', true], + [ProviderService::SETTING_SEND_ID_TOKEN_HINT, false, '0', false], + [ProviderService::SETTING_SEND_ID_TOKEN_HINT, 'test', '1', true], // Any other values are just strings [ProviderService::SETTING_MAPPING_EMAIL, false, '', false], [ProviderService::SETTING_MAPPING_EMAIL, true, '1', true],