Merge pull request #11494 from owncloud/fix-ldap-ingroup-for-9225-2

fix retrieval of group members and cache group members

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)
+											);
                                 }
                         }));