Skip to content

[stable19] LDAP: fix inGroup for memberUid type of group memberships #24739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tofuSCHNITZEL wants to merge 6 commits into from
Closed

[stable19] LDAP: fix inGroup for memberUid type of group memberships #24739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed65fd1
flatten result array as expected by following code
blizzz Nov 26, 2020
c399493
moved the array_reduce to fix large search case
tofuSCHNITZEL Nov 27, 2020
bada6d1
use faster and less hungry foreach
blizzz Nov 27, 2020
e2fea62
check number of members after potential resolving of rdns
blizzz Nov 27, 2020
c9cb14e
add unit tests
blizzz Nov 27, 2020
957f634
php-cs happyness
blizzz Nov 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions apps/user_ldap/lib/Group_LDAP.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@
class Group_LDAP extends BackendUtility implements \OCP\GroupInterface, IGroupLDAP, IGetDisplayNameBackend {
protected $enabled = false;

/**
* @var string[] $cachedGroupMembers array of users with gid as key
*/
/** @var string[][] $cachedGroupMembers array of users with gid as key */
protected $cachedGroupMembers;

/**
Expand Down Expand Up @@ -133,15 +131,11 @@ public function inGroup($uid, $gid) {

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

//extra work if we don't get back user DNs
if (strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'memberuid') {
$requestAttributes = $this->access->userManager->getAttributes(true);
$dns = [];
$users = [];
$filterParts = [];
$bytes = 0;
foreach ($members as $mid) {
Expand All @@ -151,19 +145,34 @@ public function inGroup($uid, $gid) {
if ($bytes >= 9000000) {
// AD has a default input buffer of 10 MB, we do not want
// to take even the chance to exceed it
// so we fetch results with the filterParts we collected so far
$filter = $this->access->combineFilterWithOr($filterParts);
$bytes = 0;
$filterParts = [];
$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
$dns = array_merge($dns, $users);
$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
$users = array_merge($users, $search);
}
}

if (count($filterParts) > 0) {
// if there are filterParts left we need to add their result
$filter = $this->access->combineFilterWithOr($filterParts);
$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
$dns = array_merge($dns, $users);
$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
$users = array_merge($users, $search);
}
$members = $dns;

// now we cleanup the users array to get only dns
$dns = [];
foreach ($users as $record) {
$dns[$record['dn'][0]] = 1;
}

$members = array_keys($dns);
}

if (count($members) === 0) {
$this->access->connection->writeToCache($cacheKey, false);
return false;
}

$isInGroup = in_array($userDN, $members);
Expand Down
264 changes: 216 additions & 48 deletions apps/user_ldap/tests/Group_LDAPTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,47 @@
* @package OCA\User_LDAP\Tests
*/
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')
->willReturn($groupDN);
$access->expects($this->any())
->method('readAttribute')
->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 [];
});
$access->expects($this->any())
->method('isDNPartOfBase')
->willReturn(true);
// for primary groups
$access->expects($this->once())
->method('countUsers')
->willReturn(2);

$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);

$groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->countUsersInGroup('group');

$this->assertSame(6, $users);
}

/**
* @return \PHPUnit_Framework_MockObject_MockObject|Access
*/
Expand Down Expand Up @@ -96,52 +137,6 @@ private function enableGroups($access) {
});
}

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')
->willReturn($groupDN);

$access->expects($this->any())
->method('readAttribute')
->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 [];
});
$access->expects($this->any())
->method('isDNPartOfBase')
->willReturn(true);
$access->expects($this->any())
->method('combineFilterWithAnd')
->willReturn('pseudo=filter');

// for primary groups
$access->expects($this->once())
->method('countUsers')
->willReturn(2);

$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['displayName', 'mail']);

$groupBackend = new GroupLDAP($access, $pluginManager);
$users = $groupBackend->countUsersInGroup('group');

$this->assertSame(6, $users);
}

