From 7604bcb3cb07389d76e1f4e8b2023003845efbdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Fortier?= Date: Mon, 3 Aug 2015 20:15:22 -0400 Subject: [PATCH 1/3] Properly nest groups when using memberOf to detect group membership, fixes #17759 --- apps/user_ldap/group_ldap.php | 39 ++++++++++++++++++++++++++--- apps/user_ldap/tests/group_ldap.php | 7 +++--- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index 1b83817151..44b0ceac7e 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -181,6 +181,39 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { return $allMembers; } + /** + * @param string $dnGroup + * @param array &$seen + * @return array + */ + private function _getGroupDNsFromMemberOf($DN, &$seen = null) { + if ($seen === null) { + $seen = array(); + } + if (array_key_exists($DN, $seen)) { + // avoid loops + return array(); + } + $seen[$DN] = 1; + $groups = $this->access->readAttribute($DN, 'memberOf'); + if (is_array($groups)) { + $groups = $this->access->groupsMatchFilter($groups); + $allGroups = $groups; + foreach ($groups as $group) { + $nestedGroups = $this->access->connection->ldapNestedGroups; + if (!empty($nestedGroups)) { + $subGroups = $this->_getGroupDNsFromMemberOf($group, $seen); + if ($subGroups) { + $allGroups = array_merge($allGroups, $subGroups); + } + } + } + return $allGroups; + } else { + return array(); + } + } + /** * translates a primary group ID into an ownCloud internal name * @param string $gid as given by primaryGroupID on AD @@ -377,14 +410,14 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { if(intval($this->access->connection->hasMemberOfFilterSupport) === 1 && intval($this->access->connection->useMemberOfToDetectMembership) === 1 ) { - $groupDNs = $this->access->readAttribute($userDN, 'memberOf'); - + $groupDNs = $this->_getGroupDNsFromMemberOf($userDN); + if (is_array($groupDNs)) { - $groupDNs = $this->access->groupsMatchFilter($groupDNs); foreach ($groupDNs as $dn) { $groups[] = $this->access->dn2groupname($dn); } } + if($primaryGroup !== false) { $groups[] = $primaryGroup; } diff --git a/apps/user_ldap/tests/group_ldap.php b/apps/user_ldap/tests/group_ldap.php index f716618ce4..805238e7d3 100644 --- a/apps/user_ldap/tests/group_ldap.php +++ b/apps/user_ldap/tests/group_ldap.php @@ -395,16 +395,15 @@ class Test_Group_Ldap extends \Test\TestCase { ->method('username2dn') ->will($this->returnValue($dn)); - $access->expects($this->once()) + $access->expects($this->exactly(3)) ->method('readAttribute') - ->with($dn, 'memberOf') - ->will($this->returnValue(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'])); + ->will($this->onConsecutiveCalls(['cn=groupA,dc=foobar', 'cn=groupB,dc=foobar'], [], [])); $access->expects($this->exactly(2)) ->method('dn2groupname') ->will($this->returnArgument(0)); - $access->expects($this->once()) + $access->expects($this->exactly(3)) ->method('groupsMatchFilter') ->will($this->returnArgument(0)); From e0469d001384eb9c4125ca16d9babdf673be57ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Fortier?= Date: Tue, 4 Aug 2015 11:19:57 -0400 Subject: [PATCH 2/3] Take review comments into consideration for pr #18042 / issue #17759 --- apps/user_ldap/group_ldap.php | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index 44b0ceac7e..a592913c8f 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -182,8 +182,8 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { } /** - * @param string $dnGroup - * @param array &$seen + * @param string $DN + * @param array|null &$seen * @return array */ private function _getGroupDNsFromMemberOf($DN, &$seen = null) { @@ -196,22 +196,19 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { } $seen[$DN] = 1; $groups = $this->access->readAttribute($DN, 'memberOf'); - if (is_array($groups)) { - $groups = $this->access->groupsMatchFilter($groups); - $allGroups = $groups; - foreach ($groups as $group) { - $nestedGroups = $this->access->connection->ldapNestedGroups; - if (!empty($nestedGroups)) { - $subGroups = $this->_getGroupDNsFromMemberOf($group, $seen); - if ($subGroups) { - $allGroups = array_merge($allGroups, $subGroups); - } - } - } - return $allGroups; - } else { + if (!is_array($groups)) { return array(); } + $groups = $this->access->groupsMatchFilter($groups); + $allGroups = $groups; + foreach ($groups as $group) { + $nestedGroups = $this->access->connection->ldapNestedGroups; + if (!empty($nestedGroups)) { + $subGroups = $this->_getGroupDNsFromMemberOf($group, $seen); + $allGroups = array_merge($allGroups, $subGroups); + } + } + return $allGroups; } /** From a55f233e9ffac7d492733f50a37343b4243898bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Fortier?= Date: Fri, 7 Aug 2015 08:56:28 -0400 Subject: [PATCH 3/3] fix more review comments for #18042 / #17759 --- apps/user_ldap/group_ldap.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index a592913c8f..3b47eb56b0 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -201,9 +201,9 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { } $groups = $this->access->groupsMatchFilter($groups); $allGroups = $groups; - foreach ($groups as $group) { - $nestedGroups = $this->access->connection->ldapNestedGroups; - if (!empty($nestedGroups)) { + $nestedGroups = $this->access->connection->ldapNestedGroups; + if (intval($nestedGroups) === 1) { + foreach ($groups as $group) { $subGroups = $this->_getGroupDNsFromMemberOf($group, $seen); $allGroups = array_merge($allGroups, $subGroups); } @@ -408,7 +408,6 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface { && intval($this->access->connection->useMemberOfToDetectMembership) === 1 ) { $groupDNs = $this->_getGroupDNsFromMemberOf($userDN); - if (is_array($groupDNs)) { foreach ($groupDNs as $dn) { $groups[] = $this->access->dn2groupname($dn);