Skip to content

Commit f68cab4

Browse files
blizzzblizzz
authored
Merge pull request #24402 from nextcloud/fix/24252/ldap-ingroup-memberid
LDAP: fix inGroup for memberUid type of group memberships
2 parents 70a54e1 + af6b0ec commit f68cab4

File tree

2 files changed

+238
-54
lines changed

2 files changed

+238
-54
lines changed

‎apps/user_ldap/lib/Group_LDAP.php‎

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@
5555
class Group_LDAP extends BackendUtility implements GroupInterface, IGroupLDAP, IGetDisplayNameBackend {
5656
protected $enabled = false;
5757

58-
/** @var string[] $cachedGroupMembers array of users with gid as key */
58+
/** @var string[][] $cachedGroupMembers array of users with gid as key */
5959
protected $cachedGroupMembers;
6060
/** @var string[] $cachedGroupsByMember array of groups with uid as key */
6161
protected $cachedGroupsByMember;
@@ -136,17 +136,13 @@ public function inGroup($uid, $gid) {
136136

137137
//usually, LDAP attributes are said to be case insensitive. But there are exceptions of course.
138138
$members = $this->_groupMembers($groupDN);
139-
if (!is_array($members) || count($members) === 0) {
140-
$this->access->connection->writeToCache($cacheKey, false);
141-
return false;
142-
}
143139

144140
//extra work if we don't get back user DNs
145141
switch ($this->ldapGroupMemberAssocAttr) {
146142
case 'memberuid':
147143
case 'zimbramailforwardingaddress':
148144
$requestAttributes = $this->access->userManager->getAttributes(true);
149-
$dns = [];
145+
$users = [];
150146
$filterParts = [];
151147
$bytes = 0;
152148
foreach ($members as $mid) {
@@ -160,22 +156,37 @@ public function inGroup($uid, $gid) {
160156
if ($bytes >= 9000000) {
161157
// AD has a default input buffer of 10 MB, we do not want
162158
// to take even the chance to exceed it
159+
// so we fetch results with the filterParts we collected so far
163160
$filter = $this->access->combineFilterWithOr($filterParts);
164-
$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
161+
$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
165162
$bytes = 0;
166163
$filterParts = [];
167-
$dns = array_merge($dns, $users);
164+
$users = array_merge($users, $search);
168165
}
169166
}
167+
170168
if (count($filterParts) > 0) {
169+
// if there are filterParts left we need to add their result
171170
$filter = $this->access->combineFilterWithOr($filterParts);
172-
$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
173-
$dns = array_merge($dns, $users);
171+
$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
172+
$users = array_merge($users, $search);
174173
}
175-
$members = $dns;
174+
175+
// now we cleanup the users array to get only dns
176+
$dns = [];
177+
foreach ($users as $record) {
178+
$dns[$record['dn'][0]] = 1;
179+
}
180+
$members = array_keys($dns);
181+
176182
break;
177183
}
178184

185+
if (count($members) === 0) {
186+
$this->access->connection->writeToCache($cacheKey, false);
187+
return false;
188+
}
189+
179190
$isInGroup = in_array($userDN, $members);
180191
$this->access->connection->writeToCache($cacheKey, $isInGroup);
181192
$this->access->connection->writeToCache($cacheKeyMembers, $members);

‎apps/user_ldap/tests/Group_LDAPTest.php‎

Lines changed: 216 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,47 @@
5050
* @package OCA\User_LDAP\Tests
5151
*/
5252
class Group_LDAPTest extends TestCase {
53+
public function testCountEmptySearchString() {
54+
$access = $this->getAccessMock();
55+
$pluginManager = $this->getPluginManagerMock();
56+
$groupDN = 'cn=group,dc=foo,dc=bar';
57+
58+
$this->enableGroups($access);
59+
60+
$access->expects($this->any())
61+
->method('groupname2dn')
62+
->willReturn($groupDN);
63+
$access->expects($this->any())
64+
->method('readAttribute')
65+
->willReturnCallback(function ($dn) use ($groupDN) {
66+
if ($dn === $groupDN) {
67+
return [
68+
'uid=u11,ou=users,dc=foo,dc=bar',
69+
'uid=u22,ou=users,dc=foo,dc=bar',
70+
'uid=u33,ou=users,dc=foo,dc=bar',
71+
'uid=u34,ou=users,dc=foo,dc=bar'
72+
];
73+
}
74+
return [];
75+
});
76+
$access->expects($this->any())
77+
->method('isDNPartOfBase')
78+
->willReturn(true);
79+
// for primary groups
80+
$access->expects($this->once())
81+
->method('countUsers')
82+
->willReturn(2);
83+
84+
$access->userManager->expects($this->any())
85+
->method('getAttributes')
86+
->willReturn(['displayName', 'mail']);
87+
88+
$groupBackend = new GroupLDAP($access, $pluginManager);
89+
$users = $groupBackend->countUsersInGroup('group');
90+
91+
$this->assertSame(6, $users);
92+
}
93+
5394
/**
5495
* @return MockObject|Access
5596
*/
@@ -98,47 +139,6 @@ private function enableGroups(Access $access) {
98139
});
99140
}
100141

101-
public function testCountEmptySearchString() {
102-
$access = $this->getAccessMock();
103-
$pluginManager = $this->getPluginManagerMock();
104-
$groupDN = 'cn=group,dc=foo,dc=bar';
105-
106-
$this->enableGroups($access);
107-
108-
$access->expects($this->any())
109-
->method('groupname2dn')
110-
->willReturn($groupDN);
111-
$access->expects($this->any())
112-
->method('readAttribute')
113-
->willReturnCallback(function ($dn) use ($groupDN) {
114-
if ($dn === $groupDN) {
115-
return [
116-
'uid=u11,ou=users,dc=foo,dc=bar',
117-
'uid=u22,ou=users,dc=foo,dc=bar',
118-
'uid=u33,ou=users,dc=foo,dc=bar',
119-
'uid=u34,ou=users,dc=foo,dc=bar'
120-
];
121-
}
122-
return [];
123-
});
124-
$access->expects($this->any())
125-
->method('isDNPartOfBase')
126-
->willReturn(true);
127-
// for primary groups
128-
$access->expects($this->once())
129-
->method('countUsers')
130-
->willReturn(2);
131-
132-
$access->userManager->expects($this->any())
133-
->method('getAttributes')
134-
->willReturn(['displayName', 'mail']);
135-
136-
$groupBackend = new GroupLDAP($access, $pluginManager);
137-
$users = $groupBackend->countUsersInGroup('group');
138-
139-
$this->assertSame(6, $users);
140-
}
141-
142142
public function testCountWithSearchString() {
143143
$access = $this->getAccessMock();
144144
$pluginManager = $this->getPluginManagerMock();
@@ -503,6 +503,179 @@ public function testInGroupHitsUidGidCache() {
503503
$groupBackend->inGroup($uid, $gid);
504504
}
505505

506+
public function groupWithMembersProvider() {
507+
return [
508+
[
509+
'someGroup',
510+
'cn=someGroup,ou=allTheGroups,ou=someDepartment,dc=someDomain,dc=someTld',
511+
[
512+
'uid=oneUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
513+
'uid=someUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
514+
'uid=anotherUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
515+
'uid=differentUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
516+
],
517+
],
518+
];
519+
}
520+
521+
/**
522+
* @dataProvider groupWithMembersProvider
523+
*/
524+
public function testInGroupMember(string $gid, string $groupDn, array $memberDNs) {
525+
$access = $this->getAccessMock();
526+
$pluginManager = $this->getPluginManagerMock();
527+
528+
$access->connection = $this->createMock(Connection::class);
529+
530+
$uid = 'someUser';
531+
$userDn = $memberDNs[0];
532+
533+
$access->connection->expects($this->any())
534+
->method('__get')
535+
->willReturnCallback(function ($name) {
536+
switch ($name) {
537+
case 'ldapGroupMemberAssocAttr':
538+
return 'member';
539+
case 'ldapDynamicGroupMemberURL':
540+
return '';
541+
case 'hasPrimaryGroups':
542+
case 'ldapNestedGroups':
543+
return 0;
544+
default:
545+
return 1;
546+
}
547+
});
548+
$access->connection->expects($this->any())
549+
->method('getFromCache')
550+
->willReturn(null);
551+
552+
$access->expects($this->once())
553+
->method('username2dn')
554+
->with($uid)
555+
->willReturn($userDn);
556+
$access->expects($this->once())
557+
->method('groupname2dn')
558+
->willReturn($groupDn);
559+
$access->expects($this->any())
560+
->method('readAttribute')
561+
->willReturn($memberDNs);
562+
563+
$groupBackend = new GroupLDAP($access, $pluginManager);
564+
$this->assertTrue($groupBackend->inGroup($uid, $gid));
565+
}
566+
567+
/**
568+
* @dataProvider groupWithMembersProvider
569+
*/
570+
public function testInGroupMemberNot(string $gid, string $groupDn, array $memberDNs) {
571+
$access = $this->getAccessMock();
572+
$pluginManager = $this->getPluginManagerMock();
573+
574+
$access->connection = $this->createMock(Connection::class);
575+
576+
$uid = 'unelatedUser';
577+
$userDn = 'uid=unrelatedUser,ou=unrelatedTeam,ou=unrelatedDepartment,dc=someDomain,dc=someTld';
578+
579+
$access->connection->expects($this->any())
580+
->method('__get')
581+
->willReturnCallback(function ($name) {
582+
switch ($name) {
583+
case 'ldapGroupMemberAssocAttr':
584+
return 'member';
585+
case 'ldapDynamicGroupMemberURL':
586+
return '';
587+
case 'hasPrimaryGroups':
588+
case 'ldapNestedGroups':
589+
return 0;
590+
default:
591+
return 1;
592+
}
593+
});
594+
$access->connection->expects($this->any())
595+
->method('getFromCache')
596+
->willReturn(null);
597+
598+
$access->expects($this->once())
599+
->method('username2dn')
600+
->with($uid)
601+
->willReturn($userDn);
602+
$access->expects($this->once())
603+
->method('groupname2dn')
604+
->willReturn($groupDn);
605+
$access->expects($this->any())
606+
->method('readAttribute')
607+
->willReturn($memberDNs);
608+
609+
$groupBackend = new GroupLDAP($access, $pluginManager);
610+
$this->assertFalse($groupBackend->inGroup($uid, $gid));
611+
}
612+
613+
/**
614+
* @dataProvider groupWithMembersProvider
615+
*/
616+
public function testInGroupMemberUid(string $gid, string $groupDn, array $memberDNs) {
617+
$access = $this->getAccessMock();
618+
$pluginManager = $this->getPluginManagerMock();
619+
620+
$memberUids = [];
621+
$userRecords = [];
622+
foreach ($memberDNs as $dn) {
623+
$memberUids[] = ldap_explode_dn($dn, false)[0];
624+
$userRecords[] = ['dn' => [$dn]];
625+
}
626+
627+
628+
$access->connection = $this->createMock(Connection::class);
629+
630+
$uid = 'someUser';
631+
$userDn = $memberDNs[0];
632+
633+
$access->connection->expects($this->any())
634+
->method('__get')
635+
->willReturnCallback(function ($name) {
636+
switch ($name) {
637+
case 'ldapGroupMemberAssocAttr':
638+
return 'memberUid';
639+
case 'ldapDynamicGroupMemberURL':
640+
return '';
641+
case 'ldapLoginFilter':
642+
return 'uid=%uid';
643+
case 'hasPrimaryGroups':
644+
case 'ldapNestedGroups':
645+
return 0;
646+
default:
647+
return 1;
648+
}
649+
});
650+
$access->connection->expects($this->any())
651+
->method('getFromCache')
652+
->willReturn(null);
653+
654+
$access->userManager->expects($this->any())
655+
->method('getAttributes')
656+
->willReturn(['uid', 'mail', 'displayname']);
657+
658+
$access->expects($this->once())
659+
->method('username2dn')
660+
->with($uid)
661+
->willReturn($userDn);
662+
$access->expects($this->once())
663+
->method('groupname2dn')
664+
->willReturn($groupDn);
665+
$access->expects($this->any())
666+
->method('readAttribute')
667+
->willReturn($memberUids);
668+
$access->expects($this->any())
669+
->method('fetchListOfUsers')
670+
->willReturn($userRecords);
671+
$access->expects($this->any())
672+
->method('combineFilterWithOr')
673+
->willReturn('(|(pseudo=filter)(filter=pseudo))');
674+
675+
$groupBackend = new GroupLDAP($access, $pluginManager);
676+
$this->assertTrue($groupBackend->inGroup($uid, $gid));
677+
}
678+
506679
public function testGetGroupsWithOffset() {
507680
$access = $this->getAccessMock();
508681
$pluginManager = $this->getPluginManagerMock();
@@ -721,8 +894,8 @@ public function testGetUserGroupsMemberOfDisabled() {
721894

722895
public function nestedGroupsProvider(): array {
723896
return [
724-
[ true ],
725-
[ false ],
897+
[true],
898+
[false],
726899
];
727900
}
728901

0 commit comments

Comments
 (0)