Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add client_id and id_token_hint to IdP logout #493

Merged
merged 1 commit into from
Sep 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/Command/UpsertProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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');
}
Expand Down
29 changes: 23 additions & 6 deletions lib/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());

Expand Down
11 changes: 9 additions & 2 deletions lib/Service/ProviderService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand Down Expand Up @@ -127,20 +133,21 @@ 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;
}

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;
Expand Down
1 change: 1 addition & 0 deletions src/components/AdminSettings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ export default {
settings: {
uniqueUid: true,
checkBearer: false,
sendIdTokenHint: true,
},
},
showNewProvider: false,
Expand Down
6 changes: 6 additions & 0 deletions src/components/SettingsForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@
<p class="settings-hint">
{{ t('user_oidc', 'Do you want to allow API calls and WebDav request that are authenticated with an OIDC ID token or access token?') }}
</p>
<CheckboxRadioSwitch :checked.sync="localProvider.settings.sendIdTokenHint" wrapper-element="div">
{{ t('user_oidc', 'Send ID token hint on logout') }}
</CheckboxRadioSwitch>
<p class="settings-hint">
{{ 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.') }}
</p>
<input type="button" :value="t('user_oidc', 'Cancel')" @click="$emit('cancel')">
<input type="submit" :value="submitText">
</form>
Expand Down
13 changes: 13 additions & 0 deletions tests/unit/Service/ProviderServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ public function testGetProvidersWithSettings() {
'mappingUid' => '1',
'uniqueUid' => true,
'checkBearer' => true,
'sendIdTokenHint' => true,
'extraClaims' => '1',
],
],
Expand All @@ -98,6 +99,7 @@ public function testGetProvidersWithSettings() {
'mappingUid' => '1',
'uniqueUid' => true,
'checkBearer' => true,
'sendIdTokenHint' => true,
'extraClaims' => '1',
],
],
Expand All @@ -112,6 +114,7 @@ public function testSetSettings() {
'mappingUid' => 'uid',
'uniqueUid' => true,
'checkBearer' => false,
'sendIdTokenHint' => true,
'extraClaims' => 'claim1 claim2',
];
$this->config->expects(self::any())
Expand All @@ -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'],
]);

Expand All @@ -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() {
Expand Down Expand Up @@ -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],
Expand Down