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 */