From d818975d84ed4b0876656326f8d50b8a57ea4ff6 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 26 Nov 2020 23:36:04 +0100 Subject: [PATCH 1/6] flatten result array as expected by following code Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 5444f0815e..e396999f86 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -170,9 +170,14 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if (count($filterParts) > 0) { $filter = $this->access->combineFilterWithOr($filterParts); $users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); - $dns = array_merge($dns, $users); + $dns = array_reduce($users, function (array $carry, array $record) { + if (!in_array($carry, $record['dn'][0])) { + $carry[$record['dn'][0]] = 1; + } + return $carry; + }, $dns); } - $members = $dns; + $members = array_keys($dns); break; } From 250c7535cd97bedf9af8fab5344ef08ce6c0b4df Mon Sep 17 00:00:00 2001 From: Tobias Perschon Date: Fri, 27 Nov 2020 03:02:43 +0100 Subject: [PATCH 2/6] moved the array_reduce to fix large search case also added some additional comments and renamed some vars to make it intuitive whats in them Signed-off-by: Tobias Perschon --- apps/user_ldap/lib/Group_LDAP.php | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index e396999f86..f1f6f94615 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -146,7 +146,7 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I case 'memberuid': case 'zimbramailforwardingaddress': $requestAttributes = $this->access->userManager->getAttributes(true); - $dns = []; + $users = []; $filterParts = []; $bytes = 0; foreach ($members as $mid) { @@ -160,24 +160,31 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I if ($bytes >= 9000000) { // AD has a default input buffer of 10 MB, we do not want // to take even the chance to exceed it + // so we fetch results with the filterParts we collected so far $filter = $this->access->combineFilterWithOr($filterParts); - $users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); + $search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); $bytes = 0; $filterParts = []; - $dns = array_merge($dns, $users); + $users = array_merge($users, $search); } } + if (count($filterParts) > 0) { + // if there are filterParts left we need to add their result $filter = $this->access->combineFilterWithOr($filterParts); - $users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); - $dns = array_reduce($users, function (array $carry, array $record) { - if (!in_array($carry, $record['dn'][0])) { - $carry[$record['dn'][0]] = 1; - } - return $carry; - }, $dns); + $search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); + $users = array_merge($users, $search); } + + // now we cleanup the users array to get only dns + $dns = array_reduce($users, function (array $carry, array $record) { + if (!in_array($carry, $record['dn'][0])) { + $carry[$record['dn'][0]] = 1; + } + return $carry; + }, []); $members = array_keys($dns); + break; } From 213464afcaa74e3094be05459b6f56553d57fa81 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 27 Nov 2020 18:22:59 +0100 Subject: [PATCH 3/6] use faster and less hungry foreach Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index f1f6f94615..9cedbbadf5 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -55,7 +55,7 @@ use OCP\ILogger; class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend { protected $enabled = false; - /** @var string[] $cachedGroupMembers array of users with gid as key */ + /** @var string[][] $cachedGroupMembers array of users with gid as key */ protected $cachedGroupMembers; /** @var string[] $cachedGroupsByMember array of groups with uid as key */ protected $cachedGroupsByMember; @@ -168,23 +168,21 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I $users = array_merge($users, $search); } } - + if (count($filterParts) > 0) { // if there are filterParts left we need to add their result $filter = $this->access->combineFilterWithOr($filterParts); $search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); $users = array_merge($users, $search); } - + // now we cleanup the users array to get only dns - $dns = array_reduce($users, function (array $carry, array $record) { - if (!in_array($carry, $record['dn'][0])) { - $carry[$record['dn'][0]] = 1; - } - return $carry; - }, []); + $dns = []; + foreach ($users as $record) { + $dns[$record['dn'][0]] = 1; + } $members = array_keys($dns); - + break; } From cca1fb792b67b013e70c4d98ce98669fa5dbaa32 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 27 Nov 2020 18:44:27 +0100 Subject: [PATCH 4/6] check number of members after potential resolving of rdns - the type check is not necessary anymore for the return type of _groupMembers() Signed-off-by: Arthur Schiwon --- apps/user_ldap/lib/Group_LDAP.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 9cedbbadf5..be2fbecad8 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -136,10 +136,6 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I //usually, LDAP attributes are said to be case insensitive. But there are exceptions of course. $members = $this->_groupMembers($groupDN); - if (!is_array($members) || count($members) === 0) { - $this->access->connection->writeToCache($cacheKey, false); - return false; - } //extra work if we don't get back user DNs switch ($this->ldapGroupMemberAssocAttr) { @@ -186,6 +182,11 @@ class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, I break; } + if (count($members) === 0) { + $this->access->connection->writeToCache($cacheKey, false); + return false; + } + $isInGroup = in_array($userDN, $members); $this->access->connection->writeToCache($cacheKey, $isInGroup); $this->access->connection->writeToCache($cacheKeyMembers, $members); From 43c6bc134e1f2818fa8526324f7dfbad1871ba08 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 27 Nov 2020 19:24:12 +0100 Subject: [PATCH 5/6] add unit tests Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/Group_LDAPTest.php | 173 ++++++++++++++++++++++++ 1 file changed, 173 insertions(+) diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index 74dd2b467c..63de2ee948 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -503,6 +503,179 @@ class Group_LDAPTest extends TestCase { $groupBackend->inGroup($uid, $gid); } + public function groupWithMembersProvider() { + return [ + [ + 'someGroup', + 'cn=someGroup,ou=allTheGroups,ou=someDepartment,dc=someDomain,dc=someTld', + [ + 'uid=oneUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld', + 'uid=someUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld', + 'uid=anotherUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld', + 'uid=differentUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld', + ], + ], + ]; + } + + /** + * @dataProvider groupWithMembersProvider + */ + public function testInGroupMember(string $gid, string $groupDn, array $memberDNs) { + $access = $this->getAccessMock(); + $pluginManager = $this->getPluginManagerMock(); + + $access->connection = $this->createMock(Connection::class); + + $uid = 'someUser'; + $userDn = $memberDNs[0]; + + $access->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function ($name) { + switch ($name) { + case 'ldapGroupMemberAssocAttr': + return 'member'; + case 'ldapDynamicGroupMemberURL': + return ''; + case 'hasPrimaryGroups': + case 'ldapNestedGroups'; + return 0; + default: + return 1; + } + }); + $access->connection->expects($this->any()) + ->method('getFromCache') + ->willReturn(null); + + $access->expects($this->once()) + ->method('username2dn') + ->with($uid) + ->willReturn($userDn); + $access->expects($this->once()) + ->method('groupname2dn') + ->willReturn($groupDn); + $access->expects($this->any()) + ->method('readAttribute') + ->willReturn($memberDNs); + + $groupBackend = new GroupLDAP($access, $pluginManager); + $this->assertTrue($groupBackend->inGroup($uid, $gid)); + } + + /** + * @dataProvider groupWithMembersProvider + */ + public function testInGroupMemberNot(string $gid, string $groupDn, array $memberDNs) { + $access = $this->getAccessMock(); + $pluginManager = $this->getPluginManagerMock(); + + $access->connection = $this->createMock(Connection::class); + + $uid = 'unelatedUser'; + $userDn = 'uid=unrelatedUser,ou=unrelatedTeam,ou=unrelatedDepartment,dc=someDomain,dc=someTld'; + + $access->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function ($name) { + switch ($name) { + case 'ldapGroupMemberAssocAttr': + return 'member'; + case 'ldapDynamicGroupMemberURL': + return ''; + case 'hasPrimaryGroups': + case 'ldapNestedGroups'; + return 0; + default: + return 1; + } + }); + $access->connection->expects($this->any()) + ->method('getFromCache') + ->willReturn(null); + + $access->expects($this->once()) + ->method('username2dn') + ->with($uid) + ->willReturn($userDn); + $access->expects($this->once()) + ->method('groupname2dn') + ->willReturn($groupDn); + $access->expects($this->any()) + ->method('readAttribute') + ->willReturn($memberDNs); + + $groupBackend = new GroupLDAP($access, $pluginManager); + $this->assertFalse($groupBackend->inGroup($uid, $gid)); + } + + /** + * @dataProvider groupWithMembersProvider + */ + public function testInGroupMemberUid(string $gid, string $groupDn, array $memberDNs) { + $access = $this->getAccessMock(); + $pluginManager = $this->getPluginManagerMock(); + + $memberUids = []; + $userRecords = []; + foreach ($memberDNs as $dn) { + $memberUids[] = ldap_explode_dn($dn, false)[0]; + $userRecords[] = ['dn' => [$dn]]; + } + + + $access->connection = $this->createMock(Connection::class); + + $uid = 'someUser'; + $userDn = $memberDNs[0]; + + $access->connection->expects($this->any()) + ->method('__get') + ->willReturnCallback(function ($name) { + switch ($name) { + case 'ldapGroupMemberAssocAttr': + return 'memberUid'; + case 'ldapDynamicGroupMemberURL': + return ''; + case 'ldapLoginFilter': + return 'uid=%uid'; + case 'hasPrimaryGroups': + case 'ldapNestedGroups'; + return 0; + default: + return 1; + } + }); + $access->connection->expects($this->any()) + ->method('getFromCache') + ->willReturn(null); + + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['uid', 'mail', 'displayname']); + + $access->expects($this->once()) + ->method('username2dn') + ->with($uid) + ->willReturn($userDn); + $access->expects($this->once()) + ->method('groupname2dn') + ->willReturn($groupDn); + $access->expects($this->any()) + ->method('readAttribute') + ->willReturn($memberUids); + $access->expects($this->any()) + ->method('fetchListOfUsers') + ->willReturn($userRecords); + $access->expects($this->any()) + ->method('combineFilterWithOr') + ->willReturn('(|(pseudo=filter)(filter=pseudo))'); + + $groupBackend = new GroupLDAP($access, $pluginManager); + $this->assertTrue($groupBackend->inGroup($uid, $gid)); + } + public function testGetGroupsWithOffset() { $access = $this->getAccessMock(); $pluginManager = $this->getPluginManagerMock(); From 270eb6c593f93dbe35faf6abb3cd795689a9a9ac Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Fri, 27 Nov 2020 19:34:35 +0100 Subject: [PATCH 6/6] php-cs happyness Signed-off-by: Arthur Schiwon --- apps/user_ldap/tests/Group_LDAPTest.php | 94 ++++++++++++------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index 63de2ee948..bc582ab90b 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -50,6 +50,47 @@ use Test\TestCase; * @package OCA\User_LDAP\Tests */ class Group_LDAPTest extends TestCase { + public function testCountEmptySearchString() { + $access = $this->getAccessMock(); + $pluginManager = $this->getPluginManagerMock(); + $groupDN = 'cn=group,dc=foo,dc=bar'; + + $this->enableGroups($access); + + $access->expects($this->any()) + ->method('groupname2dn') + ->willReturn($groupDN); + $access->expects($this->any()) + ->method('readAttribute') + ->willReturnCallback(function ($dn) use ($groupDN) { + if ($dn === $groupDN) { + return [ + 'uid=u11,ou=users,dc=foo,dc=bar', + 'uid=u22,ou=users,dc=foo,dc=bar', + 'uid=u33,ou=users,dc=foo,dc=bar', + 'uid=u34,ou=users,dc=foo,dc=bar' + ]; + } + return []; + }); + $access->expects($this->any()) + ->method('isDNPartOfBase') + ->willReturn(true); + // for primary groups + $access->expects($this->once()) + ->method('countUsers') + ->willReturn(2); + + $access->userManager->expects($this->any()) + ->method('getAttributes') + ->willReturn(['displayName', 'mail']); + + $groupBackend = new GroupLDAP($access, $pluginManager); + $users = $groupBackend->countUsersInGroup('group'); + + $this->assertSame(6, $users); + } + /** * @return MockObject|Access */ @@ -98,47 +139,6 @@ class Group_LDAPTest extends TestCase { }); } - public function testCountEmptySearchString() { - $access = $this->getAccessMock(); - $pluginManager = $this->getPluginManagerMock(); - $groupDN = 'cn=group,dc=foo,dc=bar'; - - $this->enableGroups($access); - - $access->expects($this->any()) - ->method('groupname2dn') - ->willReturn($groupDN); - $access->expects($this->any()) - ->method('readAttribute') - ->willReturnCallback(function ($dn) use ($groupDN) { - if ($dn === $groupDN) { - return [ - 'uid=u11,ou=users,dc=foo,dc=bar', - 'uid=u22,ou=users,dc=foo,dc=bar', - 'uid=u33,ou=users,dc=foo,dc=bar', - 'uid=u34,ou=users,dc=foo,dc=bar' - ]; - } - return []; - }); - $access->expects($this->any()) - ->method('isDNPartOfBase') - ->willReturn(true); - // for primary groups - $access->expects($this->once()) - ->method('countUsers') - ->willReturn(2); - - $access->userManager->expects($this->any()) - ->method('getAttributes') - ->willReturn(['displayName', 'mail']); - - $groupBackend = new GroupLDAP($access, $pluginManager); - $users = $groupBackend->countUsersInGroup('group'); - - $this->assertSame(6, $users); - } - public function testCountWithSearchString() { $access = $this->getAccessMock(); $pluginManager = $this->getPluginManagerMock(); @@ -503,7 +503,7 @@ class Group_LDAPTest extends TestCase { $groupBackend->inGroup($uid, $gid); } - public function groupWithMembersProvider() { + public function groupWithMembersProvider() { return [ [ 'someGroup', @@ -539,7 +539,7 @@ class Group_LDAPTest extends TestCase { case 'ldapDynamicGroupMemberURL': return ''; case 'hasPrimaryGroups': - case 'ldapNestedGroups'; + case 'ldapNestedGroups': return 0; default: return 1; @@ -585,7 +585,7 @@ class Group_LDAPTest extends TestCase { case 'ldapDynamicGroupMemberURL': return ''; case 'hasPrimaryGroups': - case 'ldapNestedGroups'; + case 'ldapNestedGroups': return 0; default: return 1; @@ -641,7 +641,7 @@ class Group_LDAPTest extends TestCase { case 'ldapLoginFilter': return 'uid=%uid'; case 'hasPrimaryGroups': - case 'ldapNestedGroups'; + case 'ldapNestedGroups': return 0; default: return 1; @@ -894,8 +894,8 @@ class Group_LDAPTest extends TestCase { public function nestedGroupsProvider(): array { return [ - [ true ], - [ false ], + [true], + [false], ]; }