diff --git a/.drone.yml b/.drone.yml index e3cfc2d7be..f821333ee9 100644 --- a/.drone.yml +++ b/.drone.yml @@ -1049,7 +1049,7 @@ services: matrix: TESTS: acceptance openldap: - image: nextcloudci/openldap:openldap-6 + image: nextcloudci/openldap:openldap-7 environment: - SLAPD_DOMAIN=nextcloud.ci - SLAPD_ORGANIZATION=Nextcloud diff --git a/apps/user_ldap/lib/Connection.php b/apps/user_ldap/lib/Connection.php index ba393dffc1..4335f8e439 100644 --- a/apps/user_ldap/lib/Connection.php +++ b/apps/user_ldap/lib/Connection.php @@ -62,6 +62,9 @@ use OCP\ILogger; * @property string ldapEmailAttribute * @property string ldapExtStorageHomeAttribute * @property string homeFolderNamingRule + * @property bool|string ldapNestedGroups + * @property string[] ldapBaseGroups + * @property string ldapGroupFilter */ class Connection extends LDAPUtility { private $ldapConnectionRes = null; diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php index 2240c2ad22..1658807c0d 100644 --- a/apps/user_ldap/lib/Group_LDAP.php +++ b/apps/user_ldap/lib/Group_LDAP.php @@ -58,6 +58,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD */ protected $cachedGroupsByMember; + /** + * @var string[] $cachedNestedGroups array of groups with gid (DN) as key + */ + protected $cachedNestedGroups; + /** @var GroupPluginManager */ protected $groupPluginManager; @@ -71,6 +76,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD $this->cachedGroupMembers = new CappedMemoryCache(); $this->cachedGroupsByMember = new CappedMemoryCache(); + $this->cachedNestedGroups = new CappedMemoryCache(); $this->groupPluginManager = $groupPluginManager; } @@ -212,12 +218,12 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD */ private function _groupMembers($dnGroup, &$seen = null) { if ($seen === null) { - $seen = array(); + $seen = []; } - $allMembers = array(); + $allMembers = []; if (array_key_exists($dnGroup, $seen)) { // avoid loops - return array(); + return []; } // used extensively in cron job, caching makes sense for nested groups $cacheKey = '_groupMembers'.$dnGroup; @@ -226,19 +232,12 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD return $groupMembers; } $seen[$dnGroup] = 1; - $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr, - $this->access->connection->ldapGroupFilter); + $members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr); if (is_array($members)) { - foreach ($members as $member) { - $allMembers[$member] = 1; - $nestedGroups = $this->access->connection->ldapNestedGroups; - if (!empty($nestedGroups)) { - $subMembers = $this->_groupMembers($member, $seen); - if ($subMembers) { - $allMembers += $subMembers; - } - } - } + $fetcher = function($memberDN, &$seen) { + return $this->_groupMembers($memberDN, $seen); + }; + $allMembers = $this->walkNestedGroups($dnGroup, $fetcher, $members); } $allMembers += $this->getDynamicGroupMembers($dnGroup); @@ -251,30 +250,69 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD * @param string $DN * @param array|null &$seen * @return array + * @throws \OC\ServerNotAvailableException */ - private function _getGroupDNsFromMemberOf($DN, &$seen = null) { - if ($seen === null) { - $seen = array(); - } - if (array_key_exists($DN, $seen)) { - // avoid loops - return array(); - } - $seen[$DN] = 1; + private function _getGroupDNsFromMemberOf($DN) { $groups = $this->access->readAttribute($DN, 'memberOf'); if (!is_array($groups)) { - return array(); + return []; } - $groups = $this->access->groupsMatchFilter($groups); - $allGroups = $groups; - $nestedGroups = $this->access->connection->ldapNestedGroups; - if ((int)$nestedGroups === 1) { - foreach ($groups as $group) { - $subGroups = $this->_getGroupDNsFromMemberOf($group, $seen); - $allGroups = array_merge($allGroups, $subGroups); + + $fetcher = function($groupDN) { + if (isset($this->cachedNestedGroups[$groupDN])) { + $nestedGroups = $this->cachedNestedGroups[$groupDN]; + } else { + $nestedGroups = $this->access->readAttribute($groupDN, 'memberOf'); + if (!is_array($nestedGroups)) { + $nestedGroups = []; + } + $this->cachedNestedGroups[$groupDN] = $nestedGroups; } + return $nestedGroups; + }; + + $groups = $this->walkNestedGroups($DN, $fetcher, $groups); + return $this->access->groupsMatchFilter($groups); + } + + /** + * @param string $dn + * @param \Closure $fetcher args: string $dn, array $seen, returns: string[] of dns + * @param array $list + * @return array + */ + private function walkNestedGroups(string $dn, \Closure $fetcher, array $list): array { + $nesting = (int) $this->access->connection->ldapNestedGroups; + // depending on the input, we either have a list of DNs or a list of LDAP records + // also, the output expects either DNs or records. Testing the first element should suffice. + $recordMode = is_array($list) && isset($list[0]) && is_array($list[0]) && isset($list[0]['dn'][0]); + + if ($nesting !== 1) { + if($recordMode) { + // the keys are numeric, but should hold the DN + return array_reduce($list, function ($transformed, $record) use ($dn) { + if($record['dn'][0] != $dn) { + $transformed[$record['dn'][0]] = $record; + } + return $transformed; + }, []); + } + return $list; } - return $allGroups; + + $seen = []; + while ($record = array_pop($list)) { + $recordDN = $recordMode ? $record['dn'][0] : $record; + if ($recordDN === $dn || array_key_exists($recordDN, $seen)) { + // Prevent loops + continue; + } + $fetched = $fetcher($record, $seen); + $list = array_merge($list, $fetched); + $seen[$recordDN] = $record; + } + + return $recordMode ? $seen : array_keys($seen); } /** @@ -737,34 +775,28 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD */ private function getGroupsByMember($dn, &$seen = null) { if ($seen === null) { - $seen = array(); + $seen = []; } - $allGroups = array(); if (array_key_exists($dn, $seen)) { // avoid loops - return array(); + return []; } + $allGroups = []; $seen[$dn] = true; - $filter = $this->access->combineFilterWithAnd(array( - $this->access->connection->ldapGroupFilter, - $this->access->connection->ldapGroupMemberAssocAttr.'='.$dn - )); + $filter = $this->access->connection->ldapGroupMemberAssocAttr.'='.$dn; $groups = $this->access->fetchListOfGroups($filter, - array($this->access->connection->ldapGroupDisplayName, 'dn')); + [$this->access->connection->ldapGroupDisplayName, 'dn']); if (is_array($groups)) { - foreach ($groups as $groupobj) { - $groupDN = $groupobj['dn'][0]; - $allGroups[$groupDN] = $groupobj; - $nestedGroups = $this->access->connection->ldapNestedGroups; - if (!empty($nestedGroups)) { - $supergroups = $this->getGroupsByMember($groupDN, $seen); - if (is_array($supergroups) && (count($supergroups)>0)) { - $allGroups = array_merge($allGroups, $supergroups); - } + $fetcher = function ($dn, &$seen) { + if(is_array($dn) && isset($dn['dn'][0])) { + $dn = $dn['dn'][0]; } - } + return $this->getGroupsByMember($dn, $seen); + }; + $allGroups = $this->walkNestedGroups($dn, $fetcher, $groups); } - return $allGroups; + $visibleGroups = $this->access->groupsMatchFilter(array_keys($allGroups)); + return array_intersect_key($allGroups, array_flip($visibleGroups)); } /** @@ -811,7 +843,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD $primaryUsers = $this->getUsersInPrimaryGroup($groupDN, $search, $limit, $offset); $posixGroupUsers = $this->getUsersInGidNumber($groupDN, $search, $limit, $offset); - $members = array_keys($this->_groupMembers($groupDN)); + $members = $this->_groupMembers($groupDN); if(!$members && empty($posixGroupUsers) && empty($primaryUsers)) { //in case users could not be retrieved, return empty result set $this->access->connection->writeToCache($cacheKey, []); @@ -886,7 +918,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD return false; } - $members = array_keys($this->_groupMembers($groupDN)); + $members = $this->_groupMembers($groupDN); $primaryUserCount = $this->countUsersInPrimaryGroup($groupDN, ''); if(!$members && $primaryUserCount === 0) { //in case users could not be retrieved, return empty result set diff --git a/apps/user_ldap/tests/Group_LDAPTest.php b/apps/user_ldap/tests/Group_LDAPTest.php index ec8d888e2a..cb5760e317 100644 --- a/apps/user_ldap/tests/Group_LDAPTest.php +++ b/apps/user_ldap/tests/Group_LDAPTest.php @@ -98,16 +98,27 @@ 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') - ->will($this->returnValue('cn=group,dc=foo,dc=bar')); + ->will($this->returnValue($groupDN)); $access->expects($this->any()) ->method('readAttribute') - ->will($this->returnValue(array('u11', 'u22', 'u33', 'u34'))); + ->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 []; + }); // for primary groups $access->expects($this->once()) @@ -132,7 +143,7 @@ class Group_LDAPTest extends TestCase { $access->expects($this->any()) ->method('fetchListOfUsers') - ->will($this->returnValue(array())); + ->will($this->returnValue([])); $access->expects($this->any()) ->method('readAttribute') @@ -145,7 +156,7 @@ class Group_LDAPTest extends TestCase { if(strpos($name, 'u') === 0) { return strpos($name, '3'); } - return array('u11', 'u22', 'u33', 'u34'); + return ['u11', 'u22', 'u33', 'u34']; })); $access->expects($this->any()) @@ -625,7 +636,7 @@ class Group_LDAPTest extends TestCase { ->method('dn2groupname') ->will($this->returnArgument(0)); - $access->expects($this->exactly(3)) + $access->expects($this->exactly(1)) ->method('groupsMatchFilter') ->will($this->returnArgument(0)); @@ -659,14 +670,15 @@ class Group_LDAPTest extends TestCase { $access->expects($this->once()) ->method('username2dn') ->will($this->returnValue($dn)); - $access->expects($this->never()) ->method('readAttribute') ->with($dn, 'memberOf'); - $access->expects($this->once()) ->method('nextcloudGroupNames') ->will($this->returnValue([])); + $access->expects($this->any()) + ->method('groupsMatchFilter') + ->willReturnArgument(0); $groupBackend = new GroupLDAP($access, $pluginManager); $groupBackend->getUserGroups('userX'); @@ -680,12 +692,15 @@ class Group_LDAPTest extends TestCase { $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; + switch($name) { + case 'useMemberOfToDetectMembership': + return 0; + case 'ldapDynamicGroupMemberURL': + return ''; + case 'ldapNestedGroups': + return false; + case 'ldapGroupMemberAssocAttr': + return 'member'; } return 1; })); @@ -716,10 +731,12 @@ class Group_LDAPTest extends TestCase { ->method('nextcloudGroupNames') ->with([$group1, $group2]) ->will($this->returnValue(['group1', 'group2'])); - $access->expects($this->once()) ->method('fetchListOfGroups') ->will($this->returnValue([$group1, $group2])); + $access->expects($this->any()) + ->method('groupsMatchFilter') + ->willReturnArgument(0); $groupBackend = new GroupLDAP($access, $pluginManager); $groups = $groupBackend->getUserGroups('userX'); @@ -999,14 +1016,6 @@ class Group_LDAPTest extends TestCase { $groups1, ['cn=Birds,' . $base => $groups1] ], - [ #2 – test uids with nested groups - 'cn=Birds,' . $base, - $expGroups2, - [ - 'cn=Birds,' . $base => $groups1, - '8427' => $groups2Nested, // simplified - nested groups would work with DNs - ], - ], ]; } @@ -1045,9 +1054,7 @@ class Group_LDAPTest extends TestCase { $ldap = new GroupLDAP($access, $pluginManager); $resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]); - $expected = array_keys(array_flip($expectedMembers)); - - $this->assertEquals($expected, array_keys($resultingMembers), '', 0.0, 10, true); + $this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true); } } diff --git a/build/integration/features/bootstrap/LDAPContext.php b/build/integration/features/bootstrap/LDAPContext.php index ee7acab6f5..2ad737bf8b 100644 --- a/build/integration/features/bootstrap/LDAPContext.php +++ b/build/integration/features/bootstrap/LDAPContext.php @@ -27,6 +27,7 @@ use PHPUnit\Framework\Assert; class LDAPContext implements Context { use BasicStructure; + use CommandLine; protected $configID; @@ -37,6 +38,8 @@ class LDAPContext implements Context { if($this->configID === null) { return; } + $this->disableLDAPConfiguration(); # via occ in case of big config issues + $this->asAn('admin'); $this->sendingTo('DELETE', $this->apiUrl . '/' . $this->configID); } @@ -196,4 +199,9 @@ class LDAPContext implements Context { $backend = (string)simplexml_load_string($this->response->getBody())->data[0]->backend; Assert::assertEquals('LDAP', $backend); } + + public function disableLDAPConfiguration() { + $configKey = $this->configID . 'ldap_configuration_active'; + $this->invokingTheCommand('config:app:set user_ldap ' . $configKey . ' --value="0"'); + } } diff --git a/build/integration/ldap_features/ldap-openldap.feature b/build/integration/ldap_features/ldap-openldap.feature index 4b0b02c5b4..6c5ed8b462 100644 --- a/build/integration/ldap_features/ldap-openldap.feature +++ b/build/integration/ldap_features/ldap-openldap.feature @@ -102,3 +102,67 @@ Feature: LDAP | ldapHost | foo.bar | | ldapPort | 2456 | Then Expect ServerException on failed web login as "alice" + + Scenario: Test LDAP group membership with intermediate groups not matching filter + Given modify LDAP configuration + | ldapBaseGroups | ou=OtherGroups,dc=nextcloud,dc=ci | + | ldapGroupFilter | (&(cn=Gardeners)(objectclass=groupOfNames)) | + | ldapNestedGroups | 1 | + | useMemberOfToDetectMembership | 1 | + | ldapUserFilter | (&(objectclass=inetorgperson)(!(uid=alice))) | + | ldapExpertUsernameAttr | uid | + | ldapGroupMemberAssocAttr | member | + And As an "admin" + # for population + And sending "GET" to "/cloud/groups" + And sending "GET" to "/cloud/groups/Gardeners/users" + Then the OCS status code should be "200" + And the "users" result should match + | alice | 0 | + | clara | 1 | + | elisa | 1 | + | gustaf | 1 | + | jesper | 1 | + + Scenario: Test LDAP group membership with intermediate groups not matching filter and without memberof + Given modify LDAP configuration + | ldapBaseGroups | ou=OtherGroups,dc=nextcloud,dc=ci | + | ldapGroupFilter | (&(cn=Gardeners)(objectclass=groupOfNames)) | + | ldapNestedGroups | 1 | + | useMemberOfToDetectMembership | 0 | + | ldapUserFilter | (&(objectclass=inetorgperson)(!(uid=alice))) | + | ldapExpertUsernameAttr | uid | + | ldapGroupMemberAssocAttr | member | + And As an "admin" + # for population + And sending "GET" to "/cloud/groups" + And sending "GET" to "/cloud/groups/Gardeners/users" + Then the OCS status code should be "200" + And the "users" result should match + | alice | 0 | + | clara | 1 | + | elisa | 1 | + | gustaf | 1 | + | jesper | 1 | + + Scenario: Test LDAP group membership with intermediate groups not matching filter, numeric group ids + Given modify LDAP configuration + | ldapBaseGroups | ou=NumericGroups,dc=nextcloud,dc=ci | + | ldapGroupFilter | (&(cn=2000)(objectclass=groupOfNames)) | + | ldapNestedGroups | 1 | + | useMemberOfToDetectMembership | 1 | + | ldapUserFilter | (&(objectclass=inetorgperson)(!(uid=alice))) | + | ldapExpertUsernameAttr | uid | + | ldapGroupMemberAssocAttr | member | + And As an "admin" + # for population + And sending "GET" to "/cloud/groups" + And sending "GET" to "/cloud/groups/2000/users" + Then the OCS status code should be "200" + And the "users" result should match + | alice | 0 | + | clara | 1 | + | elisa | 1 | + | gustaf | 1 | + | jesper | 1 | + diff --git a/build/integration/ldap_features/openldap-numerical-id.feature b/build/integration/ldap_features/openldap-numerical-id.feature index 2d87ba33e6..4112df0ae1 100644 --- a/build/integration/ldap_features/openldap-numerical-id.feature +++ b/build/integration/ldap_features/openldap-numerical-id.feature @@ -29,3 +29,39 @@ Scenario: Test by logging in And Logging in using web as "92379" And Sending a "GET" to "/remote.php/webdav/welcome.txt" with requesttoken Then the HTTP status code should be "200" + +Scenario: Test LDAP group retrieval with numeric group ids and nesting + # Nesting does not play a role here really + Given modify LDAP configuration + | ldapBaseGroups | ou=NumericGroups,dc=nextcloud,dc=ci | + | ldapGroupFilter | (objectclass=groupOfNames) | + | ldapNestedGroups | 1 | + | useMemberOfToDetectMembership | 1 | + And As an "admin" + And sending "GET" to "/cloud/groups" + Then the OCS status code should be "200" + And the "groups" result should match + | 2000 | 1 | + | 3000 | 1 | + | 3001 | 1 | + | 3002 | 1 | + +Scenario: Test LDAP group membership with intermediate groups not matching filter, numeric group ids + Given modify LDAP configuration + | ldapBaseGroups | ou=NumericGroups,dc=nextcloud,dc=ci | + | ldapGroupFilter | (&(cn=2000)(objectclass=groupOfNames)) | + | ldapNestedGroups | 1 | + | useMemberOfToDetectMembership | 1 | + | ldapUserFilter | (&(objectclass=inetorgperson)(!(uid=alice))) | + | ldapGroupMemberAssocAttr | member | + And As an "admin" + # for population + And sending "GET" to "/cloud/groups" + And sending "GET" to "/cloud/groups/2000/users" + Then the OCS status code should be "200" + And the "users" result should match + | 92379 | 0 | + | 54172 | 1 | + | 50194 | 1 | + | 59376 | 1 | + | 59463 | 1 |