Merge pull request #3324 from nextcloud/fix-2431

LDAP's checkPassword should only catch when a user was not found, fixes #2431

apps/user_ldap/lib/Access.php

@@ -965,10 +965,6 @@ class Access extends LDAPUtility implements IUserTools {
 		$sr = $this->ldap->search($linkResources, $base, $filter, $attr);
 		$error = $this->ldap->errno($cr);
 		if(!is_array($sr) || $error !== 0) {
-			\OCP\Util::writeLog('user_ldap',
-				'Error when searching: '.$this->ldap->error($cr).
-					' code '.$this->ldap->errno($cr),
-				\OCP\Util::ERROR);
 			\OCP\Util::writeLog('user_ldap', 'Attempt for Paging?  '.print_r($pagedSearchOK, true), \OCP\Util::ERROR);
 			return false;
 		}

apps/user_ldap/lib/LDAP.php

@@ -257,6 +257,31 @@ class LDAP implements ILDAPWrapper {
 		return is_resource($resource);
 	}
 
+	/**
+	 * Checks whether the return value from LDAP is wrong or not.
+	 *
+	 * When using ldap_search we provide an array, in case multiple bases are
+	 * configured. Thus, we need to check the array elements.
+	 *
+	 * @param $result
+	 * @return bool
+	 */
+	protected function isResultFalse($result) {
+		if($result === false) {
+			return true;
+		}
+
+		if($this->curFunc === 'ldap_search' && is_array($result)) {
+			foreach ($result as $singleResult) {
+				if($singleResult === false) {
+					return true;
+				}
+			}
+		}
+
+		return false;
+	}
+
 	/**
 	 * @return mixed
 	 */
@@ -266,7 +291,7 @@ class LDAP implements ILDAPWrapper {
 		if(function_exists($func)) {
 			$this->preFunctionCall($func, $arguments);
 			$result = call_user_func_array($func, $arguments);
-			if ($result === FALSE) {
+			if ($this->isResultFalse($result)) {
 				$this->postFunctionCall();
 			}
 			return $result;
@@ -283,37 +308,66 @@ class LDAP implements ILDAPWrapper {
 		$this->curArgs = $args;
 	}
 
+	/**
+	 * Analyzes the returned LDAP error and acts accordingly if not 0
+	 *
+	 * @param resource $resource the LDAP Connection resource
+	 * @throws ConstraintViolationException
+	 * @throws ServerNotAvailableException
+	 * @throws \Exception
+	 */
+	private function processLDAPError($resource) {
+		$errorCode = ldap_errno($resource);
+		if($errorCode === 0) {
+			return;
+		}
+		$errorMsg  = ldap_error($resource);
+
+		if($this->curFunc === 'ldap_get_entries'
+			&& $errorCode === -4) {
+		} else if ($errorCode === 32) {
+			//for now
+		} else if ($errorCode === 10) {
+			//referrals, we switch them off, but then there is AD :)
+		} else if ($errorCode === -1) {
+			throw new ServerNotAvailableException('Lost connection to LDAP server.');
+		} else if ($errorCode === 48) {
+			throw new \Exception('LDAP authentication method rejected', $errorCode);
+		} else if ($errorCode === 1) {
+			throw new \Exception('LDAP Operations error', $errorCode);
+		} else if ($errorCode === 19) {
+			ldap_get_option($this->curArgs[0], LDAP_OPT_ERROR_STRING, $extended_error);
+			throw new ConstraintViolationException(!empty($extended_error)?$extended_error:$errorMsg, $errorCode);
+		} else {
+			\OCP\Util::writeLog('user_ldap',
+				'LDAP error '.$errorMsg.' (' .
+				$errorCode.') after calling '.
+				$this->curFunc,
+				\OCP\Util::DEBUG);
+		}
+	}
+
+	/**
+	 * Called after an ldap method is run to act on LDAP error if necessary
+	 */
 	private function postFunctionCall() {
 		if($this->isResource($this->curArgs[0])) {
-			$errorCode = ldap_errno($this->curArgs[0]);
+			$resource = $this->curArgs[0];
-			$errorMsg  = ldap_error($this->curArgs[0]);
+		} else if(
-			if($errorCode !== 0) {
+			   $this->curFunc === 'ldap_search'
-				if($this->curFunc === 'ldap_get_entries'
+			&& is_array($this->curArgs[0])
-						  && $errorCode === -4) {
+			&& $this->isResource($this->curArgs[0][0])
-				} else if ($errorCode === 32) {
+		) {
-					//for now
+			// we use always the same LDAP connection resource, is enough to
-				} else if ($errorCode === 10) {
+			// take the first one.
-					//referrals, we switch them off, but then there is AD :)
+			$resource = $this->curArgs[0][0];
-				} else if ($errorCode === -1) {
+		} else {
-					throw new ServerNotAvailableException('Lost connection to LDAP server.');
+			return;
-				} else if ($errorCode === 48) {
-					throw new \Exception('LDAP authentication method rejected', $errorCode);
-				} else if ($errorCode === 1) {
-					throw new \Exception('LDAP Operations error', $errorCode);
-				} else if ($errorCode === 19) {
-					ldap_get_option($this->curArgs[0], LDAP_OPT_ERROR_STRING, $extended_error);
-					throw new ConstraintViolationException(!empty($extended_error)?$extended_error:$errorMsg, $errorCode);
-				} else {
-					\OCP\Util::writeLog('user_ldap',
-										'LDAP error '.$errorMsg.' (' .
-											$errorCode.') after calling '.
-											$this->curFunc,
-										\OCP\Util::DEBUG);
-				}
-			}
 		}
 
+		$this->processLDAPError($resource);
+
 		$this->curFunc = '';
-		$this->curArgs = array();
+		$this->curArgs = [];
 	}
 }

apps/user_ldap/lib/User_LDAP.php

@@ -136,17 +136,16 @@ class User_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
 	}
 
 	/**
-	 * Check if the password is correct
+	 * Check if the password is correct without logging in the user
+	 *
 	 * @param string $uid The username
 	 * @param string $password The password
 	 * @return false|string
-	 *
-	 * Check if the password is correct without logging in the user
 	 */
 	public function checkPassword($uid, $password) {
 		try {
 			$ldapRecord = $this->getLDAPUserByLoginName($uid);
-		} catch(\Exception $e) {
+		} catch(NotOnLDAP $e) {
 			if($this->ocConfig->getSystemValue('loglevel', Util::WARN) === Util::DEBUG) {
 				\OC::$server->getLogger()->logException($e, ['app' => 'user_ldap']);
 			}