From adfe20c67a0019a9f155c933c1f6de3b21d1cc99 Mon Sep 17 00:00:00 2001 From: Kostiantyn Miakshyn Date: Mon, 5 Jan 2026 01:08:47 +0100 Subject: [PATCH 1/2] feat(files_sharing): Improve user experience when user attempts to download folder with non-downloadable files Signed-off-by: Kostiantyn Miakshyn --- .../lib/Connector/Sabre/ZipFolderPlugin.php | 48 +++++++++---- .../BeforeDirectFileDownloadListener.php | 21 +++--- .../lib/Listener/BeforeZipCreatedListener.php | 40 ++++------- apps/files_sharing/lib/ViewOnly.php | 69 +------------------ apps/files_sharing/tests/ApplicationTest.php | 20 ++++-- .../Files/Events/BeforeZipCreatedEvent.php | 30 +++++++- 6 files changed, 103 insertions(+), 125 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php b/apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php index 1807ce1604756..c838a2620176c 100644 --- a/apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php +++ b/apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php @@ -61,9 +61,31 @@ public function initialize(Server $server): void { $this->server->on('afterMethod:GET', $this->afterDownload(...), 999); } + /** + * @return iterable + */ + protected function createIterator(array $rootNodes): iterable { + foreach ($rootNodes as $rootNode) { + yield from $this->iterateNodes($rootNode); + } + } + + /** + * Recursively iterate over all nodes in a folder. + * @return iterable + */ + protected function iterateNodes(NcNode $node): iterable { + yield $node; + + if ($node instanceof NcFolder) { + foreach ($node->getDirectoryListing() as $childNode) { + yield from $this->iterateNodes($childNode); + } + } + } + /** * Adding a node to the archive streamer. - * This will recursively add new nodes to the stream if the node is a directory. */ protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath): void { // Remove the root path from the filename to make it relative to the requested folder @@ -79,10 +101,6 @@ protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath $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); - } } } @@ -137,7 +155,14 @@ public function handleDownload(Request $request, Response $response): ?bool { } $folder = $node->getNode(); - $event = new BeforeZipCreatedEvent($folder, $files); + $rootNodes = empty($files) ? $folder->getDirectoryListing() : []; + foreach ($files as $path) { + $child = $node->getChild($path); + assert($child instanceof Node); + $rootNodes[] = $child->getNode(); + } + + $event = new BeforeZipCreatedEvent($folder, $files, $this->createIterator($rootNodes)); $this->eventDispatcher->dispatchTyped($event); if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) { $errorMessage = $event->getErrorMessage(); @@ -150,13 +175,6 @@ public function handleDownload(Request $request, Response $response): ?bool { throw new Forbidden($errorMessage); } - $content = empty($files) ? $folder->getDirectoryListing() : []; - foreach ($files as $path) { - $child = $node->getChild($path); - assert($child instanceof Node); - $content[] = $child->getNode(); - } - $archiveName = $folder->getName(); if (count(explode('/', trim($folder->getPath(), '/'), 3)) === 2) { // this is a download of the root folder @@ -169,13 +187,13 @@ public function handleDownload(Request $request, Response $response): ?bool { $rootPath = dirname($folder->getPath()); } - $streamer = new Streamer($tarRequest, -1, count($content), $this->timezoneFactory); + $streamer = new Streamer($tarRequest, -1, count($rootNodes), $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) { + foreach ($event->getNodes() as $node) { $this->streamNode($streamer, $node, $rootPath); } $streamer->finalize(); diff --git a/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php b/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php index 717edd4869ebd..a6d5dea90d3d2 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->isNodeCanBeDownloaded($node)) { + $event->setSuccessful(false); + $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); } } } diff --git a/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php index e638a088fbe1b..23e53d9a07021 100644 --- a/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php +++ b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php @@ -13,7 +13,7 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\Events\BeforeZipCreatedEvent; -use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\IUserSession; /** @@ -23,7 +23,7 @@ class BeforeZipCreatedListener implements IEventListener { public function __construct( private IUserSession $userSession, - private IRootFolder $rootFolder, + private ViewOnly $viewOnly, ) { } @@ -32,33 +32,19 @@ public function handle(Event $event): void { return; } - /** @psalm-suppress DeprecatedMethod should be migrated to getFolder but for now it would just duplicate code */ - $dir = $event->getDirectory(); - $files = $event->getFiles(); - - if (empty($files)) { - $pathsToCheck = [$dir]; - } else { - $pathsToCheck = []; - foreach ($files as $file) { - $pathsToCheck[] = $dir . '/' . $file; - } + $user = $this->userSession->getUser(); + if (!$user) { + return; } - // 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 { - $event->setSuccessful(true); + // Check whether the user can download the requested folder + if (!$this->viewOnly->isNodeCanBeDownloaded($event->getFolder())) { + $event->setSuccessful(false); + $event->setErrorMessage('Access to this resource has been denied.'); + return; } + + // Check recursively whether the user can download nested nodes + $event->addNodeFilter(fn (Node $node) => $this->viewOnly->isNodeCanBeDownloaded($node)); } } diff --git a/apps/files_sharing/lib/ViewOnly.php b/apps/files_sharing/lib/ViewOnly.php index 382eb190a8936..96edba94e297c 100644 --- a/apps/files_sharing/lib/ViewOnly.php +++ b/apps/files_sharing/lib/ViewOnly.php @@ -8,80 +8,15 @@ namespace OCA\Files_Sharing; -use OCP\Files\File; -use OCP\Files\Folder; use OCP\Files\Node; -use OCP\Files\NotFoundException; /** * Handles restricting for download of files */ class ViewOnly { - - public function __construct( - private Folder $userFolder, - ) { - } - - /** - * @param string[] $pathsToCheck paths to check, relative to the user folder - * @return bool - */ - public function check(array $pathsToCheck): bool { - // If any of elements cannot be downloaded, prevent whole download - foreach ($pathsToCheck as $file) { - try { - $info = $this->userFolder->get($file); - if ($info instanceof File) { - // access to filecache is expensive in the loop - if (!$this->checkFileInfo($info)) { - return false; - } - } elseif ($info instanceof Folder) { - // get directory content is rather cheap query - if (!$this->dirRecursiveCheck($info)) { - return false; - } - } - } catch (NotFoundException $e) { - continue; - } - } - return true; - } - - /** - * @param Folder $dirInfo - * @return bool - * @throws NotFoundException - */ - private function dirRecursiveCheck(Folder $dirInfo): bool { - if (!$this->checkFileInfo($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)) { - return false; - } - } elseif ($file instanceof Folder) { - return $this->dirRecursiveCheck($file); - } - } - - return true; - } - - /** - * @param Node $fileInfo - * @return bool - * @throws NotFoundException - */ - private function checkFileInfo(Node $fileInfo): bool { + public function isNodeCanBeDownloaded(Node $node): bool { // Restrict view-only to nodes which are shared - $storage = $fileInfo->getStorage(); + $storage = $node->getStorage(); 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..87ceb74562175 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); @@ -161,6 +163,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::isNodeCanBeDownloaded() 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); @@ -177,10 +186,11 @@ 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); + $event = new BeforeZipCreatedEvent($folder, $files, $directoryListing); + $listener = new BeforeZipCreatedListener( $this->userSession, - $this->rootFolder + new ViewOnly(), ); $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 + new ViewOnly(), ); $listener->handle($event); diff --git a/lib/public/Files/Events/BeforeZipCreatedEvent.php b/lib/public/Files/Events/BeforeZipCreatedEvent.php index ea84da64a1d4b..fceaa4f623f81 100644 --- a/lib/public/Files/Events/BeforeZipCreatedEvent.php +++ b/lib/public/Files/Events/BeforeZipCreatedEvent.php @@ -11,6 +11,7 @@ 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 @@ -25,16 +26,19 @@ class BeforeZipCreatedEvent extends Event { private bool $successful = true; private ?string $errorMessage = null; private ?Folder $folder = null; + 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 iterable $nodes Recursively collected nodes * @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, + private iterable $nodes, ) { parent::__construct(); if ($directory instanceof Folder) { @@ -70,6 +74,30 @@ public function getFiles(): array { return $this->files; } + /** + * @return iterable + */ + public function getNodes(): iterable { + foreach ($this->nodes as $node) { + $pass = true; + foreach ($this->nodeFilters as $filter) { + $pass = $pass && $filter($node); + } + + if ($pass) { + yield $node; + } + } + } + + /** + * @param callable $filter + * @return void + */ + public function addNodeFilter(callable $filter): void { + $this->nodeFilters[] = $filter; + } + /** * @since 25.0.0 */ From a783e8ec6d7d4258451842896440ebd5ef4a2226 Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Wed, 18 Mar 2026 17:38:29 +0100 Subject: [PATCH 2/2] fix: improve ZipFolderPlugin behaviour for different cases Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- .../composer/composer/autoload_classmap.php | 2 + .../dav/composer/composer/autoload_static.php | 2 + .../lib/Connector/Sabre/ByteCounterFilter.php | 31 ++ .../dav/lib/Connector/Sabre/ServerFactory.php | 2 + .../lib/Connector/Sabre/StreamByteCounter.php | 19 ++ .../lib/Connector/Sabre/ZipFolderPlugin.php | 177 +++++++--- apps/dav/lib/Server.php | 6 + .../Connector/Sabre/ZipFolderPluginTest.php | 322 ++++++++++++++++++ .../BeforeDirectFileDownloadListener.php | 4 +- .../lib/Listener/BeforeZipCreatedListener.php | 45 ++- apps/files_sharing/lib/ViewOnly.php | 73 +++- apps/files_sharing/tests/ApplicationTest.php | 12 +- .../Listener/BeforeZipCreatedListenerTest.php | 265 ++++++++++++++ config/config.sample.php | 9 + .../Files/Events/BeforeZipCreatedEvent.php | 97 ++++-- 15 files changed, 980 insertions(+), 86 deletions(-) create mode 100644 apps/dav/lib/Connector/Sabre/ByteCounterFilter.php create mode 100644 apps/dav/lib/Connector/Sabre/StreamByteCounter.php create mode 100644 apps/dav/tests/unit/Connector/Sabre/ZipFolderPluginTest.php create mode 100644 apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php 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'); } /** @@ -62,46 +77,78 @@ public function initialize(Server $server): void { } /** - * @return iterable + * Adding a node to the archive streamer. + * @return ?string an error message if an error occurred and reporting is enabled, null otherwise */ - protected function createIterator(array $rootNodes): iterable { - foreach ($rootNodes as $rootNode) { - yield from $this->iterateNodes($rootNode); + 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; } - } - /** - * Recursively iterate over all nodes in a folder. - * @return iterable - */ - protected function iterateNodes(NcNode $node): iterable { - yield $node; + if ($node instanceof NcFile) { + $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]); + } - if ($node instanceof NcFolder) { - foreach ($node->getDirectoryListing() as $childNode) { - yield from $this->iterateNodes($childNode); + throw $e; + } + + 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; } } /** - * Adding a node to the archive streamer. + * Checks whether $stream was fully streamed or if there were other issues + * with the stream, logging the error if necessary. + * */ - protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath): void { - // Remove the root path from the filename to make it relative to the requested folder - $filename = str_replace($rootPath, '', $node->getPath()); + 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.'); + } - $mtime = $node->getMTime(); - 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.'); - } - $streamer->addFileFromStream($resource, $filename, $node->getSize(), $mtime); - } elseif ($node instanceof NcFolder) { - $streamer->addEmptyDir($filename, $mtime); + 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; } /** @@ -155,14 +202,7 @@ public function handleDownload(Request $request, Response $response): ?bool { } $folder = $node->getNode(); - $rootNodes = empty($files) ? $folder->getDirectoryListing() : []; - foreach ($files as $path) { - $child = $node->getChild($path); - assert($child instanceof Node); - $rootNodes[] = $child->getNode(); - } - - $event = new BeforeZipCreatedEvent($folder, $files, $this->createIterator($rootNodes)); + $event = new BeforeZipCreatedEvent($folder, $files, $this->reportMissingFiles); $this->eventDispatcher->dispatchTyped($event); if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) { $errorMessage = $event->getErrorMessage(); @@ -175,6 +215,17 @@ 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) { // this is a download of the root folder @@ -187,31 +238,71 @@ public function handleDownload(Request $request, Response $response): ?bool { $rootPath = dirname($folder->getPath()); } - $streamer = new Streamer($tarRequest, -1, count($rootNodes), $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 ($event->getNodes() 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 a6d5dea90d3d2..45ee0c3260571 100644 --- a/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php +++ b/apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php @@ -41,9 +41,9 @@ public function handle(Event $event): void { } $userFolder = $this->rootFolder->getUserFolder($user->getUID()); $node = $userFolder->get($event->getPath()); - if (!$this->viewOnly->isNodeCanBeDownloaded($node)) { + if (!$this->viewOnly->isDownloadable($node)) { $event->setSuccessful(false); - $event->setErrorMessage('Access to this resource or one of its sub-items has been denied.'); + $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 23e53d9a07021..b6dfa24563396 100644 --- a/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php +++ b/apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php @@ -13,6 +13,7 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\Events\BeforeZipCreatedEvent; +use OCP\Files\IRootFolder; use OCP\Files\Node; use OCP\IUserSession; @@ -23,7 +24,7 @@ class BeforeZipCreatedListener implements IEventListener { public function __construct( private IUserSession $userSession, - private ViewOnly $viewOnly, + private IRootFolder $rootFolder, ) { } @@ -32,19 +33,51 @@ public function handle(Event $event): void { return; } + /** @psalm-suppress DeprecatedMethod should be migrated to getFolder but for now it would just duplicate code */ + $dir = $event->getDirectory(); + $files = $event->getFiles(); + + if (empty($files)) { + $pathsToCheck = [$dir]; + } else { + $pathsToCheck = []; + foreach ($files as $file) { + $pathsToCheck[] = $dir . '/' . $file; + } + } + + // Check only for user/group shares. Don't restrict e.g. share links $user = $this->userSession->getUser(); if (!$user) { + $event->setSuccessful(true); return; } - // Check whether the user can download the requested folder - if (!$this->viewOnly->isNodeCanBeDownloaded($event->getFolder())) { + $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); - $event->setErrorMessage('Access to this resource has been denied.'); return; } - // Check recursively whether the user can download nested nodes - $event->addNodeFilter(fn (Node $node) => $this->viewOnly->isNodeCanBeDownloaded($node)); + 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 96edba94e297c..123d3b67b9c77 100644 --- a/apps/files_sharing/lib/ViewOnly.php +++ b/apps/files_sharing/lib/ViewOnly.php @@ -8,15 +8,84 @@ namespace OCA\Files_Sharing; +use OCP\Files\File; +use OCP\Files\Folder; use OCP\Files\Node; +use OCP\Files\NotFoundException; /** * Handles restricting for download of files */ class ViewOnly { - public function isNodeCanBeDownloaded(Node $node): bool { + + public function __construct( + private Folder $userFolder, + ) { + } + + /** + * @param string[] $pathsToCheck paths to check, relative to the user folder + * @return bool + */ + public function check(array $pathsToCheck): bool { + // If any of elements cannot be downloaded, prevent whole download + foreach ($pathsToCheck as $file) { + try { + $info = $this->userFolder->get($file); + if ($info instanceof File) { + // access to filecache is expensive in the loop + if (!$this->isDownloadable($info)) { + return false; + } + } elseif ($info instanceof Folder) { + // get directory content is rather cheap query + if (!$this->dirRecursiveCheck($info)) { + return false; + } + } + } catch (NotFoundException) { + continue; + } + } + return true; + } + + /** + * @param Folder $dirInfo + * @return bool + * @throws NotFoundException + */ + private function dirRecursiveCheck(Folder $dirInfo): bool { + 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->isDownloadable($file)) { + return false; + } + } elseif ($file instanceof Folder) { + return $this->dirRecursiveCheck($file); + } + } + + return true; + } + + /** + * @param Node $fileInfo + * @return bool + */ + public function isDownloadable(Node $fileInfo): bool { // Restrict view-only to nodes which are shared - $storage = $node->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 87ceb74562175..1277cbfa93f9a 100644 --- a/apps/files_sharing/tests/ApplicationTest.php +++ b/apps/files_sharing/tests/ApplicationTest.php @@ -163,9 +163,9 @@ 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::isNodeCanBeDownloaded() will return false + // so that ViewOnly::isDownloadable() will return false $containsSecureSharedFiles = in_array('secureSharedStorage', $directoryListing); if ($containsSecureSharedFiles && $folderStorage === 'nonSharedStorage') { $folder->method('getStorage')->willReturn($secureSharedStorage); @@ -185,12 +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($folder, $files, $directoryListing); + // Simulate zip download of folder + $event = new BeforeZipCreatedEvent($folder, $files); $listener = new BeforeZipCreatedListener( $this->userSession, - new ViewOnly(), + $this->rootFolder, ); $listener->handle($event); @@ -205,7 +205,7 @@ public function testCheckFileUserNotFound(): void { $event = new BeforeZipCreatedEvent('/test', ['test.txt'], []); $listener = new BeforeZipCreatedListener( $this->userSession, - new ViewOnly(), + $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 fceaa4f623f81..6af2f58e7b66e 100644 --- a/lib/public/Files/Events/BeforeZipCreatedEvent.php +++ b/lib/public/Files/Events/BeforeZipCreatedEvent.php @@ -14,11 +14,16 @@ 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 { @@ -26,19 +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 Selected files, empty for folder selection - * @param iterable $nodes Recursively collected nodes + * @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, - private iterable $nodes, + public ?bool $allowPartialArchive = true, ) { parent::__construct(); if ($directory instanceof Folder) { @@ -74,30 +82,6 @@ public function getFiles(): array { return $this->files; } - /** - * @return iterable - */ - public function getNodes(): iterable { - foreach ($this->nodes as $node) { - $pass = true; - foreach ($this->nodeFilters as $filter) { - $pass = $pass && $filter($node); - } - - if ($pass) { - yield $node; - } - } - } - - /** - * @param callable $filter - * @return void - */ - public function addNodeFilter(callable $filter): void { - $this->nodeFilters[] = $filter; - } - /** * @since 25.0.0 */ @@ -122,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 */