Skip to content

Commit

Permalink
Merge pull request #724 from nextcloud/fix/599/set-logout-url-manually
Browse files Browse the repository at this point in the history
Added customizeable end session endpoint to frontend and backend
  • Loading branch information
julien-nc authored Dec 13, 2023
2 parents efb066e + 9918c41 commit 8c36838
Show file tree
Hide file tree
Showing 10 changed files with 119 additions and 15 deletions.
13 changes: 7 additions & 6 deletions lib/Command/UpsertProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ protected function configure() {
->addOption('clientid', 'c', InputOption::VALUE_REQUIRED, 'OpenID client identifier')
->addOption('clientsecret', 's', InputOption::VALUE_REQUIRED, 'OpenID client secret')
->addOption('discoveryuri', 'd', InputOption::VALUE_REQUIRED, 'OpenID discovery endpoint uri')
->addOption('endsessionendpointuri', 'e', InputOption::VALUE_OPTIONAL, 'OpenID end session endpoint uri')

->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')
Expand Down Expand Up @@ -94,6 +95,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$clientsecret = $this->crypto->encrypt($clientsecret);
}
$discoveryuri = $input->getOption('discoveryuri');
$endsessionendpointuri = $input->getOption('endsessionendpointuri');
$scope = $input->getOption('scope');

