Skip to content

Commit e957b0a

Browse files
MorrisJobkeMorrisJobke
authored
Merge pull request #14591 from nextcloud/backport/14464/stable15
[stable15] resolve user and groups in nested groups first before filtering the results
2 parents 1b53d46 + a358c4d commit e957b0a

File tree

7 files changed

+230
-80
lines changed

7 files changed

+230
-80
lines changed

‎.drone.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1170,7 +1170,7 @@ services:
11701170
matrix:
11711171
TESTS: acceptance
11721172
openldap:
1173-
image: nextcloudci/openldap:openldap-6
1173+
image: nextcloudci/openldap:openldap-7
11741174
environment:
11751175
- SLAPD_DOMAIN=nextcloud.ci
11761176
- SLAPD_ORGANIZATION=Nextcloud

‎apps/user_ldap/lib/Connection.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,9 @@
6060
* @property string ldapQuotaAttribute
6161
* @property string ldapQuotaDefault
6262
* @property string ldapEmailAttribute
63+
* @property bool|string ldapNestedGroups
64+
* @property string[] ldapBaseGroups
65+
* @property string ldapGroupFilter
6366
*/
6467
class Connection extends LDAPUtility {
6568
private $ldapConnectionRes = null;

‎apps/user_ldap/lib/Group_LDAP.php‎

Lines changed: 86 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,11 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLD
5858
*/
5959
protected $cachedGroupsByMember;
6060

61+
/**
62+
* @var string[] $cachedNestedGroups array of groups with gid (DN) as key
63+
*/
64+
protected $cachedNestedGroups;
65+
6166
/** @var GroupPluginManager */
6267
protected $groupPluginManager;
6368

@@ -71,6 +76,7 @@ public function __construct(Access $access, GroupPluginManager $groupPluginManag
7176

7277
$this->cachedGroupMembers = new CappedMemoryCache();
7378
$this->cachedGroupsByMember = new CappedMemoryCache();
79+
$this->cachedNestedGroups = new CappedMemoryCache();
7480
$this->groupPluginManager = $groupPluginManager;
7581
}
7682

@@ -212,12 +218,12 @@ public function getDynamicGroupMembers($dnGroup) {
212218
*/
213219
private function _groupMembers($dnGroup, &$seen = null) {
214220
if ($seen === null) {
215-
$seen = array();
221+
$seen = [];
216222
}
217-
$allMembers = array();
223+
$allMembers = [];
218224
if (array_key_exists($dnGroup, $seen)) {
219225
// avoid loops
220-
return array();
226+
return [];
221227
}
222228
// used extensively in cron job, caching makes sense for nested groups
223229
$cacheKey = '_groupMembers'.$dnGroup;
@@ -226,19 +232,12 @@ private function _groupMembers($dnGroup, &$seen = null) {
226232
return $groupMembers;
227233
}
228234
$seen[$dnGroup] = 1;
229-
$members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr,
230-
$this->access->connection->ldapGroupFilter);
235+
$members = $this->access->readAttribute($dnGroup, $this->access->connection->ldapGroupMemberAssocAttr);
231236
if (is_array($members)) {
232-
foreach ($members as $member) {
233-
$allMembers[$member] = 1;
234-
$nestedGroups = $this->access->connection->ldapNestedGroups;
235-
if (!empty($nestedGroups)) {
236-
$subMembers = $this->_groupMembers($member, $seen);
237-
if ($subMembers) {
238-
$allMembers += $subMembers;
239-
}
240-
}
241-
}
237+
$fetcher = function($memberDN, &$seen) {
238+
return $this->_groupMembers($memberDN, $seen);
239+
};
240+
$allMembers = $this->walkNestedGroups($dnGroup, $fetcher, $members);
242241
}
243242

244243
$allMembers += $this->getDynamicGroupMembers($dnGroup);
@@ -251,30 +250,69 @@ private function _groupMembers($dnGroup, &$seen = null) {
251250
* @param string $DN
252251
* @param array|null &$seen
253252
* @return array
253+
* @throws \OC\ServerNotAvailableException
254254
*/
255-
private function _getGroupDNsFromMemberOf($DN, &$seen = null) {
256-
if ($seen === null) {
257-
$seen = array();
258-
}
259-
if (array_key_exists($DN, $seen)) {
260-
// avoid loops
261-
return array();
262-
}
263-
$seen[$DN] = 1;
255+
private function _getGroupDNsFromMemberOf($DN) {
264256
$groups = $this->access->readAttribute($DN, 'memberOf');
265257
if (!is_array($groups)) {
266-
return array();
258+
return [];
259+
}
260+
261+
$fetcher = function($groupDN) {
262+
if (isset($this->cachedNestedGroups[$groupDN])) {
263+
$nestedGroups = $this->cachedNestedGroups[$groupDN];
264+
} else {
265+
$nestedGroups = $this->access->readAttribute($groupDN, 'memberOf');
266+
if (!is_array($nestedGroups)) {
267+
$nestedGroups = [];
268+
}
269+
$this->cachedNestedGroups[$groupDN] = $nestedGroups;
270+
}
271+
return $nestedGroups;
272+
};
273+
274+
$groups = $this->walkNestedGroups($DN, $fetcher, $groups);
275+
return $this->access->groupsMatchFilter($groups);
276+
}
277+
278+
/**
279+
* @param string $dn
280+
* @param \Closure $fetcher args: string $dn, array $seen, returns: string[] of dns
281+
* @param array $list
282+
* @return array
283+
*/
284+
private function walkNestedGroups(string $dn, \Closure $fetcher, array $list): array {
285+
$nesting = (int) $this->access->connection->ldapNestedGroups;
286+
// depending on the input, we either have a list of DNs or a list of LDAP records
287+
// also, the output expects either DNs or records. Testing the first element should suffice.
288+
$recordMode = is_array($list) && isset($list[0]) && is_array($list[0]) && isset($list[0]['dn'][0]);
289+
290+
if ($nesting !== 1) {
291+
if($recordMode) {
292+
// the keys are numeric, but should hold the DN
293+
return array_reduce($list, function ($transformed, $record) use ($dn) {
294+
if($record['dn'][0] != $dn) {
295+
$transformed[$record['dn'][0]] = $record;
296+
}
297+
return $transformed;
298+
}, []);
299+
}
300+
return $list;
267301
}
268-
$groups = $this->access->groupsMatchFilter($groups);
269-
$allGroups = $groups;
270-
$nestedGroups = $this->access->connection->ldapNestedGroups;
271-
if ((int)$nestedGroups === 1) {
272-
foreach ($groups as $group) {
273-
$subGroups = $this->_getGroupDNsFromMemberOf($group, $seen);
274-
$allGroups = array_merge($allGroups, $subGroups);
302+
303+
$seen = [];
304+
while ($record = array_pop($list)) {
305+
$recordDN = $recordMode ? $record['dn'][0] : $record;
306+
if ($recordDN === $dn || array_key_exists($recordDN, $seen)) {
307+
// Prevent loops
308+
continue;
275309
}
310+
$fetched = $fetcher($record, $seen);
311+
$list = array_merge($list, $fetched);
312+
$seen[$recordDN] = $record;
276313
}
277-
return $allGroups;
314+
315+
return $recordMode ? $seen : array_keys($seen);
278316
}
279317

280318
/**
@@ -737,34 +775,28 @@ public function getUserGroups($uid) {
737775
*/
738776
private function getGroupsByMember($dn, &$seen = null) {
739777
if ($seen === null) {
740-
$seen = array();
778+
$seen = [];
741779
}
742-
$allGroups = array();
743780
if (array_key_exists($dn, $seen)) {
744781
// avoid loops
745-
return array();
782+
return [];
746783
}
784+
$allGroups = [];
747785
$seen[$dn] = true;
748-
$filter = $this->access->combineFilterWithAnd(array(
749-
$this->access->connection->ldapGroupFilter,
750-
$this->access->connection->ldapGroupMemberAssocAttr.'='.$dn
751-
));
786+
$filter = $this->access->connection->ldapGroupMemberAssocAttr.'='.$dn;
752787
$groups = $this->access->fetchListOfGroups($filter,
753-
array($this->access->connection->ldapGroupDisplayName, 'dn'));
788+
[$this->access->connection->ldapGroupDisplayName, 'dn']);
754789
if (is_array($groups)) {
755-
foreach ($groups as $groupobj) {
756-
$groupDN = $groupobj['dn'][0];
757-
$allGroups[$groupDN] = $groupobj;
758-
$nestedGroups = $this->access->connection->ldapNestedGroups;
759-
if (!empty($nestedGroups)) {
760-
$supergroups = $this->getGroupsByMember($groupDN, $seen);
761-
if (is_array($supergroups) && (count($supergroups)>0)) {
762-
$allGroups = array_merge($allGroups, $supergroups);
763-
}
790+
$fetcher = function ($dn, &$seen) {
791+
if(is_array($dn) && isset($dn['dn'][0])) {
792+
$dn = $dn['dn'][0];
764793
}
765-
}
794+
return $this->getGroupsByMember($dn, $seen);
795+
};
796+
$allGroups = $this->walkNestedGroups($dn, $fetcher, $groups);
766797
}
767-
return $allGroups;
798+
$visibleGroups = $this->access->groupsMatchFilter(array_keys($allGroups));
799+
return array_intersect_key($allGroups, array_flip($visibleGroups));
768800
}
769801

770802
/**
@@ -811,7 +843,7 @@ public function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
811843

812844
$primaryUsers = $this->getUsersInPrimaryGroup($groupDN, $search, $limit, $offset);
813845
$posixGroupUsers = $this->getUsersInGidNumber($groupDN, $search, $limit, $offset);
814-
$members = array_keys($this->_groupMembers($groupDN));
846+
$members = $this->_groupMembers($groupDN);
815847
if(!$members && empty($posixGroupUsers) && empty($primaryUsers)) {
816848
//in case users could not be retrieved, return empty result set
817849
$this->access->connection->writeToCache($cacheKey, []);
@@ -886,7 +918,7 @@ public function countUsersInGroup($gid, $search = '') {
886918
return false;
887919
}
888920

889-
$members = array_keys($this->_groupMembers($groupDN));
921+
$members = $this->_groupMembers($groupDN);
890922
$primaryUserCount = $this->countUsersInPrimaryGroup($groupDN, '');
891923
if(!$members && $primaryUserCount === 0) {
892924
//in case users could not be retrieved, return empty result set

‎apps/user_ldap/tests/Group_LDAPTest.php‎

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -98,16 +98,27 @@ private function enableGroups($access) {
9898
public function testCountEmptySearchString() {
9999
$access = $this->getAccessMock();
100100
$pluginManager = $this->getPluginManagerMock();
101+
$groupDN = 'cn=group,dc=foo,dc=bar';
101102

102103
$this->enableGroups($access);
103104

104105
$access->expects($this->any())
105106
->method('groupname2dn')
106-
->will($this->returnValue('cn=group,dc=foo,dc=bar'));
107+
->will($this->returnValue($groupDN));
107108

108109
$access->expects($this->any())
109110
->method('readAttribute')
110-
->will($this->returnValue(array('u11', 'u22', 'u33', 'u34')));
111+
->willReturnCallback(function($dn) use ($groupDN) {
112+
if($dn === $groupDN) {
113+
return [
114+
'uid=u11,ou=users,dc=foo,dc=bar',
115+
'uid=u22,ou=users,dc=foo,dc=bar',
116+
'uid=u33,ou=users,dc=foo,dc=bar',
117+
'uid=u34,ou=users,dc=foo,dc=bar'
118+
];
119+
}
120+
return [];
121+
});
111122

112123
// for primary groups
113124
$access->expects($this->once())
@@ -132,7 +143,7 @@ public function testCountWithSearchString() {
132143

133144
$access->expects($this->any())
134145
->method('fetchListOfUsers')
135-
->will($this->returnValue(array()));
146+
->will($this->returnValue([]));
136147

137148
$access->expects($this->any())
138149
->method('readAttribute')
@@ -145,7 +156,7 @@ public function testCountWithSearchString() {
145156
if(strpos($name, 'u') === 0) {
146157
return strpos($name, '3');
147158
}
148-
return array('u11', 'u22', 'u33', 'u34');
159+
return ['u11', 'u22', 'u33', 'u34'];
149160
}));
150161

151162
$access->expects($this->any())
@@ -625,7 +636,7 @@ public function testGetUserGroupsMemberOf() {
625636
->method('dn2groupname')
626637
->will($this->returnArgument(0));
627638

628-
$access->expects($this->exactly(3))
639+
$access->expects($this->exactly(1))
629640
->method('groupsMatchFilter')
630641
->will($this->returnArgument(0));
631642

@@ -659,14 +670,15 @@ public function testGetUserGroupsMemberOfDisabled() {
659670
$access->expects($this->once())
660671
->method('username2dn')
661672
->will($this->returnValue($dn));
662-
663673
$access->expects($this->never())
664674
->method('readAttribute')
665675
->with($dn, 'memberOf');
666-
667676
$access->expects($this->once())
668677
->method('nextcloudGroupNames')
669678
->will($this->returnValue([]));
679+
$access->expects($this->any())
680+
->method('groupsMatchFilter')
681+
->willReturnArgument(0);
670682

671683
$groupBackend = new GroupLDAP($access, $pluginManager);
672684
$groupBackend->getUserGroups('userX');
@@ -680,12 +692,15 @@ public function testGetGroupsByMember() {
680692
$access->connection->expects($this->any())
681693
->method('__get')
682694
->will($this->returnCallback(function($name) {
683-
if($name === 'useMemberOfToDetectMembership') {
684-
return 0;
685-
} else if($name === 'ldapDynamicGroupMemberURL') {
686-
return '';
687-
} else if($name === 'ldapNestedGroups') {
688-
return false;
695+
switch($name) {
696+
case 'useMemberOfToDetectMembership':
697+
return 0;
698+
case 'ldapDynamicGroupMemberURL':
699+
return '';
700+
case 'ldapNestedGroups':
701+
return false;
702+
case 'ldapGroupMemberAssocAttr':
703+
return 'member';
689704
}
690705
return 1;
691706
}));
@@ -716,10 +731,12 @@ public function testGetGroupsByMember() {
716731
->method('nextcloudGroupNames')
717732
->with([$group1, $group2])
718733
->will($this->returnValue(['group1', 'group2']));
719-
720734
$access->expects($this->once())
721735
->method('fetchListOfGroups')
722736
->will($this->returnValue([$group1, $group2]));
737+
$access->expects($this->any())
738+
->method('groupsMatchFilter')
739+
->willReturnArgument(0);
723740

724741
$groupBackend = new GroupLDAP($access, $pluginManager);
725742
$groups = $groupBackend->getUserGroups('userX');
@@ -999,14 +1016,6 @@ public function groupMemberProvider() {
9991016
$groups1,
10001017
['cn=Birds,' . $base => $groups1]
10011018
],
1002-
[ #2 – test uids with nested groups
1003-
'cn=Birds,' . $base,
1004-
$expGroups2,
1005-
[
1006-
'cn=Birds,' . $base => $groups1,
1007-
'8427' => $groups2Nested, // simplified - nested groups would work with DNs
1008-
],
1009-
],
10101019
];
10111020
}
10121021

@@ -1045,9 +1054,7 @@ public function testGroupMembers($groupDN, $expectedMembers, $groupsInfo = null)
10451054
$ldap = new GroupLDAP($access, $pluginManager);
10461055
$resultingMembers = $this->invokePrivate($ldap, '_groupMembers', [$groupDN]);
10471056

1048-
$expected = array_keys(array_flip($expectedMembers));
1049-
1050-
$this->assertEquals($expected, array_keys($resultingMembers), '', 0.0, 10, true);
1057+
$this->assertEquals($expectedMembers, $resultingMembers, '', 0.0, 10, true);
10511058
}
10521059

10531060
}

‎build/integration/features/bootstrap/LDAPContext.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727

2828
class LDAPContext implements Context {
2929
use BasicStructure;
30+
use CommandLine;
3031

3132
protected $configID;
3233

@@ -37,6 +38,8 @@ public function teardown() {
3738
if($this->configID === null) {
3839
return;
3940
}
41+
$this->disableLDAPConfiguration(); # via occ in case of big config issues
42+
$this->asAn('admin');
4043
$this->sendingTo('DELETE', $this->apiUrl . '/' . $this->configID);
4144
}
4245

@@ -196,4 +199,9 @@ public function theRecordFieldsShouldMatch(TableNode $expectations) {
196199
$backend = (string)simplexml_load_string($this->response->getBody())->data[0]->backend;
197200
Assert::assertEquals('LDAP', $backend);
198201
}
202+
203+
public function disableLDAPConfiguration() {
204+
$configKey = $this->configID . 'ldap_configuration_active';
205+
$this->invokingTheCommand('config:app:set user_ldap ' . $configKey . ' --value="0"');
206+
}
199207
}

0 commit comments

Comments
 (0)