diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php index 48d097c360..e8d268d3df 100644 --- a/apps/user_ldap/group_ldap.php +++ b/apps/user_ldap/group_ldap.php @@ -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) { diff --git a/apps/user_ldap/lib/access.php b/apps/user_ldap/lib/access.php index 159b0d7300..44162e32d4 100644 --- a/apps/user_ldap/lib/access.php +++ b/apps/user_ldap/lib/access.php @@ -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); diff --git a/apps/user_ldap/tests/group_ldap.php b/apps/user_ldap/tests/group_ldap.php index c4aed25a1c..d1262e4f5b 100644 --- a/apps/user_ldap/tests/group_ldap.php +++ b/apps/user_ldap/tests/group_ldap.php @@ -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); + } + } diff --git a/lib/private/group/manager.php b/lib/private/group/manager.php index 816e7b427f..417be79ab3 100644 --- a/lib/private/group/manager.php +++ b/lib/private/group/manager.php @@ -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); diff --git a/tests/lib/group/manager.php b/tests/lib/group/manager.php index e4b3a52276..8fd19513c0 100644 --- a/tests/lib/group/manager.php +++ b/tests/lib/group/manager.php @@ -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) + ); } }));