From 7eb6d8df0add6c862e84bdb0bb618380fbc04bda Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Tue, 11 Aug 2020 18:53:50 +0200 Subject: [PATCH] do not flip available state to unavailable, allow empty results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - the detection relies that the first, requested result is not empty - it might be empty though – groups without members - protect switching from available to unavailable - switching the other way around was also not envisaged either Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Connection.php | 1 + apps/user_ldap/lib/Group_LDAP.php | 31 ++++++++++++++++--------- apps/user_ldap/tests/Group_LDAPTest.php | 22 ++++++++++++++++-- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php index 079429027c..b7556bf236 100644 --- a/apps/user_ldap/lib/Connection.php +++ b/apps/user_ldap/lib/Connection.php @@ -72,6 +72,7 @@ use OCP\ILogger; * @property string ldapGidNumber * @property int hasMemberOfFilterSupport * @property int useMemberOfToDetectMembership + * @property string ldapMatchingRuleInChainState */ class Connection extends LDAPUtility { private $ldapConnectionRes = null; diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 617b9f498e..3532188d9f 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -254,21 +254,27 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I ) { $attemptedLdapMatchingRuleInChain = true; // compatibility hack with servers supporting :1.2.840.113556.1.4.1941:, and others) - $filter = $this->access->combineFilterWithAnd([$this->access->connection->ldapUserFilter, 'memberof:1.2.840.113556.1.4.1941:=' . $dnGroup]); + $filter = $this->access->combineFilterWithAnd([ + $this->access->connection->ldapUserFilter, + $this->access->connection->ldapUserDisplayName . '=*', + 'memberof:1.2.840.113556.1.4.1941:=' . $dnGroup + ]); $memberRecords = $this->access->fetchListOfUsers( $filter, $this->access->userManager->getAttributes(true) ); - if (!empty($memberRecords)) { - if ($this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN) { - $this->access->connection->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_AVAILABLE; - $this->access->connection->saveConfiguration(); - } - return array_reduce($memberRecords, function ($carry, $record) { - $carry[] = $record['dn'][0]; - return $carry; - }, []); + $result = array_reduce($memberRecords, function ($carry, $record) { + $carry[] = $record['dn'][0]; + return $carry; + }, []); + if ($this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_AVAILABLE) { + return $result; + } elseif (!empty($memberRecords)) { + $this->access->connection->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_AVAILABLE; + $this->access->connection->saveConfiguration(); + return $result; } + // when feature availability is unknown, and the result is empty, continue and test with original approach } $seen[$dnGroup] = 1; @@ -283,7 +289,10 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $allMembers += $this->getDynamicGroupMembers($dnGroup); $this->access->connection->writeToCache($cacheKey, $allMembers); - if (isset($attemptedLdapMatchingRuleInChain) && !empty($allMembers)) { + if (isset($attemptedLdapMatchingRuleInChain) + && $this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN + && !empty($allMembers) + ) { $this->access->connection->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_UNAVAILABLE; $this->access->connection->saveConfiguration(); } diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index 3143054940..4ddf9f5247 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -129,6 +129,10 @@ class Group_LDAPTest extends TestCase { ->method('countUsers') ->willReturn(2); + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['displayName', 'mail']); + $groupBackend = new GroupLDAP($access, $pluginManager); $users = $groupBackend->countUsersInGroup('group'); @@ -172,6 +176,10 @@ class Group_LDAPTest extends TestCase { ->method('escapeFilterPart') ->willReturnArgument(0); + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['displayName', 'mail']); + $groupBackend = new GroupLDAP($access, $pluginManager); $users = $groupBackend->countUsersInGroup('group', '3'); @@ -546,7 +554,10 @@ class Group_LDAPTest extends TestCase { $access->expects($this->any()) ->method('combineFilterWithAnd') ->willReturn('pseudo=filter'); - $access->userManager = $this->createMock(Manager::class); + + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['displayName', 'mail']); $groupBackend = new GroupLDAP($access, $pluginManager); $users = $groupBackend->usersInGroup('foobar'); @@ -587,7 +598,10 @@ class Group_LDAPTest extends TestCase { $access->expects($this->any()) ->method('combineFilterWithAnd') ->willReturn('pseudo=filter'); - $access->userManager = $this->createMock(Manager::class); + + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['displayName', 'mail']); $groupBackend = new GroupLDAP($access, $pluginManager); $users = $groupBackend->usersInGroup('foobar'); @@ -627,6 +641,10 @@ class Group_LDAPTest extends TestCase { ->method('isDNPartOfBase') ->willReturn(true); + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['displayName', 'mail']); + $groupBackend = new GroupLDAP($access, $pluginManager); $users = $groupBackend->countUsersInGroup('foobar');