From ed65fd1d60729e93f2b9169941aaa235d13395fe 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: Tobias Perschon --- 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 233077c99f353..cdbb6be890770 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -161,9 +161,14 @@ public function inGroup($uid, $gid) { 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); } $isInGroup = in_array($userDN, $members); From c3994936a80ba04fb029f47fc0c8aec827d23f3c 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 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 cdbb6be890770..bd9fe1530a2e9 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -141,7 +141,7 @@ public function inGroup($uid, $gid) { //extra work if we don't get back user DNs if (strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'memberuid') { $requestAttributes = $this->access->userManager->getAttributes(true); - $dns = []; + $users = []; $filterParts = []; $bytes = 0; foreach ($members as $mid) { @@ -151,23 +151,30 @@ public function inGroup($uid, $gid) { 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); $bytes = 0; $filterParts = []; - $users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); - $dns = array_merge($dns, $users); + $search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts)); + $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); } From bada6d17405d927733f15f28c6696e4f2ac5ac54 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: Tobias Perschon --- apps/user_ldap/lib/Group_LDAP.php | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index bd9fe1530a2e9..c9f8648bde49f 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -51,9 +51,7 @@ class Group_LDAP extends BackendUtility implements \OCP\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; /** @@ -168,12 +166,10 @@ public function inGroup($uid, $gid) { } // 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); } From e2fea6269b8b9fb2fd55f8a679ba8ff0b098eea4 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 c9f8648bde49f..54888bfd2a1f6 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -131,10 +131,6 @@ public function inGroup($uid, $gid) { //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 if (strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'memberuid') { @@ -174,6 +170,11 @@ public function inGroup($uid, $gid) { $members = array_keys($dns); } + 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 c9cb14ebdfd942bc66b947d3b1f09cb9fe4b0a96 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 a770149c243a2..836694e4353c1 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -506,6 +506,179 @@ public function testInGroupHitsUidGidCache() { $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 957f6347e5228e1f549f66e3753185325afe2336 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: Tobias Perschon --- apps/user_ldap/tests/Group_LDAPTest.php | 99 ++++++++++++------------- 1 file changed, 47 insertions(+), 52 deletions(-) diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index 836694e4353c1..7ca2413f8c317 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -48,6 +48,47 @@ * @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 \PHPUnit_Framework_MockObject_MockObject|Access */ @@ -96,52 +137,6 @@ private function enableGroups($access) { }); } - 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); - $access->expects($this->any()) - ->method('combineFilterWithAnd') - ->willReturn('pseudo=filter'); - - // 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(); @@ -506,7 +501,7 @@ public function testInGroupHitsUidGidCache() { $groupBackend->inGroup($uid, $gid); } - public function groupWithMembersProvider() { + public function groupWithMembersProvider() { return [ [ 'someGroup', @@ -542,7 +537,7 @@ public function testInGroupMember(string $gid, string $groupDn, array $memberDNs case 'ldapDynamicGroupMemberURL': return ''; case 'hasPrimaryGroups': - case 'ldapNestedGroups'; + case 'ldapNestedGroups': return 0; default: return 1; @@ -588,7 +583,7 @@ public function testInGroupMemberNot(string $gid, string $groupDn, array $member case 'ldapDynamicGroupMemberURL': return ''; case 'hasPrimaryGroups': - case 'ldapNestedGroups'; + case 'ldapNestedGroups': return 0; default: return 1; @@ -644,7 +639,7 @@ public function testInGroupMemberUid(string $gid, string $groupDn, array $member case 'ldapLoginFilter': return 'uid=%uid'; case 'hasPrimaryGroups': - case 'ldapNestedGroups'; + case 'ldapNestedGroups': return 0; default: return 1; @@ -902,8 +897,8 @@ public function testGetUserGroupsMemberOfDisabled() { public function nestedGroupsProvider(): array { return [ - [ true ], - [ false ], + [true], + [false], ]; }