From be6a8e97fe8e52817e53c3493c1b55dff8aaa916 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 9 Nov 2021 02:45:45 +0100 Subject: [PATCH 01/13] Move SAML configurations to a table of their own - adds user_saml_configurations table and migrates existing configuration - Controller methods are added since appconfig endpoints cannot be used anymore. THIS IS A BREAKING CHANGE. - Frontend code is adjusted to use new endpoints. - security-sloWebServerDecode was changed from global to provider specific setting. It being global seemed to be unintended. A migration path is yet missing. Signed-off-by: Arthur Schiwon --- appinfo/app.php | 8 +- appinfo/info.xml | 2 +- appinfo/routes.php | 20 +- js/admin.js | 123 ++++++---- lib/AppInfo/Application.php | 4 +- lib/Command/GetMetadata.php | 3 +- lib/Controller/SAMLController.php | 44 ++-- lib/Controller/SettingsController.php | 73 +++--- lib/DavPlugin.php | 6 +- lib/Db/ConfigurationsEntity.php | 71 ++++++ lib/Db/ConfigurationsMapper.php | 102 ++++++++ .../Version5000Date20211025124248.php | 218 +++++++++++++++++ lib/SAMLSettings.php | 231 +++++++++++------- lib/Settings/Admin.php | 39 +-- lib/UserBackend.php | 63 ++--- lib/UserData.php | 33 +-- tests/unit/AppInfo/RoutesTest.php | 104 -------- tests/unit/Command/GetMetadataTest.php | 91 +++---- tests/unit/Settings/AdminTest.php | 71 ++---- tests/unit/phpunit.xml | 5 - 20 files changed, 841 insertions(+), 470 deletions(-) create mode 100644 lib/Db/ConfigurationsEntity.php create mode 100644 lib/Db/ConfigurationsMapper.php create mode 100644 lib/Migration/Version5000Date20211025124248.php delete mode 100644 tests/unit/AppInfo/RoutesTest.php diff --git a/appinfo/app.php b/appinfo/app.php index 5b3ec6e..d1d3732 100644 --- a/appinfo/app.php +++ b/appinfo/app.php @@ -38,12 +38,8 @@ try { \OC::$server->getLogger()->logException($e); return; } -$samlSettings = new \OCA\User_SAML\SAMLSettings( - $urlGenerator, - $config, - $request, - $session -); + +$samlSettings = \OC::$server->query(\OCA\User_SAML\SAMLSettings::class); $userData = new \OCA\User_SAML\UserData( new \OCA\User_SAML\UserResolver(\OC::$server->getUserManager()), diff --git a/appinfo/info.xml b/appinfo/info.xml index 2e3e91e..87211ae 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -16,7 +16,7 @@ The following providers are supported and tested at the moment: * Any other provider that authenticates using the environment variable While theoretically any other authentication provider implementing either one of those standards is compatible, we like to note that they are not part of any internal test matrix.]]> - 4.3.0 + 5.0.0 agpl Lukas Reschke User_SAML diff --git a/appinfo/routes.php b/appinfo/routes.php index 63e5ca9..3cb584c 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -69,14 +69,32 @@ return [ 'url' => '/saml/selectUserBackEnd', 'verb' => 'GET', ], + [ + 'name' => 'Settings#getSamlProviderIds', + 'url' => '/settings/providers', + 'verb' => 'GET', + ], [ 'name' => 'Settings#getSamlProviderSettings', 'url' => '/settings/providerSettings/{providerId}', 'verb' => 'GET', 'defaults' => [ - 'providerId' => '1' + 'providerId' => 1 ] ], + [ + 'name' => 'Settings#setProviderSetting', + 'url' => '/settings/providerSettings/{providerId}', + 'verb' => 'POST', + 'defaults' => [ + 'providerId' => 1 + ] + ], + [ + 'name' => 'Settings#newSamlProviderSettingsId', + 'url' => '/settings/providerSettings', + 'verb' => 'PUT', + ], [ 'name' => 'Settings#deleteSamlProviderSettings', 'url' => '/settings/providerSettings/{providerId}', diff --git a/js/admin.js b/js/admin.js index 71301fc..2732e54 100644 --- a/js/admin.js +++ b/js/admin.js @@ -8,20 +8,30 @@ currentConfig: '1', providerIds: '1', - _getAppConfig: function (key) { - return $.ajax({ - type: 'GET', - url: OC.linkToOCS('apps/provisioning_api/api/v1', 2) + 'config/apps' + '/user_saml/' + key + '?format=json' - }); - }, init: function(callback) { - this._getAppConfig('providerIds').done(function (data){ - if (data.ocs.data.data !== '') { - OCA.User_SAML.Admin.providerIds = data.ocs.data.data; - OCA.User_SAML.Admin.currentConfig = OCA.User_SAML.Admin.providerIds.split(',').sort()[0]; + var xhr = new XMLHttpRequest(); + xhr.open('GET', OC.generateUrl('/apps/user_saml/settings/providers')); + xhr.setRequestHeader('Content-Type', 'application/json'); + xhr.setRequestHeader('requesttoken', OC.requestToken); + + xhr.onload = function () { + var response = JSON.parse(xhr.response); + if (xhr.status >= 200 && xhr.status < 300) { + if (response.providerIds !== "") { + OCA.User_SAML.Admin.providerIds += ',' + response.providerIds; + OCA.User_SAML.Admin.currentConfig = OCA.User_SAML.Admin.providerIds.split(',').sort()[0]; + } + callback(); + } else { + console.error("Could not fetch new provider ID"); } - callback(); - }); + }; + xhr.onerror = function () { + console.error("Could not fetch new provider ID"); + } + + + xhr.send(); }, chooseEnv: function() { if (OC.PasswordConfirmation.requiresPasswordConfirmation()) { @@ -60,23 +70,49 @@ /** * Add a new provider - * @returns {number} id of the provider */ - addProvider: function(callback) { - var providerIds = OCA.User_SAML.Admin.providerIds.split(','); - var nextId = 1; - if (providerIds.indexOf('1') >= 0) { - nextId = 2; - while ($.inArray('' + nextId, providerIds) >= 0) { - nextId++; + addProvider: function (callback) { + var xhr = new XMLHttpRequest(); + xhr.open('PUT', OC.generateUrl('/apps/user_saml/settings/providerSettings')); + xhr.setRequestHeader('Content-Type', 'application/json') + xhr.setRequestHeader('requesttoken', OC.requestToken) + + xhr.onload = function () { + var response = JSON.parse(xhr.response) + if (xhr.status >= 200 && xhr.status < 300) { + OCA.User_SAML.Admin.providerIds += ',' + response.id; + callback(response.id) + } else { + console.error("Could not fetch new provider ID") } - } - OCP.AppConfig.setValue('user_saml', 'providerIds', OCA.User_SAML.Admin.providerIds + ',' + nextId, { - success: function () { - OCA.User_SAML.Admin.providerIds += ',' + nextId; - callback(nextId) + }; + xhr.onerror = function () { + console.error("Could not fetch new provider ID"); + }; + + xhr.send(); + }, + + updateProvider: function (configKey, configValue, successCb, errorCb) { + var xhr = new XMLHttpRequest(); + xhr.open('POST', OC.generateUrl('/apps/user_saml/settings/providerSettings/' + this.currentConfig)); + xhr.setRequestHeader('Content-Type', 'application/json'); + xhr.setRequestHeader('requesttoken', OC.requestToken); + + xhr.onload = function () { + if (xhr.status >= 200 && xhr.status < 300) { + successCb(); + } else { + console.error("Could not update config"); + errorCb(); } - }); + }; + xhr.onerror = function () { + console.error("Could not update config"); + errorCb(); + }; + + xhr.send(JSON.stringify({configKey: configKey, configValue: configValue.trim()})); }, removeProvider: function(callback) { @@ -86,17 +122,8 @@ if (index > -1) { providerIds.splice(index, 1); } - var config = this.currentConfig; $.ajax({ url: OC.generateUrl('/apps/user_saml/settings/providerSettings/' + this.currentConfig), type: 'DELETE'}) - .done(function(data) { - OCP.AppConfig.setValue('user_saml', 'providerIds', providerIds.join(','), { - success: function () { - OCA.User_SAML.Admin.providerIds = providerIds.join(','); - callback(config); - } - }); - }); - + .done(callback(this.currentConfig)); } }, @@ -105,14 +132,22 @@ OC.PasswordConfirmation.requirePasswordConfirmation(_.bind(this.setSamlConfigValue, this, category, setting, value)); return; } - // store global config flags without idp prefix - var configIdentifier = this.getConfigIdentifier(); - if (global === true) { - configIdentifier = ''; - } OC.msg.startSaving('#user-saml-save-indicator'); - OCP.AppConfig.setValue('user_saml', configIdentifier + category + '-' + setting, value.trim()); - OC.msg.finishedSaving('#user-saml-save-indicator', {status: 'success', data: {message: t('user_saml', 'Saved')}}); + + var callbacks = { + success: function () { + OC.msg.finishedSaving('#user-saml-save-indicator', {status: 'success', data: {message: t('user_saml', 'Saved')}}); + }, + error: function() { + OC.msg.finishedSaving('#user-saml-save-indicator', {status: 'error', data: {message: t('user_saml', 'Could not save')}}); + } + }; + + if (global) { + OCP.AppConfig.setValue('user_saml', category + '-' + setting, value, callbacks); + return; + } + this.updateProvider(category + '-' + setting, value, callbacks.success, callbacks.error); } } })(OCA); @@ -200,7 +235,7 @@ $(function() { }); }); $('input:checkbox[value="1"]').attr('checked', true); - $('input:checkbox[value="0"]').attr('checked', false); + $('input:checkbox[value="0"]').removeAttr('checked', false); var xmlDownloadButton = $('#get-metadata'); var url = xmlDownloadButton.data('base') + '?idp=' + providerId; xmlDownloadButton.attr('href', url); diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 975a095..7ae16d2 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -23,6 +23,7 @@ namespace OCA\User_SAML\AppInfo; use OCA\User_SAML\DavPlugin; use OCA\User_SAML\Middleware\OnlyLoggedInMiddleware; +use OCA\User_SAML\SAMLSettings; use OCP\AppFramework\App; use OCP\AppFramework\IAppContainer; use OCP\SabrePluginEvent; @@ -48,7 +49,8 @@ class Application extends App { return new DavPlugin( $server->getSession(), $server->getConfig(), - $_SERVER + $_SERVER, + $server->get(SAMLSettings::class) ); }); diff --git a/lib/Command/GetMetadata.php b/lib/Command/GetMetadata.php index f3a85e6..a137f3b 100644 --- a/lib/Command/GetMetadata.php +++ b/lib/Command/GetMetadata.php @@ -26,6 +26,7 @@ use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; use OCA\User_SAML\SAMLSettings; +use OneLogin\Saml2\Error; use OneLogin\Saml2\Settings; class GetMetadata extends Command { @@ -67,7 +68,7 @@ EOT * @return void */ protected function execute(InputInterface $input, OutputInterface $output) { - $idp = $input->getArgument('idp'); + $idp = (int)$input->getArgument('idp'); $settings = new Settings($this->SAMLSettings->getOneLoginSettingsArray($idp)); $metadata = $settings->getSPMetadata(); $errors = $settings->validateMetadata($metadata); diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index d3f8573..ab3609c 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -51,7 +51,7 @@ class SAMLController extends Controller { /** @var IUserSession */ private $userSession; /** @var SAMLSettings */ - private $SAMLSettings; + private $samlSettings; /** @var UserBackend */ private $userBackend; /** @var IConfig */ @@ -76,7 +76,7 @@ class SAMLController extends Controller { * @param IRequest $request * @param ISession $session * @param IUserSession $userSession - * @param SAMLSettings $SAMLSettings + * @param SAMLSettings $samlSettings * @param UserBackend $userBackend * @param IConfig $config * @param IURLGenerator $urlGenerator @@ -84,24 +84,24 @@ class SAMLController extends Controller { * @param IL10N $l */ public function __construct( - $appName, - IRequest $request, - ISession $session, - IUserSession $userSession, - SAMLSettings $SAMLSettings, - UserBackend $userBackend, - IConfig $config, + $appName, + IRequest $request, + ISession $session, + IUserSession $userSession, + SAMLSettings $samlSettings, + UserBackend $userBackend, + IConfig $config, IURLGenerator $urlGenerator, - ILogger $logger, - IL10N $l, - UserResolver $userResolver, - UserData $userData, - ICrypto $crypto + ILogger $logger, + IL10N $l, + UserResolver $userResolver, + UserData $userData, + ICrypto $crypto ) { parent::__construct($appName, $request); $this->session = $session; $this->userSession = $userSession; - $this->SAMLSettings = $SAMLSettings; + $this->samlSettings = $samlSettings; $this->userBackend = $userBackend; $this->config = $config; $this->urlGenerator = $urlGenerator; @@ -171,7 +171,7 @@ class SAMLController extends Controller { $type = $this->config->getAppValue($this->appName, 'type'); switch ($type) { case 'saml': - $auth = new Auth($this->SAMLSettings->getOneLoginSettingsArray($idp)); + $auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp)); $ssoUrl = $auth->login(null, [], false, false, true); $response = new Http\RedirectResponse($ssoUrl); @@ -242,7 +242,7 @@ class SAMLController extends Controller { * @throws Error */ public function getMetadata($idp) { - $settings = new Settings($this->SAMLSettings->getOneLoginSettingsArray($idp)); + $settings = new Settings($this->samlSettings->getOneLoginSettingsArray($idp)); $metadata = $settings->getSPMetadata(); $errors = $settings->validateMetadata($metadata); if (empty($errors)) { @@ -304,7 +304,7 @@ class SAMLController extends Controller { return new Http\RedirectResponse($this->urlGenerator->getAbsoluteURL('/')); } - $auth = new Auth($this->SAMLSettings->getOneLoginSettingsArray($idp)); + $auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp)); $auth->processResponse($AuthNRequestID); $this->logger->debug('Attributes send by the IDP: ' . json_encode($auth->getAttributes())); @@ -411,14 +411,14 @@ class SAMLController extends Controller { if ($pass) { $idp = $this->session->get('user_saml.Idp'); - $auth = new Auth($this->SAMLSettings->getOneLoginSettingsArray($idp)); + $auth = new Auth($this->samlSettings->getOneLoginSettingsArray($idp)); $stay = true ; // $auth will return the redirect URL but won't perform the redirect himself if ($isFromIDP) { $keepLocalSession = true ; // do not let processSLO to delete the entire session. Let userSession->logout do the job $targetUrl = $auth->processSLO( $keepLocalSession, null, - $this->SAMLSettings->usesSloWebServerDecode(), + $this->samlSettings->usesSloWebServerDecode(), null, $stay ); @@ -490,7 +490,7 @@ class SAMLController extends Controller { public function selectUserBackEnd($redirectUrl) { $attributes = ['loginUrls' => []]; - if ($this->SAMLSettings->allowMultipleUserBackEnds()) { + if ($this->samlSettings->allowMultipleUserBackEnds()) { $displayName = $this->l->t('Direct log in'); $customDisplayName = $this->config->getAppValue('user_saml', 'directLoginName', ''); @@ -520,7 +520,7 @@ class SAMLController extends Controller { */ private function getIdps($redirectUrl) { $result = []; - $idps = $this->SAMLSettings->getListOfIdps(); + $idps = $this->samlSettings->getListOfIdps(); foreach ($idps as $idpId => $displayName) { $result[] = [ 'url' => $this->getSSOUrl($redirectUrl, $idpId), diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 58f2361..20e71b3 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -23,8 +23,10 @@ namespace OCA\User_SAML\Controller; +use OCA\User_SAML\SAMLSettings; use OCA\User_SAML\Settings\Admin; use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\Response; use OCP\IConfig; use OCP\IRequest; @@ -35,21 +37,32 @@ class SettingsController extends Controller { private $config; /** @var Admin */ private $admin; + /** @var SAMLSettings */ + private $samlSettings; - public function __construct($appName, - IRequest $request, - IConfig $config, - Admin $admin) { + public function __construct( + $appName, + IRequest $request, + IConfig $config, + Admin $admin, + SAMLSettings $samlSettings + ) { parent::__construct($appName, $request); $this->config = $config; $this->admin = $admin; + $this->samlSettings = $samlSettings; + } + + public function getSamlProviderIds(): DataResponse { + $keys = array_keys($this->samlSettings->getListOfIdps()); + return new DataResponse([ 'providerIds' => implode(',', $keys)]); } /** * @param $providerId * @return array of categories containing entries for each config parameter with their value */ - public function getSamlProviderSettings($providerId) { + public function getSamlProviderSettings(int $providerId) { /** * This uses the list of available config parameters from the admin section * and extends it with fields that are not coming from \OCA\User_SAML\Settings\Admin @@ -64,12 +77,12 @@ class SettingsController extends Controller { ]; /* Fetch all config values for the given providerId */ $settings = []; + $storedSettings = $this->samlSettings->get($providerId); foreach ($params as $category => $content) { if (empty($content) || $category === 'providers' || $category === 'type') { continue; } foreach ($content as $setting => $details) { - $prefix = $providerId === '1' ? '' : $providerId . '-'; /* use security as category instead of security-* */ if (strpos($category, 'security-') === 0) { $category = 'security'; @@ -78,42 +91,34 @@ class SettingsController extends Controller { // as this is the only category that has the saml- prefix on config keys if (strpos($category, 'attribute-mapping') === 0) { $category = 'attribute-mapping'; - $key = $prefix . 'saml-attribute-mapping' . '-' . $setting; + $key = 'saml-attribute-mapping' . '-' . $setting; } else { - $key = $prefix . $category . '-' . $setting; + $key = $category . '-' . $setting; + } + + if (isset ($details['global']) && $details['global']) { + $settings[$category][$setting] = $this->config->getAppValue('user_saml', $key, ''); + } else { + $settings[$category][$setting] = $storedSettings[$key] ?? ''; } - $settings[$category][$setting] = $this->config->getAppValue('user_saml', $key, ''); } } return $settings; } public function deleteSamlProviderSettings($providerId) { - $params = $this->admin->getForm()->getParams(); - $params['idp'] = [ - 'singleLogoutService.url' => null, - 'singleLogoutService.responseUrl' => null, - 'singleSignOnService.url' => null, - 'idp-entityId' => null, - ]; - /* Fetch all config values for the given providerId */ - foreach ($params as $category => $content) { - if (!is_array($content) || $category === 'providers') { - continue; - } - foreach ($content as $setting => $details) { - if (isset($details['global']) && $details['global'] === true) { - continue; - } - $prefix = $providerId === '1' ? '' : $providerId . '-'; - $key = $prefix . $category . '-' . $setting; - /* use security as category instead of security-* */ - if (strpos($category, 'security-') === 0) { - $category = 'security'; - } - $this->config->deleteAppValue('user_saml', $key); - } - } + $this->samlSettings->delete($providerId); return new Response(); } + + public function setProviderSetting(int $providerId, string $configKey, string $configValue) { + $configuration = $this->samlSettings->get($providerId); + $configuration[$configKey] = $configValue; + $this->samlSettings->set($providerId, $configuration); + return new Response(); + } + + public function newSamlProviderSettingsId() { + return new DataResponse(['id' => $this->samlSettings->getNewProviderId()]); + } } diff --git a/lib/DavPlugin.php b/lib/DavPlugin.php index 59285c4..ebb18a4 100644 --- a/lib/DavPlugin.php +++ b/lib/DavPlugin.php @@ -35,11 +35,13 @@ class DavPlugin extends ServerPlugin { private $auth; /** @var Server */ private $server; + private SAMLSettings $samlSettings; - public function __construct(ISession $session, IConfig $config, array $auth) { + public function __construct(ISession $session, IConfig $config, array $auth, SAMLSettings $samlSettings) { $this->session = $session; $this->config = $config; $this->auth = $auth; + $this->samlSettings = $samlSettings; } @@ -54,7 +56,7 @@ class DavPlugin extends ServerPlugin { $this->config->getAppValue('user_saml', 'type') === 'environment-variable' && !$this->session->exists('user_saml.samlUserData') ) { - $uidMapping = $this->config->getAppValue('user_saml', 'general-uid_mapping'); + $uidMapping = $this->samlSettings->get(1)['general-uid_mapping']; if (isset($this->auth[$uidMapping])) { $this->session->set(Auth::DAV_AUTHENTICATED, $this->auth[$uidMapping]); $this->session->set('user_saml.samlUserData', $this->auth); diff --git a/lib/Db/ConfigurationsEntity.php b/lib/Db/ConfigurationsEntity.php new file mode 100644 index 0000000..fb73457 --- /dev/null +++ b/lib/Db/ConfigurationsEntity.php @@ -0,0 +1,71 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\User_SAML\Db; + +use OCP\AppFramework\Db\Entity; +use function json_decode; +use function json_encode; + +/** + * @method string getName() + * @method void setName(string $value) + * @method void setConfiguration(string $value) + * @method string getConfiguration() + */ +class ConfigurationsEntity extends Entity { + /** @var string */ + public $name; + /** @var string */ + public $configuration; + + public function __construct() { + $this->addType('name', 'string'); + $this->addType('configuration', 'string'); + } + + /** + * sets also the name, because it is a shorthand to 'general-idp0_display_name' + * + * @throws \JsonException + */ + public function importConfiguration(array $configuration): void { + $this->setConfiguration(json_encode($configuration, JSON_THROW_ON_ERROR)); + $this->setName($configuration['general-idp0_display_name'] ?? ''); + } + + public function getConfigurationArray(): array { + return json_decode($this->configuration, true) ?? []; + } + + public function asArray(): array { + return [ + 'id' => $this->getId(), + 'name' => $this->getName(), + 'configuration' => $this->getConfigurationArray() + ]; + } +} diff --git a/lib/Db/ConfigurationsMapper.php b/lib/Db/ConfigurationsMapper.php new file mode 100644 index 0000000..c1a6bee --- /dev/null +++ b/lib/Db/ConfigurationsMapper.php @@ -0,0 +1,102 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\User_SAML\Db; + +use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Db\QBMapper; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\IDBConnection; + +class ConfigurationsMapper extends QBMapper { + public function __construct(IDBConnection $db) { + parent::__construct($db, 'user_saml_configurations', ConfigurationsEntity::class); + } + + public function set(int $id, array $configuration): void { + $entity = new ConfigurationsEntity(); + $entity->setId($id); + $entity->importConfiguration($configuration); + $this->insertOrUpdate($entity); + } + + public function deleteById(int $id): void { + $entity = new ConfigurationsEntity(); + $entity->setId($id);; + $this->delete($entity); + } + + public function getAll(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'configuration') + ->from('user_saml_configurations') + ->orderBy('id', 'ASC'); + + /** @var ConfigurationsEntity $entity */ + $entities = $this->findEntities($qb); + $result = []; + foreach ($entities as $entity) { + $result[$entity->getId()] = $entity->getConfigurationArray(); + } + return $result; + } + + public function get(int $idp): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'configuration') + ->from('user_saml_configurations') + ->where($qb->expr()->eq('id', $qb->createNamedParameter($idp, IQueryBuilder::PARAM_INT))); + + /** @var ConfigurationsEntity $entity */ + try { + $entity = $this->findEntity($qb); + } catch (DoesNotExistException $e) { + return []; + } + return $entity->getConfigurationArray(); + } + + public function reserveId(): int { + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from('user_saml_configurations') + ->orderBy('id', 'DESC') + ->setMaxResults(1); + + try { + $entity = $this->findEntity($qb); + $newId = $entity->getId() + 1; + } catch (DoesNotExistException $e) { + $newId = 1; + } + + $newEntity = new ConfigurationsEntity(); + $newEntity->setId($newId); + $newEntity->importConfiguration([]); + return $this->insert($newEntity)->getId(); + } + +} diff --git a/lib/Migration/Version5000Date20211025124248.php b/lib/Migration/Version5000Date20211025124248.php new file mode 100644 index 0000000..887e965 --- /dev/null +++ b/lib/Migration/Version5000Date20211025124248.php @@ -0,0 +1,218 @@ +dbc = $dbc; + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + */ + public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @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_saml_configurations')) { + $table = $schema->createTable('user_saml_configurations'); + $table->addColumn('id', Types::INTEGER, [ + 'notnull' => true, + ]); + $table->addColumn('name', Types::STRING, [ + 'length' => 256, + 'notnull' => false, + 'default' => '', + ]); + $table->addColumn('configuration', Types::TEXT, [ + 'notnull' => true, + ]); + $table->setPrimaryKey(['id'], 'idx_user_saml_config'); + } + + return $schema; + } + + /** + * @param IOutput $output + * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + */ + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $prefixes = $this->fetchPrefixes(); + foreach ($prefixes as $prefix) { + $keyStart = $prefix === 1 ? '' : $prefix . '-'; + $configKeys = array_reduce( + self::IDP_CONFIG_KEYS, + function (array $carry, string $rawConfigKey) use ($keyStart): array { + $carry[] = $keyStart . $rawConfigKey; + return $carry; + }, + [] + ); + + $configData = $keysToDelete = []; + $gRows = $this->readConfiguration($configKeys); + while ($row = $gRows->current()) { + $configData[$this->normalizeKey($row['configkey'])] = $row['configvalue']; + $keysToDelete[] = $row['configkey']; + $gRows->next(); + } + + if ($this->insertConfiguration($prefix, $configData) && !empty($keysToDelete)) { + $this->deleteOldConfiguration($keysToDelete); + } + } + + $this->deletePrefixes(); + } + + protected function deleteOldConfiguration(array $keys): bool { + static $deleteQuery; + if (!$deleteQuery) { + $deleteQuery = $this->dbc->getQueryBuilder(); + + $deleteQuery->delete('appconfig') + ->where($deleteQuery->expr()->eq('appid', $deleteQuery->createNamedParameter('user_saml'))) + ->andWhere($deleteQuery->expr()->in('configkey', $deleteQuery->createParameter('cfgKeys'))); + } + + $deletedRows = $deleteQuery + ->setParameter('cfgKeys', $keys, IQueryBuilder::PARAM_STR_ARRAY) + ->executeStatement(); + + return $deletedRows > 0; + } + + protected function insertConfiguration(int $id, array $configData): bool { + static $insertQuery; + if (!$insertQuery) { + $insertQuery = $this->dbc->getQueryBuilder(); + $insertQuery->insert('user_saml_configurations') + ->values([ + 'id' => $insertQuery->createParameter('configId'), + 'name' => $insertQuery->createParameter('displayName'), + 'configuration' => $insertQuery->createParameter('configuration'), + ]); + } + + $insertedRows = $insertQuery + ->setParameter('configId', $id) + ->setParameter('displayName', $configData['general-idp0_display_name'] ?? '') + ->setParameter('configuration', \json_encode($configData, JSON_THROW_ON_ERROR)) + ->executeStatement(); + + return $insertedRows > 0; + } + + protected function readConfiguration(array $configKeys): \Generator { + static $readQuery; + if (!$readQuery) { + $readQuery = $this->dbc->getQueryBuilder(); + } + $readQuery->select('configkey', 'configvalue') + ->from('appconfig') + ->where($readQuery->expr()->eq('appid', $readQuery->createNamedParameter('user_saml'))) + ->andWhere($readQuery->expr()->in('configkey', $readQuery->createParameter('cfgKeys'))); + + $r = $readQuery->setParameter('cfgKeys', $configKeys, IQueryBuilder::PARAM_STR_ARRAY) + ->executeQuery(); + + while ($row = $r->fetch()) { + yield $row; + } + $r->closeCursor(); + } + + protected function normalizeKey(string $prefixedKey): string { + $isPrefixed = \preg_match('/^[0-9]*-/', $prefixedKey, $matches); + if ($isPrefixed === 0) { + return $prefixedKey; + } else if ($isPrefixed === 1) { + return \substr($prefixedKey, strlen($matches[0])); + } + throw new \RuntimeException('Invalid regex pattern'); + } + + protected function fetchPrefixes(): array { + $q = $this->dbc->getQueryBuilder(); + $q->select('configvalue') + ->from('appconfig') + ->where($q->expr()->eq('appid', $q->createNamedParameter('user_saml'))) + ->andWhere($q->expr()->eq('configkey', $q->createNamedParameter('providerIds'))); + + $r = $q->executeQuery(); + $prefixes = $r->fetchOne(); + if ($prefixes === false) { + return []; + } + return array_map('intval', explode(',', $prefixes)); + } + + protected function deletePrefixes(): void { + $q = $this->dbc->getQueryBuilder(); + $q->delete('appconfig') + ->where($q->expr()->eq('appid', $q->createNamedParameter('user_saml'))) + ->andWhere($q->expr()->eq('configkey', $q->createNamedParameter('providerIds'))) + ->executeStatement(); + } +} diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php index efc35bb..4c27c90 100644 --- a/lib/SAMLSettings.php +++ b/lib/SAMLSettings.php @@ -21,6 +21,9 @@ namespace OCA\User_SAML; +use InvalidArgumentException; +use OCA\User_SAML\Db\ConfigurationsMapper; +use OCP\DB\Exception; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; @@ -28,16 +31,59 @@ use OCP\IURLGenerator; use OneLogin\Saml2\Constants; class SAMLSettings { + private const LOADED_NONE = 0; + private const LOADED_CHOSEN = 1; + private const LOADED_ALL = 2; + + public const IDP_CONFIG_KEYS = [ + 'general-idp0_display_name', + 'general-uid_mapping', + 'idp-entityId', + 'idp-singleLogoutService.responseUrl', + 'idp-singleLogoutService.url', + 'idp-singleSignOnService.url', + 'idp-x509cert', + 'security-authnRequestsSigned', + 'security-general', + 'security-logoutRequestSigned', + 'security-logoutResponseSigned', + 'security-lowercaseUrlencoding', + 'security-nameIdEncrypted', + 'security-offer', + 'security-required', + 'security-signatureAlgorithm', + 'security-signMetadata', + 'security-sloWebServerDecode', + 'security-wantAssertionsEncrypted', + 'security-wantAssertionsSigned', + 'security-wantMessagesSigned', + 'security-wantNameId', + 'security-wantNameIdEncrypted', + 'security-wantXMLValidation', + 'saml-attribute-mapping-displayName_mapping', + 'saml-attribute-mapping-email_mapping', + 'saml-attribute-mapping-group_mapping', + 'saml-attribute-mapping-home_mapping', + 'saml-attribute-mapping-quota_mapping', + 'sp-x509cert', + 'sp-name-id-format', + 'sp-privateKey', + ]; + /** @var IURLGenerator */ private $urlGenerator; /** @var IConfig */ private $config; - /** @var IRequest */ - private $request; /** @var ISession */ private $session; /** @var array list of global settings which are valid for every idp */ private $globalSettings = ['general-require_provisioned_account', 'general-allow_multiple_user_back_ends', 'general-use_saml_auth_for_desktop']; + /** @var array> */ + private $configurations; + /** @var int */ + private $configurationsLoadedState = self::LOADED_NONE; + /** @var ConfigurationsMapper */ + private $mapper; /** * @param IURLGenerator $urlGenerator @@ -45,148 +91,165 @@ class SAMLSettings { * @param IRequest $request * @param ISession $session */ - public function __construct(IURLGenerator $urlGenerator, - IConfig $config, - IRequest $request, - ISession $session) { + public function __construct( + IURLGenerator $urlGenerator, + IConfig $config, + ISession $session, + ConfigurationsMapper $mapper + ) { $this->urlGenerator = $urlGenerator; $this->config = $config; - $this->request = $request; $this->session = $session; + $this->mapper = $mapper; } /** * get list of the configured IDPs * - * @return array + * @return array + * @throws Exception */ - public function getListOfIdps() { + public function getListOfIdps(): array { + $this->ensureConfigurationsLoaded(); + $result = []; - - $providerIds = explode(',', $this->config->getAppValue('user_saml', 'providerIds', '1')); - natsort($providerIds); - - foreach ($providerIds as $id) { - $prefix = $id === '1' ? '' : $id .'-'; - $result[$id] = $this->config->getAppValue('user_saml', $prefix . 'general-idp0_display_name', ''); + foreach ($this->configurations as $configID => $config) { + // no fancy array_* method, because there might be thousands + $result[$configID] = $config['general-idp0_display_name'] ?? ''; } - asort($result); - return $result; } /** * check if multiple user back ends are allowed - * - * @return bool */ - public function allowMultipleUserBackEnds() { + public function allowMultipleUserBackEnds(): bool { $type = $this->config->getAppValue('user_saml', 'type'); $setting = $this->config->getAppValue('user_saml', 'general-allow_multiple_user_back_ends', '0'); - return ($setting === '1' && $type === 'saml'); + return ($setting === '1' && $type === 'saml'); } - public function usesSloWebServerDecode() : bool { + public function usesSloWebServerDecode(): bool { return $this->config->getAppValue('user_saml', 'security-sloWebServerDecode', '0') === '1'; } /** * get config for given IDP * - * @param int $idp - * @return array + * @throws Exception */ - public function getOneLoginSettingsArray($idp) { - $prefix = ''; - if ($idp > 1) { - $prefix = $idp . '-'; - } + public function getOneLoginSettingsArray(int $idp): array { + $this->ensureConfigurationsLoaded($idp); $settings = [ 'strict' => true, 'debug' => $this->config->getSystemValue('debug', false), 'baseurl' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.base'), 'security' => [ - 'nameIdEncrypted' => ($this->config->getAppValue('user_saml', $prefix . 'security-nameIdEncrypted', '0') === '1') ? true : false, - 'authnRequestsSigned' => ($this->config->getAppValue('user_saml', $prefix . 'security-authnRequestsSigned', '0') === '1') ? true : false, - 'logoutRequestSigned' => ($this->config->getAppValue('user_saml', $prefix . 'security-logoutRequestSigned', '0') === '1') ? true : false, - 'logoutResponseSigned' => ($this->config->getAppValue('user_saml', $prefix . 'security-logoutResponseSigned', '0') === '1') ? true : false, - 'signMetadata' => ($this->config->getAppValue('user_saml', $prefix . 'security-signMetadata', '0') === '1') ? true : false, - 'wantMessagesSigned' => ($this->config->getAppValue('user_saml', $prefix . 'security-wantMessagesSigned', '0') === '1') ? true : false, - 'wantAssertionsSigned' => ($this->config->getAppValue('user_saml', $prefix . 'security-wantAssertionsSigned', '0') === '1') ? true : false, - 'wantAssertionsEncrypted' => ($this->config->getAppValue('user_saml', $prefix . 'security-wantAssertionsEncrypted', '0') === '1') ? true : false, - 'wantNameId' => ($this->config->getAppValue('user_saml', $prefix . 'security-wantNameId', '0') === '1') ? true : false, - 'wantNameIdEncrypted' => ($this->config->getAppValue('user_saml', $prefix . 'security-wantNameIdEncrypted', '0') === '1') ? true : false, - 'wantXMLValidation' => ($this->config->getAppValue('user_saml', $prefix . 'security-wantXMLValidation', '0') === '1') ? true : false, + 'nameIdEncrypted' => ($this->configurations[$idp]['security-nameIdEncrypted'] ?? '0') === '1', + 'authnRequestsSigned' => ($this->configurations[$idp]['security-authnRequestsSigned'] ?? '0') === '1', + 'logoutRequestSigned' => ($this->configurations[$idp]['security-logoutRequestSigned'] ?? '0') === '1', + 'logoutResponseSigned' => ($this->configurations[$idp]['security-logoutResponseSigned'] ?? '0') === '1', + 'signMetadata' => ($this->configurations[$idp]['security-signMetadata'] ?? '0') === '1', + 'wantMessagesSigned' => ($this->configurations[$idp]['security-wantMessagesSigned'] ?? '0') === '1', + 'wantAssertionsSigned' => ($this->configurations[$idp]['security-wantAssertionsSigned'] ?? '0') === '1', + 'wantAssertionsEncrypted' => ($this->configurations[$idp]['security-wantAssertionsEncrypted'] ?? '0') === '1', + 'wantNameId' => ($this->configurations[$idp]['security-wantNameId'] ?? '0') === '1', + 'wantNameIdEncrypted' => ($this->configurations[$idp]['security-wantNameIdEncrypted'] ?? '0') === '1', + 'wantXMLValidation' => ($this->configurations[$idp]['security-wantXMLValidation'] ?? '0') === '1', 'requestedAuthnContext' => false, - 'lowercaseUrlencoding' => ($this->config->getAppValue('user_saml', $prefix . 'security-lowercaseUrlencoding', '0') === '1') ? true : false, - 'signatureAlgorithm' => $this->config->getAppValue('user_saml', $prefix . 'security-signatureAlgorithm', null) + 'lowercaseUrlencoding' => ($this->configurations[$idp]['security-lowercaseUrlencoding'] ?? '0') === '1', + 'signatureAlgorithm' => $this->configurations[$idp]['security-signatureAlgorithm'] ?? null, ], 'sp' => [ 'entityId' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.getMetadata'), 'assertionConsumerService' => [ 'url' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.assertionConsumerService'), ], - 'NameIDFormat' => $this->config->getAppValue('user_saml', $prefix . 'sp-name-id-format', Constants::NAMEID_UNSPECIFIED) + 'NameIDFormat' => $this->configurations[$idp]['sp-name-id-format'] ?? Constants::NAMEID_UNSPECIFIED, + 'x509cert' => $this->configurations[$idp]['sp-x509cert'] ?? '', + 'privateKey' => $this->configurations[$idp]['sp-privateKey'] ?? '', ], 'idp' => [ - 'entityId' => $this->config->getAppValue('user_saml', $prefix . 'idp-entityId', ''), + 'entityId' => $this->configurations[$idp]['idp-entityId'] ?? '', 'singleSignOnService' => [ - 'url' => $this->config->getAppValue('user_saml', $prefix . 'idp-singleSignOnService.url', ''), + 'url' => $this->configurations[$idp]['idp-singleSignOnService.url'] ?? '', ], + 'x509cert' => $this->configurations[$idp]['idp-x509cert'] ?? '', ], ]; - $spx509cert = $this->config->getAppValue('user_saml', $prefix . 'sp-x509cert', ''); - $spxprivateKey = $this->config->getAppValue('user_saml', $prefix . 'sp-privateKey', ''); - if ($spx509cert !== '') { - $settings['sp']['x509cert'] = $spx509cert; - } - if ($spxprivateKey !== '') { - $settings['sp']['privateKey'] = $spxprivateKey; - } - - $idpx509cert = $this->config->getAppValue('user_saml', $prefix . 'idp-x509cert', ''); - if ($idpx509cert !== '') { - $settings['idp']['x509cert'] = $idpx509cert; - } - - $slo = $this->config->getAppValue('user_saml', $prefix . 'idp-singleLogoutService.url', ''); - if ($slo !== '') { - $settings['idp']['singleLogoutService'] = [ - 'url' => $this->config->getAppValue('user_saml', $prefix . 'idp-singleLogoutService.url', ''), - ]; - $settings['sp']['singleLogoutService'] = [ - 'url' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.singleLogoutService'), - ]; - - $sloResponseUrl = $this->config->getAppValue('user_saml', $prefix . 'idp-singleLogoutService.responseUrl', ''); - if ($sloResponseUrl !== '') { - $settings['idp']['singleLogoutService']['responseUrl'] = $sloResponseUrl; + // must be added only if configured + if (($this->configurations[$idp]['idp-singleLogoutService.url'] ?? '') !== '') { + $settings['idp']['singleLogoutService'] = ['url' => $this->configurations[$idp]['idp-singleLogoutService.url']]; + $settings['sp']['singleLogoutService'] = ['url' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.singleLogoutService')]; + if (($this->configurations[$idp]['idp-singleLogoutService.responseUrl'] ?? '') !== '') { + $settings['idp']['singleLogoutService']['responseUrl'] = $this->configurations[$idp]['idp-singleLogoutService.responseUrl']; } } return $settings; } + public function getProviderId(): int { + // defaults to 1, needed for env-mode + return (int)($this->session->get('user_saml.Idp') ?? 1); + } + + public function getNewProviderId(): int { + return $this->mapper->reserveId(); + } + /** - * calculate prefix for config values - * - * @param string name of the setting - * @return string + * @throws Exception */ - public function getPrefix($setting = '') { - $prefix = ''; - if (!empty($setting) && in_array($setting, $this->globalSettings)) { - return $prefix; + public function get(int $id): array { + $this->ensureConfigurationsLoaded($id); + return $this->configurations[$id] ?? []; + } + + /** + * @throws InvalidArgumentException + */ + public function set(int $id, array $settings): void { + foreach (array_keys($settings) as $configKey) { + if (!in_array($configKey, self::IDP_CONFIG_KEYS)) { + throw new InvalidArgumentException('Invalid config key'); + } } - $idp = $this->session->get('user_saml.Idp'); - if ((int)$idp > 1) { - $prefix = $idp . '-'; + $this->mapper->set($id, $settings); + } + + /** + * @throws Exception + */ + public function delete(int $id): void { + $this->mapper->deleteById($id); + } + + /** + * @throws Exception + */ + protected function ensureConfigurationsLoaded(int $idp = -1): void { + if (self::LOADED_ALL === $this->configurationsLoadedState + || (self::LOADED_CHOSEN === $this->configurationsLoadedState + && isset($this->configurations[$idp]) + ) + ) { + return; } - return $prefix; + if ($idp !== -1) { + $this->configurations[$idp] = $this->mapper->get($idp); + } else { + $configs = $this->mapper->getAll(); + foreach ($configs as $id => $config) { + $this->configurations[$id] = $config; + } + } + + $this->configurationsLoadedState = $idp === -1 ? self::LOADED_ALL : self::LOADED_CHOSEN; } } diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index 9423ad0..64c328f 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -23,6 +23,7 @@ namespace OCA\User_SAML\Settings; +use OCA\User_SAML\SAMLSettings; use OCP\AppFramework\Http\TemplateResponse; use OCP\Defaults; use OCP\IConfig; @@ -37,30 +38,33 @@ class Admin implements ISettings { private $defaults; /** @var IConfig */ private $config; + /** @var SAMLSettings */ + private $samlSettings; /** * @param IL10N $l10n * @param Defaults $defaults * @param IConfig $config */ - public function __construct(IL10N $l10n, - Defaults $defaults, - IConfig $config) { + public function __construct( + IL10N $l10n, + Defaults $defaults, + IConfig $config, + SAMLSettings $samlSettings + ) { $this->l10n = $l10n; $this->defaults = $defaults; $this->config = $config; + $this->samlSettings = $samlSettings; } /** * @return TemplateResponse */ public function getForm() { - $providerIds = explode(',', $this->config->getAppValue('user_saml', 'providerIds', '1')); - natsort($providerIds); + $providerIds = $this->samlSettings->getListOfIdps(); $providers = []; - foreach ($providerIds as $id) { - $prefix = $id === '1' ? '' : $id .'-'; - $name = $this->config->getAppValue('user_saml', $prefix . 'general-idp0_display_name', ''); + foreach ($providerIds as $id => $name) { $providers[] = [ 'id' => $id, 'name' => $name === '' ? $this->l10n->t('Provider ') . $id : $name @@ -134,43 +138,42 @@ class Admin implements ISettings { ]; - $selectedNameIdFormat = $this->config->getAppValue('user_saml', 'sp-name-id-format', Constants::NAMEID_UNSPECIFIED); $nameIdFormats = [ Constants::NAMEID_EMAIL_ADDRESS => [ 'label' => $this->l10n->t('Email address'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_EMAIL_ADDRESS, + 'selected' => false, ], Constants::NAMEID_ENCRYPTED => [ 'label' => $this->l10n->t('Encrypted'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_ENCRYPTED, + 'selected' => false, ], Constants::NAMEID_ENTITY => [ 'label' => $this->l10n->t('Entity'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_ENTITY, + 'selected' => false, ], Constants::NAMEID_KERBEROS => [ 'label' => $this->l10n->t('Kerberos'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_KERBEROS, + 'selected' => false, ], Constants::NAMEID_PERSISTENT => [ 'label' => $this->l10n->t('Persistent'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_PERSISTENT, + 'selected' => false, ], Constants::NAMEID_TRANSIENT => [ 'label' => $this->l10n->t('Transient'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_TRANSIENT, + 'selected' => false, ], Constants::NAMEID_UNSPECIFIED => [ 'label' => $this->l10n->t('Unspecified'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_UNSPECIFIED, + 'selected' => false, ], Constants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME => [ 'label' => $this->l10n->t('Windows domain qualified name'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME, + 'selected' => false, ], Constants::NAMEID_X509_SUBJECT_NAME => [ 'label' => $this->l10n->t('X509 subject name'), - 'selected' => $selectedNameIdFormat === Constants::NAMEID_X509_SUBJECT_NAME, + 'selected' => false, ], ]; diff --git a/lib/UserBackend.php b/lib/UserBackend.php index d7c1f3a..2461a70 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -107,8 +107,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @param string $uid * @param array $attributes */ - public function createUserIfNotExists($uid, array $attributes = []) { - if (!$this->userExistsInDatabase($uid)) { + public function createUserIfNotExists($uid, array $attributes = array()) { + if(!$this->userExistsInDatabase($uid)) { $values = [ 'uid' => $uid, ]; @@ -123,12 +123,12 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { if ($home !== '') { //if attribute's value is an absolute path take this, otherwise append it to data dir //check for / at the beginning or pattern c:\ resp. c:/ - if ('/' !== $home[0] + if( '/' !== $home[0] && !(3 < strlen($home) && ctype_alpha($home[0]) && $home[1] === ':' && ('\\' === $home[2] || '/' === $home[2])) ) { $home = $this->config->getSystemValue('datadirectory', - \OC::$SERVERROOT.'/data') . '/' . $home; + \OC::$SERVERROOT.'/data' ) . '/' . $home; } $values['home'] = $home; @@ -137,12 +137,13 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->insert('user_saml_users'); - foreach ($values as $column => $value) { + foreach($values as $column => $value) { $qb->setValue($column, $qb->createNamedParameter($value)); } $qb->execute(); $this->initializeHomeDir($uid); + } } @@ -202,8 +203,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { $data = $result->fetchAll(); $result->closeCursor(); - foreach ($data as $passwords) { - if (password_verify($password, $passwords['token'])) { + foreach($data as $passwords) { + if(password_verify($password, $passwords['token'])) { return $uid; } } @@ -218,7 +219,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function deleteUser($uid) { - if ($this->userExistsInDatabase($uid)) { + if($this->userExistsInDatabase($uid)) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->delete('user_saml_users') @@ -236,7 +237,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @return string */ public function getHome($uid) { - if ($this->userExistsInDatabase($uid)) { + if($this->userExistsInDatabase($uid)) { $qb = $this->db->getQueryBuilder(); $qb->select('home') ->from('user_saml_users') @@ -276,7 +277,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function userExists($uid) { - if ($backend = $this->getActualUserBackend($uid)) { + if($backend = $this->getActualUserBackend($uid)) { return $backend->userExists($uid); } else { return $this->userExistsInDatabase($uid); @@ -284,7 +285,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { } public function setDisplayName($uid, $displayName) { - if ($backend = $this->getActualUserBackend($uid)) { + if($backend = $this->getActualUserBackend($uid)) { return $backend->setDisplayName($uid, $displayName); } @@ -308,10 +309,10 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function getDisplayName($uid) { - if ($backend = $this->getActualUserBackend($uid)) { + if($backend = $this->getActualUserBackend($uid)) { return $backend->getDisplayName($uid); } else { - if ($this->userExistsInDatabase($uid)) { + if($this->userExistsInDatabase($uid)) { $qb = $this->db->getQueryBuilder(); $qb->select('displayname') ->from('user_saml_users') @@ -373,7 +374,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function hasUserListings() { - if ($this->autoprovisionAllowed()) { + if($this->autoprovisionAllowed()) { return true; } @@ -394,8 +395,10 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * {@inheritdoc} */ public function getLogoutUrl() { - $prefix = $this->settings->getPrefix(); - $slo = $this->config->getAppValue('user_saml', $prefix . 'idp-singleLogoutService.url', ''); + $id = $this->settings->getProviderId(); + $settings = $this->settings->get($id); + $slo = $settings['idp-singleLogoutService.url'] ?? ''; + if ($slo === '') { return ''; } @@ -484,14 +487,14 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { public function getCurrentUserId() { $user = \OC::$server->getUserSession()->getUser(); - if ($user instanceof IUser && $this->session->get('user_saml.samlUserData')) { + if($user instanceof IUser && $this->session->get('user_saml.samlUserData')) { $uid = $user->getUID(); } else { $this->userData->setAttributes($this->session->get('user_saml.samlUserData') ?? []); $uid = $this->userData->getEffectiveUid(); } - if ($uid !== '' && $this->userExists($uid)) { + if($uid !== '' && $this->userExists($uid)) { $this->session->set('last-password-confirm', strtotime('+4 year', time())); return $uid; } @@ -524,8 +527,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @return null|UserInterface */ public function getActualUserBackend($uid) { - foreach (self::$backends as $backend) { - if ($backend->userExists($uid)) { + foreach(self::$backends as $backend) { + if($backend->userExists($uid)) { return $backend; } } @@ -543,9 +546,13 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { self::$backends = $backends; } - private function getAttributeKeys($name) { - $prefix = $this->settings->getPrefix($name); - $keys = explode(' ', $this->config->getAppValue('user_saml', $prefix . $name, '')); + /** + * @throws \OCP\DB\Exception + */ + private function getAttributeKeys($name) + { + $settings = $this->settings->get($this->settings->getProviderId()); + $keys = explode(' ', $settings[$name] ?? $this->config->getAppValue('user_saml', $name, '')); if (count($keys) === 1 && $keys[0] === '') { throw new \InvalidArgumentException('Attribute is not configured'); @@ -557,17 +564,17 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { $keys = $this->getAttributeKeys($name); $value = ''; - foreach ($keys as $key) { + foreach($keys as $key) { if (isset($attributes[$key])) { if (is_array($attributes[$key])) { foreach ($attributes[$key] as $attribute_part_value) { - if ($value !== '') { + if($value !== '') { $value .= ' '; } $value .= $attribute_part_value; } } else { - if ($value !== '') { + if($value !== '') { $value .= ' '; } $value .= $attributes[$key]; @@ -581,8 +588,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { private function getAttributeArrayValue($name, array $attributes) { $keys = $this->getAttributeKeys($name); - $value = []; - foreach ($keys as $key) { + $value = array(); + foreach($keys as $key) { if (isset($attributes[$key])) { if (is_array($attributes[$key])) { $value = array_merge($value, array_values($attributes[$key])); diff --git a/lib/UserData.php b/lib/UserData.php index 01613c4..e0e0607 100644 --- a/lib/UserData.php +++ b/lib/UserData.php @@ -1,5 +1,4 @@ @@ -57,9 +56,8 @@ class UserData { public function hasUidMappingAttribute(): bool { $this->assertIsInitialized(); - $prefix = $this->samlSettings->getPrefix(); - $uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping'); - return isset($this->attributes[$uidMapping]); + $attribute = $this->getUidMappingAttribute(); + return $attribute !== null && isset($this->attributes[$attribute]); } public function getOriginalUid(): string { @@ -68,7 +66,7 @@ class UserData { } public function getEffectiveUid(): string { - if ($this->uid !== null) { + if($this->uid !== null) { return $this->uid; } $this->assertIsInitialized(); @@ -84,9 +82,8 @@ class UserData { } protected function extractSamlUserId(): string { - $prefix = $this->samlSettings->getPrefix(); - $uidMapping = $this->config->getAppValue('user_saml', $prefix . 'general-uid_mapping'); - if (isset($this->attributes[$uidMapping])) { + $uidMapping = $this->getUidMappingAttribute(); + if($uidMapping !== null && isset($this->attributes[$uidMapping])) { if (is_array($this->attributes[$uidMapping])) { return trim($this->attributes[$uidMapping][0]); } else { @@ -108,13 +105,13 @@ class UserData { } $candidate = base64_decode($uid, true); - if ($candidate === false) { + if($candidate === false) { return $uid; } $candidate = $this->convertObjectGUID2Str($candidate); // the regex only matches the structure of the UUID, not its semantic // (i.e. version or variant) simply to be future compatible - if (preg_match('/^[a-f0-9]{8}(-[a-f0-9]{4}){4}[a-f0-9]{8}$/i', $candidate) === 1) { + if(preg_match('/^[a-f0-9]{8}(-[a-f0-9]{4}){4}[a-f0-9]{8}$/i', $candidate) === 1) { $uid = $candidate; } return $uid; @@ -126,15 +123,15 @@ class UserData { protected function convertObjectGUID2Str($oguid): string { $hex_guid = bin2hex($oguid); $hex_guid_to_guid_str = ''; - for ($k = 1; $k <= 4; ++$k) { + for($k = 1; $k <= 4; ++$k) { $hex_guid_to_guid_str .= substr($hex_guid, 8 - 2 * $k, 2); } $hex_guid_to_guid_str .= '-'; - for ($k = 1; $k <= 2; ++$k) { + for($k = 1; $k <= 2; ++$k) { $hex_guid_to_guid_str .= substr($hex_guid, 12 - 2 * $k, 2); } $hex_guid_to_guid_str .= '-'; - for ($k = 1; $k <= 2; ++$k) { + for($k = 1; $k <= 2; ++$k) { $hex_guid_to_guid_str .= substr($hex_guid, 16 - 2 * $k, 2); } $hex_guid_to_guid_str .= '-' . substr($hex_guid, 16, 4); @@ -144,8 +141,16 @@ class UserData { } protected function assertIsInitialized() { - if ($this->attributes === null) { + if($this->attributes === null) { throw new \LogicException('UserData have to be initialized with setAttributes first'); } } + + protected function getProviderSettings(): array { + return $this->samlSettings->get($this->samlSettings->getProviderId()); + } + + protected function getUidMappingAttribute(): ?string { + return $this->getProviderSettings()['general-uid_mapping'] ?? null; + } } diff --git a/tests/unit/AppInfo/RoutesTest.php b/tests/unit/AppInfo/RoutesTest.php deleted file mode 100644 index a72c7f9..0000000 --- a/tests/unit/AppInfo/RoutesTest.php +++ /dev/null @@ -1,104 +0,0 @@ - - * - * @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 . - * - */ - -namespace OCA\User_SAML\Tests\AppInfo; - -use Test\TestCase; - -class Test extends TestCase { - public function testFile() { - $dir = __DIR__; - $routes = require_once __DIR__ . '/../../../appinfo/routes.php'; - - $expected = [ - 'routes' => [ - [ - 'name' => 'SAML#login', - 'url' => '/saml/login', - 'verb' => 'GET', - ], - [ - 'name' => 'SAML#base', - 'url' => '/saml', - 'verb' => 'GET', - ], - [ - 'name' => 'SAML#getMetadata', - 'url' => '/saml/metadata', - 'verb' => 'GET', - ], - [ - 'name' => 'SAML#assertionConsumerService', - 'url' => '/saml/acs', - 'verb' => 'POST', - ], - [ - 'name' => 'SAML#singleLogoutService', - 'url' => '/saml/sls', - 'verb' => 'GET', - ], - [ - 'name' => 'SAML#singleLogoutService', - 'url' => '/saml/sls', - 'verb' => 'POST', - 'postfix' => 'slspost', - ], - [ - 'name' => 'SAML#notProvisioned', - 'url' => '/saml/notProvisioned', - 'verb' => 'GET', - ], - [ - 'name' => 'SAML#genericError', - 'url' => '/saml/error', - 'verb' => 'GET', - ], - [ - 'name' => 'SAML#selectUserBackEnd', - 'url' => '/saml/selectUserBackEnd', - 'verb' => 'GET', - ], - [ - 'name' => 'Settings#getSamlProviderSettings', - 'url' => '/settings/providerSettings/{providerId}', - 'verb' => 'GET', - 'defaults' => [ - 'providerId' => '1' - ] - ], - [ - 'name' => 'Settings#deleteSamlProviderSettings', - 'url' => '/settings/providerSettings/{providerId}', - 'verb' => 'DELETE', - 'defaults' => [ - 'providerId' => '1' - ] - ], - [ - 'name' => 'Timezone#setTimezone', - 'url' => '/config/timezone', - 'verb' => 'POST', - ], - ], - ]; - $this->assertSame($expected, $routes); - } -} diff --git a/tests/unit/Command/GetMetadataTest.php b/tests/unit/Command/GetMetadataTest.php index 14ee71f..bb0e9f5 100644 --- a/tests/unit/Command/GetMetadataTest.php +++ b/tests/unit/Command/GetMetadataTest.php @@ -23,79 +23,48 @@ namespace OCA\User_SAML\Tests\Command; use OCA\User_SAML\Command\GetMetadata; use OCA\User_SAML\SAMLSettings; +use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; -use OCP\IConfig; -use OCP\IRequest; -use OCP\ISession; -use OCP\IURLGenerator; class GetMetadataTest extends \Test\TestCase { - /** @var GetMetadata|\PHPUnit_Framework_MockObject_MockObject*/ - protected $GetMetadata; - /** @var IRequest|\PHPUnit_Framework_MockObject_MockObject */ - private $request; - /** @var ISession|\PHPUnit_Framework_MockObject_MockObject */ - private $session; - /** @var SAMLSettings|\PHPUnit_Framework_MockObject_MockObject*/ - private $samlSettings; - /** @var IConfig|\PHPUnit_Framework_MockObject_MockObject */ - private $config; - /** @var IURLGenerator|\PHPUnit_Framework_MockObject_MockObject */ - private $urlGenerator; + /** @var GetMetadata|MockObject*/ + protected $GetMetadata; + /** @var SAMLSettings|MockObject*/ + private $samlSettings; + protected function setUp(): void { + $this->samlSettings = $this->createMock(SAMLSettings::class); + $this->GetMetadata = new GetMetadata($this->samlSettings); - protected function setUp(): void { - $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->config = $this->createMock(IConfig::class); - $this->request = $this->createMock(IRequest::class); - $this->session = $this->createMock(ISession::class); - - $this->samlSettings = new SAMLSettings($this->urlGenerator, - $this->config, - $this->request, - $this->session); - $this->GetMetadata = new GetMetadata($this->samlSettings); - - parent::setUp(); - } - - public function testGetMetadata() { + parent::setUp(); + } + public function testGetMetadata(){ $inputInterface = $this->createMock(InputInterface::class); $outputInterface = $this->createMock(OutputInterface::class); - $this->urlGenerator - ->expects($this->at(0)) - ->method('linkToRouteAbsolute') - ->with('user_saml.SAML.base') - ->willReturn('https://nextcloud.com/base/'); - $this->urlGenerator - ->expects($this->at(1)) - ->method('linkToRouteAbsolute') - ->with('user_saml.SAML.getMetadata') - ->willReturn('https://nextcloud.com/metadata/'); - $this->urlGenerator - ->expects($this->at(2)) - ->method('linkToRouteAbsolute') - ->with('user_saml.SAML.assertionConsumerService') - ->willReturn('https://nextcloud.com/acs/'); - $this->config->expects($this->any())->method('getAppValue') - ->willReturnCallback(function ($app, $key, $default) { - if ($key == 'idp-entityId') { - return "dummy"; - } - if ($key == 'idp-singleSignOnService.url') { - return "https://example.com/sso"; - } - if ($key == 'idp-x509cert') { - return "DUMMY CERTIFICATE"; - } - return $default; - }); + + $this->samlSettings->expects($this->any()) + ->method('getOneLoginSettingsArray') + ->willReturn([ + 'baseurl' => 'https://nextcloud.com/base/', + 'idp' => [ + 'entityId' => 'dummy', + 'singleSignOnService' => ['url' => 'https://example.com/sso'], + 'x509cert' => 'DUMMY CERTIFICATE', + ], + 'sp' => [ + 'entityId' => 'https://nextcloud.com/metadata/', + 'assertionConsumerService' => [ + 'url' => 'https://nextcloud.com/acs/', + ], + ] + ]); $outputInterface->expects($this->once())->method('writeln') - ->with($this->stringContains('md:EntityDescriptor')); + ->with($this->stringContains('md:EntityDescriptor')); $this->invokePrivate($this->GetMetadata, 'execute', [$inputInterface, $outputInterface]); } + } diff --git a/tests/unit/Settings/AdminTest.php b/tests/unit/Settings/AdminTest.php index 03d1df2..6162455 100644 --- a/tests/unit/Settings/AdminTest.php +++ b/tests/unit/Settings/AdminTest.php @@ -21,14 +21,19 @@ namespace OCA\User_SAML\Tests\Settings; +use OCA\User_SAML\SAMLSettings; +use OCA\User_SAML\Settings\Admin; use OCP\AppFramework\Http\TemplateResponse; use OCP\Defaults; use OCP\IConfig; use OCP\IL10N; use OneLogin\Saml2\Constants; +use PHPUnit\Framework\MockObject\MockObject; -class AdminTest extends \Test\TestCase { - /** @var \OCA\User_SAML\Settings\Admin */ +class AdminTest extends \Test\TestCase { + /** @var SAMLSettings|MockObject */ + private $settings; + /** @var Admin */ private $admin; /** @var IL10N|\PHPUnit_Framework_MockObject_MockObject */ private $l10n; @@ -41,11 +46,13 @@ class AdminTest extends \Test\TestCase { $this->l10n = $this->createMock(IL10N::class); $this->defaults = $this->createMock(Defaults::class); $this->config = $this->createMock(IConfig::class); + $this->settings = $this->createMock(SAMLSettings::class); - $this->admin = new \OCA\User_SAML\Settings\Admin( + $this->admin = new Admin( $this->l10n, $this->defaults, - $this->config + $this->config, + $this->settings ); parent::setUp(); @@ -55,7 +62,7 @@ class AdminTest extends \Test\TestCase { $this->l10n ->expects($this->any()) ->method('t') - ->will($this->returnCallback(function ($text, $parameters = []) { + ->will($this->returnCallback(function($text, $parameters = array()) { return vsprintf($text, $parameters); })); @@ -169,7 +176,7 @@ class AdminTest extends \Test\TestCase { ], Constants::NAMEID_UNSPECIFIED => [ 'label' => 'Unspecified', - 'selected' => true, + 'selected' => false, ], Constants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME => [ 'label' => 'Windows domain qualified name', @@ -199,26 +206,14 @@ class AdminTest extends \Test\TestCase { } public function testGetFormWithoutType() { + $this->settings->expects($this->once()) + ->method('getListOfIdps') + ->willReturn([ + 1 => 'Provider 1', + 2 => 'Provider 2', + ]); $this->config - ->expects($this->at(0)) - ->method('getAppValue') - ->with('user_saml', 'providerIds') - ->willReturn('1,2'); - $this->config - ->expects($this->at(1)) - ->method('getAppValue') - ->willReturn('Provider 1'); - $this->config - ->expects($this->at(2)) - ->method('getAppValue') - ->willReturn('Provider 2'); - $this->config - ->expects($this->at(3)) - ->method('getAppValue') - ->with('user_saml', 'sp-name-id-format') - ->will($this->returnArgument(2)); - $this->config - ->expects($this->at(4)) + ->expects($this->once()) ->method('getAppValue') ->with('user_saml', 'type') ->willReturn(''); @@ -234,26 +229,14 @@ class AdminTest extends \Test\TestCase { } public function testGetFormWithSaml() { + $this->settings->expects($this->once()) + ->method('getListOfIdps') + ->willReturn([ + 1 => 'Provider 1', + 2 => 'Provider 2', + ]); $this->config - ->expects($this->at(0)) - ->method('getAppValue') - ->with('user_saml', 'providerIds') - ->willReturn('1,2'); - $this->config - ->expects($this->at(1)) - ->method('getAppValue') - ->willReturn('Provider 1'); - $this->config - ->expects($this->at(2)) - ->method('getAppValue') - ->willReturn('Provider 2'); - $this->config - ->expects($this->at(3)) - ->method('getAppValue') - ->with('user_saml', 'sp-name-id-format') - ->will($this->returnArgument(2)); - $this->config - ->expects($this->at(4)) + ->expects($this->once()) ->method('getAppValue') ->with('user_saml', 'type') ->willReturn('saml'); diff --git a/tests/unit/phpunit.xml b/tests/unit/phpunit.xml index 873b0b0..b474397 100644 --- a/tests/unit/phpunit.xml +++ b/tests/unit/phpunit.xml @@ -1,6 +1,5 @@ ../../../user_saml/lib - - - - From 7bdad55dc91c83e3fc3942e6118552bec2063082 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 19 Nov 2021 18:42:02 +0100 Subject: [PATCH 02/13] add occ commands for config manipulation Signed-off-by: Arthur Schiwon --- appinfo/info.xml | 4 + lib/Command/ConfigCreate.php | 53 ++++++++++++ lib/Command/ConfigDelete.php | 59 +++++++++++++ lib/Command/ConfigGet.php | 74 ++++++++++++++++ lib/Command/ConfigSet.php | 84 +++++++++++++++++++ .../features/bootstrap/FeatureContext.php | 35 +++++++- 6 files changed, 306 insertions(+), 3 deletions(-) create mode 100644 lib/Command/ConfigCreate.php create mode 100644 lib/Command/ConfigDelete.php create mode 100644 lib/Command/ConfigGet.php create mode 100644 lib/Command/ConfigSet.php diff --git a/appinfo/info.xml b/appinfo/info.xml index 87211ae..f9edb01 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -36,6 +36,10 @@ While theoretically any other authentication provider implementing either one of + OCA\User_SAML\Command\ConfigCreate + OCA\User_SAML\Command\ConfigDelete + OCA\User_SAML\Command\ConfigGet + OCA\User_SAML\Command\ConfigSet OCA\User_SAML\Command\GetMetadata diff --git a/lib/Command/ConfigCreate.php b/lib/Command/ConfigCreate.php new file mode 100644 index 0000000..de000a1 --- /dev/null +++ b/lib/Command/ConfigCreate.php @@ -0,0 +1,53 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\User_SAML\Command; + +use OC\Core\Command\Base; +use OCA\User_SAML\SAMLSettings; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; + +class ConfigCreate extends Base { + + /** @var SAMLSettings */ + private $samlSettings; + + public function __construct(SAMLSettings $samlSettings) { + parent::__construct(); + $this->samlSettings = $samlSettings; + } + + protected function configure() { + $this->setName('saml:config:create'); + $this->setDescription('Creates a new config and prints the new provider ID'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $output->writeln($this->samlSettings->getNewProviderId()); + return 0; + } +} diff --git a/lib/Command/ConfigDelete.php b/lib/Command/ConfigDelete.php new file mode 100644 index 0000000..09310de --- /dev/null +++ b/lib/Command/ConfigDelete.php @@ -0,0 +1,59 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\User_SAML\Command; + +use OC\Core\Command\Base; +use OCA\User_SAML\SAMLSettings; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Output\OutputInterface; + +class ConfigDelete extends Base { + /** @var SAMLSettings */ + private $samlSettings; + + public function __construct(SAMLSettings $samlSettings) { + parent::__construct(); + $this->samlSettings = $samlSettings; + } + + protected function configure() { + $this->setName('saml:config:delete'); + + $this->addArgument( + 'providerId', + InputArgument::REQUIRED, + 'ProviderID of the SAML config to edit' + ); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $pId = (int)$input->getArgument('providerId'); + $this->samlSettings->delete($pId); + return 0; + } +} diff --git a/lib/Command/ConfigGet.php b/lib/Command/ConfigGet.php new file mode 100644 index 0000000..3172f72 --- /dev/null +++ b/lib/Command/ConfigGet.php @@ -0,0 +1,74 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\User_SAML\Command; + +use OC\Core\Command\Base; +use OCA\User_SAML\SAMLSettings; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; + +class ConfigGet extends Base { + + /** @var SAMLSettings */ + private $samlSettings; + + public function __construct(SAMLSettings $samlSettings) { + parent::__construct(); + $this->samlSettings = $samlSettings; + } + + protected function configure() { + $this->setName('saml:config:get'); + + $this->addOption( + 'providerId', + 'p', + InputOption::VALUE_REQUIRED, + 'ProviderID of a SAML config to print' + ); + parent::configure(); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $providerId = (int)$input->getOption('providerId'); + if (!empty($providerId)) { + $providerIds = [$providerId]; + } else { + $providerIds = array_keys($this->samlSettings->getListOfIdps()); + } + + $settings = []; + foreach ($providerIds as $pid) { + $settings[$pid] = $this->samlSettings->get($pid); + } + + $this->writeArrayInOutputFormat($input, $output, $settings); + + return 0; + } +} diff --git a/lib/Command/ConfigSet.php b/lib/Command/ConfigSet.php new file mode 100644 index 0000000..8bc4af8 --- /dev/null +++ b/lib/Command/ConfigSet.php @@ -0,0 +1,84 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace OCA\User_SAML\Command; + +use OC\Core\Command\Base; +use OCA\User_SAML\SAMLSettings; +use Symfony\Component\Console\Input\InputArgument; +use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; +use Symfony\Component\Console\Output\OutputInterface; + +class ConfigSet extends Base { + + /** @var SAMLSettings */ + private $samlSettings; + + public function __construct(SAMLSettings $samlSettings) { + parent::__construct(); + $this->samlSettings = $samlSettings; + } + + protected function configure() { + $this->setName('saml:config:set'); + + $this->addArgument( + 'providerId', + InputArgument::REQUIRED, + 'ProviderID of the SAML config to edit' + ); + + foreach (SAMLSettings::IDP_CONFIG_KEYS as $key) { + $this->addOption( + $key, + null, + InputOption::VALUE_REQUIRED, + ); + } + + parent::configure(); + } + + protected function execute(InputInterface $input, OutputInterface $output): int { + $pId = (int)$input->getArgument('providerId'); + $settings = $this->samlSettings->get($pId); + + foreach ($input->getOptions() as $key => $value) { + if(!in_array($key, SAMLSettings::IDP_CONFIG_KEYS) || $value === null) { + continue; + } + if ($value === '') { + unset($settings[$key]); + continue; + } + $settings[$key] = $value; + } + $this->samlSettings->set($pId, $settings); + + return 0; + } +} diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index b7e9f0f..1d3ebf5 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -74,6 +74,15 @@ class FeatureContext implements Context { ) ); } + + shell_exec( + sprintf( + 'sudo -u apache %s %s saml:config:delete 1', + PHP_BINARY, + __DIR__ . '/../../../../../../occ', + ) + ); + $this->changedSettings = []; } @@ -85,14 +94,34 @@ class FeatureContext implements Context { */ public function theSettingIsSetTo($settingName, $value) { - $this->changedSettings[] = $settingName; + + if (in_array($settingName, [ + 'type', + 'general-require_provisioned_account', + 'general-allow_multiple_user_back_ends', + 'general-use_saml_auth_for_desktop' + ])) { + $this->changedSettings[] = $settingName; + shell_exec( + sprintf( + 'sudo -u apache %s %s config:app:set --value="%s" user_saml %s', + PHP_BINARY, + __DIR__ . '/../../../../../../occ', + $value, + $settingName + ) + ); + return; + } + shell_exec( sprintf( - 'sudo -u apache %s %s config:app:set --value="%s" user_saml %s', + 'sudo -u apache %s %s saml:config:set --"%s"="%s" %d', PHP_BINARY, __DIR__ . '/../../../../../../occ', + $settingName, $value, - $settingName + 1 ) ); } From 66759ce3ebd56106c7b0988e6ae521891812c1db Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 29 Nov 2021 16:55:56 +0100 Subject: [PATCH 03/13] add info to changelog Signed-off-by: Arthur Schiwon --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d74dc5..7b2de7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,13 @@ # Changelog All notable changes to this project will be documented in this file. +## 5.0.0 +### Changed +- store configurations in a separate database table, not appconfig + +### Added +- occ commands for modifying SAML configurations + ## 4.1.0 ### Added - Nextcloud 22 support From 24a632588caa53a8a657ba5a0d01bd7b4941381e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20Molakvo=C3=A6?= Date: Fri, 10 Dec 2021 08:16:06 +0100 Subject: [PATCH 04/13] Add regex routes requirement to providerId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: John Molakvoæ --- appinfo/routes.php | 11 +++- lib/Command/ConfigSet.php | 2 +- lib/Controller/SettingsController.php | 2 +- lib/Db/ConfigurationsMapper.php | 4 +- .../Version5000Date20211025124248.php | 3 +- lib/SAMLSettings.php | 2 +- lib/UserBackend.php | 50 +++++++++---------- lib/UserData.php | 17 ++++--- .../features/bootstrap/FeatureContext.php | 1 - tests/unit/Command/GetMetadataTest.php | 21 ++++---- tests/unit/Settings/AdminTest.php | 4 +- 11 files changed, 61 insertions(+), 56 deletions(-) diff --git a/appinfo/routes.php b/appinfo/routes.php index 3cb584c..ae6a88a 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -80,6 +80,9 @@ return [ 'verb' => 'GET', 'defaults' => [ 'providerId' => 1 + ], + 'requirements' => [ + 'providerId' => '\d+' ] ], [ @@ -88,6 +91,9 @@ return [ 'verb' => 'POST', 'defaults' => [ 'providerId' => 1 + ], + 'requirements' => [ + 'providerId' => '\d+' ] ], [ @@ -100,7 +106,10 @@ return [ 'url' => '/settings/providerSettings/{providerId}', 'verb' => 'DELETE', 'defaults' => [ - 'providerId' => '1' + 'providerId' => 1 + ], + 'requirements' => [ + 'providerId' => '\d+' ] ], [ diff --git a/lib/Command/ConfigSet.php b/lib/Command/ConfigSet.php index 8bc4af8..8f55f8b 100644 --- a/lib/Command/ConfigSet.php +++ b/lib/Command/ConfigSet.php @@ -68,7 +68,7 @@ class ConfigSet extends Base { $settings = $this->samlSettings->get($pId); foreach ($input->getOptions() as $key => $value) { - if(!in_array($key, SAMLSettings::IDP_CONFIG_KEYS) || $value === null) { + if (!in_array($key, SAMLSettings::IDP_CONFIG_KEYS) || $value === null) { continue; } if ($value === '') { diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 20e71b3..c35fb35 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -96,7 +96,7 @@ class SettingsController extends Controller { $key = $category . '-' . $setting; } - if (isset ($details['global']) && $details['global']) { + if (isset($details['global']) && $details['global']) { $settings[$category][$setting] = $this->config->getAppValue('user_saml', $key, ''); } else { $settings[$category][$setting] = $storedSettings[$key] ?? ''; diff --git a/lib/Db/ConfigurationsMapper.php b/lib/Db/ConfigurationsMapper.php index c1a6bee..709e736 100644 --- a/lib/Db/ConfigurationsMapper.php +++ b/lib/Db/ConfigurationsMapper.php @@ -45,7 +45,8 @@ class ConfigurationsMapper extends QBMapper { public function deleteById(int $id): void { $entity = new ConfigurationsEntity(); - $entity->setId($id);; + $entity->setId($id); + ; $this->delete($entity); } @@ -98,5 +99,4 @@ class ConfigurationsMapper extends QBMapper { $newEntity->importConfiguration([]); return $this->insert($newEntity)->getId(); } - } diff --git a/lib/Migration/Version5000Date20211025124248.php b/lib/Migration/Version5000Date20211025124248.php index 887e965..336c92b 100644 --- a/lib/Migration/Version5000Date20211025124248.php +++ b/lib/Migration/Version5000Date20211025124248.php @@ -16,7 +16,6 @@ use OCP\Migration\SimpleMigrationStep; * Auto-generated migration step: Please modify to your needs! */ class Version5000Date20211025124248 extends SimpleMigrationStep { - private const IDP_CONFIG_KEYS = [ 'general-idp0_display_name', 'general-uid_mapping', @@ -187,7 +186,7 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { $isPrefixed = \preg_match('/^[0-9]*-/', $prefixedKey, $matches); if ($isPrefixed === 0) { return $prefixedKey; - } else if ($isPrefixed === 1) { + } elseif ($isPrefixed === 1) { return \substr($prefixedKey, strlen($matches[0])); } throw new \RuntimeException('Invalid regex pattern'); diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php index 4c27c90..7a63586 100644 --- a/lib/SAMLSettings.php +++ b/lib/SAMLSettings.php @@ -241,7 +241,7 @@ class SAMLSettings { return; } - if ($idp !== -1) { + if ($idp !== -1) { $this->configurations[$idp] = $this->mapper->get($idp); } else { $configs = $this->mapper->getAll(); diff --git a/lib/UserBackend.php b/lib/UserBackend.php index 2461a70..5b0c6f8 100644 --- a/lib/UserBackend.php +++ b/lib/UserBackend.php @@ -107,8 +107,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @param string $uid * @param array $attributes */ - public function createUserIfNotExists($uid, array $attributes = array()) { - if(!$this->userExistsInDatabase($uid)) { + public function createUserIfNotExists($uid, array $attributes = []) { + if (!$this->userExistsInDatabase($uid)) { $values = [ 'uid' => $uid, ]; @@ -123,12 +123,12 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { if ($home !== '') { //if attribute's value is an absolute path take this, otherwise append it to data dir //check for / at the beginning or pattern c:\ resp. c:/ - if( '/' !== $home[0] + if ('/' !== $home[0] && !(3 < strlen($home) && ctype_alpha($home[0]) && $home[1] === ':' && ('\\' === $home[2] || '/' === $home[2])) ) { $home = $this->config->getSystemValue('datadirectory', - \OC::$SERVERROOT.'/data' ) . '/' . $home; + \OC::$SERVERROOT.'/data') . '/' . $home; } $values['home'] = $home; @@ -137,13 +137,12 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->insert('user_saml_users'); - foreach($values as $column => $value) { + foreach ($values as $column => $value) { $qb->setValue($column, $qb->createNamedParameter($value)); } $qb->execute(); $this->initializeHomeDir($uid); - } } @@ -203,8 +202,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { $data = $result->fetchAll(); $result->closeCursor(); - foreach($data as $passwords) { - if(password_verify($password, $passwords['token'])) { + foreach ($data as $passwords) { + if (password_verify($password, $passwords['token'])) { return $uid; } } @@ -219,7 +218,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function deleteUser($uid) { - if($this->userExistsInDatabase($uid)) { + if ($this->userExistsInDatabase($uid)) { /* @var $qb IQueryBuilder */ $qb = $this->db->getQueryBuilder(); $qb->delete('user_saml_users') @@ -237,7 +236,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @return string */ public function getHome($uid) { - if($this->userExistsInDatabase($uid)) { + if ($this->userExistsInDatabase($uid)) { $qb = $this->db->getQueryBuilder(); $qb->select('home') ->from('user_saml_users') @@ -277,7 +276,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function userExists($uid) { - if($backend = $this->getActualUserBackend($uid)) { + if ($backend = $this->getActualUserBackend($uid)) { return $backend->userExists($uid); } else { return $this->userExistsInDatabase($uid); @@ -285,7 +284,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { } public function setDisplayName($uid, $displayName) { - if($backend = $this->getActualUserBackend($uid)) { + if ($backend = $this->getActualUserBackend($uid)) { return $backend->setDisplayName($uid, $displayName); } @@ -309,10 +308,10 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function getDisplayName($uid) { - if($backend = $this->getActualUserBackend($uid)) { + if ($backend = $this->getActualUserBackend($uid)) { return $backend->getDisplayName($uid); } else { - if($this->userExistsInDatabase($uid)) { + if ($this->userExistsInDatabase($uid)) { $qb = $this->db->getQueryBuilder(); $qb->select('displayname') ->from('user_saml_users') @@ -374,7 +373,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @since 4.5.0 */ public function hasUserListings() { - if($this->autoprovisionAllowed()) { + if ($this->autoprovisionAllowed()) { return true; } @@ -487,14 +486,14 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { public function getCurrentUserId() { $user = \OC::$server->getUserSession()->getUser(); - if($user instanceof IUser && $this->session->get('user_saml.samlUserData')) { + if ($user instanceof IUser && $this->session->get('user_saml.samlUserData')) { $uid = $user->getUID(); } else { $this->userData->setAttributes($this->session->get('user_saml.samlUserData') ?? []); $uid = $this->userData->getEffectiveUid(); } - if($uid !== '' && $this->userExists($uid)) { + if ($uid !== '' && $this->userExists($uid)) { $this->session->set('last-password-confirm', strtotime('+4 year', time())); return $uid; } @@ -527,8 +526,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { * @return null|UserInterface */ public function getActualUserBackend($uid) { - foreach(self::$backends as $backend) { - if($backend->userExists($uid)) { + foreach (self::$backends as $backend) { + if ($backend->userExists($uid)) { return $backend; } } @@ -549,8 +548,7 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { /** * @throws \OCP\DB\Exception */ - private function getAttributeKeys($name) - { + private function getAttributeKeys($name) { $settings = $this->settings->get($this->settings->getProviderId()); $keys = explode(' ', $settings[$name] ?? $this->config->getAppValue('user_saml', $name, '')); @@ -564,17 +562,17 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { $keys = $this->getAttributeKeys($name); $value = ''; - foreach($keys as $key) { + foreach ($keys as $key) { if (isset($attributes[$key])) { if (is_array($attributes[$key])) { foreach ($attributes[$key] as $attribute_part_value) { - if($value !== '') { + if ($value !== '') { $value .= ' '; } $value .= $attribute_part_value; } } else { - if($value !== '') { + if ($value !== '') { $value .= ' '; } $value .= $attributes[$key]; @@ -588,8 +586,8 @@ class UserBackend implements IApacheBackend, UserInterface, IUserBackend { private function getAttributeArrayValue($name, array $attributes) { $keys = $this->getAttributeKeys($name); - $value = array(); - foreach($keys as $key) { + $value = []; + foreach ($keys as $key) { if (isset($attributes[$key])) { if (is_array($attributes[$key])) { $value = array_merge($value, array_values($attributes[$key])); diff --git a/lib/UserData.php b/lib/UserData.php index e0e0607..b4a360c 100644 --- a/lib/UserData.php +++ b/lib/UserData.php @@ -1,4 +1,5 @@ @@ -66,7 +67,7 @@ class UserData { } public function getEffectiveUid(): string { - if($this->uid !== null) { + if ($this->uid !== null) { return $this->uid; } $this->assertIsInitialized(); @@ -83,7 +84,7 @@ class UserData { protected function extractSamlUserId(): string { $uidMapping = $this->getUidMappingAttribute(); - if($uidMapping !== null && isset($this->attributes[$uidMapping])) { + if ($uidMapping !== null && isset($this->attributes[$uidMapping])) { if (is_array($this->attributes[$uidMapping])) { return trim($this->attributes[$uidMapping][0]); } else { @@ -105,13 +106,13 @@ class UserData { } $candidate = base64_decode($uid, true); - if($candidate === false) { + if ($candidate === false) { return $uid; } $candidate = $this->convertObjectGUID2Str($candidate); // the regex only matches the structure of the UUID, not its semantic // (i.e. version or variant) simply to be future compatible - if(preg_match('/^[a-f0-9]{8}(-[a-f0-9]{4}){4}[a-f0-9]{8}$/i', $candidate) === 1) { + if (preg_match('/^[a-f0-9]{8}(-[a-f0-9]{4}){4}[a-f0-9]{8}$/i', $candidate) === 1) { $uid = $candidate; } return $uid; @@ -123,15 +124,15 @@ class UserData { protected function convertObjectGUID2Str($oguid): string { $hex_guid = bin2hex($oguid); $hex_guid_to_guid_str = ''; - for($k = 1; $k <= 4; ++$k) { + for ($k = 1; $k <= 4; ++$k) { $hex_guid_to_guid_str .= substr($hex_guid, 8 - 2 * $k, 2); } $hex_guid_to_guid_str .= '-'; - for($k = 1; $k <= 2; ++$k) { + for ($k = 1; $k <= 2; ++$k) { $hex_guid_to_guid_str .= substr($hex_guid, 12 - 2 * $k, 2); } $hex_guid_to_guid_str .= '-'; - for($k = 1; $k <= 2; ++$k) { + for ($k = 1; $k <= 2; ++$k) { $hex_guid_to_guid_str .= substr($hex_guid, 16 - 2 * $k, 2); } $hex_guid_to_guid_str .= '-' . substr($hex_guid, 16, 4); @@ -141,7 +142,7 @@ class UserData { } protected function assertIsInitialized() { - if($this->attributes === null) { + if ($this->attributes === null) { throw new \LogicException('UserData have to be initialized with setAttributes first'); } } diff --git a/tests/integration/features/bootstrap/FeatureContext.php b/tests/integration/features/bootstrap/FeatureContext.php index 1d3ebf5..ec82d0c 100644 --- a/tests/integration/features/bootstrap/FeatureContext.php +++ b/tests/integration/features/bootstrap/FeatureContext.php @@ -94,7 +94,6 @@ class FeatureContext implements Context { */ public function theSettingIsSetTo($settingName, $value) { - if (in_array($settingName, [ 'type', 'general-require_provisioned_account', diff --git a/tests/unit/Command/GetMetadataTest.php b/tests/unit/Command/GetMetadataTest.php index bb0e9f5..3e76a0d 100644 --- a/tests/unit/Command/GetMetadataTest.php +++ b/tests/unit/Command/GetMetadataTest.php @@ -29,18 +29,18 @@ use Symfony\Component\Console\Output\OutputInterface; class GetMetadataTest extends \Test\TestCase { - /** @var GetMetadata|MockObject*/ - protected $GetMetadata; - /** @var SAMLSettings|MockObject*/ - private $samlSettings; + /** @var GetMetadata|MockObject*/ + protected $GetMetadata; + /** @var SAMLSettings|MockObject*/ + private $samlSettings; - protected function setUp(): void { - $this->samlSettings = $this->createMock(SAMLSettings::class); - $this->GetMetadata = new GetMetadata($this->samlSettings); + protected function setUp(): void { + $this->samlSettings = $this->createMock(SAMLSettings::class); + $this->GetMetadata = new GetMetadata($this->samlSettings); - parent::setUp(); - } - public function testGetMetadata(){ + parent::setUp(); + } + public function testGetMetadata() { $inputInterface = $this->createMock(InputInterface::class); $outputInterface = $this->createMock(OutputInterface::class); @@ -66,5 +66,4 @@ class GetMetadataTest extends \Test\TestCase { $this->invokePrivate($this->GetMetadata, 'execute', [$inputInterface, $outputInterface]); } - } diff --git a/tests/unit/Settings/AdminTest.php b/tests/unit/Settings/AdminTest.php index 6162455..c36cb6b 100644 --- a/tests/unit/Settings/AdminTest.php +++ b/tests/unit/Settings/AdminTest.php @@ -30,7 +30,7 @@ use OCP\IL10N; use OneLogin\Saml2\Constants; use PHPUnit\Framework\MockObject\MockObject; -class AdminTest extends \Test\TestCase { +class AdminTest extends \Test\TestCase { /** @var SAMLSettings|MockObject */ private $settings; /** @var Admin */ @@ -62,7 +62,7 @@ class AdminTest extends \Test\TestCase { $this->l10n ->expects($this->any()) ->method('t') - ->will($this->returnCallback(function($text, $parameters = array()) { + ->will($this->returnCallback(function ($text, $parameters = []) { return vsprintf($text, $parameters); })); From c51048b5666e6846fc520154f6248165a2da08ed Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Tue, 4 Jan 2022 16:50:09 +0100 Subject: [PATCH 05/13] Minor fixes Signed-off-by: Carl Schwan --- CHANGELOG.md | 3 + appinfo/routes.php | 4 +- js/admin.js | 6 +- lib/Command/ConfigDelete.php | 15 +++- lib/Command/ConfigSet.php | 15 +++- lib/Controller/SettingsController.php | 1 + lib/DavPlugin.php | 3 +- lib/Db/ConfigurationsMapper.php | 3 +- .../Version5000Date20211025124248.php | 88 +++++++++++-------- lib/SAMLSettings.php | 6 +- 10 files changed, 92 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b2de7f..5055ba9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ All notable changes to this project will be documented in this file. ### Added - occ commands for modifying SAML configurations +### Removed +- Ability to change SAML configuration with occ app-config, use the new occ commands instead + ## 4.1.0 ### Added - Nextcloud 22 support diff --git a/appinfo/routes.php b/appinfo/routes.php index ae6a88a..8e04e94 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -88,7 +88,7 @@ return [ [ 'name' => 'Settings#setProviderSetting', 'url' => '/settings/providerSettings/{providerId}', - 'verb' => 'POST', + 'verb' => 'PUT', 'defaults' => [ 'providerId' => 1 ], @@ -99,7 +99,7 @@ return [ [ 'name' => 'Settings#newSamlProviderSettingsId', 'url' => '/settings/providerSettings', - 'verb' => 'PUT', + 'verb' => 'POST', ], [ 'name' => 'Settings#deleteSamlProviderSettings', diff --git a/js/admin.js b/js/admin.js index 2732e54..589ece3 100644 --- a/js/admin.js +++ b/js/admin.js @@ -73,7 +73,7 @@ */ addProvider: function (callback) { var xhr = new XMLHttpRequest(); - xhr.open('PUT', OC.generateUrl('/apps/user_saml/settings/providerSettings')); + xhr.open('POST', OC.generateUrl('/apps/user_saml/settings/providerSettings')); xhr.setRequestHeader('Content-Type', 'application/json') xhr.setRequestHeader('requesttoken', OC.requestToken) @@ -95,7 +95,7 @@ updateProvider: function (configKey, configValue, successCb, errorCb) { var xhr = new XMLHttpRequest(); - xhr.open('POST', OC.generateUrl('/apps/user_saml/settings/providerSettings/' + this.currentConfig)); + xhr.open('PUT', OC.generateUrl('/apps/user_saml/settings/providerSettings/' + this.currentConfig)); xhr.setRequestHeader('Content-Type', 'application/json'); xhr.setRequestHeader('requesttoken', OC.requestToken); @@ -235,7 +235,7 @@ $(function() { }); }); $('input:checkbox[value="1"]').attr('checked', true); - $('input:checkbox[value="0"]').removeAttr('checked', false); + $('input:checkbox[value="0"]').prop('checked', false); var xmlDownloadButton = $('#get-metadata'); var url = xmlDownloadButton.data('base') + '?idp=' + providerId; xmlDownloadButton.attr('href', url); diff --git a/lib/Command/ConfigDelete.php b/lib/Command/ConfigDelete.php index 09310de..2ccd6ff 100644 --- a/lib/Command/ConfigDelete.php +++ b/lib/Command/ConfigDelete.php @@ -28,6 +28,7 @@ namespace OCA\User_SAML\Command; use OC\Core\Command\Base; use OCA\User_SAML\SAMLSettings; +use OCP\DB\Exception; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -53,7 +54,19 @@ class ConfigDelete extends Base { protected function execute(InputInterface $input, OutputInterface $output): int { $pId = (int)$input->getArgument('providerId'); - $this->samlSettings->delete($pId); + + if ((string)$pId !== $input->getArgument('providerId')) { + // Make sure we don't delete provider with id 0 by error + $output->writeln('providerId argument needs to be an number. Got: ' . $pId . ''); + return 1; + } + try { + $this->samlSettings->delete($pId); + $output->writeln('Provider deleted.'); + } catch (Exception $e) { + $output->writeln('Provider with id: ' . $providerId . ' does not exist.'); + return 1; + } return 0; } } diff --git a/lib/Command/ConfigSet.php b/lib/Command/ConfigSet.php index 8f55f8b..1e0a577 100644 --- a/lib/Command/ConfigSet.php +++ b/lib/Command/ConfigSet.php @@ -28,6 +28,7 @@ namespace OCA\User_SAML\Command; use OC\Core\Command\Base; use OCA\User_SAML\SAMLSettings; +use OCP\DB\Exception; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -65,7 +66,18 @@ class ConfigSet extends Base { protected function execute(InputInterface $input, OutputInterface $output): int { $pId = (int)$input->getArgument('providerId'); - $settings = $this->samlSettings->get($pId); + + if ((string)$pId !== $input->getArgument('providerId')) { + // Make sure we don't delete provider with id 0 by error + $output->writeln('providerId argument needs to be an number. Got: ' . $pId . ''); + return 1; + } + try { + $settings = $this->samlSettings->get($pId); + } catch (Exception $e) { + $output->writeln('Provider with id: ' . $providerId . ' does not exist.'); + return 1; + } foreach ($input->getOptions() as $key => $value) { if (!in_array($key, SAMLSettings::IDP_CONFIG_KEYS) || $value === null) { @@ -78,6 +90,7 @@ class ConfigSet extends Base { $settings[$key] = $value; } $this->samlSettings->set($pId, $settings); + $output->writeln('The provider\'s config was updated.'); return 0; } diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index c35fb35..3bc9891 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -97,6 +97,7 @@ class SettingsController extends Controller { } if (isset($details['global']) && $details['global']) { + // Read legacy data from oc_appconfig $settings[$category][$setting] = $this->config->getAppValue('user_saml', $key, ''); } else { $settings[$category][$setting] = $storedSettings[$key] ?? ''; diff --git a/lib/DavPlugin.php b/lib/DavPlugin.php index ebb18a4..505cd52 100644 --- a/lib/DavPlugin.php +++ b/lib/DavPlugin.php @@ -35,7 +35,8 @@ class DavPlugin extends ServerPlugin { private $auth; /** @var Server */ private $server; - private SAMLSettings $samlSettings; + /** @var SAMLSettings */ + private $samlSettings; public function __construct(ISession $session, IConfig $config, array $auth, SAMLSettings $samlSettings) { $this->session = $session; diff --git a/lib/Db/ConfigurationsMapper.php b/lib/Db/ConfigurationsMapper.php index 709e736..e4aaa27 100644 --- a/lib/Db/ConfigurationsMapper.php +++ b/lib/Db/ConfigurationsMapper.php @@ -46,7 +46,6 @@ class ConfigurationsMapper extends QBMapper { public function deleteById(int $id): void { $entity = new ConfigurationsEntity(); $entity->setId($id); - ; $this->delete($entity); } @@ -89,7 +88,7 @@ class ConfigurationsMapper extends QBMapper { try { $entity = $this->findEntity($qb); - $newId = $entity->getId() + 1; + $newId = $entity->getId() + 1; // autoincrement manually } catch (DoesNotExistException $e) { $newId = 1; } diff --git a/lib/Migration/Version5000Date20211025124248.php b/lib/Migration/Version5000Date20211025124248.php index 336c92b..764605d 100644 --- a/lib/Migration/Version5000Date20211025124248.php +++ b/lib/Migration/Version5000Date20211025124248.php @@ -50,21 +50,22 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { /** @var IDBConnection */ private $dbc; + /** @var ?IQueryBuilder */ + private $insertQuery; + + /** @var ?IQueryBuilder */ + private $deleteQuery; + + /** @var ?IQueryBuilder */ + private $readQuery; + public function __construct(IDBConnection $dbc) { $this->dbc = $dbc; } /** * @param IOutput $output - * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` - * @param array $options - */ - public function preSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { - } - - /** - * @param IOutput $output - * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param Closure():ISchemaWrapper $schemaClosure * @param array $options * @return null|ISchemaWrapper */ @@ -93,7 +94,7 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { /** * @param IOutput $output - * @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param Closure():IschemaWrapper $schemaClosure * @param array $options */ public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { @@ -117,6 +118,10 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { $gRows->next(); } + if (empty($configData)) { + continue; // No config found + } + if ($this->insertConfiguration($prefix, $configData) && !empty($keysToDelete)) { $this->deleteOldConfiguration($keysToDelete); } @@ -125,56 +130,60 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { $this->deletePrefixes(); } + /** + * @psalm-param list $keys the list of keys to delete + */ protected function deleteOldConfiguration(array $keys): bool { - static $deleteQuery; - if (!$deleteQuery) { - $deleteQuery = $this->dbc->getQueryBuilder(); + if (!$this->deleteQuery) { + $this->deleteQuery = $this->dbc->getQueryBuilder(); - $deleteQuery->delete('appconfig') - ->where($deleteQuery->expr()->eq('appid', $deleteQuery->createNamedParameter('user_saml'))) - ->andWhere($deleteQuery->expr()->in('configkey', $deleteQuery->createParameter('cfgKeys'))); + $this->deleteQuery->delete('appconfig') + ->where($this->deleteQuery->expr()->eq('appid', $this->deleteQuery->createNamedParameter('user_saml'))) + ->andWhere($this->deleteQuery->expr()->in('configkey', $this->deleteQuery->createParameter('cfgKeys'))); } - $deletedRows = $deleteQuery + $deletedRows = $this->deleteQuery ->setParameter('cfgKeys', $keys, IQueryBuilder::PARAM_STR_ARRAY) - ->executeStatement(); + ->execute(); return $deletedRows > 0; } + /** + * @param array $configData The key-value map of config to save + */ protected function insertConfiguration(int $id, array $configData): bool { - static $insertQuery; - if (!$insertQuery) { - $insertQuery = $this->dbc->getQueryBuilder(); - $insertQuery->insert('user_saml_configurations') + if (!$this->insertQuery) { + $this->insertQuery = $this->dbc->getQueryBuilder(); + $this->insertQuery->insert('user_saml_configurations') ->values([ - 'id' => $insertQuery->createParameter('configId'), - 'name' => $insertQuery->createParameter('displayName'), - 'configuration' => $insertQuery->createParameter('configuration'), + 'id' => $this->insertQuery->createParameter('configId'), + 'name' => $this->insertQuery->createParameter('displayName'), + 'configuration' => $this->insertQuery->createParameter('configuration'), ]); } - $insertedRows = $insertQuery + $insertedRows = $this->insertQuery ->setParameter('configId', $id) ->setParameter('displayName', $configData['general-idp0_display_name'] ?? '') ->setParameter('configuration', \json_encode($configData, JSON_THROW_ON_ERROR)) - ->executeStatement(); + ->execute(); return $insertedRows > 0; } + /** @psalm-param list $configKeys */ protected function readConfiguration(array $configKeys): \Generator { - static $readQuery; - if (!$readQuery) { - $readQuery = $this->dbc->getQueryBuilder(); + if (!$this->readQuery) { + $this->readQuery = $this->dbc->getQueryBuilder(); + $this->readQuery->select('configkey', 'configvalue') + ->from('appconfig') + ->where($this->readQuery->expr()->eq('appid', $this->readQuery->createNamedParameter('user_saml'))) + ->andWhere($this->readQuery->expr()->in('configkey', $this->readQuery->createParameter('cfgKeys'))); } - $readQuery->select('configkey', 'configvalue') - ->from('appconfig') - ->where($readQuery->expr()->eq('appid', $readQuery->createNamedParameter('user_saml'))) - ->andWhere($readQuery->expr()->in('configkey', $readQuery->createParameter('cfgKeys'))); - $r = $readQuery->setParameter('cfgKeys', $configKeys, IQueryBuilder::PARAM_STR_ARRAY) - ->executeQuery(); + $r = $this->readQuery->setParameter('cfgKeys', $configKeys, IQueryBuilder::PARAM_STR_ARRAY) + ->execute(); while ($row = $r->fetch()) { yield $row; @@ -192,6 +201,7 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { throw new \RuntimeException('Invalid regex pattern'); } + /** @psalm-return list */ protected function fetchPrefixes(): array { $q = $this->dbc->getQueryBuilder(); $q->select('configvalue') @@ -199,10 +209,10 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { ->where($q->expr()->eq('appid', $q->createNamedParameter('user_saml'))) ->andWhere($q->expr()->eq('configkey', $q->createNamedParameter('providerIds'))); - $r = $q->executeQuery(); + $r = $q->execute(); $prefixes = $r->fetchOne(); if ($prefixes === false) { - return []; + return [1]; // 1 is the default value for providerIds } return array_map('intval', explode(',', $prefixes)); } @@ -212,6 +222,6 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { $q->delete('appconfig') ->where($q->expr()->eq('appid', $q->createNamedParameter('user_saml'))) ->andWhere($q->expr()->eq('configkey', $q->createNamedParameter('providerIds'))) - ->executeStatement(); + ->execute(); } } diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php index 7a63586..894de91 100644 --- a/lib/SAMLSettings.php +++ b/lib/SAMLSettings.php @@ -104,7 +104,7 @@ class SAMLSettings { } /** - * get list of the configured IDPs + * Get list of the configured IDPs * * @return array * @throws Exception @@ -122,7 +122,7 @@ class SAMLSettings { } /** - * check if multiple user back ends are allowed + * Check if multiple user back ends are allowed */ public function allowMultipleUserBackEnds(): bool { $type = $this->config->getAppValue('user_saml', 'type'); @@ -135,7 +135,7 @@ class SAMLSettings { } /** - * get config for given IDP + * Get config for given IDP * * @throws Exception */ From 0000691857077433ecb2f21cbf0e3e0412fffd0c Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 12 Jan 2022 23:55:40 +0100 Subject: [PATCH 06/13] do not evaluate SAML cfg every time app is loaded - it tests only the first configuration, others were not taken into account - the configuration check is also only needed when SAML auth is actually happening Signed-off-by: Arthur Schiwon --- appinfo/app.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/appinfo/app.php b/appinfo/app.php index d1d3732..6df723b 100644 --- a/appinfo/app.php +++ b/appinfo/app.php @@ -68,11 +68,6 @@ $returnScript = false; $type = ''; switch ($config->getAppValue('user_saml', 'type')) { case 'saml': - try { - $oneLoginSettings = new \OneLogin\Saml2\Settings($samlSettings->getOneLoginSettingsArray(1)); - } catch (\OneLogin\SAML2\Error $e) { - $returnScript = true; - } $type = 'saml'; break; case 'environment-variable': From 7f0986c38734a3177daa168765fb7ea131f05697 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 26 Jan 2022 22:47:46 +0100 Subject: [PATCH 07/13] fix settings of first provider are not present on initial load Signed-off-by: Arthur Schiwon --- lib/Settings/Admin.php | 3 ++- templates/admin.php | 30 +++++++++++++++--------------- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index 64c328f..fc953ad 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -206,7 +206,8 @@ class Admin implements ISettings { 'attribute-mapping' => $attributeMappingSettings, 'name-id-formats' => $nameIdFormats, 'type' => $type, - 'providers' => $providers + 'providers' => $providers, + 'config' => isset($providers[0]) ? $this->samlSettings->get($providers[0]['id']) : null, ]; return new TemplateResponse('user_saml', 'admin', $params); diff --git a/templates/admin.php b/templates/admin.php index 4d03984..73a9ef5 100644 --- a/templates/admin.php +++ b/templates/admin.php @@ -55,12 +55,12 @@ style('user_saml', 'admin'); $attribute): ?>

- +

- class="required" placeholder=""/> + class="required" placeholder=""/>

@@ -85,12 +85,12 @@ style('user_saml', 'admin'); $attribute): ?>

- +

- class="required" placeholder=""/> + class="required" placeholder=""/>

@@ -118,7 +118,7 @@ style('user_saml', 'admin'); $text): ?>

- +

@@ -129,13 +129,13 @@ style('user_saml', 'admin'); t('Configure your IdP settings here.')) ?>

-

-

+

+

t('Show optional Identity Provider settings…')) ?>

@@ -151,7 +151,7 @@ style('user_saml', 'admin');

- class="required" placeholder=""/> + class="required" placeholder=""/>

@@ -168,14 +168,14 @@ style('user_saml', 'admin');

t('Signatures and encryption offered')) ?>

