Merge pull request #50475 from nextcloud/enh/ldap-clearer-errors

Improve error detail when saving an incorrect LDAP config

apps/user_ldap/ajax/testConfiguration.php

@@ -1,5 +1,6 @@
 <?php
 
+use OCA\User_LDAP\Exceptions\ConfigurationIssueException;
 use OCA\User_LDAP\LDAP;
 use OCP\ISession;
 use OCP\Server;
@@ -22,14 +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';
-		$configurationOk = $connection->setConfiguration($conf);
 	}
-	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
@@ -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()]);

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

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

apps/user_ldap/lib/Connection.php

@@ -8,11 +8,14 @@
 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 OCP\Util;
 use Psr\Log\LoggerInterface;
 
 /**
@@ -134,8 +137,8 @@ class Connection extends LDAPUtility {
 	 */
 	protected $bindResult = [];
 
-	/** @var LoggerInterface */
-	protected $logger;
+	protected LoggerInterface $logger;
+	private IL10N $l10n;
 
 	/**
 	 * Constructor
@@ -157,6 +160,7 @@ class Connection extends LDAPUtility {
 		$this->doNotValidate = !in_array($this->configPrefix,
 			$helper->getServerConfigurationPrefixes());
 		$this->logger = Server::get(LoggerInterface::class);
+		$this->l10n = Util::getL10N('user_ldap');
 	}
 
 	public function __destruct() {
@@ -332,16 +336,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,10 +452,10 @@ class Connection extends LDAPUtility {
 		}
 	}
 
-	private function doCriticalValidation(): bool {
-		$configurationOK = true;
-		$errorStr = 'Configuration Error (prefix ' . $this->configPrefix . '): ';
-
+	/**
+	 * @throws ConfigurationIssueException
+	 */
+	private function doCriticalValidation(): void {
 		//options that shall not be empty
 		$options = ['ldapHost', 'ldapUserDisplayName',
 			'ldapGroupDisplayName', 'ldapLoginFilter'];
@@ -483,10 +488,9 @@ class Connection extends LDAPUtility {
 						$subj = $key;
 						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,47 +498,76 @@ 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 !== '') {
+			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 === '') {
+			throw new ConfigurationIssueException(
+				'No password is given for the user agent',
+				$this->l10n->t('No password is given for the user agent'),
 			);
-			$configurationOK = false;
 		}
 
 		$base = $this->configuration->ldapBase;
 		$baseUsers = $this->configuration->ldapBaseUsers;
 		$baseGroups = $this->configuration->ldapBaseGroups;
 
-		if (empty($base) && empty($baseUsers) && empty($baseGroups)) {
-			$this->logger->warning(
-				$errorStr . 'Not a single Base DN given.',
-				['app' => 'user_ldap']
+		if (empty($base)) {
+			throw new ConfigurationIssueException(
+				'Not a single Base DN given',
+				$this->l10n->t('No LDAP base DN was given'),
 			);
-			$configurationOK = false;
 		}
 
-		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 (!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'),
 			);
-			$configurationOK = false;
 		}
 
-		return $configurationOK;
+		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) {
+			throw new ConfigurationIssueException(
+				'Login filter does not contain %uid place holder.',
+				$this->l10n->t('Login filter does not contain %uid place holder'),
+			);
+		}
+	}
+
+	/**
+	 * 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
 	 */
-	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 +581,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;
+		}
 	}
 
 

apps/user_ldap/lib/Exceptions/ConfigurationIssueException.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+namespace OCA\User_LDAP\Exceptions;
+
+use OCP\HintException;
+
+class ConfigurationIssueException extends HintException {
+}

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