diff --git a/apps/dav/lib/Paginate/PaginateCache.php b/apps/dav/lib/Paginate/PaginateCache.php index 6165ead4690ef..31e674e43b2d4 100644 --- a/apps/dav/lib/Paginate/PaginateCache.php +++ b/apps/dav/lib/Paginate/PaginateCache.php @@ -15,6 +15,9 @@ use OCP\IDBConnection; use OCP\Security\ISecureRandom; +/** + * @psalm-type PaginateContext = array + */ class PaginateCache { public const TTL = 60 * 60; private const string CACHE_COUNT_SUFFIX = 'count'; @@ -32,11 +35,12 @@ public function __construct( /** * @param string $uri * @param \Iterator $items + * @param PaginateContext $context * @return array{'token': string, 'count': int} */ - public function store(string $uri, \Iterator $items): array { + public function store(string $uri, \Iterator $items, array $context): array { $token = $this->random->generate(32); - $cacheKey = $this->buildCacheKey($uri, $token); + $cacheKey = $this->buildCacheKey($uri, $token, $context); $count = 0; foreach ($items as $item) { @@ -50,10 +54,11 @@ public function store(string $uri, \Iterator $items): array { } /** + * @param PaginateContext $context * @return Generator */ - public function get(string $uri, string $token, int $offset, int $count): Generator { - $cacheKey = $this->buildCacheKey($uri, $token); + public function get(string $uri, string $token, int $offset, int $count, array $context): Generator { + $cacheKey = $this->buildCacheKey($uri, $token, $context); $nbItems = $this->cache->get($cacheKey . self::CACHE_COUNT_SUFFIX); if (!$nbItems || $offset > $nbItems) { return []; @@ -65,11 +70,17 @@ public function get(string $uri, string $token, int $offset, int $count): Genera } } - public function exists(string $uri, string $token): bool { - return $this->cache->get($this->buildCacheKey($uri, $token) . self::CACHE_COUNT_SUFFIX) > 0; + /** + * @param PaginateContext $context + */ + public function exists(string $uri, string $token, array $context): bool { + return $this->cache->get($this->buildCacheKey($uri, $token, $context) . self::CACHE_COUNT_SUFFIX) > 0; } - private function buildCacheKey(string $uri, string $token): string { - return $token . '_' . crc32($uri) . '_'; + /** + * @param PaginateContext $context + */ + private function buildCacheKey(string $uri, string $token, array $context): string { + return $token . '_' . crc32($uri) . '_' . hash('sha256', json_encode($context, JSON_THROW_ON_ERROR)) . '_'; } } diff --git a/apps/dav/lib/Paginate/PaginatePlugin.php b/apps/dav/lib/Paginate/PaginatePlugin.php index 234c7e25b75c6..b6d43461b1719 100644 --- a/apps/dav/lib/Paginate/PaginatePlugin.php +++ b/apps/dav/lib/Paginate/PaginatePlugin.php @@ -9,6 +9,7 @@ namespace OCA\DAV\Paginate; +use Sabre\DAV\Auth\Plugin as AuthPlugin; use Sabre\DAV\Server; use Sabre\DAV\ServerPlugin; use Sabre\DAV\Xml\Element\Response; @@ -53,7 +54,7 @@ public function onMultiStatus(&$fileProperties): void { $url = $request->getUrl(); if ( $request->hasHeader(self::PAGINATE_HEADER) - && (!$request->hasHeader(self::PAGINATE_TOKEN_HEADER) || !$this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER))) + && (!$request->hasHeader(self::PAGINATE_TOKEN_HEADER) || !$this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER), $this->getCacheContext())) ) { $pageSize = (int)$request->getHeader(self::PAGINATE_COUNT_HEADER) ?: $this->pageSize; $offset = (int)$request->getHeader(self::PAGINATE_OFFSET_HEADER); @@ -65,7 +66,7 @@ public function onMultiStatus(&$fileProperties): void { // cache XML, but we keep the first $pageSize elements as objects // to use for the response of the first page. $rendererGenerator = $this->getXmlRendererGenerator($copyIterator); - ['token' => $token, 'count' => $count] = $this->cache->store($url, $rendererGenerator); + ['token' => $token, 'count' => $count] = $this->cache->store($url, $rendererGenerator, $this->getCacheContext()); $this->server->httpResponse->addHeader(self::PAGINATE_HEADER, 'true'); $this->server->httpResponse->addHeader(self::PAGINATE_TOKEN_HEADER, $token); @@ -83,8 +84,7 @@ public function onMultiStatus(&$fileProperties): void { */ private function getXmlRendererGenerator(iterable $fileProperties): \Generator { $writer = $this->server->xml->getWriter(); - $prefer = $this->server->getHTTPPrefer(); - $minimal = $prefer['return'] === 'minimal'; + $minimal = $this->isMinimal(); $writer->contextUri = $this->server->getBaseUri(); $writer->openMemory(); @@ -115,18 +115,34 @@ private function getXmlRendererGenerator(iterable $fileProperties): \Generator { } } + private function isMinimal(): bool { + return $this->server->getHTTPPrefer()['return'] === 'minimal'; + } + + /** + * @return array{principal: ?string, minimal: bool} + */ + private function getCacheContext(): array { + $authPlugin = $this->server->getPlugin('auth'); + return [ + 'principal' => $authPlugin instanceof AuthPlugin ? $authPlugin->getCurrentPrincipal() : null, + 'minimal' => $this->isMinimal(), + ]; + } + public function onMethod(RequestInterface $request, ResponseInterface $response) { $url = $this->server->httpRequest->getUrl(); + $context = $this->getCacheContext(); if ( $request->hasHeader(self::PAGINATE_TOKEN_HEADER) && $request->hasHeader(self::PAGINATE_OFFSET_HEADER) - && $this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER)) + && $this->cache->exists($url, $request->getHeader(self::PAGINATE_TOKEN_HEADER), $context) ) { $token = $request->getHeader(self::PAGINATE_TOKEN_HEADER); $offset = (int)$request->getHeader(self::PAGINATE_OFFSET_HEADER); $count = (int)$request->getHeader(self::PAGINATE_COUNT_HEADER) ?: $this->pageSize; - $items = $this->cache->get($url, $token, $offset, $count); + $items = $this->cache->get($url, $token, $offset, $count, $context); $response->setStatus(207); $response->addHeader(self::PAGINATE_HEADER, 'true'); diff --git a/apps/dav/tests/unit/Paginate/PaginateCacheTest.php b/apps/dav/tests/unit/Paginate/PaginateCacheTest.php new file mode 100644 index 0000000000000..cf92dc49e9776 --- /dev/null +++ b/apps/dav/tests/unit/Paginate/PaginateCacheTest.php @@ -0,0 +1,73 @@ + 'principals/users/alice', 'minimal' => false]; + + private PaginateCache $paginateCache; + + protected function setUp(): void { + parent::setUp(); + + $random = $this->createMock(ISecureRandom::class); + $random->method('generate') + ->willReturn('token'); + + $cacheFactory = $this->createMock(ICacheFactory::class); + $cacheFactory->method('createDistributed') + ->willReturn(new ArrayCache('pagination_')); + + $this->paginateCache = new PaginateCache( + $this->createMock(IDBConnection::class), + $random, + $cacheFactory, + ); + } + + public function testStoreAndGetWithSameContext(): void { + $result = $this->paginateCache->store('url', new \ArrayIterator(['a', 'b', 'c']), self::CONTEXT); + + self::assertSame(['token' => 'token', 'count' => 3], $result); + self::assertTrue($this->paginateCache->exists('url', 'token', self::CONTEXT)); + self::assertSame(['b', 'c'], iterator_to_array($this->paginateCache->get('url', 'token', 1, 5, self::CONTEXT), false)); + } + + public static function dataDifferentContextDoesNotUseCache(): array { + return [ + 'other principal' => [['principal' => 'principals/users/bob', 'minimal' => false]], + 'no principal' => [['principal' => null, 'minimal' => false]], + 'other preference' => [['principal' => 'principals/users/alice', 'minimal' => true]], + 'empty context' => [[]], + ]; + } + + #[DataProvider(methodName: 'dataDifferentContextDoesNotUseCache')] + public function testDifferentContextDoesNotUseCache(array $context): void { + $this->paginateCache->store('url', new \ArrayIterator(['a', 'b']), self::CONTEXT); + + self::assertFalse($this->paginateCache->exists('url', 'token', $context)); + self::assertSame([], iterator_to_array($this->paginateCache->get('url', 'token', 0, 5, $context), false)); + } + + public function testDifferentUrlCannotAccess(): void { + $this->paginateCache->store('url', new \ArrayIterator(['a', 'b']), self::CONTEXT); + + self::assertFalse($this->paginateCache->exists('other-url', 'token', self::CONTEXT)); + } +} diff --git a/apps/dav/tests/unit/Paginate/PaginatePluginTest.php b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php index 8d3d4cc9f781e..9e1058a2cf59e 100644 --- a/apps/dav/tests/unit/Paginate/PaginatePluginTest.php +++ b/apps/dav/tests/unit/Paginate/PaginatePluginTest.php @@ -12,6 +12,7 @@ use OCA\DAV\Paginate\PaginateCache; use OCA\DAV\Paginate\PaginatePlugin; use PHPUnit\Framework\MockObject\MockObject; +use Sabre\DAV\Auth\Plugin as AuthPlugin; use Sabre\DAV\Server; use Sabre\HTTP\RequestInterface; use Sabre\HTTP\ResponseInterface; @@ -19,12 +20,14 @@ use Test\TestCase; class PaginatePluginTest extends TestCase { + private const CONTEXT = ['principal' => 'principals/users/alice', 'minimal' => false]; private PaginateCache&MockObject $cache; private PaginatePlugin $plugin; private Server&MockObject $server; private RequestInterface&MockObject $request; private ResponseInterface&MockObject $response; + private ?string $preferReturn = null; public function testOnMultiStatusCachesAndUpdatesResponse(): void { $this->initializePlugin(); @@ -88,6 +91,7 @@ public function testOnMultiStatusCachesAndUpdatesResponse(): void { self::assertStringContainsString($this->getResponseXmlForFile('/dav/file3', 'File 3'), $items[2]); return true; }), + self::CONTEXT, ) ->willReturn([ 'token' => 'token', @@ -187,7 +191,7 @@ public function testOnMultiStatusSkipsWhenHeadersAndCacheExist(): void { $this->cache->expects(self::once()) ->method('exists') - ->with('', 'token') + ->with('', 'token', self::CONTEXT) ->willReturn(true); $this->cache->expects(self::never()) @@ -231,12 +235,12 @@ public function testOnMethodReturnsCachedResponse(): void { $this->cache->expects(self::once()) ->method('exists') - ->with('url', 'token') + ->with('url', 'token', self::CONTEXT) ->willReturn(true); $this->cache->expects(self::once()) ->method('get') - ->with('url', 'token', 2, 4) + ->with('url', 'token', 2, 4, self::CONTEXT) ->willReturn((function (): \Generator { yield $this->getResponseXmlForFile('/file1', 'File 1'); yield $this->getResponseXmlForFile('/file2', 'File 2'); @@ -279,6 +283,27 @@ public function testOnMethodReturnsCachedResponse(): void { self::assertFalse($this->plugin->onMethod($this->request, $response)); } + public function testOnMethodPassesMinimalPreferenceInContext(): void { + $this->initializePlugin(); + $this->preferReturn = 'minimal'; + + $this->request->method('hasHeader') + ->willReturn(true); + $this->request->method('getUrl') + ->willReturn('url'); + $this->request->method('getHeader') + ->willReturn('token'); + + $this->cache->expects(self::once()) + ->method('exists') + ->with('url', 'token', ['principal' => 'principals/users/alice', 'minimal' => true]) + ->willReturn(false); + $this->cache->expects(self::never()) + ->method('get'); + + $this->plugin->onMethod($this->request, $this->response); + } + public function testOnMultiStatusNoPaginateHeaderShouldSucceed(): void { $this->initializePlugin(); @@ -316,7 +341,7 @@ protected function setUp(): void { $this->server = $this->getMockBuilder(Server::class) ->disableOriginalConstructor() - ->onlyMethods(['on', 'getHTTPPrefer', 'getBaseUri']) + ->onlyMethods(['on', 'getHTTPPrefer', 'getBaseUri', 'getPlugin']) ->getMock(); $this->request = $this->createMock(RequestInterface::class); @@ -328,7 +353,13 @@ protected function setUp(): void { $this->server->xml->namespaceMap = [ 'DAV:' => 'd' ]; $this->server->method('getHTTPPrefer') - ->willReturn(['return' => null]); + ->willReturnCallback(fn () => ['return' => $this->preferReturn]); + + $authPlugin = $this->createMock(AuthPlugin::class); + $authPlugin->method('getCurrentPrincipal') + ->willReturn('principals/users/alice'); + $this->server->method('getPlugin') + ->willReturnMap([['auth', $authPlugin]]); $this->server->method('getBaseUri') ->willReturn('/dav/');