Merge pull request #11494 from owncloud/fix-ldap-ingroup-for-9225-2
fix retrieval of group members and cache group members
This commit is contained in:
commit
2b6281fc9d
|
@ -29,6 +29,16 @@ use OCA\user_ldap\lib\BackendUtility;
|
|||
class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
|
||||
protected $enabled = false;
|
||||
|
||||
/**
|
||||
* @var string[] $cachedGroupMembers array of users with gid as key
|
||||
*/
|
||||
protected $cachedGroupMembers = array();
|
||||
|
||||
/**
|
||||
* @var string[] $cachedGroupsByMember array of groups with uid as key
|
||||
*/
|
||||
protected $cachedGroupsByMember = array();
|
||||
|
||||
public function __construct(Access $access) {
|
||||
parent::__construct($access);
|
||||
$filter = $this->access->connection->ldapGroupFilter;
|
||||
|
@ -56,6 +66,21 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
|
|||
}
|
||||
|
||||
$userDN = $this->access->username2dn($uid);
|
||||
|
||||
if(isset($this->cachedGroupMembers[$gid])) {
|
||||
$isInGroup = in_array($userDN, $this->cachedGroupMembers[$gid]);
|
||||
return $isInGroup;
|
||||
}
|
||||
|
||||
$cacheKeyMembers = 'inGroup-members:'.$gid;
|
||||
if($this->access->connection->isCached($cacheKeyMembers)) {
|
||||
$members = $this->access->connection->getFromCache($cacheKeyMembers);
|
||||
$this->cachedGroupMembers[$gid] = $members;
|
||||
$isInGroup = in_array($userDN, $members);
|
||||
$this->access->connection->writeToCache($cacheKey, $isInGroup);
|
||||
return $isInGroup;
|
||||
}
|
||||
|
||||
$groupDN = $this->access->groupname2dn($gid);
|
||||
// just in case
|
||||
if(!$groupDN || !$userDN) {
|
||||
|
@ -70,29 +95,44 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
|
|||
}
|
||||
|
||||
//usually, LDAP attributes are said to be case insensitive. But there are exceptions of course.
|
||||
$members = array_keys($this->_groupMembers($groupDN));
|
||||
if(!$members) {
|
||||
$members = $this->_groupMembers($groupDN);
|
||||
$members = array_keys($members); // uids are returned as keys
|
||||
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
|
||||
//TODO: this can be done with one LDAP query
|
||||
if(strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'memberuid') {
|
||||
$dns = array();
|
||||
$filterParts = array();
|
||||
$bytes = 0;
|
||||
foreach($members as $mid) {
|
||||
$filter = str_replace('%uid', $mid, $this->access->connection->ldapLoginFilter);
|
||||
$ldap_users = $this->access->fetchListOfUsers($filter, 'dn');
|
||||
if(count($ldap_users) < 1) {
|
||||
continue;
|
||||
$filterParts[] = $filter;
|
||||
$bytes += strlen($filter);
|
||||
if($bytes >= 9000000) {
|
||||
// AD has a default input buffer of 10 MB, we do not want
|
||||
// to take even the chance to exceed it
|
||||
$filter = $this->access->combineFilterWithOr($filterParts);
|
||||
$bytes = 0;
|
||||
$filterParts = array();
|
||||
$users = $this->access->fetchListOfUsers($filter, 'dn', count($filterParts));
|
||||
$dns = array_merge($dns, $users);
|
||||
}
|
||||
$dns[] = $ldap_users[0];
|
||||
}
|
||||
if(count($filterParts) > 0) {
|
||||
$filter = $this->access->combineFilterWithOr($filterParts);
|
||||
$users = $this->access->fetchListOfUsers($filter, 'dn', count($filterParts));
|
||||
$dns = array_merge($dns, $users);
|
||||
}
|
||||
$members = $dns;
|
||||
}
|
||||
|
||||
$isInGroup = in_array($userDN, $members);
|
||||
$this->access->connection->writeToCache($cacheKey, $isInGroup);
|
||||
$this->access->connection->writeToCache($cacheKeyMembers, $members);
|
||||
$this->cachedGroupMembers[$gid] = $members;
|
||||
|
||||
return $isInGroup;
|
||||
}
|
||||
|
@ -293,8 +333,13 @@ class GROUP_LDAP extends BackendUtility implements \OCP\GroupInterface {
|
|||
$uid = $userDN;
|
||||
}
|
||||
|
||||
$groups = array_values($this->getGroupsByMember($uid));
|
||||
$groups = $this->access->ownCloudGroupNames($groups);
|
||||
if(isset($this->cachedGroupsByMember[$uid])) {
|
||||
$groups = $this->cachedGroupsByMember[$uid];
|
||||
} else {
|
||||
$groups = array_values($this->getGroupsByMember($uid));
|
||||
$groups = $this->access->ownCloudGroupNames($groups);
|
||||
$this->cachedGroupsByMember[$uid] = $groups;
|
||||
}
|
||||
|
||||
$primaryGroup = $this->getUserPrimaryGroup($userDN);
|
||||
if($primaryGroup !== false) {
|
||||
|
|
|
@ -1359,7 +1359,7 @@ class Access extends LDAPUtility implements user\IUserTools {
|
|||
* @param string[] $bases array containing the allowed base DN or DNs
|
||||
* @return bool
|
||||
*/
|
||||
private function isDNPartOfBase($dn, $bases) {
|
||||
public function isDNPartOfBase($dn, $bases) {
|
||||
$belongsToBase = false;
|
||||
$bases = $this->sanitizeDN($bases);
|
||||
|
||||
|
|
|
@ -59,10 +59,7 @@ class Test_Group_Ldap extends \PHPUnit_Framework_TestCase {
|
|||
private function enableGroups($access) {
|
||||
$access->connection->expects($this->any())
|
||||
->method('__get')
|
||||
->will($this->returnCallback(function($name) {
|
||||
// if($name === 'ldapLoginFilter') {
|
||||
// return '%uid';
|
||||
// }
|
||||
->will($this->returnCallback(function() {
|
||||
return 1;
|
||||
}));
|
||||
}
|
||||
|
@ -269,4 +266,32 @@ class Test_Group_Ldap extends \PHPUnit_Framework_TestCase {
|
|||
$this->assertSame(false, $gid);
|
||||
}
|
||||
|
||||
/**
|
||||
* tests whether Group Backend behaves correctly when cache with uid and gid
|
||||
* is hit
|
||||
*/
|
||||
public function testInGroupHitsUidGidCache() {
|
||||
$access = $this->getAccessMock();
|
||||
$this->enableGroups($access);
|
||||
|
||||
$uid = 'someUser';
|
||||
$gid = 'someGroup';
|
||||
$cacheKey = 'inGroup'.$uid.':'.$gid;
|
||||
$access->connection->expects($this->once())
|
||||
->method('isCached')
|
||||
->with($cacheKey)
|
||||
->will($this->returnValue(true));
|
||||
|
||||
$access->connection->expects($this->once())
|
||||
->method('getFromCache')
|
||||
->with($cacheKey)
|
||||
->will($this->returnValue(true));
|
||||
|
||||
$access->expects($this->never())
|
||||
->method('username2dn');
|
||||
|
||||
$groupBackend = new GroupLDAP($access);
|
||||
$groupBackend->inGroup($uid, $gid);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -223,10 +223,9 @@ class Manager extends PublicEmitter implements IGroupManager {
|
|||
if(!empty($search)) {
|
||||
// only user backends have the capability to do a complex search for users
|
||||
$searchOffset = 0;
|
||||
$searchLimit = $limit * 100;
|
||||
if($limit === -1) {
|
||||
$searchLimit = $group->count('');
|
||||
} else {
|
||||
$searchLimit = $limit * 2;
|
||||
$searchLimit = 500;
|
||||
}
|
||||
|
||||
do {
|
||||
|
@ -237,7 +236,7 @@ class Manager extends PublicEmitter implements IGroupManager {
|
|||
}
|
||||
}
|
||||
$searchOffset += $searchLimit;
|
||||
} while(count($groupUsers) < $searchLimit+$offset && count($filteredUsers) === $searchLimit);
|
||||
} while(count($groupUsers) < $searchLimit+$offset && count($filteredUsers) >= $searchLimit);
|
||||
|
||||
if($limit === -1) {
|
||||
$groupUsers = array_slice($groupUsers, $offset);
|
||||
|
|
|
@ -369,15 +369,6 @@ class Manager extends \PHPUnit_Framework_TestCase {
|
|||
}
|
||||
}));
|
||||
|
||||
$backend->expects($this->once())
|
||||
->method('implementsActions')
|
||||
->will($this->returnValue(true));
|
||||
|
||||
$backend->expects($this->once())
|
||||
->method('countUsersInGroup')
|
||||
->with('testgroup', '')
|
||||
->will($this->returnValue(2));
|
||||
|
||||
/**
|
||||
* @var \OC\User\Manager $userManager
|
||||
*/
|
||||
|
@ -496,9 +487,9 @@ class Manager extends \PHPUnit_Framework_TestCase {
|
|||
->with('testgroup')
|
||||
->will($this->returnValue(true));
|
||||
|
||||
$backend->expects($this->any())
|
||||
->method('InGroup')
|
||||
->will($this->returnCallback(function($uid, $gid) {
|
||||
$backend->expects($this->any())
|
||||
->method('inGroup')
|
||||
->will($this->returnCallback(function($uid) {
|
||||
switch($uid) {
|
||||
case 'user1' : return false;
|
||||
case 'user2' : return true;
|
||||
|
@ -521,9 +512,12 @@ class Manager extends \PHPUnit_Framework_TestCase {
|
|||
->with('user3')
|
||||
->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) {
|
||||
switch($offset) {
|
||||
case 0 : return array('user3' => new User('user3', $userBackend),
|
||||
'user33' => new User('user33', $userBackend));
|
||||
case 2 : return array('user333' => new User('user333', $userBackend));
|
||||
case 0 :
|
||||
return array(
|
||||
'user3' => new User('user3', $userBackend),
|
||||
'user33' => new User('user33', $userBackend),
|
||||
'user333' => new User('user333', $userBackend)
|
||||
);
|
||||
}
|
||||
}));
|
||||
|
||||
|
|
Loading…
Reference in New Issue