diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index c010a3e98d602..e5898aa04c2b1 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -215,6 +215,7 @@ 'OCA\\DAV\\Connector\\Sabre\\Auth' => $baseDir . '/../lib/Connector/Sabre/Auth.php', 'OCA\\DAV\\Connector\\Sabre\\BearerAuth' => $baseDir . '/../lib/Connector/Sabre/BearerAuth.php', 'OCA\\DAV\\Connector\\Sabre\\BlockLegacyClientPlugin' => $baseDir . '/../lib/Connector/Sabre/BlockLegacyClientPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\ByteCounterFilter' => $baseDir . '/../lib/Connector/Sabre/ByteCounterFilter.php', 'OCA\\DAV\\Connector\\Sabre\\CachingTree' => $baseDir . '/../lib/Connector/Sabre/CachingTree.php', 'OCA\\DAV\\Connector\\Sabre\\ChecksumList' => $baseDir . '/../lib/Connector/Sabre/ChecksumList.php', 'OCA\\DAV\\Connector\\Sabre\\ChecksumUpdatePlugin' => $baseDir . '/../lib/Connector/Sabre/ChecksumUpdatePlugin.php', @@ -253,6 +254,7 @@ 'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => $baseDir . '/../lib/Connector/Sabre/ShareTypeList.php', 'OCA\\DAV\\Connector\\Sabre\\ShareeList' => $baseDir . '/../lib/Connector/Sabre/ShareeList.php', 'OCA\\DAV\\Connector\\Sabre\\SharesPlugin' => $baseDir . '/../lib/Connector/Sabre/SharesPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\StreamByteCounter' => $baseDir . '/../lib/Connector/Sabre/StreamByteCounter.php', 'OCA\\DAV\\Connector\\Sabre\\TagList' => $baseDir . '/../lib/Connector/Sabre/TagList.php', 'OCA\\DAV\\Connector\\Sabre\\TagsPlugin' => $baseDir . '/../lib/Connector/Sabre/TagsPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\UserIdHeaderPlugin' => $baseDir . '/../lib/Connector/Sabre/UserIdHeaderPlugin.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index bdb965aab3ee0..c9d3aba8e0ba2 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -230,6 +230,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\Auth' => __DIR__ . '/..' . '/../lib/Connector/Sabre/Auth.php', 'OCA\\DAV\\Connector\\Sabre\\BearerAuth' => __DIR__ . '/..' . '/../lib/Connector/Sabre/BearerAuth.php', 'OCA\\DAV\\Connector\\Sabre\\BlockLegacyClientPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/BlockLegacyClientPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\ByteCounterFilter' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ByteCounterFilter.php', 'OCA\\DAV\\Connector\\Sabre\\CachingTree' => __DIR__ . '/..' . '/../lib/Connector/Sabre/CachingTree.php', 'OCA\\DAV\\Connector\\Sabre\\ChecksumList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ChecksumList.php', 'OCA\\DAV\\Connector\\Sabre\\ChecksumUpdatePlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ChecksumUpdatePlugin.php', @@ -268,6 +269,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\Connector\\Sabre\\ShareTypeList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ShareTypeList.php', 'OCA\\DAV\\Connector\\Sabre\\ShareeList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/ShareeList.php', 'OCA\\DAV\\Connector\\Sabre\\SharesPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/SharesPlugin.php', + 'OCA\\DAV\\Connector\\Sabre\\StreamByteCounter' => __DIR__ . '/..' . '/../lib/Connector/Sabre/StreamByteCounter.php', 'OCA\\DAV\\Connector\\Sabre\\TagList' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TagList.php', 'OCA\\DAV\\Connector\\Sabre\\TagsPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/TagsPlugin.php', 'OCA\\DAV\\Connector\\Sabre\\UserIdHeaderPlugin' => __DIR__ . '/..' . '/../lib/Connector/Sabre/UserIdHeaderPlugin.php', diff --git a/apps/dav/lib/Connector/Sabre/ByteCounterFilter.php b/apps/dav/lib/Connector/Sabre/ByteCounterFilter.php new file mode 100644 index 0000000000000..380a0f1108bbd --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/ByteCounterFilter.php @@ -0,0 +1,31 @@ +params['counter'] ?? null; + + while ($bucket = stream_bucket_make_writeable($in)) { + $length = $bucket->datalen; + $consumed += $length; + if ($counter instanceof StreamByteCounter) { + $counter->bytes += $length; + } + stream_bucket_append($out, $bucket); + } + + return PSFS_PASS_ON; + } +} diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index 65765df7c2266..40b5059f84231 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -110,6 +110,8 @@ public function createServer( $this->logger, $this->eventDispatcher, \OCP\Server::get(IDateTimeZone::class), + $this->config, + $this->l10n, )); // Some WebDAV clients do require Class 2 WebDAV support (locking), since diff --git a/apps/dav/lib/Connector/Sabre/StreamByteCounter.php b/apps/dav/lib/Connector/Sabre/StreamByteCounter.php new file mode 100644 index 0000000000000..37cae03df36e2 --- /dev/null +++ b/apps/dav/lib/Connector/Sabre/StreamByteCounter.php @@ -0,0 +1,19 @@ +reportMissingFiles = $this->config->getSystemValueBool('archive_report_missing_files', false); + + if ($this->reportMissingFiles) { + stream_filter_register('count.bytes', ByteCounterFilter::class); + } + + $this->l10n = $this->l10nFactory->get('dav'); } /** @@ -63,27 +78,77 @@ public function initialize(Server $server): void { /** * Adding a node to the archive streamer. - * This will recursively add new nodes to the stream if the node is a directory. + * @return ?string an error message if an error occurred and reporting is enabled, null otherwise */ - protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath): void { + protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath): ?string { // Remove the root path from the filename to make it relative to the requested folder $filename = str_replace($rootPath, '', $node->getPath()); $mtime = $node->getMTime(); + if ($node instanceof NcFolder) { + $streamer->addEmptyDir($filename, $mtime); + return null; + } + if ($node instanceof NcFile) { - $resource = $node->fopen('rb'); - if ($resource === false) { - $this->logger->info('Cannot read file for zip stream', ['filePath' => $node->getPath()]); - throw new \Sabre\DAV\Exception\ServiceUnavailable('Requested file can currently not be accessed.'); + $nodeSize = $node->getSize(); + try { + $stream = $node->fopen('rb'); + } catch (\Exception $e) { + // opening failed, log the failure as reason for the missing file + if ($this->reportMissingFiles) { + $exceptionClass = get_class($e); + return $this->l10n->t('Error while opening the file: %s', [$exceptionClass]); + } + + throw $e; } - $streamer->addFileFromStream($resource, $filename, $node->getSize(), $mtime); - } elseif ($node instanceof NcFolder) { - $streamer->addEmptyDir($filename, $mtime); - $content = $node->getDirectoryListing(); - foreach ($content as $subNode) { - $this->streamNode($streamer, $subNode, $rootPath); + + if ($this->reportMissingFiles) { + if ($stream === false) { + return $this->l10n->t('File could not be opened (fopen). Please check the server logs for more information.'); + } + + $byteCounter = new StreamByteCounter(); + $wrapped = stream_filter_append($stream, 'count.bytes', STREAM_FILTER_READ, ['counter' => $byteCounter]); + if ($wrapped === false) { + return $this->l10n->t('Unable to check file for consistency check'); + } + } + + $fileAddedToStream = $streamer->addFileFromStream($stream, $filename, $nodeSize, $mtime); + if ($this->reportMissingFiles) { + if (!$fileAddedToStream) { + return $this->l10n->t('The archive was already finalized'); + } + + return $this->logStreamErrors($stream, $filename, $nodeSize, $byteCounter->bytes); } + + return null; + } + } + + /** + * Checks whether $stream was fully streamed or if there were other issues + * with the stream, logging the error if necessary. + * + */ + private function logStreamErrors(mixed $stream, string $path, float|int $expectedFileSize, float|int $readFileSize): ?string { + $streamMetadata = stream_get_meta_data($stream); + if (!is_resource($stream) || get_resource_type($stream) !== 'stream') { + return $this->l10n->t('Resource is not a stream or is closed.'); + } + + if ($streamMetadata['timed_out'] ?? false) { + return $this->l10n->t('Timeout while reading from stream.'); } + + if (!($streamMetadata['eof'] ?? true) || $readFileSize != $expectedFileSize) { + return $this->l10n->t('Read %d out of %d bytes from storage. This means the connection may have been closed due to a network/storage error.', [$expectedFileSize, $readFileSize]); + } + + return null; } /** @@ -137,7 +202,7 @@ public function handleDownload(Request $request, Response $response): ?bool { } $folder = $node->getNode(); - $event = new BeforeZipCreatedEvent($folder, $files); + $event = new BeforeZipCreatedEvent($folder, $files, $this->reportMissingFiles); $this->eventDispatcher->dispatchTyped($event); if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) { $errorMessage = $event->getErrorMessage(); @@ -150,12 +215,16 @@ public function handleDownload(Request $request, Response $response): ?bool { throw new Forbidden($errorMessage); } + // At this point either the event handlers did not block the download + // or they support the new mechanism that filters out nodes that are not + // downloadable, in either case we can use the new API to set the iterator $content = empty($files) ? $folder->getDirectoryListing() : []; foreach ($files as $path) { $child = $node->getChild($path); assert($child instanceof Node); $content[] = $child->getNode(); } + $event->setNodesIterable($this->getIterableFromNodes($content)); $archiveName = $folder->getName(); if (count(explode('/', trim($folder->getPath(), '/'), 3)) === 2) { @@ -169,31 +238,71 @@ public function handleDownload(Request $request, Response $response): ?bool { $rootPath = dirname($folder->getPath()); } - $streamer = new Streamer($tarRequest, -1, count($content), $this->timezoneFactory); + // numberOfFiles is irrelevant as size=-1 forces the use of zip64 already + $streamer = new Streamer($tarRequest, -1, 0, $this->timezoneFactory); $streamer->sendHeaders($archiveName); // For full folder downloads we also add the folder itself to the archive if (empty($files)) { $streamer->addEmptyDir($archiveName); } - foreach ($content as $node) { - $this->streamNode($streamer, $node, $rootPath); + + foreach ($event->getNodes() as $path => [$node, $reason]) { + $filename = str_replace($rootPath, '', $path); + if ($node === null) { + if ($this->reportMissingFiles) { + $this->missingInfo[$filename] = $reason; + } + continue; + } + + $streamError = $this->streamNode($streamer, $node, $rootPath); + if ($this->reportMissingFiles && $streamError !== null) { + $this->missingInfo[$filename] = $streamError; + } + } + + if ($this->reportMissingFiles && !empty($this->missingInfo)) { + $json = json_encode($this->missingInfo, JSON_UNESCAPED_SLASHES | JSON_PRETTY_PRINT); + $stream = fopen('php://temp', 'r+'); + fwrite($stream, $json); + rewind($stream); + $streamer->addFileFromStream($stream, 'missing_files.json', (float)strlen($json), false); } $streamer->finalize(); return false; } /** - * Tell sabre/dav not to trigger it's own response sending logic as the handleDownload will have already send the response + * Given a set of nodes, produces a list of all nodes contained in them + * recursively. + * + * @param NcNode[] $nodes + * @return iterable + */ + private function getIterableFromNodes(array $nodes): iterable { + foreach ($nodes as $node) { + yield $node; + + if ($node instanceof NcFolder) { + foreach ($node->getDirectoryListing() as $child) { + yield from $this->getIterableFromNodes([$child]); + } + } + } + } + + /** + * Tell sabre/dav not to trigger its own response sending logic as the handleDownload will have already send the response * * @return false|null */ public function afterDownload(Request $request, Response $response): ?bool { $node = $this->tree->getNodeForPath($request->getPath()); - if (!($node instanceof Directory)) { + if ($node instanceof Directory) { // only handle directories - return null; - } else { return false; } + + return null; } } diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index a57526f1faca2..43002ab4cb6c7 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -8,6 +8,7 @@ namespace OCA\DAV; use OC\Files\Filesystem; +use OC\L10N\L10N; use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\BulkUpload\BulkUploadPlugin; use OCA\DAV\CalDAV\BirthdayCalendar\EnablePlugin; @@ -87,12 +88,14 @@ use OCP\IDateTimeZone; use OCP\IDBConnection; use OCP\IGroupManager; +use OCP\IL10N; use OCP\IPreview; use OCP\IRequest; use OCP\ISession; use OCP\ITagManager; use OCP\IURLGenerator; use OCP\IUserSession; +use OCP\L10N\IFactory; use OCP\Mail\IEmailValidator; use OCP\Mail\IMailer; use OCP\Profiler\IProfiler; @@ -242,6 +245,7 @@ public function __construct( \OCP\Server::get(IUserSession::class) )); + $config = \OCP\Server::get(IConfig::class); // performance improvement plugins $this->server->addPlugin(new CopyEtagHeaderPlugin()); $this->server->addPlugin(new RequestIdHeaderPlugin(\OCP\Server::get(IRequest::class))); @@ -254,6 +258,8 @@ public function __construct( $logger, $eventDispatcher, \OCP\Server::get(IDateTimeZone::class), + $config, + \OCP\Server::get(IFactory::class), )); $this->server->addPlugin(\OCP\Server::get(PaginatePlugin::class)); $this->server->addPlugin(new PropFindPreloadNotifyPlugin()); diff --git a/apps/dav/tests/unit/Connector/Sabre/ZipFolderPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/ZipFolderPluginTest.php new file mode 100644 index 0000000000000..04174ed89fc03 --- /dev/null +++ b/apps/dav/tests/unit/Connector/Sabre/ZipFolderPluginTest.php @@ -0,0 +1,322 @@ +tree = $this->createMock(Tree::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->timezoneFactory = $this->createMock(IDateTimeZone::class); + $this->config = $this->createMock(IConfig::class); + $this->response = $this->createMock(Response::class); + $this->l10n = $this->createMock(IL10N::class); + $this->l10n->method('t')->willReturnCallback(static fn (string $text, array $parameters = []): string => vsprintf($text, $parameters)); + $this->l10nFactory = $this->createMock(IFactory::class); + $this->l10nFactory->method('get')->willReturn($this->l10n); + } + + public static function dataDownloadingABlockedFolderShouldFail(): array { + return [ + 'missing files reporting feature off' => [ false ], + 'missing files reporting feature on' => [ true ], + ]; + } + + /* + * Tests that the plugin throws a Forbidden exception when the user is trying + * to download a collection they have no access to. + */ + #[DataProvider(methodName: 'dataDownloadingABlockedFolderShouldFail')] + public function testDownloadingABlockedFolderShouldFail(bool $reportMissingFiles): void { + $plugin = $this->createPlugin($reportMissingFiles); + $folderPath = '/user/files/folder'; + $folder = $this->createFolderNode($folderPath, []); + $directory = $this->createDirectoryNode($folder); + + $this->tree->expects($this->once()) + ->method('getNodeForPath') + ->with($folderPath) + ->willReturn($directory); + + $errorMessage = 'Blocked by ACL'; + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->willReturnCallback(function (BeforeZipCreatedEvent $event) use ($reportMissingFiles, $errorMessage): BeforeZipCreatedEvent { + $this->assertSame([], $event->getFiles()); + $this->assertEquals($reportMissingFiles, $event->allowPartialArchive); + $event->setSuccessful(false); + $event->setErrorMessage($errorMessage); + + return $event; + }); + + $this->expectException(Forbidden::class); + $this->expectExceptionMessage($errorMessage); + $directory->expects($this->never())->method('getChild'); + $folder->expects($this->never())->method('getDirectoryListing'); + + $plugin->handleDownload($this->createRequest($folderPath), $this->response); + } + + public static function dataDownloadingAFolderShouldFailWhenItemsAreBlocked(): array { + return [ + 'no files filtering' => [ [] ], + 'files filtering' => [ ['allowed.txt', 'blocked.txt'] ], + ]; + } + + /* + * Tests that when `archive_report_missing_files` is disabled, downloading + * a directory which contains a non-downloadable item stops the entire + * download. + */ + #[DataProvider(methodName: 'dataDownloadingAFolderShouldFailWhenItemsAreBlocked')] + public function testDownloadingAFolderShouldFailWhenItemsAreBlocked(array $filesFilter): void { + $plugin = $this->createPlugin(false); + $folderPath = '/user/files/folder'; + $allowedFile = $this->createFile("{$folderPath}/allowed.txt", 'allowed'); + $blockedFile = $this->createFile("{$folderPath}/blocked.txt", 'secret'); + $files = [$allowedFile, $blockedFile]; + $childNodes = [ + 'allowed.txt' => $this->createNode($allowedFile), + 'blocked.txt' => $this->createNode($blockedFile), + ]; + + $folder = $this->createFolderNode($folderPath, $files); + $directory = $this->createDirectoryNode($folder, $childNodes); + + $this->tree->expects($this->once()) + ->method('getNodeForPath') + ->with($folderPath) + ->willReturn($directory); + + $errorMessage = 'Blocked by ACL'; + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->willReturnCallback(function (BeforeZipCreatedEvent $event) use ($errorMessage, $filesFilter): BeforeZipCreatedEvent { + $this->assertSame($filesFilter, $event->getFiles()); + $this->assertFalse($event->allowPartialArchive); + $event->setSuccessful(false); + $event->setErrorMessage($errorMessage); + + return $event; + }); + + $this->expectException(Forbidden::class); + $this->expectExceptionMessage($errorMessage); + + $plugin->handleDownload($this->createRequest($folderPath, $filesFilter), $this->response); + } + + + public static function dataDownloadingAFolderWithMissingFilesReportingShouldSucceed(): array { + return [ + // files are reporting as missing either because they are download-blocked or because some error happened + 'full directory download' => [ + 'children' => ['allowed.txt' => 'allowed', 'blocked.txt' => 'blocked', 'error.txt' => new \RuntimeException('read error')], + 'filesFilter' => [], + 'downloadBlocked' => [ 'blocked.txt' ], + 'expectedMissingFiles' => [ 'blocked.txt' => 'blocked', 'error.txt' => 'Error while opening the file: RuntimeException' ], + ], + // files filtered out should not be reported as missing + 'filtering some files' => [ + 'children' => [ 'allowed.txt' => 'allowed', 'blocked.txt' => 'blocked', 'error.txt' => new \RuntimeException('read error') ], + 'filesFilter' => ['allowed.txt', 'blocked.txt'], + 'downloadBlocked' => ['blocked.txt'], + 'expectedMissingFiles' => [ 'blocked.txt' => 'blocked' ], + ], + ]; + } + + /* + * Tests that when files in a directory cannot be downloaded and the + * `archive_report_missing_files` is enabled, an entry is added to the + * `missing_files.json` file. + */ + #[DataProvider(methodName: 'dataDownloadingAFolderWithMissingFilesReportingShouldSucceed')] + public function testDownloadingAFolderWithMissingFilesReportingShouldSucceed(array $children, array $filesFilter, array $downloadBlocked, array $expectedMissingFiles): void { + $plugin = $this->createPlugin(true); + + $folderPath = '/user/files/folder'; + $childFiles = []; + $childNodes = []; + foreach ($children as $childName => $content) { + $childFile = $this->createFile("{$folderPath}/{$childName}", $content); + $childFiles[$childName] = $childFile; + $childNodes[$childName] = $this->createNode($childFile); + } + + $folder = $this->createFolderNode($folderPath, array_values($childFiles)); + $directory = $this->createDirectoryNode($folder, $childNodes); + + $this->tree->expects($this->once()) + ->method('getNodeForPath') + ->with($folderPath) + ->willReturn($directory); + + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->willReturnCallback(function (BeforeZipCreatedEvent $event) use ($downloadBlocked, $filesFilter): BeforeZipCreatedEvent { + $this->assertSame($filesFilter, $event->getFiles()); + $this->assertTrue($event->allowPartialArchive); + $event->addNodeFilter(static function ($node) use ($downloadBlocked): array { + if (in_array($node->getName(), $downloadBlocked)) { + return [false, 'blocked']; + } + + return [true, null]; + }); + + return $event; + }); + + ob_start(); + $request = $this->createRequest($folderPath, $filesFilter); + $continueHandling = $plugin->handleDownload($request, $this->response); + + $output = $this->getActualOutputForAssertion(); + $this->assertStringContainsString('missing_files.json', $output, "$output does not contain missin_files.json"); + foreach ($expectedMissingFiles as $file => $error) { + $stringToMatch = sprintf('%s": "%s"', $file, $error); + $this->assertStringContainsString($stringToMatch, $output, "$output does not contain $stringToMatch"); + } + + // assert that the handling should be stopped + $this->assertFalse($continueHandling); + } + + + private function createPlugin(bool $reportMissingFiles): ZipFolderPlugin { + $this->config->method('getSystemValueBool') + ->with('archive_report_missing_files', false) + ->willReturn($reportMissingFiles); + + return new ZipFolderPlugin( + $this->tree, + $this->logger, + $this->eventDispatcher, + $this->timezoneFactory, + $this->config, + $this->l10nFactory, + ); + } + + /** + * @param list $filesFilter + * @throws \JsonException + */ + private function createRequest(string $resource, array $filesFilter = []): Request&MockObject { + $query = []; + if ($filesFilter !== []) { + $query['files'] = json_encode($filesFilter, JSON_THROW_ON_ERROR); + } + + $request = $this->createMock(Request::class); + $request->method('getPath')->willReturn($resource); + $request->method('getQueryParameters')->willReturn($query); + + // file filtering can be done via header or QS parameters. Use only one. + $request->method('getHeaderAsArray')->willReturnMap([ + ['Accept', ['application/zip']], + ['X-NC-Files', []], + ]); + + return $request; + } + + /** + * @param list $children + * @param array $childNodes + */ + private function createDirectoryNode(Folder $folder, array $childNodes = []): Directory&MockObject { + $directory = $this->createMock(Directory::class); + $directory->method('getNode')->willReturn($folder); + $directory->method('getChild')->willReturnCallback(static fn (string $name, ...$_): Node => $childNodes[$name]); + + return $directory; + } + + /** + * @param list $children + */ + private function createFolderNode(string $path, array $children): Folder&MockObject { + $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn($path); + $folder->method('getName')->willReturn(basename($path)); + $folder->method('getDirectoryListing')->willReturn($children); + + return $folder; + } + + private function createNode(File $node): Node&MockObject { + $child = $this->createMock(Node::class); + $child->method('getNode')->willReturn($node); + $child->method('getPath')->willReturn($node->getPath()); + + return $child; + } + + private function createFile(string $path, string|Exception $contents): File&MockObject { + $length = is_string($contents) ? strlen($contents) : 0; + $file = $this->createMock(File::class); + $file->method('getPath')->willReturn($path); + $file->method('getName')->willReturn(basename($path)); + $file->method('getSize')->willReturn($length); + $file->method('getMTime')->willReturn(123); + $file->method('fopen')->with('rb')->willReturnCallback(static function () use ($contents) { + if (!is_string($contents)) { + throw $contents; + } + + $stream = fopen('php://temp', 'r+'); + fwrite($stream, $contents); + rewind($stream); + + return $stream; + }); + + return $file; + } +} diff --git a/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php b/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php index 717edd4869ebd..45ee0c3260571 100644 --- a/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php +++ b/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php @@ -24,6 +24,7 @@ class BeforeDirectFileDownloadListener implements IEventListener { public function __construct( private IUserSession $userSession, private IRootFolder $rootFolder, + private ViewOnly $viewOnly, ) { } @@ -32,17 +33,17 @@ public function handle(Event $event): void { return; } - $pathsToCheck = [$event->getPath()]; - // Check only for user/group shares. Don't restrict e.g. share links $user = $this->userSession->getUser(); - if ($user) { - $viewOnlyHandler = new ViewOnly( - $this->rootFolder->getUserFolder($user->getUID()) - ); - if (!$viewOnlyHandler->check($pathsToCheck)) { - $event->setSuccessful(false); - $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); - } + // Check only for user/group shares. Don't restrict e.g. share links + if (!$user) { + return; + + } + $userFolder = $this->rootFolder->getUserFolder($user->getUID()); + $node = $userFolder->get($event->getPath()); + if (!$this->viewOnly->isDownloadable($node)) { + $event->setSuccessful(false); + $event->setErrorMessage('Access to this resource has been denied.'); } } } diff --git a/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php index e638a088fbe1b..b6dfa24563396 100644 --- a/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php +++ b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php @@ -14,6 +14,7 @@ use OCP\EventDispatcher\IEventListener; use OCP\Files\Events\BeforeZipCreatedEvent; use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\IUserSession; /** @@ -47,18 +48,36 @@ public function handle(Event $event): void { // Check only for user/group shares. Don't restrict e.g. share links $user = $this->userSession->getUser(); - if ($user) { - $viewOnlyHandler = new ViewOnly( - $this->rootFolder->getUserFolder($user->getUID()) - ); - if (!$viewOnlyHandler->check($pathsToCheck)) { - $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); - $event->setSuccessful(false); - } else { - $event->setSuccessful(true); - } - } else { + if (!$user) { $event->setSuccessful(true); + return; + } + + $userFolder = $this->rootFolder->getUserFolder($user->getUID()); + $viewOnlyHandler = new ViewOnly($userFolder); + + $node = $userFolder->get($dir); + $isRootDownloadable = $viewOnlyHandler->isDownloadable($node); + + if (!$isRootDownloadable) { + $message = $event->allowPartialArchive ? 'Access to this resource and its children has been denied.' : 'Access to this resource or one of its sub-items has been denied.'; + $event->setErrorMessage($message); + $event->setSuccessful(false); + return; + } + + if ($event->allowPartialArchive) { + $event->setSuccessful(true); + $event->addNodeFilter(fn (Node $node): array => [ + $viewOnlyHandler->isDownloadable($node), + 'Download is disabled for this resource' + ]); + } elseif ($viewOnlyHandler->check($pathsToCheck)) { + // keep the old behaviour + $event->setSuccessful(true); + } else { + $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); + $event->setSuccessful(false); } } } diff --git a/apps/files_sharing/lib/ViewOnly.php b/apps/files_sharing/lib/ViewOnly.php index 382eb190a8936..123d3b67b9c77 100644 --- a/apps/files_sharing/lib/ViewOnly.php +++ b/apps/files_sharing/lib/ViewOnly.php @@ -34,7 +34,7 @@ public function check(array $pathsToCheck): bool { $info = $this->userFolder->get($file); if ($info instanceof File) { // access to filecache is expensive in the loop - if (!$this->checkFileInfo($info)) { + if (!$this->isDownloadable($info)) { return false; } } elseif ($info instanceof Folder) { @@ -43,7 +43,7 @@ public function check(array $pathsToCheck): bool { return false; } } - } catch (NotFoundException $e) { + } catch (NotFoundException) { continue; } } @@ -56,14 +56,14 @@ public function check(array $pathsToCheck): bool { * @throws NotFoundException */ private function dirRecursiveCheck(Folder $dirInfo): bool { - if (!$this->checkFileInfo($dirInfo)) { + if (!$this->isDownloadable($dirInfo)) { return false; } // If any of elements cannot be downloaded, prevent whole download $files = $dirInfo->getDirectoryListing(); foreach ($files as $file) { if ($file instanceof File) { - if (!$this->checkFileInfo($file)) { + if (!$this->isDownloadable($file)) { return false; } } elseif ($file instanceof Folder) { @@ -77,11 +77,15 @@ private function dirRecursiveCheck(Folder $dirInfo): bool { /** * @param Node $fileInfo * @return bool - * @throws NotFoundException */ - private function checkFileInfo(Node $fileInfo): bool { + public function isDownloadable(Node $fileInfo): bool { // Restrict view-only to nodes which are shared - $storage = $fileInfo->getStorage(); + try { + $storage = $fileInfo->getStorage(); + } catch (NotFoundException) { + return true; + } + if (!$storage->instanceOfStorage(SharedStorage::class)) { return true; } diff --git a/apps/files_sharing/tests/ApplicationTest.php b/apps/files_sharing/tests/ApplicationTest.php index a6ff620722a5d..1277cbfa93f9a 100644 --- a/apps/files_sharing/tests/ApplicationTest.php +++ b/apps/files_sharing/tests/ApplicationTest.php @@ -13,6 +13,7 @@ use OCA\Files_Sharing\Listener\BeforeDirectFileDownloadListener; use OCA\Files_Sharing\Listener\BeforeZipCreatedListener; use OCA\Files_Sharing\SharedStorage; +use OCA\Files_Sharing\ViewOnly; use OCP\Files\Events\BeforeDirectFileDownloadEvent; use OCP\Files\Events\BeforeZipCreatedEvent; use OCP\Files\File; @@ -91,7 +92,8 @@ public function testCheckDirectCanBeDownloaded( $event = new BeforeDirectFileDownloadEvent($path); $listener = new BeforeDirectFileDownloadListener( $this->userSession, - $this->rootFolder + $this->rootFolder, + new ViewOnly(), ); $listener->handle($event); @@ -162,6 +164,13 @@ function (string $fileStorage) use ($nonSharedStorage, $secureSharedStorage) { $folder->method('getDirectoryListing')->willReturn($directoryListing); } + // If the folder contains any secure-shared files, make it appear as a secure-shared folder + // so that ViewOnly::isDownloadable() will return false + $containsSecureSharedFiles = in_array('secureSharedStorage', $directoryListing); + if ($containsSecureSharedFiles && $folderStorage === 'nonSharedStorage') { + $folder->method('getStorage')->willReturn($secureSharedStorage); + } + $rootFolder = $this->createMock(Folder::class); $rootFolder->method('getStorage')->willReturn($nonSharedStorage); $rootFolder->method('getDirectoryListing')->willReturn([$folder]); @@ -176,11 +185,12 @@ function (string $fileStorage) use ($nonSharedStorage, $secureSharedStorage) { $this->rootFolder->method('getUserFolder')->with('test')->willReturn($userFolder); - // Simulate zip download of folder folder - $event = new BeforeZipCreatedEvent($dir, $files); + // Simulate zip download of folder + $event = new BeforeZipCreatedEvent($folder, $files); + $listener = new BeforeZipCreatedListener( $this->userSession, - $this->rootFolder + $this->rootFolder, ); $listener->handle($event); @@ -192,10 +202,10 @@ public function testCheckFileUserNotFound(): void { $this->userSession->method('isLoggedIn')->willReturn(false); // Simulate zip download of folder folder - $event = new BeforeZipCreatedEvent('/test', ['test.txt']); + $event = new BeforeZipCreatedEvent('/test', ['test.txt'], []); $listener = new BeforeZipCreatedListener( $this->userSession, - $this->rootFolder + $this->rootFolder, ); $listener->handle($event); diff --git a/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php b/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php new file mode 100644 index 0000000000000..17227e3782868 --- /dev/null +++ b/apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php @@ -0,0 +1,265 @@ +userSession = $this->createMock(IUserSession::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->userFolder = $this->createMock(Folder::class); + $this->listener = new BeforeZipCreatedListener($this->userSession, $this->rootFolder); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user'); + $this->userSession->method('getUser')->willReturn($user); + $this->rootFolder->method('getUserFolder')->with('user')->willReturn($this->userFolder); + } + + public static function dataHandle(): array { + $rootFromFolder = '/folder'; + // files are relative to $folderPath + // filesFilter are relative to $folderPath but without leading / + // expectedNodeList are ... + return [ + 'partial archive disabled, no filtering, 1 blocked file => should fail event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['blocked.txt' => false], + 'filesFilter' => [], + 'allowPartialArchive' => false, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource or one of its sub-items has been denied.', + 'expectedNodeList' => [], + ], + 'partial archive disabled, no filtering, 1 blocked 1 non-blocked file => should fail event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ["blocked.txt" => false, "allowed.txt" => true], + 'filesFilter' => [], + 'allowPartialArchive' => false, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource or one of its sub-items has been denied.', + 'expectedNodeList' => [], + ], + 'partial archive enabled, no filtering, 1 blocked file => should not fail event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['blocked.txt' => false], + 'filesFilter' => [], + 'allowPartialArchive' => true, + 'expectedSuccess' => true, + 'expectedMessage' => null, + 'expectedNodeList' => ['blocked.txt' => [null, 'Download is disabled for this resource']], + ], + 'partial archive enabled, no filtering, 1 blocked 1 non-blocked file => should not fail event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['blocked.txt' => false, 'allowed.txt' => true], + 'filesFilter' => [], + 'allowPartialArchive' => true, + 'expectedSuccess' => true, + 'expectedMessage' => null, + 'expectedNodeList' => ['blocked.txt' => [null, 'Download is disabled for this resource'], 'allowed.txt' => null], + ], + 'partial archive disabled, with filtering, 1 blocked 2 non-blocked files => should fail event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['blocked.txt' => false, 'allowed.txt' => true, 'notinfilter.txt' => true], + 'filesFilter' => ['blocked.txt', 'allowed.txt'], + 'allowPartialArchive' => false, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource or one of its sub-items has been denied.', + 'expectedNodeList' => [], + ], + 'partial archive enabled, with filtering, 1 blocked 2 non-blocked files => should not fail event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['blocked.txt' => false, 'allowed.txt' => true, 'notinfilter.txt' => true], + 'filesFilter' => ['blocked.txt', 'allowed.txt'], + 'allowPartialArchive' => true, + 'expectedSuccess' => true, + 'expectedMessage' => null, + 'expectedNodeList' => ['blocked.txt' => [null, 'Download is disabled for this resource'], 'allowed.txt' => null], + ], + 'partial archive disabled, with filtering on non-blocked file, 1 blocked 1 non-blocked file => should succeed event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['allowed.txt' => true, 'notinfilter.txt' => false], + 'filesFilter' => ['allowed.txt'], + 'allowPartialArchive' => false, + 'expectedSuccess' => true, + 'expectedMessage' => null, + 'expectedNodeList' => ['allowed.txt' => null], + ], + 'partial archive enabled, with filtering on non-blocked file, 1 downloadable 1 non-blocked file => should succeed event' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => true, + 'files' => ['allowed.txt' => true, 'notinfilter.txt' => false], + 'filesFilter' => ['allowed.txt'], + 'allowPartialArchive' => true, + 'expectedSuccess' => true, + 'expectedMessage' => null, + 'expectedNodeList' => ['allowed.txt' => null], + ], + 'partial archive disabled, root (containing) folder not downloadable, with filtering' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => false, + 'files' => ['allowed.txt' => true, 'notinfilter.txt' => false], + 'filesFilter' => ['allowed.txt'], + 'allowPartialArchive' => false, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource or one of its sub-items has been denied.', + 'expectedNodeList' => [], + ], + 'partial archive enabled, root (containing) folder not downloadable, with filtering' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => false, + 'files' => ['allowed.txt' => true, 'notinfilter.txt' => false], + 'filesFilter' => ['allowed.txt'], + 'allowPartialArchive' => true, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource and its children has been denied.', + 'expectedNodeList' => [], + ], + 'partial archive disabled, root (containing) folder not downloadable, no filtering' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => false, + 'files' => ['allowed.txt' => true, 'notinfilter.txt' => false], + 'filesFilter' => [], + 'allowPartialArchive' => false, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource or one of its sub-items has been denied.', + 'expectedNodeList' => [], + ], + 'partial archive enabled, root (containing) folder not downloadable, no filtering' => [ + 'folderPath' => $rootFromFolder, + 'rootDownloadable' => false, + 'files' => ['allowed.txt' => true, 'notinfilter.txt' => false], + 'filesFilter' => [], + 'allowPartialArchive' => true, + 'expectedSuccess' => false, + 'expectedMessage' => 'Access to this resource and its children has been denied.', + 'expectedNodeList' => [], + ], + ]; + } + + #[DataProvider(methodName: 'dataHandle')] + public function testHandle( + string $folderPath, + bool $rootDownloadable, + array $files, + array $filesFilter, + bool $allowPartialArchive, + bool $expectedSuccess, + ?string $expectedMessage, + array $expectedNodeList, + ): void { + $fileNodes = []; + $fileNodesByName = []; + foreach ($files as $relativePath => $downloadable) { + $pathWithFolder = "{$folderPath}/{$relativePath}"; + $file = $this->createSharedFile($downloadable, $pathWithFolder); + $fileNodesByName[$pathWithFolder] = $file; + $fileNodes[] = $file; + } + $folderPathFromUserRoot = "/user/files{$folderPath}"; + $folder = $this->createSharedFolder($rootDownloadable, $folderPathFromUserRoot, $fileNodes); + $this->userFolder->method('get')->willReturnCallback(function (string $path) use ($folderPath, $fileNodesByName, $folder) { + return match (true) { + $path === $folderPath => $folder, + isset($fileNodesByName[$path]) => $fileNodesByName[$path], + default => throw new \RuntimeException("Mock node not set for {$path}"), + }; + }); + + $event = new BeforeZipCreatedEvent($folder, $filesFilter, $allowPartialArchive); + $this->listener->handle($event); + + $this->assertEquals($expectedSuccess, $event->isSuccessful()); + $this->assertSame($expectedMessage, $event->getErrorMessage()); + + $event->setNodesIterable($fileNodes); + $actualNodes = iterator_to_array($event->getNodes()); + $this->assertCount(count($expectedNodeList), $actualNodes); + foreach ($expectedNodeList as $relativePath => $expectedValue) { + $path = "{$folderPath}/{$relativePath}"; + if ($expectedValue === null) { + // cannot reference the node in the data provider, add it here + $node = $fileNodesByName[$path] ?? null; + $this->assertNotNull($node, 'Node mock must be present for the test to be correct.'); + $expectedValue = [$node, null]; + } + + $this->assertEquals($expectedValue, $actualNodes[$path]); + } + } + + private function createSharedFile(bool $downloadable, string $path): File&MockObject { + $file = $this->createMock(File::class); + $file->method('getStorage')->willReturn($this->createSharedStorage($downloadable)); + $file->method('getPath')->willReturn($path); + $file->method('getName')->willReturn(basename($path)); + + return $file; + } + + /** + * @param list $children + */ + private function createSharedFolder(bool $downloadable, string $path, array $children = []): Folder&MockObject { + $folder = $this->createMock(Folder::class); + $folder->method('getStorage')->willReturn($this->createSharedStorage($downloadable)); + $folder->method('getDirectoryListing')->willReturn($children); + $folder->method('getPath')->willReturn($path); + $folder->method('getName')->willReturn(basename($path)); + + return $folder; + } + + private function createSharedStorage(bool $downloadable): SharedStorage&MockObject { + $attributes = $this->createMock(IAttributes::class); + $attributes->method('getAttribute')->with('permissions', 'download')->willReturn($downloadable); + + $share = $this->createMock(IShare::class); + $share->method('getAttributes')->willReturn($attributes); + + $storage = $this->getMockBuilder(SharedStorage::class) + ->disableOriginalConstructor() + ->onlyMethods(['instanceOfStorage', 'getShare']) + ->getMock(); + $storage->method('instanceOfStorage')->with(SharedStorage::class)->willReturn(true); + $storage->method('getShare')->willReturn($share); + + return $storage; + } +} diff --git a/config/config.sample.php b/config/config.sample.php index f85a1da6a5be5..abbaadc512b91 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -387,6 +387,15 @@ */ 'session_keepalive' => true, + /** + * When enabled, this flag allows the download of a directory to proceed + * even if some files or subdirectories are marked as non-downloadable. + * If files are corrupted, missing, or the server encounter temporary errors + * they are logged in a `missing_files.json` within the archive, along with + * the cause. + */ + 'archive_report_missing_files' => false, + /** * Enable or disable the automatic logout after session_lifetime, even if session * keepalive is enabled. This will make sure that an inactive browser will log itself out diff --git a/lib/public/Files/Events/BeforeZipCreatedEvent.php b/lib/public/Files/Events/BeforeZipCreatedEvent.php index ea84da64a1d4b..6af2f58e7b66e 100644 --- a/lib/public/Files/Events/BeforeZipCreatedEvent.php +++ b/lib/public/Files/Events/BeforeZipCreatedEvent.php @@ -11,13 +11,19 @@ use OCP\EventDispatcher\Event; use OCP\Files\Folder; +use OCP\Files\Node; /** - * This event is triggered before a archive is created when a user requested + * This event is triggered before an archive is created when a user requested * downloading a folder or multiple files. * * By setting `successful` to false the tar creation can be aborted and the download denied. * + * If `allowPartialArchive` is set to true, the archive creation should be blocked only + * if access to the entire directory/all files is to be blocked. To block + * archiving of certain files only, `addNodeFilter` should be used to add a callable + * to filter out nodes. + * * @since 25.0.0 */ class BeforeZipCreatedEvent extends Event { @@ -25,16 +31,22 @@ class BeforeZipCreatedEvent extends Event { private bool $successful = true; private ?string $errorMessage = null; private ?Folder $folder = null; + /** @var iterable|null */ + private ?iterable $nodesIterable; + /** @var array */ + private array $nodeFilters = []; /** * @param string|Folder $directory Folder instance, or (deprecated) string path relative to user folder - * @param list $files + * @param list $files Selected files, empty for folder selection + * @param ?bool $allowPartialArchive True if missing/blocked files should not block the creation of the archive * @since 25.0.0 * @since 31.0.0 support `OCP\Files\Folder` as `$directory` parameter - passing a string is deprecated now */ public function __construct( string|Folder $directory, private array $files, + public ?bool $allowPartialArchive = true, ) { parent::__construct(); if ($directory instanceof Folder) { @@ -94,6 +106,65 @@ public function getErrorMessage(): ?string { return $this->errorMessage; } + /** + * Sets the iterable that will be used to yield nodes to be included in the + * archive. Nodes can be filtered out by adding filters via `addNodeFilter`. + * + * @param iterable $iterable + * @return void + */ + public function setNodesIterable(iterable $iterable): void { + $this->nodesIterable = $iterable; + } + + /** + * @param callable(Node): array{0: bool, 1: ?string} $filter filter that + * receives a Node and returns an array with a bool telling if the file is + * to be included in the archive and an optional reason string. + * + * @return void + */ + public function addNodeFilter(callable $filter): void { + $this->nodeFilters[] = $filter; + } + + /** + * Returns a generator yielding a string key with the node's path relative + * to the downloaded folder and an array which contains a node or null in + * the first position (indicating whether the node should be skipped) and a + * reason for skipping in the second position. + * + * @return iterable + */ + public function getNodes(): iterable { + if (!isset($this->nodesIterable)) { + throw new \LogicException('No nodes iterable set'); + } + + if (!$this->successful) { + return; + } + + $directory = $this->getDirectory(); + foreach ($this->nodesIterable as $node) { + $relativePath = $directory . '/' . $node->getName(); + if (!empty($this->files) && !in_array($node->getName(), $this->files)) { + // the node is supposed to be filtered out + continue; + } + + foreach ($this->nodeFilters as $filter) { + [$include, $reason] = $filter($node); + if (!$include) { + yield $relativePath => [null, $reason]; + continue 2; + } + } + + yield $relativePath => [$node, null]; + } + } + /** * @since 25.0.0 */