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>

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

@@ -61,9 +61,31 @@ class ZipFolderPlugin extends ServerPlugin {
 		$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;
+
+		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 @@ class ZipFolderPlugin extends ServerPlugin {
 			$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 @@ class ZipFolderPlugin extends ServerPlugin {
 		}
 
 		$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 @@ class ZipFolderPlugin extends ServerPlugin {
 			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 @@ class ZipFolderPlugin extends ServerPlugin {
 			$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();

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 @@ class BeforeDirectFileDownloadListener implements IEventListener {
 			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.');
 		}
 	}
 }

apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php

@@ -13,7 +13,7 @@ use OCA\Files_Sharing\ViewOnly;
 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 @@ class BeforeZipCreatedListener implements IEventListener {
 			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) {
-			$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);
+		if (!$user) {
+			return;
 		}
+
+		// 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));
 	}
 }

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

apps/files_sharing/tests/ApplicationTest.php

@@ -13,6 +13,7 @@ use OCA\Files_Sharing\AppInfo\Application;
 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 @@ class ApplicationTest extends TestCase {
 		$event = new BeforeDirectFileDownloadEvent($path);
 		$listener = new BeforeDirectFileDownloadListener(
 			$this->userSession,
-			$this->rootFolder
+			$this->rootFolder,
+			new ViewOnly(),
 		);
 		$listener->handle($event);
 
@@ -161,6 +163,13 @@ class ApplicationTest extends TestCase {
 			);
 			$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 @@ class ApplicationTest extends TestCase {
 		$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 @@ class ApplicationTest extends TestCase {
 		$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

@@ -11,6 +11,7 @@ namespace OCP\Files\Events;
 
 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<string> $files
+	 * @param list<string> $files Selected files, empty for folder selection
+	 * @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 iterable $nodes,
 	) {
 		parent::__construct();
 		if ($directory instanceof Folder) {
@@ -70,6 +74,30 @@ class BeforeZipCreatedEvent extends Event {
 		return $this->files;
 	}
 
+	/**
+	 * @return iterable<Node>
+	 */
+	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
 	 */