Merge pull request #6453 from nextcloud/fix-5273

Fix initializing paged search under some circumstances

4 files changed, 72 insertions, 25 deletions

diff --git a/apps/user_ldap/lib/Access.php b/apps/user_ldap/lib/Access.php
index 69e1f3c52f4..b6674cf0332 100644
--- a/apps/user_ldap/lib/Access.php
+++ b/apps/user_ldap/lib/Access.php
@@ -1030,7 +1030,7 @@ class Access extends LDAPUtility implements IUserTools {
 	 * @param array $sr the array containing the LDAP search resources
 	 * @param string $filter the LDAP filter for the search
 	 * @param array $base an array containing the LDAP subtree(s) that shall be searched
-	 * @param int $iFoundItems number of results in the search operation
+	 * @param int $iFoundItems number of results in the single search operation
 	 * @param int $limit maximum results to be counted
 	 * @param int $offset a starting point
 	 * @param bool $pagedSearchOK whether a paged search has been executed
@@ -1149,9 +1149,9 @@ class Access extends LDAPUtility implements IUserTools {
 	 * @return array with the search result
 	 */
 	public function search($filter, $base, $attr = null, $limit = null, $offset = null, $skipHandling = false) {
-		if($limit <= 0) {
-			//otherwise search will fail
-			$limit = null;
+		$limitPerPage = intval($this->connection->ldapPagingSize);
+		if(!is_null($limit) && $limit < $limitPerPage && $limit > 0) {
+			$limitPerPage = $limit;
 		}
 
 		/* ++ Fixing RHDS searches with pages with zero results ++
@@ -1163,7 +1163,7 @@ class Access extends LDAPUtility implements IUserTools {
 		$findings = array();
 		$savedoffset = $offset;
 		do {
-			$search = $this->executeSearch($filter, $base, $attr, $limit, $offset);
+			$search = $this->executeSearch($filter, $base, $attr, $limitPerPage, $offset);
 			if($search === false) {
 				return array();
 			}
@@ -1174,21 +1174,24 @@ class Access extends LDAPUtility implements IUserTools {
 				//i.e. result do not need to be fetched, we just need the cookie
 				//thus pass 1 or any other value as $iFoundItems because it is not
 				//used
-				$this->processPagedSearchStatus($sr, $filter, $base, 1, $limit,
+				$this->processPagedSearchStatus($sr, $filter, $base, 1, $limitPerPage,
 								$offset, $pagedSearchOK,
 								$skipHandling);
 				return array();
 			}
 
+			$iFoundItems = 0;
 			foreach($sr as $res) {
 				$findings = array_merge($findings, $this->invokeLDAPMethod('getEntries', $cr, $res));
+				$iFoundItems = max($iFoundItems, $findings['count']);
+				unset($findings['count']);
 			}
 
-			$continue = $this->processPagedSearchStatus($sr, $filter, $base, $findings['count'],
-								$limit, $offset, $pagedSearchOK,
+			$continue = $this->processPagedSearchStatus($sr, $filter, $base, $iFoundItems,
+				$limitPerPage, $offset, $pagedSearchOK,
 										$skipHandling);
-			$offset += $limit;
-		} while ($continue && $pagedSearchOK && $findings['count'] < $limit);
+			$offset += $limitPerPage;
+		} while ($continue && $pagedSearchOK && ($limit === null || count($findings) < $limit));
 		// reseting offset
 		$offset = $savedoffset;
 
diff --git a/apps/user_ldap/lib/Group_LDAP.php b/apps/user_ldap/lib/Group_LDAP.php
index f7617fa5a51..55d31649f10 100644
--- a/apps/user_ldap/lib/Group_LDAP.php
+++ b/apps/user_ldap/lib/Group_LDAP.php
@@ -333,7 +333,7 @@ class Group_LDAP extends BackendUtility implements \OCP\GroupInterface {
 	public function getUserGidNumber($dn) {
 		$gidNumber = false;
 		if($this->access->connection->hasGidNumber) {
-			$gidNumber = $this->getEntryGidNumber($dn, 'gidNumber');
+			$gidNumber = $this->getEntryGidNumber($dn, $this->access->connection->ldapGidNumber);
 			if($gidNumber === false) {
 				$this->access->connection->hasGidNumber = false;
 			}
diff --git a/apps/user_ldap/tests/Integration/Lib/IntegrationTestPaging.php b/apps/user_ldap/tests/Integration/Lib/IntegrationTestPaging.php
index 44ee1c3bb5c..35d8524fd88 100644
--- a/apps/user_ldap/tests/Integration/Lib/IntegrationTestPaging.php
+++ b/apps/user_ldap/tests/Integration/Lib/IntegrationTestPaging.php
@@ -36,6 +36,9 @@ class IntegrationTestPaging extends AbstractIntegrationTest {
 	/** @var User_LDAP */
 	protected $backend;
 
+	/** @var int */
+	protected $pagingSize = 2;
+
 	/**
 	 * prepares the LDAP environment and sets up a test configuration for
 	 * the LDAP backend.
@@ -47,28 +50,69 @@ class IntegrationTestPaging extends AbstractIntegrationTest {
 		$this->backend = new User_LDAP($this->access, \OC::$server->getConfig(), \OC::$server->getNotificationManager());
 	}
 
+	public function initConnection() {
+		parent::initConnection();
+		$this->connection->setConfiguration([
+			'ldapPagingSize' => $this->pagingSize
+		]);
+	}
+
 	/**
 	 * tests that paging works properly against a simple example (reading all
-	 * of few users in smallest steps)
+	 * of few users in small steps)
 	 *
 	 * @return bool
 	 */
 	protected function case1() {
-		$limit = 1;
-		$offset = 0;
+		$filter = 'objectclass=inetorgperson';
+		$attributes = ['cn', 'dn'];
+
+		$result = $this->access->searchUsers($filter, $attributes);
+		if(count($result) === 7) {
+			return true;
+		}
 
+		return false;
+	}
+
+	/**
+	 * fetch first three, afterwards all users
+	 *
+	 * @return bool
+	 */
+	protected function case2() {
 		$filter = 'objectclass=inetorgperson';
 		$attributes = ['cn', 'dn'];
-		$users = [];
-		do {
-			$result = $this->access->searchUsers($filter, $attributes, $limit, $offset);
-			foreach($result as $user) {
-				$users[] = $user['cn'];
-			}
-			$offset += $limit;
-		} while ($this->access->hasMoreResults());
-
-		if(count($users) === 2) {
+
+		$result = $this->access->searchUsers($filter, $attributes, 4);
+		// beware, under circumstances, the result  set can be larger then
+		// the specified limit! In this case, if we specify a limit of 3,
+		// the result will be 4, because the highest possible paging size
+		// is 2 (as configured).
+		// But also with more than one search base, the limit can be outpaced.
+		if(count($result) !== 4) {
+			return false;
+		}
+
+		$result = $this->access->searchUsers($filter, $attributes);
+		if(count($result) !== 7) {
+			return false;
+		}
+
+		return true;
+	}
+
+	/**
+	 * reads all remaining users starting first page
+	 *
+	 * @return bool
+	 */
+	protected function case3() {
+		$filter = 'objectclass=inetorgperson';
+		$attributes = ['cn', 'dn'];
+
+		$result = $this->access->searchUsers($filter, $attributes, null, $this->pagingSize);
+		if(count($result) === (7 - $this->pagingSize)) {
 			return true;
 		}
 
diff --git a/apps/user_ldap/tests/Integration/setup-scripts/createExplicitUsers.php b/apps/user_ldap/tests/Integration/setup-scripts/createExplicitUsers.php
index 11f3ce310db..b01c498f859 100644
--- a/apps/user_ldap/tests/Integration/setup-scripts/createExplicitUsers.php
+++ b/apps/user_ldap/tests/Integration/setup-scripts/createExplicitUsers.php
@@ -50,7 +50,7 @@ if (true) {
 	}
 }
 
-$users = ['alice', 'boris'];
+$users = ['alice', 'boris', 'cynthia', 'derek', 'evelina', 'fatima', 'gregor'];
 
 foreach ($users as $uid) {
 	$newDN = 'uid=' . $uid . ',' . $ouDN;