Skip to content

Commit

Permalink
Merge pull request #551 from owncloud/bugfix/reissue-cont-page-search
Browse files Browse the repository at this point in the history 
Reissue search in case of missing cookie in continued paged search
  • Loading branch information
@phil-davis
phil-davis authored Jun 15, 2020
2 parents 0ebfc56 + 04e3b97 commit 996159b
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 38 deletions.
90 changes: 55 additions & 35 deletions lib/Access.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ private function countEntriesInSearchResults($searchResults) {
* @return array with the search result
* @throws \OC\ServerNotAvailableException
*/
private function search($filter, $base, $attr = null, $limit = null, $offset = null, $skipHandling = false) {
private function search($filter, $base, $attr = null, $limit = null, $offset = null) {
if ($attr !== null && !\is_array($attr)) {
$attr = [\mb_strtolower($attr, 'UTF-8')];
}
Expand All @@ -1165,6 +1165,7 @@ private function search($filter, $base, $attr = null, $limit = null, $offset = n
*/
$findings = [];
$savedoffset = $offset;
$shouldRetryForMissingCookie = true;
do {
$search = $this->executeSearch($filter, $base, $attr, $limit, $offset);
if ($search === false) {
Expand All @@ -1173,25 +1174,53 @@ private function search($filter, $base, $attr = null, $limit = null, $offset = n
list($sr, $pagedSearchOK) = $search;
$cr = $this->connection->getConnectionResource();

if ($skipHandling) {
//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,
$offset, $pagedSearchOK,
$skipHandling);
return [];
}

foreach ($sr as $res) {
$findings = \array_merge($findings, $this->getLDAP()->getEntries($cr, $res));
}

list($continue, ) = $this->processPagedSearchStatus($sr, $filter, $base, $findings['count'],
list($hasMorePages, ) = $this->processPagedSearchStatus($sr, $filter, $base, $findings['count'],
$limit, $offset, $pagedSearchOK,
$skipHandling);
$offset += $limit;
} while ($continue && $pagedSearchOK && $findings['count'] < $limit);
false);

// according to LDAP documentation, when cookie is missing for
// continued paged search, we should retry search from scratch
// up to required offset. Do not try reissuing cache next
// time as it could be another issue
if (!$pagedSearchOK && $shouldRetryForMissingCookie && $limit !== null && $offset > 0) {
// abandon paged searches to start missing paged search
$this->abandonPagedSearch();

\OC::$server->getLogger()->info(
"Reissuing paged search at range 0-$offset with limit $limit to retrieve missing cookie"
);
$shouldRetryForMissingCookie = false;

// reoffset to 0
$reOffset = 0;
do {
$retrySearch = $this->executeSearch($filter, $base, $attr, $limit, $reOffset);
if ($retrySearch === false) {
$retryPagedSearchOK = false;
} else {
list($retrySr, $retryPagedSearchOK) = $retrySearch;

// i.e. result does 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($retrySr, $filter, $base, 1, $limit,
$reOffset, $retryPagedSearchOK,
true);
}

// do not continue both retry and original query on error
$continue = $retryPagedSearchOK;
$reOffset += $limit;
} while ($continue && $reOffset < $offset);
} else {
$continue = $hasMorePages && $pagedSearchOK;
$offset += $limit;
}
} while ($continue && $findings['count'] < $limit);
// resetting offset
$offset = $savedoffset;

Expand Down Expand Up @@ -1810,7 +1839,7 @@ public function isDNPartOfBase($dn, $bases) {
private function abandonPagedSearch() {
if ($this->connection->hasPagedResultSupport) {
\OC::$server->getLogger()->debug(
'abandoning paged search. last cookie: '.$this->cookie2str($this->lastCookie).', cookies: <'.\implode(',', \array_map([$this, 'cookie2str'], $this->cookies)).'>',
'Abandoning paged search - last cookie: '.$this->cookie2str($this->lastCookie).', cookies: <'.\implode(',', \array_map([$this, 'cookie2str'], $this->cookies)).'>',
['app' => 'user_ldap']);
$cr = $this->connection->getConnectionResource();
$this->getLDAP()->controlPagedResult($cr, 0, false, $this->lastCookie);
Expand Down Expand Up @@ -1919,7 +1948,7 @@ private function initPagedSearch($filter, $bases, $attr, $limit, $offset) {
$offset = (int)$offset; //can be null
$range = $offset . "-" . ($offset + $limit);
\OC::$server->getLogger()->debug(
"initializing paged search for Filter $filter base ".\print_r($bases, true)
"Initializing paged search for Filter $filter base ".\print_r($bases, true)
.' attr '.\print_r($attr, true)." at $range",
['app' => 'user_ldap']);
//get the cookie from the search for the previous search, required by LDAP
Expand All @@ -1928,25 +1957,16 @@ private function initPagedSearch($filter, $bases, $attr, $limit, $offset) {
\OC::$server->getLogger()->debug(
"Cookie for $base at $range is ".$this->cookie2str($cookie),
['app' => 'user_ldap']);

// '0' is valid, because 389ds
if (empty($cookie) && $cookie !== '0' && ($offset > 0)) {
// no cookie known, although the offset is not 0. Maybe cache run out. We need
// to start all over *sigh* (btw, Dear Reader, did you know LDAP paged
// searching was designed by MSFT?)
// Lukas: No, but thanks to reading that source I finally know!
// '0' is valid, because 389ds
if (($offset - $limit) < 0) {
$reOffset = 0;
} else {
$reOffset = $offset - $limit;
}
// just try the previous one
$cookie = $this->getPagedResultCookie($base, $filter, $limit, $reOffset);
//still no cookie? obviously, the server does not like us. Let's skip paging efforts.
//TODO: remember this, probably does not change in the next request...
if (empty($cookie) && $cookie !== '0') {
// '0' is valid, because 389ds
$cookie = null;
}
// Abandon - no cookie known although the offset is not 0.
// Maybe cache run out (abandoned paged search or cookie deletion).
$this->abandonPagedSearch();
\OC::$server->getLogger()->debug(
"Cache not available for continued paged search at $range, aborting.",
['app' => 'user_ldap']);
return false;
}
if ($cookie !== null) {
if (empty($offset)) {
Expand Down
37 changes: 35 additions & 2 deletions tests/integration/Lib/IntegrationTestPaging.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public function init() {
* @return bool
*/
protected function case1() {
$limit = 1;
$limit = 2;
$offset = 0;

$filter = 'objectclass=inetorgperson';
Expand All @@ -67,12 +67,45 @@ protected function case1() {
$offset += $limit;
} while ($this->access->hasMoreResults());

if (\count($users) === 2) {
if (\count($users) === 3) {
return true;
}

return false;
}

/**
* tests that paging can retry for missing cookie for continued paged search
*
* @return bool
*/
protected function case2() {
$filter = 'objectclass=inetorgperson';
$attributes = ['cn', 'dn'];

// start paged search from offset 0 with limit 2
// and thus sets cookie so search can continue
$result = $this->access->searchUsers($filter, $attributes, 2, 0);
if (\count($result) !== 2) {
return false;
}

// interrupt previous paged search with paged search that returns complete result
// and thus sets '' cookie indicating completion
$result = $this->access->searchUsers($filter, $attributes, 4, 0);
if (\count($result) !== 3) {
return false;
}

// we should be able to continue previous paged search when interrupted
// by retrying search to repopulate cookie
$result = $this->access->searchUsers($filter, $attributes, 2, 2);
if (\count($result) !== 1) {
return false;
}

return true;
}
}

require_once(__DIR__ . '/../setup-scripts/config.php');
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/setup-scripts/createExplicitUsers.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
}
}

$users = ['alice', 'boris'];
$users = ['alice', 'boris', 'carl'];

foreach ($users as $uid) {
$newDN = 'uid=' . $uid . ',' . $ouDN;
Expand Down

0 comments on commit 996159b

Please sign in to comment.