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..57c8a97cb20db 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); @@ -177,10 +179,10 @@ 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($dir, $files, []); $listener = new BeforeZipCreatedListener( $this->userSession, - $this->rootFolder + new ViewOnly(), ); $listener->handle($event); @@ -192,10 +194,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 */