Skip to content

Commit

Permalink
Merge pull request #24402 from nextcloud/fix/24252/ldap-ingroup-memberid
Browse files Browse the repository at this point in the history 
LDAP: fix inGroup for memberUid type of group memberships
  • Loading branch information
@blizzz
blizzz committed Dec 15, 2020
2 parents 70a54e1 + af6b0ec commit f68cab4
Show file tree
Hide file tree
Showing 2 changed files with 238 additions and 54 deletions.
33 changes: 22 additions & 11 deletions apps/user_ldap/lib/Group_LDAP.php
View file
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
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

0 comments on commit f68cab4

Please sign in to comment.