public function testCountWithSearchString() {
$access = $this->getAccessMock();
$pluginManager = $this->getPluginManagerMock();
Expand Down Expand Up @@ -506,6 +501,179 @@ public function testInGroupHitsUidGidCache() {
$groupBackend->inGroup($uid, $gid);
}

public function groupWithMembersProvider() {
return [
[
'someGroup',
'cn=someGroup,ou=allTheGroups,ou=someDepartment,dc=someDomain,dc=someTld',
[
'uid=oneUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
'uid=someUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
'uid=anotherUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
'uid=differentUser,ou=someTeam,ou=someDepartment,dc=someDomain,dc=someTld',
],
],
];
}

/**
* @dataProvider groupWithMembersProvider
*/
public function testInGroupMember(string $gid, string $groupDn, array $memberDNs) {
$access = $this->getAccessMock();
$pluginManager = $this->getPluginManagerMock();

$access->connection = $this->createMock(Connection::class);

$uid = 'someUser';
$userDn = $memberDNs[0];

$access->connection->expects($this->any())
->method('__get')
->willReturnCallback(function ($name) {
switch ($name) {
case 'ldapGroupMemberAssocAttr':
return 'member';
case 'ldapDynamicGroupMemberURL':
return '';
case 'hasPrimaryGroups':
case 'ldapNestedGroups':
return 0;
default:
return 1;
}
});
$access->connection->expects($this->any())
->method('getFromCache')
->willReturn(null);

$access->expects($this->once())
->method('username2dn')
->with($uid)
->willReturn($userDn);
$access->expects($this->once())
->method('groupname2dn')
->willReturn($groupDn);
$access->expects($this->any())
->method('readAttribute')
->willReturn($memberDNs);

$groupBackend = new GroupLDAP($access, $pluginManager);
$this->assertTrue($groupBackend->inGroup($uid, $gid));
}

/**
* @dataProvider groupWithMembersProvider
*/
public function testInGroupMemberNot(string $gid, string $groupDn, array $memberDNs) {
$access = $this->getAccessMock();
$pluginManager = $this->getPluginManagerMock();

$access->connection = $this->createMock(Connection::class);

$uid = 'unelatedUser';
$userDn = 'uid=unrelatedUser,ou=unrelatedTeam,ou=unrelatedDepartment,dc=someDomain,dc=someTld';

$access->connection->expects($this->any())
->method('__get')
->willReturnCallback(function ($name) {
switch ($name) {
case 'ldapGroupMemberAssocAttr':
return 'member';
case 'ldapDynamicGroupMemberURL':
return '';
case 'hasPrimaryGroups':
case 'ldapNestedGroups':
return 0;
default:
return 1;
}
});
$access->connection->expects($this->any())
->method('getFromCache')
->willReturn(null);

$access->expects($this->once())
->method('username2dn')
->with($uid)
->willReturn($userDn);
$access->expects($this->once())
->method('groupname2dn')
->willReturn($groupDn);
$access->expects($this->any())
->method('readAttribute')
->willReturn($memberDNs);

$groupBackend = new GroupLDAP($access, $pluginManager);
$this->assertFalse($groupBackend->inGroup($uid, $gid));
}

/**
* @dataProvider groupWithMembersProvider
*/
public function testInGroupMemberUid(string $gid, string $groupDn, array $memberDNs) {
$access = $this->getAccessMock();
$pluginManager = $this->getPluginManagerMock();

$memberUids = [];
$userRecords = [];
foreach ($memberDNs as $dn) {
$memberUids[] = ldap_explode_dn($dn, false)[0];
$userRecords[] = ['dn' => [$dn]];
}


$access->connection = $this->createMock(Connection::class);

$uid = 'someUser';
$userDn = $memberDNs[0];

$access->connection->expects($this->any())
->method('__get')
->willReturnCallback(function ($name) {
switch ($name) {
case 'ldapGroupMemberAssocAttr':
return 'memberUid';
case 'ldapDynamicGroupMemberURL':
return '';
case 'ldapLoginFilter':
return 'uid=%uid';
case 'hasPrimaryGroups':
case 'ldapNestedGroups':
return 0;
default:
return 1;
}
});
$access->connection->expects($this->any())
->method('getFromCache')
->willReturn(null);

$access->userManager->expects($this->any())
->method('getAttributes')
->willReturn(['uid', 'mail', 'displayname']);

$access->expects($this->once())
->method('username2dn')
->with($uid)
->willReturn($userDn);
$access->expects($this->once())
->method('groupname2dn')
->willReturn($groupDn);
$access->expects($this->any())
->method('readAttribute')
->willReturn($memberUids);
$access->expects($this->any())
->method('fetchListOfUsers')
->willReturn($userRecords);
$access->expects($this->any())
->method('combineFilterWithOr')
->willReturn('(|(pseudo=filter)(filter=pseudo))');

$groupBackend = new GroupLDAP($access, $pluginManager);
$this->assertTrue($groupBackend->inGroup($uid, $gid));
}

public function testGetGroupsWithOffset() {
$access = $this->getAccessMock();
$pluginManager = $this->getPluginManagerMock();
Expand Down Expand Up @@ -729,8 +897,8 @@ public function testGetUserGroupsMemberOfDisabled() {

public function nestedGroupsProvider(): array {
return [
[ true ],
[ false ],
[true],
[false],
];
}

Expand Down