From 6d74ef71b5b990c1685e95500d56c47645c379b8 Mon Sep 17 00:00:00 2001 From: alexweirig Date: Thu, 17 Mar 2016 11:31:28 +0100 Subject: [PATCH 1/6] Fixed dynamic group ldap access getUserGroups: Using $userDN instead of $uid to query LDAP Converting groupDN to group name using API instead of substring Removing cache processing at the end of the method --- apps/user_ldap/group_ldap.php | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index eba39ca50f77..c698723bebce 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -469,17 +469,18 @@ public function getUserGroups($uid) { // apply filter via ldap search to see if this user is in this // dynamic group $userMatch = $this->access->readAttribute( - $uid, + $userDN, $this->access->connection->ldapUserDisplayName, $memberUrlFilter ); if ($userMatch !== false) { // match found so this user is in this group - $pos = strpos($dynamicGroup['dn'][0], ','); - if ($pos !== false) { - $membershipGroup = substr($dynamicGroup['dn'][0],3,$pos-3); - $groups[] = $membershipGroup; - } + $groupName = $this->access->dn2groupname($dynamicGroup['dn'][0]); + if(is_string($groupName)) { + // be sure to never return false if the dn could not be + // resolved to a name, for whatever reason. + $groups[] = $groupName; + } } } else { \OCP\Util::writeLog('user_ldap', 'No search filter found on member url '. @@ -529,14 +530,6 @@ public function getUserGroups($uid) { $uid = $userDN; } - if(isset($this->cachedGroupsByMember[$uid])) { - $groups = $this->cachedGroupsByMember[$uid]; - } else { - $groups = array_values($this->getGroupsByMember($uid)); - $groups = $this->access->ownCloudGroupNames($groups); - $this->cachedGroupsByMember[$uid] = $groups; - } - if($primaryGroup !== false) { $groups[] = $primaryGroup; } From c342dcbcee24e31544c2f2c91d5259d6dee56370 Mon Sep 17 00:00:00 2001 From: alexweirig Date: Mon, 21 Mar 2016 08:06:38 +0100 Subject: [PATCH 2/6] Fixing group handling added back the cache processing and fixed --- apps/user_ldap/group_ldap.php | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index c698723bebce..069a7c161bcb 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -530,12 +530,21 @@ public function getUserGroups($uid) { $uid = $userDN; } - if($primaryGroup !== false) { - $groups[] = $primaryGroup; - } - - $groups = array_unique($groups, SORT_LOCALE_STRING); - $this->access->connection->writeToCache($cacheKey, $groups); + if(isset($this->cachedGroupsByMember[$uid])) { + $groups[] = $this->cachedGroupsByMember[$uid]; + } else { + $groupsByMember = array_values($this->getGroupsByMember($uid)); + $groupsByMember = $this->access->ownCloudGroupNames($groupsByMember); + $this->cachedGroupsByMember[$uid] = $groupsByMember; + $groups = array_merge($groups, $groupsByMember); + } + + if($primaryGroup !== false) { + $groups[] = $primaryGroup; + } + + $groups = array_unique($groups, SORT_LOCALE_STRING); + $this->access->connection->writeToCache($cacheKey, $groups); return $groups; } From f112ece1f23ac994c1701903c7bcde9dc55e4de3 Mon Sep 17 00:00:00 2001 From: alexweirig Date: Mon, 21 Mar 2016 08:08:36 +0100 Subject: [PATCH 3/6] fixed possible indention problem spaces -> tab conversion --- apps/user_ldap/group_ldap.php | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index 069a7c161bcb..278eadfdddd1 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -531,20 +531,20 @@ public function getUserGroups($uid) { } if(isset($this->cachedGroupsByMember[$uid])) { - $groups[] = $this->cachedGroupsByMember[$uid]; + $groups[] = $this->cachedGroupsByMember[$uid]; } else { - $groupsByMember = array_values($this->getGroupsByMember($uid)); - $groupsByMember = $this->access->ownCloudGroupNames($groupsByMember); - $this->cachedGroupsByMember[$uid] = $groupsByMember; - $groups = array_merge($groups, $groupsByMember); - } - - if($primaryGroup !== false) { - $groups[] = $primaryGroup; - } - - $groups = array_unique($groups, SORT_LOCALE_STRING); - $this->access->connection->writeToCache($cacheKey, $groups); + $groupsByMember = array_values($this->getGroupsByMember($uid)); + $groupsByMember = $this->access->ownCloudGroupNames($groupsByMember); + $this->cachedGroupsByMember[$uid] = $groupsByMember; + $groups = array_merge($groups, $groupsByMember); + } + + if($primaryGroup !== false) { + $groups[] = $primaryGroup; + } + + $groups = array_unique($groups, SORT_LOCALE_STRING); + $this->access->connection->writeToCache($cacheKey, $groups); return $groups; } From 087f005936ff5acfa58a874579aa235d86222945 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Mon, 21 Mar 2016 16:56:08 +0100 Subject: [PATCH 4/6] formatting, white-space changes only --- apps/user_ldap/group_ldap.php | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index 278eadfdddd1..63476525dfa8 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -476,11 +476,11 @@ public function getUserGroups($uid) { if ($userMatch !== false) { // match found so this user is in this group $groupName = $this->access->dn2groupname($dynamicGroup['dn'][0]); - if(is_string($groupName)) { - // be sure to never return false if the dn could not be - // resolved to a name, for whatever reason. - $groups[] = $groupName; - } + if(is_string($groupName)) { + // be sure to never return false if the dn could not be + // resolved to a name, for whatever reason. + $groups[] = $groupName; + } } } else { \OCP\Util::writeLog('user_ldap', 'No search filter found on member url '. @@ -530,9 +530,9 @@ public function getUserGroups($uid) { $uid = $userDN; } - if(isset($this->cachedGroupsByMember[$uid])) { + if(isset($this->cachedGroupsByMember[$uid])) { $groups[] = $this->cachedGroupsByMember[$uid]; - } else { + } else { $groupsByMember = array_values($this->getGroupsByMember($uid)); $groupsByMember = $this->access->ownCloudGroupNames($groupsByMember); $this->cachedGroupsByMember[$uid] = $groupsByMember; From b37f2e230b0ca8aa4498567b40e5b465240586ff Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 8 Jun 2016 11:22:01 +0200 Subject: [PATCH 5/6] Use array_merge when reading cached groups members --- apps/user_ldap/group_ldap.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index 63476525dfa8..f87fe1b5cab3 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -531,7 +531,7 @@ public function getUserGroups($uid) { } if(isset($this->cachedGroupsByMember[$uid])) { - $groups[] = $this->cachedGroupsByMember[$uid]; + $groups = array_merge($groups, $this->cachedGroupsByMember[$uid]); } else { $groupsByMember = array_values($this->getGroupsByMember($uid)); $groupsByMember = $this->access->ownCloudGroupNames($groupsByMember); From fba4460342bdcea7753e70c89a11c2ecc1b8824d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 9 Jun 2016 18:02:09 +0200 Subject: [PATCH 6/6] Add unit test for LDAP multi group caching --- apps/user_ldap/tests/group_ldap.php | 53 +++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/apps/user_ldap/tests/group_ldap.php b/apps/user_ldap/tests/group_ldap.php index 51bb1d84732a..a81bf70f54a7 100644 --- a/apps/user_ldap/tests/group_ldap.php +++ b/apps/user_ldap/tests/group_ldap.php @@ -455,4 +455,57 @@ public function testGetUserGroupsMemberOfDisabled() { $groupBackend->getUserGroups('userX'); } + public function testGetGroupsByMember() { + $access = $this->getAccessMock(); + + $access->connection->expects($this->any()) + ->method('__get') + ->will($this->returnCallback(function($name) { + if($name === 'useMemberOfToDetectMembership') { + return 0; + } else if($name === 'ldapDynamicGroupMemberURL') { + return ''; + } else if($name === 'ldapNestedGroups') { + return false; + } + return 1; + })); + + $dn = 'cn=userX,dc=foobar'; + + $access->connection->hasPrimaryGroups = false; + + $access->expects($this->exactly(2)) + ->method('username2dn') + ->will($this->returnValue($dn)); + + $access->expects($this->never()) + ->method('readAttribute') + ->with($dn, 'memberOf'); + + $group1 = [ + 'cn' => 'group1', + 'dn' => ['cn=group1,ou=groups,dc=domain,dc=com'], + ]; + $group2 = [ + 'cn' => 'group2', + 'dn' => ['cn=group2,ou=groups,dc=domain,dc=com'], + ]; + + $access->expects($this->once()) + ->method('ownCloudGroupNames') + ->with([$group1, $group2]) + ->will($this->returnValue(['group1', 'group2'])); + + $access->expects($this->once()) + ->method('fetchListOfGroups') + ->will($this->returnValue([$group1, $group2])); + + $groupBackend = new GroupLDAP($access); + $groups = $groupBackend->getUserGroups('userX'); + $this->assertEquals(['group1', 'group2'], $groups); + + $groupsAgain = $groupBackend->getUserGroups('userX'); + $this->assertEquals(['group1', 'group2'], $groupsAgain); + } }