$text): ?>

- +

t('Signatures and encryption required')) ?>

$text): ?>

- +

@@ -185,12 +185,12 @@ style('user_saml', 'admin');


- class="required" placeholder="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/> + class="required" placeholder="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/>

- +

From 6548abb0f9aa58b4191aa145b33dfbf71f8f4ca6 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 25 Feb 2022 19:17:30 +0100 Subject: [PATCH 08/13] makes sloWebServerDecode IdP-sensitive as it should be Signed-off-by: Arthur Schiwon --- js/admin.js | 43 +++++++++---------- lib/Controller/SAMLController.php | 3 +- .../Version5000Date20211025124248.php | 1 + lib/SAMLSettings.php | 6 ++- 4 files changed, 27 insertions(+), 26 deletions(-) diff --git a/js/admin.js b/js/admin.js index 589ece3..d21dfe8 100644 --- a/js/admin.js +++ b/js/admin.js @@ -210,35 +210,34 @@ $(function() { $('.account-list li[data-id="' + providerId + '"]').addClass('active'); OCA.User_SAML.Admin.currentConfig = '' + providerId; $.get(OC.generateUrl('/apps/user_saml/settings/providerSettings/' + providerId)).done(function(data) { - Object.keys(data).forEach(function(category, index){ + Object.keys(data).forEach(function(category){ var entries = data[category]; Object.keys(entries).forEach(function (configKey) { - var element = $('#user-saml-settings *[data-key="' + configKey + '"]'); - if ($('#user-saml-settings #user-saml-' + category + ' #user-saml-' + configKey).length) { - element = $('#user-saml-' + category + ' #user-saml-' + configKey); + var htmlElement = document.querySelector('#user-saml-settings *[data-key="' + configKey + '"]') + || document.querySelector('#user-saml-' + category + ' #user-saml-' + configKey) + || document.querySelector('#user-saml-' + category + ' [name="' + configKey + '"]'); + + if (!htmlElement) { + console.log("could not find element for " + configKey); + return; } - if ($('#user-saml-settings #user-saml-' + category + ' [name="' + configKey + '"]').length) { - element = $('#user-saml-' + category + ' [name="' + configKey + '"]'); - } - if(element.is('input') && element.prop('type') === 'text') { - element.val(entries[configKey]) - } - else if(element.is('textarea')) { - element.val(entries[configKey]); - } - else if(element.prop('type') === 'checkbox') { - var value = entries[configKey] === '1' ? '1' : '0'; - element.val(value); + + if ((htmlElement.tagName === 'INPUT' && htmlElement.getAttribute('type') === 'text') + || htmlElement.tagName === 'TEXTAREA' + ) { + htmlElement.nodeValue = entries[configKey]; + } else if (htmlElement.tagName === 'INPUT' && htmlElement.getAttribute('type') === 'checkbox') { + htmlElement.checked = entries[configKey] === '1'; + htmlElement.setAttribute('value', entries[configKey] === '1' ? '1' : '0'); } else { - console.log('unable to find element for ' + configKey); + console.error("Could not handle " + configKey + " Tag is " + htmlElement.tagName + " and type is " + htmlElement.getAttribute("type")); } }); }); - $('input:checkbox[value="1"]').attr('checked', true); - $('input:checkbox[value="0"]').prop('checked', false); - var xmlDownloadButton = $('#get-metadata'); - var url = xmlDownloadButton.data('base') + '?idp=' + providerId; - xmlDownloadButton.attr('href', url); + + var xmlDownloadButton = document.getElementById('get-metadata'); + var url = xmlDownloadButton.dataset.base + '?idp=' + providerId; + xmlDownloadButton.setAttribute('href', url); }); }; diff --git a/lib/Controller/SAMLController.php b/lib/Controller/SAMLController.php index ab3609c..4f35430 100644 --- a/lib/Controller/SAMLController.php +++ b/lib/Controller/SAMLController.php @@ -418,7 +418,7 @@ class SAMLController extends Controller { $targetUrl = $auth->processSLO( $keepLocalSession, null, - $this->samlSettings->usesSloWebServerDecode(), + $this->samlSettings->usesSloWebServerDecode($idp), null, $stay ); @@ -432,7 +432,6 @@ class SAMLController extends Controller { } } else { // If request is not from IDP, we send the logout request to the IDP - $parameters = []; $nameId = $this->session->get('user_saml.samlNameId'); $nameIdFormat = $this->session->get('user_saml.samlNameIdFormat'); $nameIdNameQualifier = $this->session->get('user_saml.samlNameIdNameQualifier'); diff --git a/lib/Migration/Version5000Date20211025124248.php b/lib/Migration/Version5000Date20211025124248.php index 764605d..a14f15e 100644 --- a/lib/Migration/Version5000Date20211025124248.php +++ b/lib/Migration/Version5000Date20211025124248.php @@ -34,6 +34,7 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { 'security-required', 'security-signatureAlgorithm', 'security-signMetadata', + 'security-sloWebServerDecode', 'security-wantAssertionsEncrypted', 'security-wantAssertionsSigned', 'security-wantMessagesSigned', diff --git a/lib/SAMLSettings.php b/lib/SAMLSettings.php index 894de91..be047c1 100644 --- a/lib/SAMLSettings.php +++ b/lib/SAMLSettings.php @@ -130,8 +130,9 @@ class SAMLSettings { return ($setting === '1' && $type === 'saml'); } - public function usesSloWebServerDecode(): bool { - return $this->config->getAppValue('user_saml', 'security-sloWebServerDecode', '0') === '1'; + public function usesSloWebServerDecode(int $idp): bool { + $config = $this->get($idp); + return ($config['security-sloWebServerDecode'] ?? false) === '1'; } /** @@ -161,6 +162,7 @@ class SAMLSettings { 'requestedAuthnContext' => false, 'lowercaseUrlencoding' => ($this->configurations[$idp]['security-lowercaseUrlencoding'] ?? '0') === '1', 'signatureAlgorithm' => $this->configurations[$idp]['security-signatureAlgorithm'] ?? null, + // "sloWebServerDecode" is not expected to be passed to the OneLogin class ], 'sp' => [ 'entityId' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.getMetadata'), From ee8845252af954017ce4ecaec6c124fa20ddeaac Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 28 Feb 2022 21:56:07 +0100 Subject: [PATCH 09/13] also migrate sp-x509cert, sp-name-id-format, sp-privateKey Signed-off-by: Arthur Schiwon --- lib/Migration/Version5000Date20211025124248.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Migration/Version5000Date20211025124248.php b/lib/Migration/Version5000Date20211025124248.php index a14f15e..79252a0 100644 --- a/lib/Migration/Version5000Date20211025124248.php +++ b/lib/Migration/Version5000Date20211025124248.php @@ -46,6 +46,9 @@ class Version5000Date20211025124248 extends SimpleMigrationStep { 'saml-attribute-mapping-group_mapping', 'saml-attribute-mapping-home_mapping', 'saml-attribute-mapping-quota_mapping', + 'sp-x509cert', + 'sp-name-id-format', + 'sp-privateKey', ]; /** @var IDBConnection */ From 4c97efc51b77cdd5fe08b0868f22587cf632d923 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 28 Feb 2022 22:45:38 +0100 Subject: [PATCH 10/13] fix reading and updated name-id-format selection Signed-off-by: Arthur Schiwon --- js/admin.js | 2 ++ lib/Controller/SettingsController.php | 11 ++++++++++- lib/Settings/Admin.php | 8 +++++++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/js/admin.js b/js/admin.js index d21dfe8..346c6e1 100644 --- a/js/admin.js +++ b/js/admin.js @@ -229,6 +229,8 @@ $(function() { } else if (htmlElement.tagName === 'INPUT' && htmlElement.getAttribute('type') === 'checkbox') { htmlElement.checked = entries[configKey] === '1'; htmlElement.setAttribute('value', entries[configKey] === '1' ? '1' : '0'); + } else if (htmlElement.tagName === 'SELECT') { + htmlElement.querySelector('[value="' + entries[configKey] + '"]').selected = true; } else { console.error("Could not handle " + configKey + " Tag is " + htmlElement.tagName + " and type is " + htmlElement.getAttribute("type")); } diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 3bc9891..49fc797 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -30,6 +30,7 @@ use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\Http\Response; use OCP\IConfig; use OCP\IRequest; +use OneLogin\Saml2\Constants; class SettingsController extends Controller { @@ -76,7 +77,9 @@ class SettingsController extends Controller { 'x509cert' => ['required' => false], ]; /* Fetch all config values for the given providerId */ - $settings = []; + + // initialize settings with default value for option box (others are left empty) + $settings['sp']['name-id-format'] = Constants::NAMEID_UNSPECIFIED; $storedSettings = $this->samlSettings->get($providerId); foreach ($params as $category => $content) { if (empty($content) || $category === 'providers' || $category === 'type') { @@ -92,6 +95,12 @@ class SettingsController extends Controller { if (strpos($category, 'attribute-mapping') === 0) { $category = 'attribute-mapping'; $key = 'saml-attribute-mapping' . '-' . $setting; + } else if($category === 'name-id-formats') { + if ($setting === $storedSettings['sp-name-id-format']) { + $settings['sp']['name-id-format'] = $storedSettings['sp-name-id-format']; + //continue 2; + } + continue; } else { $key = $category . '-' . $setting; } diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index fc953ad..97cc844 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -138,6 +138,7 @@ class Admin implements ISettings { ]; + $firstIdPConfig = isset($providers[0]) ? $this->samlSettings->get($providers[0]['id']) : null; $nameIdFormats = [ Constants::NAMEID_EMAIL_ADDRESS => [ 'label' => $this->l10n->t('Email address'), @@ -176,6 +177,11 @@ class Admin implements ISettings { 'selected' => false, ], ]; + if ($firstIdPConfig !== null && isset($nameIdFormats[$firstIdPConfig['sp-name-id-format']])) { + $nameIdFormats[$firstIdPConfig['sp-name-id-format']]['selected'] = true; + } else { + $nameIdFormats[Constants::NAMEID_UNSPECIFIED]['selected'] = true; + } $type = $this->config->getAppValue('user_saml', 'type'); if ($type === 'saml') { @@ -207,7 +213,7 @@ class Admin implements ISettings { 'name-id-formats' => $nameIdFormats, 'type' => $type, 'providers' => $providers, - 'config' => isset($providers[0]) ? $this->samlSettings->get($providers[0]['id']) : null, + 'config' => $firstIdPConfig, ]; return new TemplateResponse('user_saml', 'admin', $params); From 97c0594ab0ac41a1e0186637ab589016af61311d Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 1 Mar 2022 10:15:20 +0100 Subject: [PATCH 11/13] code style Signed-off-by: Arthur Schiwon --- lib/Controller/SettingsController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 49fc797..384e6db 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -95,7 +95,7 @@ class SettingsController extends Controller { if (strpos($category, 'attribute-mapping') === 0) { $category = 'attribute-mapping'; $key = 'saml-attribute-mapping' . '-' . $setting; - } else if($category === 'name-id-formats') { + } elseif ($category === 'name-id-formats') { if ($setting === $storedSettings['sp-name-id-format']) { $settings['sp']['name-id-format'] = $storedSettings['sp-name-id-format']; //continue 2; From 77b14b6c6f1186b729f716dd87cc03172ffe7c57 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 1 Mar 2022 19:30:16 +0100 Subject: [PATCH 12/13] fix old settings present when switching providers - wrongly used way to set value attribute Signed-off-by: Arthur Schiwon --- js/admin.js | 14 +++++++++++++- lib/Settings/Admin.php | 5 +++-- tests/unit/Settings/AdminTest.php | 3 ++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/js/admin.js b/js/admin.js index 346c6e1..0764ca8 100644 --- a/js/admin.js +++ b/js/admin.js @@ -210,6 +210,17 @@ $(function() { $('.account-list li[data-id="' + providerId + '"]').addClass('active'); OCA.User_SAML.Admin.currentConfig = '' + providerId; $.get(OC.generateUrl('/apps/user_saml/settings/providerSettings/' + providerId)).done(function(data) { + document.querySelectorAll('#user-saml-settings input[type="text"], #user-saml-settings textarea').forEach(function (inputNode) { + inputNode.value = ''; + }); + document.querySelectorAll('#user-saml-settings input[type="checkbox"]').forEach(function (inputNode) { + inputNode.checked = false; + inputNode.setAttribute('value', '0'); + }); + document.querySelectorAll('#user-saml-settings select option').forEach(function (inputNode) { + inputNode.selected = false; + }); + Object.keys(data).forEach(function(category){ var entries = data[category]; Object.keys(entries).forEach(function (configKey) { @@ -226,9 +237,10 @@ $(function() { || htmlElement.tagName === 'TEXTAREA' ) { htmlElement.nodeValue = entries[configKey]; + htmlElement.value = entries[configKey]; } else if (htmlElement.tagName === 'INPUT' && htmlElement.getAttribute('type') === 'checkbox') { htmlElement.checked = entries[configKey] === '1'; - htmlElement.setAttribute('value', entries[configKey] === '1' ? '1' : '0'); + htmlElement.value = entries[configKey] === '1' ? '1' : '0'; } else if (htmlElement.tagName === 'SELECT') { htmlElement.querySelector('[value="' + entries[configKey] + '"]').selected = true; } else { diff --git a/lib/Settings/Admin.php b/lib/Settings/Admin.php index 97cc844..f579fc9 100644 --- a/lib/Settings/Admin.php +++ b/lib/Settings/Admin.php @@ -177,8 +177,9 @@ class Admin implements ISettings { 'selected' => false, ], ]; - if ($firstIdPConfig !== null && isset($nameIdFormats[$firstIdPConfig['sp-name-id-format']])) { - $nameIdFormats[$firstIdPConfig['sp-name-id-format']]['selected'] = true; + $chosenFormat = $firstIdPConfig['sp-name-id-format'] ?? ''; + if ($firstIdPConfig !== null && isset($nameIdFormats[$chosenFormat])) { + $nameIdFormats[$chosenFormat]['selected'] = true; } else { $nameIdFormats[Constants::NAMEID_UNSPECIFIED]['selected'] = true; } diff --git a/tests/unit/Settings/AdminTest.php b/tests/unit/Settings/AdminTest.php index c36cb6b..cbc99a7 100644 --- a/tests/unit/Settings/AdminTest.php +++ b/tests/unit/Settings/AdminTest.php @@ -176,7 +176,7 @@ class AdminTest extends \Test\TestCase { ], Constants::NAMEID_UNSPECIFIED => [ 'label' => 'Unspecified', - 'selected' => false, + 'selected' => true, ], Constants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME => [ 'label' => 'Windows domain qualified name', @@ -200,6 +200,7 @@ class AdminTest extends \Test\TestCase { ['id' => 2, 'name' => 'Provider 2'] ], 'name-id-formats' => $nameIdFormats, + 'config' => [], ]; return $params; From e514fed6b212c4baa0423f229a1f191f86edb7e6 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 1 Mar 2022 19:58:04 +0100 Subject: [PATCH 13/13] do not sort sorted provider ids Signed-off-by: Arthur Schiwon --- js/admin.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/admin.js b/js/admin.js index 0764ca8..57f1eb9 100644 --- a/js/admin.js +++ b/js/admin.js @@ -19,7 +19,7 @@ if (xhr.status >= 200 && xhr.status < 300) { if (response.providerIds !== "") { OCA.User_SAML.Admin.providerIds += ',' + response.providerIds; - OCA.User_SAML.Admin.currentConfig = OCA.User_SAML.Admin.providerIds.split(',').sort()[0]; + OCA.User_SAML.Admin.currentConfig = OCA.User_SAML.Admin.providerIds.split(',')[0]; } callback(); } else {