if ($identifier === null) {
Expand All @@ -104,7 +106,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$updateOptions = array_filter($input->getOptions(), static function ($value, $option) {
return in_array($option, [
'identifier', 'clientid', 'clientsecret', 'discoveryuri',
'scope', 'unique-uid', 'check-bearer',
'scope', 'unique-uid', 'check-bearer', 'endsessionendpointuri',
'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota',
'extra-claims'
]) && $value !== null;
Expand All @@ -131,7 +133,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$provider['settings'][ProviderService::SETTING_UNIQUE_UID] = $provider['settings'][ProviderService::SETTING_UNIQUE_UID] ? '1' : '0';
$provider['settings'] = json_encode($provider['settings']);
$table = new Table($output);
$table->setHeaders(['ID', 'Identifier', 'Client ID', 'Discovery endpoint', 'Advanced settings']);
$table->setHeaders(['ID', 'Identifier', 'Client ID', 'Discovery endpoint', 'End session endpoint', 'Advanced settings']);
$table->addRow($provider);
$table->render();
return 0;
Expand All @@ -146,7 +148,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$scope = $scope ?? 'openid email profile';
}
try {
$provider = $this->providerMapper->createOrUpdateProvider($identifier, $clientid, $clientsecret, $discoveryuri, $scope);
$provider = $this->providerMapper->createOrUpdateProvider($identifier, $clientid, $clientsecret, $discoveryuri, $scope, $endsessionendpointuri);
// invalidate JWKS cache (even if it was just created)
$this->providerService->setSetting($provider->getId(), ProviderService::SETTING_JWKS_CACHE, '');
$this->providerService->setSetting($provider->getId(), ProviderService::SETTING_JWKS_CACHE_TIMESTAMP, '');
Expand Down Expand Up @@ -179,13 +181,11 @@ protected function execute(InputInterface $input, OutputInterface $output) {
if ($extraClaims = $input->getOption('extra-claims')) {
$this->providerService->setSetting($provider->getId(), ProviderService::SETTING_EXTRA_CLAIMS, $extraClaims);
}

return 0;
}

private function listProviders(InputInterface $input, OutputInterface $output) {
$outputFormat = $input->getOption('output') ?? 'table';

$providers = $this->providerMapper->getProviders();

if ($outputFormat === 'json') {
Expand All @@ -204,12 +204,13 @@ private function listProviders(InputInterface $input, OutputInterface $output) {
}

$table = new Table($output);
$table->setHeaders(['ID', 'Identifier', 'Discovery endpoint', 'Client ID']);
$table->setHeaders(['ID', 'Identifier', 'Discovery endpoint', 'End session endpoint', 'Client ID']);
$providers = array_map(function ($provider) {
return [
$provider->getId(),
$provider->getIdentifier(),
$provider->getDiscoveryEndpoint(),
$provider->getEndSessionEndpoint(),
$provider->getClientId()
];
}, $providers);
Expand Down
8 changes: 7 additions & 1 deletion lib/Controller/LoginController.php
Original file line number Diff line number Diff line change
Expand Up @@ -570,7 +570,13 @@ public function singleLogoutService() {
$message = $this->l10n->t('There is not such OpenID Connect provider.');
return $this->buildErrorTemplateResponse($message, Http::STATUS_NOT_FOUND, ['provider_id' => $providerId]);
}
$endSessionEndpoint = $this->discoveryService->obtainDiscovery($provider)['end_session_endpoint'];

// Check if a custom end_session_endpoint is deposited otherwise use the default one provided by the openid-configuration
$discoveryData = $this->discoveryService->obtainDiscovery($provider);
$defaultEndSessionEndpoint = $discoveryData['end_session_endpoint'];
$customEndSessionEndpoint = $provider->getEndSessionEndpoint();
$endSessionEndpoint = $customEndSessionEndpoint ?: $defaultEndSessionEndpoint;

if ($endSessionEndpoint) {
$endSessionEndpoint .= '?post_logout_redirect_uri=' . $targetUrl;
$endSessionEndpoint .= '&client_id=' . $provider->getClientId();
Expand Down
6 changes: 4 additions & 2 deletions lib/Controller/SettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public function __construct(
}

public function createProvider(string $identifier, string $clientId, string $clientSecret, string $discoveryEndpoint,
array $settings = [], string $scope = 'openid email profile'): JSONResponse {
array $settings = [], string $scope = 'openid email profile', ?string $endSessionEndpoint = null): JSONResponse {
if ($this->providerService->getProviderByIdentifier($identifier) !== null) {
return new JSONResponse(['message' => 'Provider with the given identifier already exists'], Http::STATUS_CONFLICT);
}
Expand All @@ -75,6 +75,7 @@ public function createProvider(string $identifier, string $clientId, string $cli
$encryptedClientSecret = $this->crypto->encrypt($clientSecret);
$provider->setClientSecret($encryptedClientSecret);
$provider->setDiscoveryEndpoint($discoveryEndpoint);
$provider->setEndSessionEndpoint($endSessionEndpoint ?: null);
$provider->setScope($scope);
$provider = $this->providerMapper->insert($provider);

Expand All @@ -84,7 +85,7 @@ public function createProvider(string $identifier, string $clientId, string $cli
}

public function updateProvider(int $providerId, string $identifier, string $clientId, string $discoveryEndpoint, string $clientSecret = null,
array $settings = [], string $scope = 'openid email profile'): JSONResponse {
array $settings = [], string $scope = 'openid email profile', ?string $endSessionEndpoint = null): JSONResponse {
$provider = $this->providerMapper->getProvider($providerId);

if ($this->providerService->getProviderByIdentifier($identifier) === null) {
Expand All @@ -98,6 +99,7 @@ public function updateProvider(int $providerId, string $identifier, string $clie
$provider->setClientSecret($encryptedClientSecret);
}
$provider->setDiscoveryEndpoint($discoveryEndpoint);
$provider->setEndSessionEndpoint($endSessionEndpoint ?: null);
$provider->setScope($scope);
$provider = $this->providerMapper->update($provider);

Expand Down
6 changes: 6 additions & 0 deletions lib/Db/Provider.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
* @method void setClientSecret(string $clientSecret)
* @method string getDiscoveryEndpoint()
* @method void setDiscoveryEndpoint(string $discoveryEndpoint)
* @method string getEndSessionEndpoint()
* @method void setEndSessionEndpoint(string $endSessionEndpoint)
* @method void setScope(string $scope)
*/
class Provider extends Entity implements \JsonSerializable {
Expand All @@ -52,6 +54,9 @@ class Provider extends Entity implements \JsonSerializable {
/** @var string */
protected $discoveryEndpoint;

/** @var string */
protected $endSessionEndpoint;

/** @var string */
protected $scope;

Expand All @@ -69,6 +74,7 @@ public function jsonSerialize() {
'identifier' => $this->identifier,
'clientId' => $this->clientId,
'discoveryEndpoint' => $this->discoveryEndpoint,
'endSessionEndpoint' => $this->endSessionEndpoint,
'scope' => trim($this->getScope()),
];
}
Expand Down
22 changes: 16 additions & 6 deletions lib/Db/ProviderMapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@

namespace OCA\UserOIDC\Db;

use OCP\AppFramework\Db\Entity;
use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\AppFramework\Db\QBMapper;
use OCP\DB\Exception;
use OCP\IDBConnection;

use OCP\AppFramework\Db\DoesNotExistException;
Expand Down Expand Up @@ -88,17 +91,20 @@ public function getProviders() {
/**
* Create or update provider settings
*
* @param string identifier
* @param string $identifier
* @param string|null $clientid
* @param string|null $clientsecret
* @param string|null $discoveryuri
* @param string scope
* @throws \OCP\AppFramework\Db\DoesNotExistException
* @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException
* @param string $scope
* @param string|null $endsessionendpointuri
* @return Provider|Entity
* @throws DoesNotExistException
* @throws MultipleObjectsReturnedException
* @throws Exception
*/
public function createOrUpdateProvider(string $identifier, string $clientid = null,
string $clientsecret = null, string $discoveryuri = null,
string $scope = 'openid email profile') {
string $clientsecret = null, string $discoveryuri = null, string $scope = 'openid email profile',
string $endsessionendpointuri = null) {
try {
$provider = $this->findProviderByIdentifier($identifier);
} catch (DoesNotExistException $eNotExist) {
Expand All @@ -114,6 +120,7 @@ public function createOrUpdateProvider(string $identifier, string $clientid = nu
$provider->setClientId($clientid);
$provider->setClientSecret($clientsecret);
$provider->setDiscoveryEndpoint($discoveryuri);
$provider->setEndSessionEndpoint($endsessionendpointuri);
$provider->setScope($scope);
return $this->insert($provider);
} else {
Expand All @@ -126,6 +133,9 @@ public function createOrUpdateProvider(string $identifier, string $clientid = nu
if ($discoveryuri !== null) {
$provider->setDiscoveryEndpoint($discoveryuri);
}
if ($endsessionendpointuri !== null) {
$provider->setEndSessionEndpoint($endsessionendpointuri ?: null);
}
$provider->setScope($scope);
return $this->update($provider);
}
Expand Down
62 changes: 62 additions & 0 deletions lib/Migration/Version010304Date20231130104459.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

declare(strict_types=1);

/**
* @copyright Copyright (c) 2023 Your name <[email protected]>
*
* @author Your name <[email protected]>
*
* @license GNU AGPL version 3 or any later version
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as
* published by the Free Software Foundation, either version 3 of the
* License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

namespace OCA\UserOIDC\Migration;

use Closure;
use OCP\DB\ISchemaWrapper;
use OCP\Migration\IOutput;
use OCP\Migration\SimpleMigrationStep;
use OCP\DB\Types;

/**
* Auto-generated migration step: Please modify to your needs!
*/
class Version010304Date20231130104459 extends SimpleMigrationStep {
/**
* @param IOutput $output
* @param Closure(): ISchemaWrapper $schemaClosure
* @param array $options
* @return null|ISchemaWrapper
*/
public function changeSchema(IOutput $output, Closure $schemaClosure, array $options): ?ISchemaWrapper {
/** @var ISchemaWrapper $schema */
$schema = $schemaClosure();

if ($schema->hasTable('user_oidc_providers')) {
$table = $schema->getTable('user_oidc_providers');
if (!$table->hasColumn('end_session_endpoint')) {
$table->addColumn('end_session_endpoint', Types::STRING, [
'notnull' => false,
'length' => 255,
]);
return $schema;
}
}

return null;
}
}
1 change: 1 addition & 0 deletions lib/Service/DiscoveryService.php
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ public function obtainDiscovery(Provider $provider): array {
$client = $this->clientService->newClient();
$response = $client->get($url);
$cachedDiscovery = $response->getBody();

$this->cache->set($cacheKey, $cachedDiscovery, self::INVALIDATE_DISCOVERY_CACHE_AFTER_SECONDS);
}

Expand Down
2 changes: 2 additions & 0 deletions src/components/AdminSettings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ export default {
clientId: '',
clientSecret: '',
discoveryEndpoint: '',
endSessionEndpoint: '',
settings: {
uniqueUid: true,
checkBearer: false,
Expand Down Expand Up @@ -246,6 +247,7 @@ export default {
this.newProvider.clientId = ''
this.newProvider.clientSecret = ''
this.newProvider.discoveryEndpoint = ''
this.newProvider.endSessionEndpoint = ''
this.showNewProvider = false
} catch (error) {
logger.error('Could not register a provider: ' + error.message, { error })
Expand Down
12 changes: 12 additions & 0 deletions src/components/SettingsForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,15 @@
type="text"
required>
</p>
<p>
<label for="oidc-end-session-endpoint">{{ t('user_oidc', 'Custom end session endpoint') }}</label>
<input id="oidc-end-session-endpoint"
v-model="localProvider.endSessionEndpoint"
class="italic-placeholder"
type="text"
maxlength="255"
placeholder="(Optional)">
</p>
<p>
<label for="oidc-scope">{{ t('user_oidc', 'Scope') }}</label>
<input id="oidc-scope"
Expand Down Expand Up @@ -240,6 +249,9 @@ export default {
min-width: 200px;
flex-grow: 1;
}
.italic-placeholder::placeholder {
font-style: italic;
}
}
}
</style>
2 changes: 2 additions & 0 deletions tests/unit/Service/ProviderServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public function testGetProvidersWithSettings() {
'identifier' => null,
'clientId' => null,
'discoveryEndpoint' => null,
'endSessionEndpoint' => null,
'scope' => null,
'settings' => [
'mappingDisplayName' => '1',
Expand All @@ -95,6 +96,7 @@ public function testGetProvidersWithSettings() {
'identifier' => null,
'clientId' => null,
'discoveryEndpoint' => null,
'endSessionEndpoint' => null,
'scope' => null,
'settings' => [
'mappingDisplayName' => '1',
Expand Down

0 comments on commit 8c36838

Please sign in to comment.