From 5dd2da28b3a0c3c717585310695d32dbdb88d8b9 Mon Sep 17 00:00:00 2001 From: Piotr Mrowczynski Date: Sun, 26 Apr 2020 22:04:30 +0200 Subject: [PATCH] Reissue search in case of missing cookie in continued paged search --- lib/Access.php | 90 +++++++++++-------- .../integration/Lib/IntegrationTestPaging.php | 37 +++++++- .../setup-scripts/createExplicitUsers.php | 2 +- 3 files changed, 91 insertions(+), 38 deletions(-) diff --git a/lib/Access.php b/lib/Access.php index 6c8a9cb03..20dcc80ab 100644 --- a/lib/Access.php +++ b/lib/Access.php @@ -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')]; } @@ -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) { @@ -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; @@ -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); @@ -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 @@ -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)) { diff --git a/tests/integration/Lib/IntegrationTestPaging.php b/tests/integration/Lib/IntegrationTestPaging.php index ae3fd5cac..0b2a3f4b1 100644 --- a/tests/integration/Lib/IntegrationTestPaging.php +++ b/tests/integration/Lib/IntegrationTestPaging.php @@ -53,7 +53,7 @@ public function init() { * @return bool */ protected function case1() { - $limit = 1; + $limit = 2; $offset = 0; $filter = 'objectclass=inetorgperson'; @@ -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'); diff --git a/tests/integration/setup-scripts/createExplicitUsers.php b/tests/integration/setup-scripts/createExplicitUsers.php index 0486c7452..4de29eaca 100644 --- a/tests/integration/setup-scripts/createExplicitUsers.php +++ b/tests/integration/setup-scripts/createExplicitUsers.php @@ -51,7 +51,7 @@ } } -$users = ['alice', 'boris']; +$users = ['alice', 'boris', 'carl']; foreach ($users as $uid) { $newDN = 'uid=' . $uid . ',' . $ouDN;