fixup! feat: improve ZipFolderPlugin behaviour for different cases

Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com>

Author: salmart-dev

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

@@ -12,6 +12,7 @@
  * A stream filter to track how many bytes have been streamed from a stream.
  */
 class ByteCounterFilter extends \php_user_filter {
+	/** @psalm-suppress NonInvariantPropertyType PHP crashes without string type */
 	public string $filtername = 'ByteCounter';
 
 	public function filter($in, $out, &$consumed, bool $closing): int {

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

@@ -18,7 +18,6 @@
 use OCP\IConfig;
 use OCP\IDateTimeZone;
 use OCP\IL10N;
-use OCP\L10N\IFactory;
 use Psr\Log\LoggerInterface;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
@@ -244,7 +243,7 @@ public function handleDownload(Request $request, Response $response): ?bool {
 			$streamer->addEmptyDir($archiveName);
 		}
 
-		foreach ($event->getNodes() as $path => [$node, $reason]) {
+		foreach ($event->getNodes($rootPath) as $path => [$node, $reason]) {
 			$filename = str_replace($rootPath, '', $path);
 			if ($node === null) {
 				if ($this->reportMissingFiles) {

apps/dav/lib/Server.php

@@ -8,7 +8,6 @@
 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;
@@ -89,7 +88,6 @@
 use OCP\IDateTimeZone;
 use OCP\IDBConnection;
 use OCP\IGroupManager;
-use OCP\IL10N;
 use OCP\IPreview;
 use OCP\IRequest;
 use OCP\ISession;

apps/dav/tests/unit/Connector/Sabre/ZipFolderPluginTest.php

@@ -10,7 +10,6 @@
 namespace OCA\DAV\Tests\unit\Connector\Sabre;
 
 use Exception;
-use OC\L10N\L10N;
 use OCA\DAV\Connector\Sabre\Directory;
 use OCA\DAV\Connector\Sabre\Exception\Forbidden;
 use OCA\DAV\Connector\Sabre\Node;

apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php

@@ -37,8 +37,8 @@ public function handle(Event $event): void {
 		// Check only for user/group shares. Don't restrict e.g. share links
 		if (!$user) {
 			return;
-
 		}
+
 		$userFolder = $this->rootFolder->getUserFolder($user->getUID());
 		$node = $userFolder->get($event->getPath());
 		if (!$this->viewOnly->isDownloadable($node)) {

apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php

@@ -46,18 +46,20 @@ public function handle(Event $event): void {
 			}
 		}
 
-		// Check only for user/group shares. Don't restrict e.g. share links
 		$user = $this->userSession->getUser();
-		if (!$user) {
+		$folder = $event->getFolder();
+		if ($user === null && $event->getFolder() === null) {
+			// there is no way to know if the file is downloadable or not, allow it
 			$event->setSuccessful(true);
 			return;
 		}
 
-		$userFolder = $this->rootFolder->getUserFolder($user->getUID());
-		$viewOnlyHandler = new ViewOnly($userFolder);
+		// in link-shares there may be no user, in that case we check that the share folder is downloadable
+		$userFolder = $user ? $this->rootFolder->getUserFolder($user->getUID()) : null;
+		$folderToCheck = $userFolder ? $userFolder->get($dir) : $folder;
 
-		$node = $userFolder->get($dir);
-		$isRootDownloadable = $viewOnlyHandler->isDownloadable($node);
+		$viewOnlyHandler = new ViewOnly($userFolder);
+		$isRootDownloadable = $viewOnlyHandler->isDownloadable($folderToCheck);
 
 		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.';

apps/files_sharing/lib/ViewOnly.php

@@ -19,7 +19,7 @@
 class ViewOnly {
 
 	public function __construct(
-		private Folder $userFolder,
+		private ?Folder $userFolder,
 	) {
 	}
 
@@ -28,6 +28,10 @@ public function __construct(
 	 * @return bool
 	 */
 	public function check(array $pathsToCheck): bool {
+		if ($this->userFolder === null) {
+			throw new \LogicException('The user folder is not set but this check requires it.');
+		}
+
 		// If any of elements cannot be downloaded, prevent whole download
 		foreach ($pathsToCheck as $file) {
 			try {

apps/files_sharing/tests/ApplicationTest.php

@@ -81,6 +81,7 @@ public function testCheckDirectCanBeDownloaded(
 
 		$userFolder = $this->createMock(Folder::class);
 		$userFolder->method('get')->willReturn($file);
+		$userFolder->method('getPath')->willReturn($path);
 
 		$user = $this->createMock(IUser::class);
 		$user->method('getUID')->willReturn('test');
@@ -93,7 +94,7 @@ public function testCheckDirectCanBeDownloaded(
 		$listener = new BeforeDirectFileDownloadListener(
 			$this->userSession,
 			$this->rootFolder,
-			new ViewOnly(),
+			new ViewOnly($userFolder),
 		);
 		$listener->handle($event);
 
@@ -139,6 +140,7 @@ public function testCheckZipCanBeDownloaded(
 		$secureSharedStorage->method('getShare')->willReturn($secureReceiverFileShare);
 
 		$folder = $this->createMock(Folder::class);
+		$folder->method('getPath')->willReturn($dir);
 		if ($folderStorage === 'nonSharedStorage') {
 			$folder->method('getStorage')->willReturn($nonSharedStorage);
 		} elseif ($folderStorage === 'secureSharedStorage') {
@@ -202,7 +204,7 @@ 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,

apps/files_sharing/tests/unit/Listener/BeforeZipCreatedListenerTest.php

@@ -63,7 +63,7 @@ public static function dataHandle(): array {
 			'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],
+				'files' => ['blocked.txt' => false, 'allowed.txt' => true],
 				'filesFilter' => [],
 				'allowPartialArchive' => false,
 				'expectedSuccess' => false,
@@ -186,15 +186,16 @@ public function testHandle(
 	): void {
 		$fileNodes = [];
 		$fileNodesByName = [];
+		$folderPathFromUserRoot = "/user/files{$folderPath}";
 		foreach ($files as $relativePath => $downloadable) {
-			$pathWithFolder = "{$folderPath}/{$relativePath}";
+			$pathWithFolder = "{$folderPathFromUserRoot}/{$relativePath}";
 			$file = $this->createSharedFile($downloadable, $pathWithFolder);
-			$fileNodesByName[$pathWithFolder] = $file;
+			$fileNodesByName[$relativePath] = $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) {
+			$path = str_replace($folderPath . '/', '', $path);
 			return match (true) {
 				$path === $folderPath => $folder,
 				isset($fileNodesByName[$path]) => $fileNodesByName[$path],
@@ -209,18 +210,18 @@ public function testHandle(
 		$this->assertSame($expectedMessage, $event->getErrorMessage());
 
 		$event->setNodesIterable($fileNodes);
-		$actualNodes = iterator_to_array($event->getNodes());
+		$actualNodes = iterator_to_array($event->getNodes($folderPathFromUserRoot));
 		$this->assertCount(count($expectedNodeList), $actualNodes);
 		foreach ($expectedNodeList as $relativePath => $expectedValue) {
-			$path = "{$folderPath}/{$relativePath}";
+			$path = "$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]);
+			$this->assertEquals($expectedValue, $actualNodes[$path] ?? []);
 		}
 	}
 

lib/public/Files/Events/BeforeZipCreatedEvent.php

@@ -46,7 +46,7 @@ class BeforeZipCreatedEvent extends Event {
 	public function __construct(
 		string|Folder $directory,
 		private array $files,
-		public ?bool $allowPartialArchive = true,
+		public ?bool $allowPartialArchive = false,
 	) {
 		parent::__construct();
 		if ($directory instanceof Folder) {
@@ -119,8 +119,8 @@ public function setNodesIterable(iterable $iterable): void {
 
 	/**
 	 * @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.
+	 *                                                           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
 	 */
@@ -136,7 +136,7 @@ public function addNodeFilter(callable $filter): void {
 	 *
 	 * @return iterable<string, array{0: ?Node, 1: ?string}>
 	 */
-	public function getNodes(): iterable {
+	public function getNodes(string $rootPath): iterable {
 		if (!isset($this->nodesIterable)) {
 			throw new \LogicException('No nodes iterable set');
 		}
@@ -145,10 +145,12 @@ public function getNodes(): iterable {
 			return;
 		}
 
-		$directory = $this->getDirectory();
 		foreach ($this->nodesIterable as $node) {
-			$relativePath = $directory . '/' . $node->getName();
-			if (!empty($this->files) && !in_array($node->getName(), $this->files)) {
+			$relativePath = trim(str_replace($rootPath, '', $node->getPath()), '/');
+			$parent = rtrim(substr($relativePath, 0, strpos($relativePath, '/', 1) ?: 0), '/');
+			// we need to filter by file name or first parent folder
+			$filterName = $parent === '' ? $node->getName() : $parent;
+			if (!empty($this->files) && !in_array($filterName, $this->files)) {
 				// the node is supposed to be filtered out
 				continue;
 			}