From 9983e05121686f4b76433b76887396cc7ba2d686 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 30 Jan 2017 16:57:40 +0100 Subject: [PATCH] LDAP's checkPassword should only catch when a user was not found, fixes #2431 Also fixes error processing after ldap_search, due to different return format Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Access.php | 4 -- apps/user_ldap/lib/LDAP.php | 110 +++++++++++++++++++++++-------- apps/user_ldap/lib/User_LDAP.php | 7 +- 3 files changed, 85 insertions(+), 36 deletions(-) diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php index 9f6639c0db..9e93ef2eca 100644 --- a/apps/user_ldap/lib/Access.php +++ b/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; } diff --git a/apps/user_ldap/lib/LDAP.php b/apps/user_ldap/lib/LDAP.php index cac09f2599..ebee078413 100644 --- a/apps/user_ldap/lib/LDAP.php +++ b/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]); - $errorMsg = ldap_error($this->curArgs[0]); - if($errorCode !== 0) { - 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); - } - } + $resource = $this->curArgs[0]; + } else if( + $this->curFunc === 'ldap_search' + && is_array($this->curArgs[0]) + && $this->isResource($this->curArgs[0][0]) + ) { + // we use always the same LDAP connection resource, is enough to + // take the first one. + $resource = $this->curArgs[0][0]; + } else { + return; } + $this->processLDAPError($resource); + $this->curFunc = ''; - $this->curArgs = array(); + $this->curArgs = []; } } diff --git a/apps/user_ldap/lib/User_LDAP.php b/apps/user_ldap/lib/User_LDAP.php index 448b34524f..cfd2450a12 100644 --- a/apps/user_ldap/lib/User_LDAP.php +++ b/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']); }