Merge pull request #31777 from nextcloud/backport/31514/stable22

[stable22] user_ldap fix ldap connection resets #31421
This commit is contained in:
blizzz 2022-04-14 12:50:41 +02:00 committed by GitHub
commit 74c3e6d8a4
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 128 additions and 147 deletions

View file

@ -218,7 +218,7 @@ class Access extends LDAPUtility {
$values = [];
$isRangeRequest = false;
do {
$result = $this->executeRead($cr, $dn, $attrToRead, $filter, $maxResults);
$result = $this->executeRead($dn, $attrToRead, $filter, $maxResults);
if (is_bool($result)) {
// when an exists request was run and it was successful, an empty
// array must be returned
@ -260,17 +260,12 @@ class Access extends LDAPUtility {
/**
* Runs an read operation against LDAP
*
* @param resource $cr the LDAP connection
* @param string $dn
* @param string $attribute
* @param string $filter
* @param int $maxResults
* @return array|bool false if there was any error, true if an exists check
* was performed and the requested DN found, array with the
* returned data on a successful usual operation
* @throws ServerNotAvailableException
*/
public function executeRead($cr, $dn, $attribute, $filter, $maxResults) {
public function executeRead(string $dn, string $attribute, string $filter, int $maxResults) {
try {
$this->initPagedSearch($filter, $dn, [$attribute], $maxResults, 0);
} catch (NoMoreResults $e) {
@ -280,7 +275,7 @@ class Access extends LDAPUtility {
return false;
}
$dn = $this->helper->DNasBaseParameter($dn);
$rr = @$this->invokeLDAPMethod('read', $cr, $dn, $filter, [$attribute]);
$rr = @$this->invokeLDAPMethod('read', $dn, $filter, [$attribute]);
if (!$this->ldap->isResource($rr)) {
if ($attribute !== '') {
//do not throw this message on userExists check, irritates
@ -289,18 +284,18 @@ class Access extends LDAPUtility {
//in case an error occurs , e.g. object does not exist
return false;
}
if ($attribute === '' && ($filter === 'objectclass=*' || $this->invokeLDAPMethod('countEntries', $cr, $rr) === 1)) {
if ($attribute === '' && ($filter === 'objectclass=*' || $this->invokeLDAPMethod('countEntries', $rr) === 1)) {
$this->logger->debug('readAttribute: ' . $dn . ' found', ['app' => 'user_ldap']);
return true;
}
$er = $this->invokeLDAPMethod('firstEntry', $cr, $rr);
$er = $this->invokeLDAPMethod('firstEntry', $rr);
if (!$this->ldap->isResource($er)) {
//did not match the filter, return false
return false;
}
//LDAP attributes are not case sensitive
$result = \OCP\Util::mb_array_change_key_case(
$this->invokeLDAPMethod('getAttributes', $cr, $er), MB_CASE_LOWER, 'UTF-8');
$this->invokeLDAPMethod('getAttributes', $er), MB_CASE_LOWER, 'UTF-8');
return $result;
}
@ -381,10 +376,10 @@ class Access extends LDAPUtility {
}
try {
// try PASSWD extended operation first
return @$this->invokeLDAPMethod('exopPasswd', $cr, $userDN, '', $password) ||
@$this->invokeLDAPMethod('modReplace', $cr, $userDN, $password);
return @$this->invokeLDAPMethod('exopPasswd', $userDN, '', $password) ||
@$this->invokeLDAPMethod('modReplace', $userDN, $password);
} catch (ConstraintViolationException $e) {
throw new HintException('Password change rejected.', \OC::$server->getL10N('user_ldap')->t('Password change rejected. Hint: ') . $e->getMessage(), $e->getCode());
throw new HintException('Password change rejected.', \OC::$server->getL10N('user_ldap')->t('Password change rejected. Hint: ') . $e->getMessage(), (int)$e->getCode());
}
}
@ -1092,26 +1087,23 @@ class Access extends LDAPUtility {
*/
/**
* @param mixed[] $arguments
* @return mixed
* @throws \OC\ServerNotAvailableException
*/
private function invokeLDAPMethod() {
$arguments = func_get_args();
$command = array_shift($arguments);
$cr = array_shift($arguments);
private function invokeLDAPMethod(string $command, ...$arguments) {
if ($command == 'controlPagedResultResponse') {
// php no longer supports call-time pass-by-reference
// thus cannot support controlPagedResultResponse as the third argument
// is a reference
throw new \InvalidArgumentException('Invoker does not support controlPagedResultResponse, call LDAP Wrapper directly instead.');
}
if (!method_exists($this->ldap, $command)) {
return null;
}
array_unshift($arguments, $cr);
// php no longer supports call-time pass-by-reference
// thus cannot support controlPagedResultResponse as the third argument
// is a reference
array_unshift($arguments, $this->connection->getConnectionResource());
$doMethod = function () use ($command, &$arguments) {
if ($command == 'controlPagedResultResponse') {
throw new \InvalidArgumentException('Invoker does not support controlPagedResultResponse, call LDAP Wrapper directly instead.');
} else {
return call_user_func_array([$this->ldap, $command], $arguments);
}
return call_user_func_array([$this->ldap, $command], $arguments);
};
try {
$ret = $doMethod();
@ -1172,8 +1164,7 @@ class Access extends LDAPUtility {
return false;
}
$sr = $this->invokeLDAPMethod('search', $cr, $base, $filter, $attr);
// cannot use $cr anymore, might have changed in the previous call!
$sr = $this->invokeLDAPMethod('search', $base, $filter, $attr);
$error = $this->ldap->errno($this->connection->getConnectionResource());
if (!$this->ldap->isResource($sr) || $error !== 0) {
$this->logger->error('Attempt for Paging? ' . print_r($pagedSearchOK, true), ['app' => 'user_ldap']);
@ -1308,7 +1299,7 @@ class Access extends LDAPUtility {
* @throws ServerNotAvailableException
*/
private function countEntriesInSearchResults($sr): int {
return (int)$this->invokeLDAPMethod('countEntries', $this->connection->getConnectionResource(), $sr);
return (int)$this->invokeLDAPMethod('countEntries', $sr);
}
/**
@ -1349,7 +1340,6 @@ class Access extends LDAPUtility {
return [];
}
[$sr, $pagedSearchOK] = $search;
$cr = $this->connection->getConnectionResource();
if ($skipHandling) {
//i.e. result do not need to be fetched, we just need the cookie
@ -1359,7 +1349,7 @@ class Access extends LDAPUtility {
return [];
}
$findings = array_merge($findings, $this->invokeLDAPMethod('getEntries', $cr, $sr));
$findings = array_merge($findings, $this->invokeLDAPMethod('getEntries', $sr));
$iFoundItems = max($iFoundItems, $findings['count']);
unset($findings['count']);
@ -1536,7 +1526,7 @@ class Access extends LDAPUtility {
* @param string $search the search term
* @return string the final filter part to use in LDAP searches
*/
public function getFilterPartForUserSearch($search) {
public function getFilterPartForUserSearch($search): string {
return $this->getFilterPartForSearch($search,
$this->connection->ldapAttributesForUserSearch,
$this->connection->ldapUserDisplayName);
@ -1548,7 +1538,7 @@ class Access extends LDAPUtility {
* @param string $search the search term
* @return string the final filter part to use in LDAP searches
*/
public function getFilterPartForGroupSearch($search) {
public function getFilterPartForGroupSearch($search): string {
return $this->getFilterPartForSearch($search,
$this->connection->ldapAttributesForGroupSearch,
$this->connection->ldapGroupDisplayName);
@ -1642,10 +1632,8 @@ class Access extends LDAPUtility {
/**
* returns the filter used for counting users
*
* @return string
*/
public function getFilterForUserCount() {
public function getFilterForUserCount(): string {
$filter = $this->combineFilterWithAnd([
$this->connection->ldapUserFilter,
$this->connection->ldapUserDisplayName . '=*'
@ -1994,8 +1982,7 @@ class Access extends LDAPUtility {
if ($this->lastCookie === '') {
return;
}
$cr = $this->connection->getConnectionResource();
$this->invokeLDAPMethod('controlPagedResult', $cr, 0, false);
$this->invokeLDAPMethod('controlPagedResult', 0, false);
$this->getPagedSearchResultState();
$this->lastCookie = '';
}
@ -2082,7 +2069,7 @@ class Access extends LDAPUtility {
$this->abandonPagedSearch();
}
$pagedSearchOK = true === $this->invokeLDAPMethod(
'controlPagedResult', $this->connection->getConnectionResource(), $limit, false
'controlPagedResult', $limit, false
);
if ($pagedSearchOK) {
$this->logger->debug('Ready for a paged search', ['app' => 'user_ldap']);
@ -2102,7 +2089,6 @@ class Access extends LDAPUtility {
// be returned.
$pageSize = (int)$this->connection->ldapPagingSize > 0 ? (int)$this->connection->ldapPagingSize : 500;
$pagedSearchOK = $this->invokeLDAPMethod('controlPagedResult',
$this->connection->getConnectionResource(),
$pageSize, false);
}

View file

@ -75,10 +75,25 @@ use Psr\Log\LoggerInterface;
*/
class Connection extends LDAPUtility {
private $ldapConnectionRes = null;
/**
* @var string
*/
private $configPrefix;
/**
* @var ?string
*/
private $configID;
/**
* @var bool
*/
private $configured = false;
//whether connection should be kept on __destruct
/**
* @var bool whether connection should be kept on __destruct
*/
private $dontDestruct = false;
/**
@ -91,16 +106,27 @@ class Connection extends LDAPUtility {
*/
public $hasGidNumber = true;
//cache handler
protected $cache;
/**
* @var \OCP\ICache|null
*/
protected $cache = null;
/** @var Configuration settings handler **/
protected $configuration;
/**
* @var bool
*/
protected $doNotValidate = false;
/**
* @var bool
*/
protected $ignoreValidation = false;
/**
* @var array{dn?: mixed, hash?: string, result?: bool}
*/
protected $bindResult = [];
/** @var LoggerInterface */
@ -108,16 +134,14 @@ class Connection extends LDAPUtility {
/**
* Constructor
* @param ILDAPWrapper $ldap
* @param string $configPrefix a string with the prefix for the configkey column (appconfig table)
* @param string|null $configID a string with the value for the appid column (appconfig table) or null for on-the-fly connections
*/
public function __construct(ILDAPWrapper $ldap, $configPrefix = '', $configID = 'user_ldap') {
public function __construct(ILDAPWrapper $ldap, string $configPrefix = '', ?string $configID = 'user_ldap') {
parent::__construct($ldap);
$this->configPrefix = $configPrefix;
$this->configID = $configID;
$this->configuration = new Configuration($configPrefix,
!is_null($configID));
$this->configuration = new Configuration($configPrefix, !is_null($configID));
$memcache = \OC::$server->getMemCacheFactory();
if ($memcache->isAvailable()) {
$this->cache = $memcache->createDistributed();

View file

@ -1349,7 +1349,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I
$this->access->groupname2dn($gid),
$this->access->connection->ldapGroupDisplayName);
if ($displayName && (count($displayName) > 0)) {
if (($displayName !== false) && (count($displayName) > 0)) {
$displayName = $displayName[0];
$this->access->connection->writeToCache($cacheKey, $displayName);
return $displayName;

View file

@ -147,7 +147,7 @@ interface ILDAPWrapper {
/**
* Read an entry
* @param resource $link LDAP link resource
* @param array $baseDN The DN of the entry to read from
* @param string $baseDN The DN of the entry to read from
* @param string $filter An LDAP filter
* @param array $attr array of the attributes to read
* @return resource an LDAP search result resource
@ -178,8 +178,8 @@ interface ILDAPWrapper {
/**
* Sets the value of the specified option to be $value
* @param resource $link LDAP link resource
* @param string $option a defined LDAP Server option
* @param int $value the new value for the option
* @param int $option a defined LDAP Server option
* @param mixed $value the new value for the option
* @return bool true on success, false otherwise
*/
public function setOption($link, $option, $value);

View file

@ -49,19 +49,14 @@ class LDAP implements ILDAPWrapper {
}
/**
* @param resource $link
* @param string $dn
* @param string $password
* @return bool|mixed
* {@inheritDoc}
*/
public function bind($link, $dn, $password) {
return $this->invokeLDAPMethod('bind', $link, $dn, $password);
}
/**
* @param string $host
* @param string $port
* @return mixed
* {@inheritDoc}
*/
public function connect($host, $port) {
if (strpos($host, '://') === false) {
@ -74,6 +69,9 @@ class LDAP implements ILDAPWrapper {
return $this->invokeLDAPMethod('connect', $host);
}
/**
* {@inheritDoc}
*/
public function controlPagedResultResponse($link, $result, &$cookie): bool {
$this->preFunctionCall(
$this->pagedResultsAdapter->getResponseCallFunc(),
@ -91,10 +89,7 @@ class LDAP implements ILDAPWrapper {
}
/**
* @param LDAP $link
* @param int $pageSize
* @param bool $isCritical
* @return mixed|true
* {@inheritDoc}
*/
public function controlPagedResult($link, $pageSize, $isCritical) {
$fn = $this->pagedResultsAdapter->getRequestCallFunc();
@ -114,25 +109,21 @@ class LDAP implements ILDAPWrapper {
}
/**
* @param LDAP $link
* @param LDAP $result
* @return mixed
* {@inheritDoc}
*/
public function countEntries($link, $result) {
return $this->invokeLDAPMethod('count_entries', $link, $result);
}
/**
* @param LDAP $link
* @return integer
* {@inheritDoc}
*/
public function errno($link) {
return $this->invokeLDAPMethod('errno', $link);
}
/**
* @param LDAP $link
* @return string
* {@inheritDoc}
*/
public function error($link) {
return $this->invokeLDAPMethod('error', $link);
@ -150,56 +141,42 @@ class LDAP implements ILDAPWrapper {
}
/**
* @param LDAP $link
* @param LDAP $result
* @return mixed
* {@inheritDoc}
*/
public function firstEntry($link, $result) {
return $this->invokeLDAPMethod('first_entry', $link, $result);
}
/**
* @param LDAP $link
* @param LDAP $result
* @return array|mixed
* {@inheritDoc}
*/
public function getAttributes($link, $result) {
return $this->invokeLDAPMethod('get_attributes', $link, $result);
}
/**
* @param LDAP $link
* @param LDAP $result
* @return mixed|string
* {@inheritDoc}
*/
public function getDN($link, $result) {
return $this->invokeLDAPMethod('get_dn', $link, $result);
}
/**
* @param LDAP $link
* @param LDAP $result
* @return array|mixed
* {@inheritDoc}
*/
public function getEntries($link, $result) {
return $this->invokeLDAPMethod('get_entries', $link, $result);
}
/**
* @param LDAP $link
* @param resource $result
* @return mixed
* {@inheritDoc}
*/
public function nextEntry($link, $result) {
return $this->invokeLDAPMethod('next_entry', $link, $result);
}
/**
* @param LDAP $link
* @param string $baseDN
* @param string $filter
* @param array $attr
* @return mixed
* {@inheritDoc}
*/
public function read($link, $baseDN, $filter, $attr) {
$this->pagedResultsAdapter->setReadArgs($link, $baseDN, $filter, $attr);
@ -207,14 +184,7 @@ class LDAP implements ILDAPWrapper {
}
/**
* @param LDAP $link
* @param string[] $baseDN
* @param string $filter
* @param array $attr
* @param int $attrsOnly
* @param int $limit
* @return mixed
* @throws \Exception
* {@inheritDoc}
*/
public function search($link, $baseDN, $filter, $attr, $attrsOnly = 0, $limit = 0) {
$oldHandler = set_error_handler(function ($no, $message, $file, $line) use (&$oldHandler) {
@ -237,47 +207,35 @@ class LDAP implements ILDAPWrapper {
}
/**
* @param LDAP $link
* @param string $userDN
* @param string $password
* @return bool
* {@inheritDoc}
*/
public function modReplace($link, $userDN, $password) {
return $this->invokeLDAPMethod('mod_replace', $link, $userDN, ['userPassword' => $password]);
}
/**
* @param LDAP $link
* @param string $userDN
* @param string $oldPassword
* @param string $password
* @return bool
* {@inheritDoc}
*/
public function exopPasswd($link, $userDN, $oldPassword, $password) {
return $this->invokeLDAPMethod('exop_passwd', $link, $userDN, $oldPassword, $password);
}
/**
* @param LDAP $link
* @param string $option
* @param int $value
* @return bool|mixed
* {@inheritDoc}
*/
public function setOption($link, $option, $value) {
return $this->invokeLDAPMethod('set_option', $link, $option, $value);
}
/**
* @param LDAP $link
* @return mixed|true
* {@inheritDoc}
*/
public function startTls($link) {
return $this->invokeLDAPMethod('start_tls', $link);
}
/**
* @param resource $link
* @return bool|mixed
* {@inheritDoc}
*/
public function unbind($link) {
return $this->invokeLDAPMethod('unbind', $link);
@ -292,9 +250,7 @@ class LDAP implements ILDAPWrapper {
}
/**
* Checks whether the submitted parameter is a resource
* @param Resource $resource the resource variable to check
* @return bool true if it is a resource, false otherwise
* {@inheritDoc}
*/
public function isResource($resource) {
return is_resource($resource);

View file

@ -274,8 +274,8 @@ class User {
/**
* returns the home directory of the user if specified by LDAP settings
* @param string $valueFromLDAP
* @return bool|string
* @param ?string $valueFromLDAP
* @return false|string
* @throws \Exception
*/
public function getHomePath($valueFromLDAP = null) {
@ -286,8 +286,7 @@ class User {
&& strpos($this->access->connection->homeFolderNamingRule, 'attr:') === 0
&& $this->access->connection->homeFolderNamingRule !== 'attr:') {
$attr = substr($this->access->connection->homeFolderNamingRule, strlen('attr:'));
$homedir = $this->access->readAttribute(
$this->access->username2dn($this->getUsername()), $attr);
$homedir = $this->access->readAttribute($this->access->username2dn($this->getUsername()), $attr);
if ($homedir && isset($homedir[0])) {
$path = $homedir[0];
}

View file

@ -113,7 +113,7 @@ class AccessTest extends TestCase {
private function getConnectorAndLdapMock() {
$lw = $this->createMock(ILDAPWrapper::class);
$connector = $this->getMockBuilder(Connection::class)
->setConstructorArgs([$lw, null, null])
->setConstructorArgs([$lw, '', null])
->getMock();
$um = $this->getMockBuilder(Manager::class)
->setConstructorArgs([
@ -495,7 +495,7 @@ class AccessTest extends TestCase {
->willReturn(true);
$connection = $this->createMock(LDAP::class);
$this->connection
->expects($this->once())
->expects($this->any())
->method('getConnectionResource')
->willReturn($connection);
$this->ldap
@ -519,7 +519,7 @@ class AccessTest extends TestCase {
->willReturn(true);
$connection = $this->createMock(LDAP::class);
$this->connection
->expects($this->once())
->expects($this->any())
->method('getConnectionResource')
->willReturn($connection);
$this->ldap

View file

@ -104,14 +104,12 @@ class Group_LDAPTest extends TestCase {
$lw = $this->createMock(ILDAPWrapper::class);
$connector = $this->getMockBuilder(Connection::class)
->setMethods($conMethods)
->setConstructorArgs([$lw, null, null])
->setConstructorArgs([$lw, '', null])
->getMock();
$access = $this->createMock(Access::class);
$access->expects($this->any())
->method('getConnection')
->willReturn($connector);
$access->connection = $connector;
$access->userManager = $this->createMock(Manager::class);
@ -133,6 +131,8 @@ class Group_LDAPTest extends TestCase {
->willReturnCallback(function ($name) {
if ($name === 'ldapDynamicGroupMemberURL') {
return '';
} elseif ($name === 'ldapBaseGroups') {
return [];
}
return 1;
});
@ -953,6 +953,8 @@ class Group_LDAPTest extends TestCase {
return 'member';
case 'ldapGroupFilter':
return $groupFilter;
case 'ldapBaseGroups':
return [];
}
return 1;
});
@ -1321,16 +1323,16 @@ class Group_LDAPTest extends TestCase {
});
$access->connection = $this->createMock(Connection::class);
if (count($groupsInfo) > 1) {
$access->connection->expects($this->any())
->method('__get')
->willReturnCallback(function ($name) {
if ($name === 'ldapNestedGroups') {
return 1;
}
return null;
});
}
$access->connection->expects($this->any())
->method('__get')
->willReturnCallback(function ($name) {
if ($name === 'ldapNestedGroups') {
return 1;
} elseif ($name === 'ldapGroupMemberAssocAttr') {
return 'attr';
}
return null;
});
/** @var GroupPluginManager $pluginManager */
$pluginManager = $this->createMock(GroupPluginManager::class);
@ -1373,6 +1375,10 @@ class Group_LDAPTest extends TestCase {
return null;
});
$access->expects($this->any())
->method('groupname2dn')
->willReturn('fakedn');
/** @var GroupPluginManager $pluginManager */
$pluginManager = $this->createMock(GroupPluginManager::class);

View file

@ -1016,6 +1016,10 @@ class UserTest extends \Test\TestCase {
->method('readAttribute')
->willReturn(false);
$this->access->expects($this->once())
->method('username2dn')
->willReturn($this->dn);
// asks for "enforce_home_folder_naming_rule"
$this->config->expects($this->once())
->method('getAppValue')
@ -1038,6 +1042,10 @@ class UserTest extends \Test\TestCase {
->method('readAttribute')
->willReturn(false);
$this->access->expects($this->once())
->method('username2dn')
->willReturn($this->dn);
// asks for "enforce_home_folder_naming_rule"
$this->config->expects($this->once())
->method('getAppValue')

View file

@ -815,13 +815,15 @@ class User_LDAPTest extends TestCase {
private function prepareAccessForGetDisplayName() {
$this->connection->expects($this->any())
->method('__get')
->willReturnCallback(function ($name) {
if ($name === 'ldapUserDisplayName') {
return 'displayname';
}
return null;
});
->method('__get')
->willReturnCallback(function ($name) {
if ($name === 'ldapUserDisplayName') {
return 'displayname';
} elseif ($name === 'ldapUserDisplayName2') {
return 'displayname2';
}
return null;
});
$this->access->expects($this->any())
->method('readAttribute')

View file

@ -72,7 +72,7 @@ class WizardTest extends TestCase {
/** @var Configuration|\PHPUnit\Framework\MockObject\MockObject $conf */
$conf = $this->getMockBuilder(Configuration::class)
->setMethods($confMethods)
->setConstructorArgs([$lw, null, null])
->setConstructorArgs(['', true])
->getMock();
/** @var Access|\PHPUnit\Framework\MockObject\MockObject $access */