Minor fixes

Signed-off-by: Carl Schwan <carl@carlschwan.eu>

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

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',

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);

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('<error>providerId argument needs to be an number. Got: ' . $pId . '</error>');
+			return 1;
+		}
+		try {
+			$this->samlSettings->delete($pId);
+			$output->writeln('Provider deleted.');
+		} catch (Exception $e) {
+			$output->writeln('<error>Provider with id: ' . $providerId . ' does not exist.</error>');
+			return 1;
+		}
 		return 0;
 	}
 }

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('<error>providerId argument needs to be an number. Got: ' . $pId . '</error>');
+			return 1;
+		}
+		try {
+			$settings = $this->samlSettings->get($pId);
+		} catch (Exception $e) {
+			$output->writeln('<error>Provider with id: ' . $providerId . ' does not exist.</error>');
+			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;
 	}

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] ?? '';

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;

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;
 		}

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 Closure():ISchemaWrapper $schemaClosure
-	 * @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
 	 */
@@ -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<string> $keys the list of keys to delete
+	 */
 	protected function deleteOldConfiguration(array $keys): bool {
-		static $deleteQuery;
+		if (!$this->deleteQuery) {
-		if (!$deleteQuery) {
+			$this->deleteQuery = $this->dbc->getQueryBuilder();
-			$deleteQuery = $this->dbc->getQueryBuilder();
 
-			$deleteQuery->delete('appconfig')
+			$this->deleteQuery->delete('appconfig')
-				->where($deleteQuery->expr()->eq('appid', $deleteQuery->createNamedParameter('user_saml')))
+				->where($this->deleteQuery->expr()->eq('appid', $this->deleteQuery->createNamedParameter('user_saml')))
-				->andWhere($deleteQuery->expr()->in('configkey', $deleteQuery->createParameter('cfgKeys')));
+				->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<string, string> $configData The key-value map of config to save
+	 */
 	protected function insertConfiguration(int $id, array $configData): bool {
-		static $insertQuery;
+		if (!$this->insertQuery) {
-		if (!$insertQuery) {
+			$this->insertQuery = $this->dbc->getQueryBuilder();
-			$insertQuery = $this->dbc->getQueryBuilder();
+			$this->insertQuery->insert('user_saml_configurations')
-			$insertQuery->insert('user_saml_configurations')
 				->values([
-					'id' => $insertQuery->createParameter('configId'),
+					'id' => $this->insertQuery->createParameter('configId'),
-					'name' => $insertQuery->createParameter('displayName'),
+					'name' => $this->insertQuery->createParameter('displayName'),
-					'configuration' => $insertQuery->createParameter('configuration'),
+					'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<string> $configKeys */
 	protected function readConfiguration(array $configKeys): \Generator {
-		static $readQuery;
+		if (!$this->readQuery) {
-		if (!$readQuery) {
+			$this->readQuery = $this->dbc->getQueryBuilder();
-			$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)
+		$r = $this->readQuery->setParameter('cfgKeys', $configKeys, IQueryBuilder::PARAM_STR_ARRAY)
-			->executeQuery();
+			->execute();
 
 		while ($row = $r->fetch()) {
 			yield $row;
@@ -192,6 +201,7 @@ class Version5000Date20211025124248 extends SimpleMigrationStep {
 		throw new \RuntimeException('Invalid regex pattern');
 	}
 
+	/** @psalm-return list<int> */
 	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();
 	}
 }

lib/SAMLSettings.php

@@ -104,7 +104,7 @@ class SAMLSettings {
 	}
 
 	/**
-	 * get list of the configured IDPs
+	 * Get list of the configured IDPs
 	 *
 	 * @return array<int, string>
 	 * @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
 	 */