do not flip available state to unavailable, allow empty results

- 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 <blizzz@arthur-schiwon.de>
This commit is contained in:
Arthur Schiwon 2020-08-12 00:34:15 +02:00
parent 74bde3eb49
commit ebb565f7a8
No known key found for this signature in database
GPG Key ID: 7424F1874854DF23
3 changed files with 41 additions and 13 deletions

View File

@ -67,6 +67,7 @@ use OCP\ILogger;
* @property string[] ldapBaseGroups * @property string[] ldapBaseGroups
* @property string ldapGroupFilter * @property string ldapGroupFilter
* @property string ldapGroupDisplayName * @property string ldapGroupDisplayName
* @property string ldapMatchingRuleInChainState
*/ */
class Connection extends LDAPUtility { class Connection extends LDAPUtility {
private $ldapConnectionRes = null; private $ldapConnectionRes = null;

View File

@ -243,21 +243,27 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
) { ) {
$attemptedLdapMatchingRuleInChain = true; $attemptedLdapMatchingRuleInChain = true;
// compatibility hack with servers supporting :1.2.840.113556.1.4.1941:, and others) // 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( $memberRecords = $this->access->fetchListOfUsers(
$filter, $filter,
$this->access->userManager->getAttributes(true) $this->access->userManager->getAttributes(true)
); );
if (!empty($memberRecords)) { $result = array_reduce($memberRecords, function ($carry, $record) {
if ($this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_UNKNOWN) { $carry[] = $record['dn'][0];
$this->access->connection->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_AVAILABLE; return $carry;
$this->access->connection->saveConfiguration(); }, []);
} if ($this->access->connection->ldapMatchingRuleInChainState === Configuration::LDAP_SERVER_FEATURE_AVAILABLE) {
return array_reduce($memberRecords, function ($carry, $record) { return $result;
$carry[] = $record['dn'][0]; } elseif (!empty($memberRecords)) {
return $carry; $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; $seen[$dnGroup] = 1;
@ -272,7 +278,10 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
$allMembers += $this->getDynamicGroupMembers($dnGroup); $allMembers += $this->getDynamicGroupMembers($dnGroup);
$this->access->connection->writeToCache($cacheKey, $allMembers); $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->ldapMatchingRuleInChainState = Configuration::LDAP_SERVER_FEATURE_UNAVAILABLE;
$this->access->connection->saveConfiguration(); $this->access->connection->saveConfiguration();
} }

View File

@ -129,6 +129,10 @@ class Group_LDAPTest extends TestCase {
->method('countUsers') ->method('countUsers')
->willReturn(2); ->willReturn(2);
$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);
$groupBackend = new GroupLDAP($access, $pluginManager); $groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->countUsersInGroup('group'); $users = $groupBackend->countUsersInGroup('group');
@ -169,6 +173,10 @@ class Group_LDAPTest extends TestCase {
->method('isDNPartOfBase') ->method('isDNPartOfBase')
->willReturn(true); ->willReturn(true);
$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);
$groupBackend = new GroupLDAP($access, $pluginManager); $groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->countUsersInGroup('group', '3'); $users = $groupBackend->countUsersInGroup('group', '3');
@ -543,7 +551,10 @@ class Group_LDAPTest extends TestCase {
$access->expects($this->any()) $access->expects($this->any())
->method('combineFilterWithAnd') ->method('combineFilterWithAnd')
->willReturn('pseudo=filter'); ->willReturn('pseudo=filter');
$access->userManager = $this->createMock(Manager::class);
$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);
$groupBackend = new GroupLDAP($access, $pluginManager); $groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->usersInGroup('foobar'); $users = $groupBackend->usersInGroup('foobar');
@ -584,7 +595,10 @@ class Group_LDAPTest extends TestCase {
$access->expects($this->any()) $access->expects($this->any())
->method('combineFilterWithAnd') ->method('combineFilterWithAnd')
->willReturn('pseudo=filter'); ->willReturn('pseudo=filter');
$access->userManager = $this->createMock(Manager::class);
$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);
$groupBackend = new GroupLDAP($access, $pluginManager); $groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->usersInGroup('foobar'); $users = $groupBackend->usersInGroup('foobar');
@ -624,6 +638,10 @@ class Group_LDAPTest extends TestCase {
->method('isDNPartOfBase') ->method('isDNPartOfBase')
->willReturn(true); ->willReturn(true);
$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);
$groupBackend = new GroupLDAP($access, $pluginManager); $groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->countUsersInGroup('foobar'); $users = $groupBackend->countUsersInGroup('foobar');