Merge pull request #61497 from nextcloud/fix/viewonlyplugin/allow-copy-move-same-storage

fix(ViewOnlyPlugin): Allow COPY and MOVE operations within the same storage

Author: AndyScherzinger

apps/dav/lib/DAV/ViewOnlyPlugin.php

@@ -8,6 +8,7 @@
 
 namespace OCA\DAV\DAV;
 
+use OCA\DAV\Connector\Sabre\Directory;
 use OCA\DAV\Connector\Sabre\Exception\Forbidden;
 use OCA\DAV\Connector\Sabre\File as DavFile;
 use OCA\Files_Versions\Sabre\VersionFile;
@@ -80,30 +81,37 @@ public function checkViewOnly(RequestInterface $request): bool {
 			}
 
 			$storage = $node->getStorage();
-
 			if (!$storage->instanceOfStorage(ISharedStorage::class)) {
 				return true;
 			}
 
-			// Extract extra permissions
 			/** @var ISharedStorage $storage */
 			$share = $storage->getShare();
-			$attributes = $share->getAttributes();
-			if ($attributes === null) {
-				return true;
-			}
-
-			// We have two options here, if download is disabled, but viewing is allowed,
-			// we still allow the GET request to return the file content.
-			$canDownload = $attributes->getAttribute('permissions', 'download');
-			if (!$share->canSeeContent()) {
-				throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
-			}
+			switch ($request->getMethod()) {
+				case 'GET':
+					// If download is disabled, but viewing is allowed, we still allow the GET method to return the file content.
+					if (!$share->canSeeContent()) {
+						throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
+					}
+					break;
+				case 'COPY':
+				case 'MOVE':
+					$destinationPath = $this->server->getCopyAndMoveInfo($request)['destination'];
+					$destinationParentPath = dirname($destinationPath);
+					if ($destinationParentPath === '.') {
+						$destinationParentPath = '';
+					}
+					$destinationParent = $this->server->tree->getNodeForPath($destinationParentPath);
+					// Copy and move operations within the same storage are allowed, because the destination has the same restrictions.
+					if (($destinationParent instanceof Directory) && $destinationParent->getNode()->getStorage()->getId() === $storage->getId()) {
+						break;
+					}
 
-			// If download is disabled, we disable the COPY and MOVE methods even if the
-			// shareapi_allow_view_without_download is set to true.
-			if ($request->getMethod() !== 'GET' && ($canDownload !== null && !$canDownload)) {
-				throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
+					// If download is disabled, we disable the COPY and MOVE methods even if the shareapi_allow_view_without_download is set to true.
+					if (!$share->canDownload()) {
+						throw new Forbidden('Access to this shared resource has been denied because its download permission is disabled.');
+					}
+					break;
 			}
 		} catch (NotFound $e) {
 			// File not found

apps/dav/tests/unit/DAV/ViewOnlyPluginTest.php

@@ -20,7 +20,6 @@
 use OCP\Files\Storage\ISharedStorage;
 use OCP\Files\Storage\IStorage;
 use OCP\IUser;
-use OCP\Share\IAttributes;
 use OCP\Share\IShare;
 use PHPUnit\Framework\MockObject\MockObject;
 use Sabre\DAV\Server;
@@ -136,6 +135,11 @@ public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanD
 			$davNode->method('getNode')->willReturn($nodeInfo);
 		}
 
+		$this->request
+			->expects($this->once())
+			->method('getMethod')
+			->willReturn('GET');
+
 		$this->request->expects($this->once())->method('getPath')->willReturn($davPath);
 
 		$this->tree->expects($this->once())
@@ -151,16 +155,11 @@ public function testCanGet(bool $isVersion, ?bool $attrEnabled, bool $expectCanD
 		$storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(true);
 		$storage->method('getShare')->willReturn($share);
 
-		$extAttr = $this->createMock(IAttributes::class);
-		$share->method('getAttributes')->willReturn($extAttr);
-		$extAttr->expects($this->once())
-			->method('getAttribute')
-			->with('permissions', 'download')
-			->willReturn($attrEnabled);
+		$share->method('canDownload')->willReturn($attrEnabled ?? true);
 
 		$share->expects($this->once())
 			->method('canSeeContent')
-			->willReturn($allowViewWithoutDownload);
+			->willReturn($allowViewWithoutDownload && ($attrEnabled ?? true));
 
 		if (!$expectCanDownloadFile) {
 			$this->expectException(Forbidden::class);