Skip to content
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

Jump to bottom

[stable20] LDAP: fix inGroup for memberUid type of group memberships #24716

Merged
merged 6 commits into from
Jan 7, 2021
Merged

[stable20] LDAP: fix inGroup for memberUid type of group memberships #24716

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
d818975
flatten result array as expected by following code
blizzz Nov 26, 2020
250c753
moved the array_reduce to fix large search case
tofuSCHNITZEL Nov 27, 2020
213464a
use faster and less hungry foreach
blizzz Nov 27, 2020
cca1fb7
check number of members after potential resolving of rdns
blizzz Nov 27, 2020
43c6bc1
add unit tests
blizzz Nov 27, 2020
270eb6c
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
33 changes: 22 additions & 11 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 @@ -55,7 +55,7 @@
class Group_LDAP extends BackendUtility implements 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;
/** @var string[] $cachedGroupsByMember array of groups with uid as key */
protected $cachedGroupsByMember;
Expand Down Expand Up @@ -136,17 +136,13 @@ 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
switch ($this->ldapGroupMemberAssocAttr) {
case 'memberuid':
case 'zimbramailforwardingaddress':
$requestAttributes = $this->access->userManager->getAttributes(true);
$dns = [];
$users = [];
$filterParts = [];
$bytes = 0;
foreach ($members as $mid) {
Expand All @@ -160,22 +156,37 @@ 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);
$users = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
$search = $this->access->fetchListOfUsers($filter, $requestAttributes, count($filterParts));
$bytes = 0;
$filterParts = [];
$dns = array_merge($dns, $users);
$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);

break;
}

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

$isInGroup = in_array($userDN, $members);
$this->access->connection->writeToCache($cacheKey, $isInGroup);
$this->access->connection->writeToCache($cacheKeyMembers, $members);
Expand Down
259 changes: 216 additions & 43 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 @@ -50,6 +50,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 MockObject|Access
*/
Expand Down Expand Up @@ -98,47 +139,6 @@ private function enableGroups(Access $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);
// 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 @@ -503,6 +503,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 @@ -721,8 +894,8 @@ public function testGetUserGroupsMemberOfDisabled() {

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

Expand Down