backport of #11494

fix retrievel of group members and cache group members

fix changed variable name

with several backends, more than limit can be returned

make performance less bad. Still far from good, but at least it works

add one simple cache test

adjust group manager tests

5 files changed, 96 insertions, 33 deletions

diff --git a/apps/user_ldap/group_ldap.php b/apps/user_ldap/group_ldap.php
index 48d097c3600..e8d268d3df2 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 159b0d73000..44162e32d47 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 c4aed25a1cc..d1262e4f5b8 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 816e7b427f5..417be79ab30 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 e4b3a522767..8fd19513c0a 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)
+											);
                                 }
                         }));