Cleanup typing and improve logging

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

3 files changed, 55 insertions, 44 deletions

diff --git a/apps/user_ldap/lib/ILDAPWrapper.php b/apps/user_ldap/lib/ILDAPWrapper.php
index 433ca96c071..b5c5568348e 100644
--- a/apps/user_ldap/lib/ILDAPWrapper.php
+++ b/apps/user_ldap/lib/ILDAPWrapper.php
@@ -166,6 +166,13 @@ interface ILDAPWrapper {
 	public function modReplace($link, $userDN, $password);
 
 	/**
+	 * Performs a PASSWD extended operation.
+	 * @param resource|\LDAP\Connection $link LDAP link resource
+	 * @return bool|string The generated password if new_password is empty or omitted. Otherwise true on success and false on failure.
+	 */
+	public function exopPasswd($link, string $userDN, string $oldPassword, string $password);
+
+	/**
 	 * Sets the value of the specified option to be $value
 	 * @param resource|\LDAP\Connection $link LDAP link resource
 	 * @param int $option a defined LDAP Server option
diff --git a/apps/user_ldap/lib/LDAP.php b/apps/user_ldap/lib/LDAP.php
index 8a840673a03..c03337a9e51 100644
--- a/apps/user_ldap/lib/LDAP.php
+++ b/apps/user_ldap/lib/LDAP.php
@@ -37,11 +37,12 @@ use OCP\Profiler\IProfiler;
 use OC\ServerNotAvailableException;
 use OCA\User_LDAP\DataCollector\LdapDataCollector;
 use OCA\User_LDAP\Exceptions\ConstraintViolationException;
+use Psr\Log\LoggerInterface;
 
 class LDAP implements ILDAPWrapper {
-	protected $logFile = '';
-	protected $curFunc = '';
-	protected $curArgs = [];
+	protected string $logFile = '';
+	protected array $curArgs = [];
+	protected LoggerInterface $logger;
 
 	private ?LdapDataCollector $dataCollector = null;
 
@@ -54,6 +55,8 @@ class LDAP implements ILDAPWrapper {
 			$this->dataCollector = new LdapDataCollector();
 			$profiler->add($this->dataCollector);
 		}
+
+		$this->logger = \OCP\Server::get(LoggerInterface::class);
 	}
 
 	/**
@@ -67,10 +70,12 @@ class LDAP implements ILDAPWrapper {
 	 * {@inheritDoc}
 	 */
 	public function connect($host, $port) {
-		if (strpos($host, '://') === false) {
+		$pos = strpos($host, '://');
+		if ($pos === false) {
 			$host = 'ldap://' . $host;
+			$pos = 4;
 		}
-		if (strpos($host, ':', strpos($host, '://') + 1) === false) {
+		if (strpos($host, ':', $pos + 1) === false) {
 			//ldap_connect ignores port parameter when URLs are passed
 			$host .= ':' . $port;
 		}
@@ -82,7 +87,7 @@ class LDAP implements ILDAPWrapper {
 	 */
 	public function controlPagedResultResponse($link, $result, &$cookie): bool {
 		$errorCode = 0;
-		$errorMessage = '';
+		$errorMsg = '';
 		$controls = [];
 		$matchedDn = null;
 		$referrals = [];
@@ -92,11 +97,11 @@ class LDAP implements ILDAPWrapper {
 		$success = ldap_parse_result($link, $result,
 			$errorCode,
 			$matchedDn,
-			$errorMessage,
+			$errorMsg,
 			$referrals,
 			$controls);
-		if ($this->isResultFalse($result)) {
-			$this->postFunctionCall();
+		if ($errorCode !== 0) {
+			$this->processLDAPError($link, 'ldap_parse_result', $errorCode, $errorMsg);
 		}
 		if ($this->dataCollector !== null) {
 			$this->dataCollector->stopLastLdapRequest();
@@ -104,8 +109,6 @@ class LDAP implements ILDAPWrapper {
 
 		$cookie = $controls[LDAP_CONTROL_PAGEDRESULTS]['value']['cookie'] ?? '';
 
-		// TODO do not ignore error code and message
-
 		return $success;
 	}
 
@@ -133,7 +136,7 @@ class LDAP implements ILDAPWrapper {
 	/**
 	 * Splits DN into its component parts
 	 * @param string $dn
-	 * @param int @withAttrib
+	 * @param int $withAttrib
 	 * @return array|false
 	 * @link https://www.php.net/manual/en/function.ldap-explode-dn.php
 	 */
@@ -224,7 +227,7 @@ class LDAP implements ILDAPWrapper {
 	/**
 	 * {@inheritDoc}
 	 */
-	public function exopPasswd($link, $userDN, $oldPassword, $password) {
+	public function exopPasswd($link, string $userDN, string $oldPassword, string $password) {
 		return $this->invokeLDAPMethod('exop_passwd', $link, $userDN, $oldPassword, $password);
 	}
 
@@ -270,15 +273,14 @@ class LDAP implements ILDAPWrapper {
 	 * 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
+	 * @param mixed $result
 	 */
-	protected function isResultFalse($result) {
+	protected function isResultFalse(string $functionName, $result): bool {
 		if ($result === false) {
 			return true;
 		}
 
-		if ($this->curFunc === 'ldap_search' && is_array($result)) {
+		if ($functionName === 'ldap_search' && is_array($result)) {
 			foreach ($result as $singleResult) {
 				if ($singleResult === false) {
 					return true;
@@ -298,8 +300,8 @@ class LDAP implements ILDAPWrapper {
 		if (function_exists($func)) {
 			$this->preFunctionCall($func, $arguments);
 			$result = call_user_func_array($func, $arguments);
-			if ($this->isResultFalse($result)) {
-				$this->postFunctionCall();
+			if ($this->isResultFalse($func, $result)) {
+				$this->postFunctionCall($func);
 			}
 			if ($this->dataCollector !== null) {
 				$this->dataCollector->stopLastLdapRequest();
@@ -310,8 +312,12 @@ class LDAP implements ILDAPWrapper {
 	}
 
 	private function preFunctionCall(string $functionName, array $args): void {
-		$this->curFunc = $functionName;
 		$this->curArgs = $args;
+		$this->logger->debug('Calling LDAP function {func} with parameters {args}', [
+			'app' => 'user_ldap',
+			'func' => $functionName,
+			'args' => json_encode($args),
+		]);
 
 		if ($this->dataCollector !== null) {
 			$args = array_map(function ($item) {
@@ -324,14 +330,14 @@ class LDAP implements ILDAPWrapper {
 				return $item;
 			}, $this->curArgs);
 
-			$this->dataCollector->startLdapRequest($this->curFunc, $args);
+			$this->dataCollector->startLdapRequest($functionName, $args);
 		}
 
 		if ($this->logFile !== '' && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) {
 			$args = array_map(fn ($item) => (!$this->isResource($item) ? $item : '(resource)'), $this->curArgs);
 			file_put_contents(
 				$this->logFile,
-				$this->curFunc . '::' . json_encode($args) . "\n",
+				$functionName . '::' . json_encode($args) . "\n",
 				FILE_APPEND
 			);
 		}
@@ -345,14 +351,14 @@ class LDAP implements ILDAPWrapper {
 	 * @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'
+	private function processLDAPError($resource, string $functionName, int $errorCode, string $errorMsg): void {
+		$this->logger->debug('LDAP error {message} ({code}) after calling {func}', [
+			'app' => 'user_ldap',
+			'message' => $errorMsg,
+			'code' => $errorCode,
+			'func' => $functionName,
+		]);
+		if ($functionName === 'ldap_get_entries'
 			&& $errorCode === -4) {
 		} elseif ($errorCode === 32) {
 			//for now
@@ -367,15 +373,8 @@ class LDAP implements ILDAPWrapper {
 		} elseif ($errorCode === 1) {
 			throw new \Exception('LDAP Operations error', $errorCode);
 		} elseif ($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 {
-			\OC::$server->getLogger()->debug('LDAP error {message} ({code}) after calling {func}', [
-				'app' => 'user_ldap',
-				'message' => $errorMsg,
-				'code' => $errorCode,
-				'func' => $this->curFunc,
-			]);
+			ldap_get_option($resource, LDAP_OPT_ERROR_STRING, $extended_error);
+			throw new ConstraintViolationException(!empty($extended_error) ? $extended_error : $errorMsg, $errorCode);
 		}
 	}
 
@@ -383,11 +382,11 @@ class LDAP implements ILDAPWrapper {
 	 * Called after an ldap method is run to act on LDAP error if necessary
 	 * @throw \Exception
 	 */
-	private function postFunctionCall() {
+	private function postFunctionCall(string $functionName): void {
 		if ($this->isResource($this->curArgs[0])) {
 			$resource = $this->curArgs[0];
 		} elseif (
-			   $this->curFunc === 'ldap_search'
+			   $functionName === 'ldap_search'
 			&& is_array($this->curArgs[0])
 			&& $this->isResource($this->curArgs[0][0])
 		) {
@@ -398,9 +397,14 @@ class LDAP implements ILDAPWrapper {
 			return;
 		}
 
-		$this->processLDAPError($resource);
+		$errorCode = ldap_errno($resource);
+		if ($errorCode === 0) {
+			return;
+		}
+		$errorMsg = ldap_error($resource);
+
+		$this->processLDAPError($resource, $functionName, $errorCode, $errorMsg);
 
-		$this->curFunc = '';
 		$this->curArgs = [];
 	}
 }
diff --git a/apps/user_ldap/lib/LDAPUtility.php b/apps/user_ldap/lib/LDAPUtility.php
index 0b16f74333b..a8e4c16fac7 100644
--- a/apps/user_ldap/lib/LDAPUtility.php
+++ b/apps/user_ldap/lib/LDAPUtility.php
@@ -25,7 +25,7 @@
 namespace OCA\User_LDAP;
 
 abstract class LDAPUtility {
-	protected $ldap;
+	protected ILDAPWrapper $ldap;
 
 	/**
 	 * constructor, make sure the subclasses call this one!