From 7629d4df1775c6e1a646ca8d9b5df970a174161e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 27 Jan 2025 16:23:14 +0100 Subject: [PATCH 1/5] feat(user_ldap): Improve error detail when saving an incorrect configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/user_ldap/ajax/testConfiguration.php | 9 +- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/user_ldap/lib/Connection.php | 82 +++++++++++-------- .../ConfigurationIssueException.php | 15 ++++ 5 files changed, 74 insertions(+), 34 deletions(-) create mode 100644 apps/user_ldap/lib/Exceptions/ConfigurationIssueException.php diff --git a/apps/user_ldap/ajax/testConfiguration.php b/apps/user_ldap/ajax/testConfiguration.php index b91eab8fd92..189949d515a 100644 --- a/apps/user_ldap/ajax/testConfiguration.php +++ b/apps/user_ldap/ajax/testConfiguration.php @@ -23,11 +23,16 @@ $connection = new \OCA\User_LDAP\Connection($ldapWrapper, $_POST['ldap_servercon try { $configurationOk = true; + $configurationError = ''; $conf = $connection->getConfiguration(); if ($conf['ldap_configuration_active'] === '0') { //needs to be true, otherwise it will also fail with an irritating message $conf['ldap_configuration_active'] = '1'; - $configurationOk = $connection->setConfiguration($conf); + try { + $configurationOk = $connection->setConfiguration($conf, throw:true); + } catch (ConfigurationIssueException $e) { + $configurationError = $e->getHint(); + } } if ($configurationOk) { //Configuration is okay @@ -64,7 +69,7 @@ try { } } else { \OC_JSON::error(['message' - => $l->t('Invalid configuration. Please have a look at the logs for further details.')]); + => $l->t('Invalid configuration: %s', $configurationError)]); } } catch (\Exception $e) { \OC_JSON::error(['message' => $e->getMessage()]); diff --git a/apps/user_ldap/composer/composer/autoload_classmap.php b/apps/user_ldap/composer/composer/autoload_classmap.php index 48472f36b65..36259880928 100644 --- a/apps/user_ldap/composer/composer/autoload_classmap.php +++ b/apps/user_ldap/composer/composer/autoload_classmap.php @@ -36,6 +36,7 @@ return array( 'OCA\\User_LDAP\\Events\\GroupBackendRegistered' => $baseDir . '/../lib/Events/GroupBackendRegistered.php', 'OCA\\User_LDAP\\Events\\UserBackendRegistered' => $baseDir . '/../lib/Events/UserBackendRegistered.php', 'OCA\\User_LDAP\\Exceptions\\AttributeNotSet' => $baseDir . '/../lib/Exceptions/AttributeNotSet.php', + 'OCA\\User_LDAP\\Exceptions\\ConfigurationIssueException' => $baseDir . '/../lib/Exceptions/ConfigurationIssueException.php', 'OCA\\User_LDAP\\Exceptions\\ConstraintViolationException' => $baseDir . '/../lib/Exceptions/ConstraintViolationException.php', 'OCA\\User_LDAP\\Exceptions\\NoMoreResults' => $baseDir . '/../lib/Exceptions/NoMoreResults.php', 'OCA\\User_LDAP\\Exceptions\\NotOnLDAP' => $baseDir . '/../lib/Exceptions/NotOnLDAP.php', diff --git a/apps/user_ldap/composer/composer/autoload_static.php b/apps/user_ldap/composer/composer/autoload_static.php index fc67caabce6..be985838393 100644 --- a/apps/user_ldap/composer/composer/autoload_static.php +++ b/apps/user_ldap/composer/composer/autoload_static.php @@ -51,6 +51,7 @@ class ComposerStaticInitUser_LDAP 'OCA\\User_LDAP\\Events\\GroupBackendRegistered' => __DIR__ . '/..' . '/../lib/Events/GroupBackendRegistered.php', 'OCA\\User_LDAP\\Events\\UserBackendRegistered' => __DIR__ . '/..' . '/../lib/Events/UserBackendRegistered.php', 'OCA\\User_LDAP\\Exceptions\\AttributeNotSet' => __DIR__ . '/..' . '/../lib/Exceptions/AttributeNotSet.php', + 'OCA\\User_LDAP\\Exceptions\\ConfigurationIssueException' => __DIR__ . '/..' . '/../lib/Exceptions/ConfigurationIssueException.php', 'OCA\\User_LDAP\\Exceptions\\ConstraintViolationException' => __DIR__ . '/..' . '/../lib/Exceptions/ConstraintViolationException.php', 'OCA\\User_LDAP\\Exceptions\\NoMoreResults' => __DIR__ . '/..' . '/../lib/Exceptions/NoMoreResults.php', 'OCA\\User_LDAP\\Exceptions\\NotOnLDAP' => __DIR__ . '/..' . '/../lib/Exceptions/NotOnLDAP.php', diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php index e7f2a413643..ed6ce1e632e 100644 --- a/apps/user_ldap/lib/Connection.php +++ b/apps/user_ldap/lib/Connection.php @@ -8,10 +8,12 @@ namespace OCA\User_LDAP; use OC\ServerNotAvailableException; +use OCA\User_LDAP\Exceptions\ConfigurationIssueException; use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; use OCP\IDBConnection; +use OCP\IL10N; use OCP\Server; use Psr\Log\LoggerInterface; @@ -134,8 +136,8 @@ class Connection extends LDAPUtility { */ protected $bindResult = []; - /** @var LoggerInterface */ - protected $logger; + protected LoggerInterface $logger; + private IL10N $l10n; /** * Constructor @@ -157,6 +159,7 @@ class Connection extends LDAPUtility { $this->doNotValidate = !in_array($this->configPrefix, $helper->getServerConfigurationPrefixes()); $this->logger = Server::get(LoggerInterface::class); + $this->l10n = Server::get(IL10N::class); } public function __destruct() { @@ -332,16 +335,17 @@ class Connection extends LDAPUtility { * set LDAP configuration with values delivered by an array, not read from configuration * @param array $config array that holds the config parameters in an associated array * @param array &$setParameters optional; array where the set fields will be given to + * @param bool $throw if true, throw ConfigurationIssueException with details instead of returning false * @return bool true if config validates, false otherwise. Check with $setParameters for detailed success on single parameters */ - public function setConfiguration($config, &$setParameters = null): bool { + public function setConfiguration(array $config, ?array &$setParameters = null, bool $throw = false): bool { if (is_null($setParameters)) { $setParameters = []; } $this->doNotValidate = false; $this->configuration->setConfiguration($config, $setParameters); if (count($setParameters) > 0) { - $this->configured = $this->validateConfiguration(); + $this->configured = $this->validateConfiguration($throw); } @@ -447,9 +451,11 @@ class Connection extends LDAPUtility { } } - private function doCriticalValidation(): bool { + /** + * @throws ConfigurationIssueException + */ + private function doCriticalValidation(): void { $configurationOK = true; - $errorStr = 'Configuration Error (prefix ' . $this->configPrefix . '): '; //options that shall not be empty $options = ['ldapHost', 'ldapUserDisplayName', @@ -484,9 +490,9 @@ class Connection extends LDAPUtility { break; } $configurationOK = false; - $this->logger->warning( - $errorStr . 'No ' . $subj . ' given!', - ['app' => 'user_ldap'] + throw new ConfigurationIssueException( + 'No ' . $subj . ' given!', + $this->l10n->t('Mandatory field "%s" left empty', $subj), ); } } @@ -494,16 +500,19 @@ class Connection extends LDAPUtility { //combinations $agent = $this->configuration->ldapAgentName; $pwd = $this->configuration->ldapAgentPassword; - if ( - ($agent === '' && $pwd !== '') - || ($agent !== '' && $pwd === '') - ) { - $this->logger->warning( - $errorStr . 'either no password is given for the user ' . - 'agent or a password is given, but not an LDAP agent.', - ['app' => 'user_ldap'] - ); + if ($agent === '' && $pwd !== '') { $configurationOK = false; + throw new ConfigurationIssueException( + 'A password is given, but not an LDAP agent', + $this->l10n->t('A password is given, but not an LDAP agent'), + ); + } + if ($agent !== '' && $pwd === '') { + $configurationOK = false; + throw new ConfigurationIssueException( + 'No password is given for the user agent', + $this->l10n->t('No password is given for the user agent'), + ); } $base = $this->configuration->ldapBase; @@ -511,30 +520,27 @@ class Connection extends LDAPUtility { $baseGroups = $this->configuration->ldapBaseGroups; if (empty($base) && empty($baseUsers) && empty($baseGroups)) { - $this->logger->warning( - $errorStr . 'Not a single Base DN given.', - ['app' => 'user_ldap'] - ); $configurationOK = false; + throw new ConfigurationIssueException( + 'Not a single Base DN given.', + $this->l10n->t('No LDAP base DN was given'), + ); } - if (mb_strpos((string)$this->configuration->ldapLoginFilter, '%uid', 0, 'UTF-8') - === false) { - $this->logger->warning( - $errorStr . 'login filter does not contain %uid place holder.', - ['app' => 'user_ldap'] - ); + if (mb_strpos((string)$this->configuration->ldapLoginFilter, '%uid', 0, 'UTF-8') === false) { $configurationOK = false; + throw new ConfigurationIssueException( + 'Login filter does not contain %uid place holder.', + $this->l10n->t('Login filter does not contain %uid place holder'), + ); } - - return $configurationOK; } /** * Validates the user specified configuration * @return bool true if configuration seems OK, false otherwise */ - private function validateConfiguration(): bool { + private function validateConfiguration(bool $throw = false): bool { if ($this->doNotValidate) { //don't do a validation if it is a new configuration with pure //default values. Will be allowed on changes via __set or @@ -548,7 +554,19 @@ class Connection extends LDAPUtility { //second step: critical checks. If left empty or filled wrong, mark as //not configured and give a warning. - return $this->doCriticalValidation(); + try { + $this->doCriticalValidation(); + return true; + } catch (ConfigurationIssueException $e) { + if ($throw) { + throw $e; + } + $this->logger->warning( + 'Configuration Error (prefix ' . $this->configPrefix . '): ' . $e->getMessage(), + ['exception' => $e] + ); + return false; + } } diff --git a/apps/user_ldap/lib/Exceptions/ConfigurationIssueException.php b/apps/user_ldap/lib/Exceptions/ConfigurationIssueException.php new file mode 100644 index 00000000000..efeb426b13d --- /dev/null +++ b/apps/user_ldap/lib/Exceptions/ConfigurationIssueException.php @@ -0,0 +1,15 @@ + Date: Tue, 28 Jan 2025 10:08:08 +0100 Subject: [PATCH 2/5] fix(user_ldap): Add missing use in ajax endpoint and fix L10N injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/user_ldap/ajax/testConfiguration.php | 1 + apps/user_ldap/lib/Connection.php | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/user_ldap/ajax/testConfiguration.php b/apps/user_ldap/ajax/testConfiguration.php index 189949d515a..480209354b5 100644 --- a/apps/user_ldap/ajax/testConfiguration.php +++ b/apps/user_ldap/ajax/testConfiguration.php @@ -1,5 +1,6 @@ doNotValidate = !in_array($this->configPrefix, $helper->getServerConfigurationPrefixes()); $this->logger = Server::get(LoggerInterface::class); - $this->l10n = Server::get(IL10N::class); + $this->l10n = Util::getL10N('user_ldap'); } public function __destruct() { From 36d756ab0f3cf8a6037b050015073245719fcccc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 28 Jan 2025 10:59:59 +0100 Subject: [PATCH 3/5] fix(user_ldap): Check that all user and group bases are in the global one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/user_ldap/ajax/testConfiguration.php | 13 ++++--- apps/user_ldap/lib/Connection.php | 44 ++++++++++++++++++----- 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/apps/user_ldap/ajax/testConfiguration.php b/apps/user_ldap/ajax/testConfiguration.php index 480209354b5..4cf7bb1ba65 100644 --- a/apps/user_ldap/ajax/testConfiguration.php +++ b/apps/user_ldap/ajax/testConfiguration.php @@ -23,19 +23,18 @@ $connection = new \OCA\User_LDAP\Connection($ldapWrapper, $_POST['ldap_servercon try { - $configurationOk = true; $configurationError = ''; $conf = $connection->getConfiguration(); if ($conf['ldap_configuration_active'] === '0') { //needs to be true, otherwise it will also fail with an irritating message $conf['ldap_configuration_active'] = '1'; - try { - $configurationOk = $connection->setConfiguration($conf, throw:true); - } catch (ConfigurationIssueException $e) { - $configurationError = $e->getHint(); - } } - if ($configurationOk) { + try { + $connection->setConfiguration($conf, throw:true); + } catch (ConfigurationIssueException $e) { + $configurationError = $e->getHint(); + } + if ($configurationError === '') { //Configuration is okay /* * Closing the session since it won't be used from this point on. There might be a potential diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php index b3adbfcc397..14dfcdb1bc5 100644 --- a/apps/user_ldap/lib/Connection.php +++ b/apps/user_ldap/lib/Connection.php @@ -456,8 +456,6 @@ class Connection extends LDAPUtility { * @throws ConfigurationIssueException */ private function doCriticalValidation(): void { - $configurationOK = true; - //options that shall not be empty $options = ['ldapHost', 'ldapUserDisplayName', 'ldapGroupDisplayName', 'ldapLoginFilter']; @@ -490,7 +488,6 @@ class Connection extends LDAPUtility { $subj = $key; break; } - $configurationOK = false; throw new ConfigurationIssueException( 'No ' . $subj . ' given!', $this->l10n->t('Mandatory field "%s" left empty', $subj), @@ -502,14 +499,12 @@ class Connection extends LDAPUtility { $agent = $this->configuration->ldapAgentName; $pwd = $this->configuration->ldapAgentPassword; if ($agent === '' && $pwd !== '') { - $configurationOK = false; throw new ConfigurationIssueException( 'A password is given, but not an LDAP agent', $this->l10n->t('A password is given, but not an LDAP agent'), ); } if ($agent !== '' && $pwd === '') { - $configurationOK = false; throw new ConfigurationIssueException( 'No password is given for the user agent', $this->l10n->t('No password is given for the user agent'), @@ -520,16 +515,28 @@ class Connection extends LDAPUtility { $baseUsers = $this->configuration->ldapBaseUsers; $baseGroups = $this->configuration->ldapBaseGroups; - if (empty($base) && empty($baseUsers) && empty($baseGroups)) { - $configurationOK = false; + if (empty($base)) { throw new ConfigurationIssueException( - 'Not a single Base DN given.', + 'Not a single Base DN given', $this->l10n->t('No LDAP base DN was given'), ); } + if (!empty($baseUsers) && !$this->checkBasesAreValid($baseUsers, $base)) { + throw new ConfigurationIssueException( + 'User base is not in root base', + $this->l10n->t('User base DN is not a subnode of global base DN'), + ); + } + + if (!empty($baseGroups) && !$this->checkBasesAreValid($baseGroups, $base)) { + throw new ConfigurationIssueException( + 'Group base is not in root base', + $this->l10n->t('Group base DN is not a subnode of global base DN'), + ); + } + if (mb_strpos((string)$this->configuration->ldapLoginFilter, '%uid', 0, 'UTF-8') === false) { - $configurationOK = false; throw new ConfigurationIssueException( 'Login filter does not contain %uid place holder.', $this->l10n->t('Login filter does not contain %uid place holder'), @@ -537,6 +544,25 @@ class Connection extends LDAPUtility { } } + /** + * Checks that all bases are subnodes of one of the root bases + */ + private function checkBasesAreValid(array $bases, array $rootBases): bool { + foreach ($bases as $base) { + $ok = false; + foreach ($rootBases as $rootBase) { + if (str_ends_with($base, $rootBase)) { + $ok = true; + break; + } + } + if (!$ok) { + return false; + } + } + return true; + } + /** * Validates the user specified configuration * @return bool true if configuration seems OK, false otherwise From 1b818382ba7788c05912dab2d7e77b05c3a5be14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= <91878298+come-nc@users.noreply.github.com> Date: Mon, 24 Feb 2025 09:31:05 +0100 Subject: [PATCH 4/5] chore: style fix for named parameter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Arthur Schiwon Signed-off-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> --- apps/user_ldap/ajax/testConfiguration.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/user_ldap/ajax/testConfiguration.php b/apps/user_ldap/ajax/testConfiguration.php index 4cf7bb1ba65..b77439fa3e8 100644 --- a/apps/user_ldap/ajax/testConfiguration.php +++ b/apps/user_ldap/ajax/testConfiguration.php @@ -30,7 +30,7 @@ try { $conf['ldap_configuration_active'] = '1'; } try { - $connection->setConfiguration($conf, throw:true); + $connection->setConfiguration($conf, throw: true); } catch (ConfigurationIssueException $e) { $configurationError = $e->getHint(); } From 48d69c727ad7cf9c8122743e204a0493584494e4 Mon Sep 17 00:00:00 2001 From: Andy Scherzinger Date: Tue, 25 Feb 2025 22:35:50 +0100 Subject: [PATCH 5/5] fix(lint): correct comment identation Signed-off-by: Andy Scherzinger --- lib/private/DB/QueryBuilder/QueryBuilder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/DB/QueryBuilder/QueryBuilder.php b/lib/private/DB/QueryBuilder/QueryBuilder.php index e45ab40516b..8b224c28dfe 100644 --- a/lib/private/DB/QueryBuilder/QueryBuilder.php +++ b/lib/private/DB/QueryBuilder/QueryBuilder.php @@ -1340,7 +1340,7 @@ class QueryBuilder implements IQueryBuilder { * Returns the table name with database prefix as needed by the implementation * * Was protected until version 30. - * + * * @param string $table * @return string */