feat(files_sharing): Improve user experience when user attempts to download folder with non-downloadable files (optimized approach using generators)

Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>

Author: Koc

apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php

@@ -61,8 +61,18 @@ public function initialize(Server $server): void {
 		$this->server->on('afterMethod:GET', $this->afterDownload(...), 999);
 	}
 
+	/**
+	 * @return iterable<NcNode>
+	 */
+	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<NcNode>
 	 */
 	protected function iterateNodes(NcNode $node): iterable {
 		yield $node;
@@ -151,14 +161,8 @@ public function handleDownload(Request $request, Response $response): ?bool {
 			assert($child instanceof Node);
 			$rootNodes[] = $child->getNode();
 		}
-		$allNodes = [];
-		foreach ($rootNodes as $rootNode) {
-			foreach ($this->iterateNodes($rootNode) as $node) {
-				$allNodes[] = $node;
-			}
-		}
 
-		$event = new BeforeZipCreatedEvent($folder, $files, $allNodes);
+		$event = new BeforeZipCreatedEvent($folder, $files, $this->createIterator($rootNodes));
 		$this->eventDispatcher->dispatchTyped($event);
 		if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) {
 			$errorMessage = $event->getErrorMessage();
@@ -170,7 +174,6 @@ public function handleDownload(Request $request, Response $response): ?bool {
 			// Downloading was denied by an app
 			throw new Forbidden($errorMessage);
 		}
-		$allNodes = $event->getNodes();
 
 		$archiveName = $folder->getName();
 		if (count(explode('/', trim($folder->getPath(), '/'), 3)) === 2) {
@@ -190,7 +193,7 @@ public function handleDownload(Request $request, Response $response): ?bool {
 		if (empty($files)) {
 			$streamer->addEmptyDir($archiveName);
 		}
-		foreach ($allNodes as $node) {
+		foreach ($event->getNodes() as $node) {
 			$this->streamNode($streamer, $node, $rootPath);
 		}
 		$streamer->finalize();

apps/files/lib/Controller/ConversionApiController.php

@@ -21,9 +21,7 @@
 use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
-use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Conversion\IConversionManager;
-use OCP\Files\Events\BeforeDirectFileDownloadEvent;
 use OCP\Files\File;
 use OCP\Files\GenericFileException;
 use OCP\Files\IRootFolder;
@@ -39,7 +37,6 @@ public function __construct(
 		private IRootFolder $rootFolder,
 		private IL10N $l10n,
 		private ?string $userId,
-		private IEventDispatcher $eventDispatcher,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -70,13 +67,6 @@ public function convert(int $fileId, string $targetMimeType, ?string $destinatio
 			throw new OCSNotFoundException($this->l10n->t('The file cannot be found'));
 		}
 
-		$event = new BeforeDirectFileDownloadEvent($userFolder->getRelativePath($file->getPath()));
-		$this->eventDispatcher->dispatchTyped($event);
-
-		if ($event->isSuccessful() === false) {
-			throw new OCSForbiddenException('Permission denied to download file');
-		}
-
 		if ($destination !== null) {
 			$destination = PathHelper::normalizePath($destination);
 			$parentDir = dirname($destination);

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,6 @@ class BeforeZipCreatedListener implements IEventListener {
 
 	public function __construct(
 		private IUserSession $userSession,
-		private IRootFolder $rootFolder,
 		private ViewOnly $viewOnly,
 	) {
 	}
@@ -38,17 +37,14 @@ public function handle(Event $event): void {
 			return;
 		}
 
-		$userFolder = $this->rootFolder->getUserFolder($user->getUID());
 		// Check whether the user can download the requested folder
-		$folder = $userFolder->get(substr($event->getDirectory(), strlen($userFolder->getPath())));
-		if (!$this->viewOnly->isNodeCanBeDownloaded($folder)) {
+		if (!$this->viewOnly->isNodeCanBeDownloaded($event->getFolder())) {
 			$event->setSuccessful(false);
 			$event->setErrorMessage('Access to this resource has been denied.');
 			return;
 		}
 
-		$nodes = array_filter($event->getNodes(), fn ($node) => $this->viewOnly->isNodeCanBeDownloaded($node));
-		$event->setNodes(array_values($nodes));
-		$event->setSuccessful(true);
+		// Check recursively whether the user can download nested nodes
+		$event->addNodeFilter(fn (Node $node) => $this->viewOnly->isNodeCanBeDownloaded($node));
 	}
 }

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);
 

lib/public/Files/Events/BeforeZipCreatedEvent.php

@@ -26,19 +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<string> $files
 	 * @param list<string> $files Selected files, empty for folder selection
-	 * @param list<Node> $nodes Recursively collected nodes
+	 * @param iterable<Node> $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 array $nodes = [],
+		private iterable $nodes,
 	) {
 		parent::__construct();
 		if ($directory instanceof Folder) {
@@ -75,17 +75,27 @@ public function getFiles(): array {
 	}
 
 	/**
-	 * @return Node[]
+	 * @return iterable<Node>
 	 */
-	public function getNodes(): array {
-		return $this->nodes;
+	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 Node[] $nodes
+	 * @param callable $filter
+	 * @return void
 	 */
-	public function setNodes(array $nodes): void {
-		$this->nodes = $nodes;
+	public function addNodeFilter(callable $filter): void {
+		$this->nodeFilters[] = $filter;
 	}
 
 	/**