[stable30] fix(sharing): properly handle verification of copy/move actions

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

@@ -16,6 +16,7 @@
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\Folder;
 use OCP\Files\IFilenameValidator;
+use OCP\Files\IRootFolder;
 use OCP\Files\Mount\IMountManager;
 use OCP\IConfig;
 use OCP\IDBConnection;
@@ -152,8 +153,8 @@ public function createServer(string $baseUri,
 				$server->addPlugin(new \OCA\DAV\Connector\Sabre\SharesPlugin(
 					$objectTree,
 					$this->userSession,
-					$userFolder,
-					\OC::$server->getShareManager()
+					\OCP\Server::get(\OCP\Share\IManager::class),
+					\OCP\Server::get(IRootFolder::class),
 				));
 				$server->addPlugin(new \OCA\DAV\Connector\Sabre\CommentPropertiesPlugin(\OC::$server->getCommentsManager(), $this->userSession));
 				$server->addPlugin(new \OCA\DAV\Connector\Sabre\FilesReportPlugin(

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

@@ -11,6 +11,7 @@
 use OCA\DAV\Connector\Sabre\Exception\Forbidden;
 use OCA\DAV\Connector\Sabre\Node as DavNode;
 use OCP\Files\Folder;
+use OCP\Files\IRootFolder;
 use OCP\Files\Node;
 use OCP\Files\NotFoundException;
 use OCP\Files\Storage\ISharedStorage;
@@ -47,8 +48,8 @@ class SharesPlugin extends \Sabre\DAV\ServerPlugin {
 	public function __construct(
 		private Tree $tree,
 		private IUserSession $userSession,
-		private Folder $userFolder,
 		private IManager $shareManager,
+		private IRootFolder $rootFolder,
 	) {
 		$this->userId = $userSession->getUser()->getUID();
 	}
@@ -79,7 +80,7 @@ public function initialize(Server $server) {
 	 * @param Node $node
 	 * @return IShare[]
 	 */
-	private function getShare(Node $node): array {
+	private function getShare(Node $node, bool $includeIncoming = true): array {
 		$result = [];
 		$requestedShareTypes = [
 			IShare::TYPE_USER,
@@ -102,6 +103,10 @@ private function getShare(Node $node): array {
 				-1
 			));
 
+			if (!$includeIncoming) {
+				continue;
+			}
+
 			// Also check for shares where the user is the recipient
 			try {
 				$result = array_merge($result, $this->shareManager->getSharedWith(
@@ -118,6 +123,24 @@ private function getShare(Node $node): array {
 		return $result;
 	}
 
+	/**
+	 * @return IShare[]
+	 */
+	private function getSharesForTarget(Node $node): array {
+		$shares = $this->getShare($node);
+		if ($shares !== []) {
+			return $shares;
+		}
+
+		// also check the owner side
+		$userRoot = $this->rootFolder->getUserFolder($this->userId);
+		while (str_starts_with($node->getPath(), $userRoot->getPath() . '/')) {
+			$shares = array_merge($shares, $this->getShare($node, false));
+			$node = $node->getParent();
+		}
+		return $shares;
+	}
+
 	/**
 	 * @param Folder $node
 	 * @return IShare[][]
@@ -233,8 +256,8 @@ public function validateMoveOrCopy(string $source, string $target): bool {
 			return true;
 		}
 
-		$targetShares = $this->getShare($targetNode->getNode());
-		if (empty($targetShares)) {
+		$targetShares = $this->getSharesForTarget($targetNode->getNode());
+		if ($targetShares === []) {
 			// Target is not a share so no re-sharing inprogress
 			return true;
 		}

apps/dav/lib/Server.php

@@ -58,6 +58,7 @@
 use OCP\Diagnostics\IEventLogger;
 use OCP\EventDispatcher\IEventDispatcher;
 use OCP\Files\IFilenameValidator;
+use OCP\Files\IRootFolder;
 use OCP\FilesMetadata\IFilesMetadataManager;
 use OCP\IAppConfig;
 use OCP\ICacheFactory;
@@ -290,8 +291,8 @@ public function __construct(IRequest $request, string $baseUri) {
 				$this->server->addPlugin(new SharesPlugin(
 					$this->server->tree,
 					$userSession,
-					$userFolder,
 					$shareManager,
+					\OCP\Server::get(IRootFolder::class),
 				));
 				$this->server->addPlugin(new CommentPropertiesPlugin(
 					\OC::$server->getCommentsManager(),

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

@@ -12,10 +12,12 @@
 use OCA\DAV\Connector\Sabre\Node;
 use OCA\DAV\Upload\UploadFile;
 use OCP\Files\Folder;
+use OCP\Files\IRootFolder;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
+use PHPUnit\Framework\MockObject\MockObject;
 use Sabre\DAV\Tree;
 
 class SharesPluginTest extends \Test\TestCase {
@@ -37,20 +39,21 @@ class SharesPluginTest extends \Test\TestCase {
 	private $shareManager;
 
 	/**
-	 * @var \OCP\Files\Folder
+	 * @var \OCA\DAV\Connector\Sabre\SharesPlugin
 	 */
-	private $userFolder;
+	private $plugin;
 
 	/**
-	 * @var \OCA\DAV\Connector\Sabre\SharesPlugin
+	 * @var IRootFolder&MockObject
 	 */
-	private $plugin;
+	private $rootFolder;
 
 	protected function setUp(): void {
 		parent::setUp();
 		$this->server = new \Sabre\DAV\Server();
 		$this->tree = $this->createMock(Tree::class);
 		$this->shareManager = $this->createMock(IManager::class);
+		$this->rootFolder = $this->createMock(IRootFolder::class);
 		$user = $this->createMock(IUser::class);
 		$user->expects($this->once())
 			->method('getUID')
@@ -59,13 +62,12 @@ protected function setUp(): void {
 		$userSession->expects($this->once())
 			->method('getUser')
 			->willReturn($user);
-		$this->userFolder = $this->createMock(Folder::class);
 
 		$this->plugin = new \OCA\DAV\Connector\Sabre\SharesPlugin(
 			$this->tree,
 			$userSession,
-			$this->userFolder,
-			$this->shareManager
+			$this->shareManager,
+			$this->rootFolder,
 		);
 		$this->plugin->initialize($this->server);
 	}

build/integration/dav_features/dav-v2-public.feature

@@ -57,34 +57,6 @@ Feature: dav-v2-public
 		When Downloading public file "/image.png" without ajax header
 		Then the downloaded file has the content of "/testshare/image.png" from "user1" data
 
-	Scenario: Finalizing a public chunked upload with COPY is not allowed
-		Given using new dav path
-		And user "user0" exists
-		And As an "user0"
-		And user "user0" created a folder "/public-upload"
-		And as "user0" creating a share with
-			| path         | public-upload |
-			| shareType    | 3             |
-			| publicUpload | true          |
-		And creating a new public chunking upload with id "chunking-public-copy"
-		And uploading new public chunk file "1" with "AAAAA" to id "chunking-public-copy"
-		When copying new public chunk file with id "chunking-public-copy" to "/target.txt"
-		Then the HTTP status code should be "409"
-		# Then the HTTP status code should be "405"
-
-	Scenario: Finalizing a public chunked upload with MOVE overwrites the target
-		Given using new dav path
-		And user "user0" exists
-		And As an "user0"
-		And user "user0" created a folder "/public-upload"
-		And User "user0" uploads file with content "original content" to "/public-upload/target.txt"
-		And as "user0" creating a share with
-			| path         | public-upload |
-			| shareType    | 3             |
-			| publicUpload | true          |
-		And creating a new public chunking upload with id "chunking-public-move"
-		And uploading new public chunk file "1" with "AAAAA" to id "chunking-public-move"
-		When moving new public chunk file with id "chunking-public-move" to "/target.txt"
-		Then the HTTP status code should be "204"
-		And Downloading file "/public-upload/target.txt" as "user0"
-		Then Downloaded content should be "AAAAA"
+	# The chunked upload scenarios of this feature are not backported:
+	# `/public.php/dav/uploads/<token>` does not exist on this branch, the public
+	# DAV endpoint only serves `/public.php/dav/files/<token>`.

build/integration/sharing_features/sharing-v1-part4.feature

@@ -289,3 +289,114 @@ Scenario: Can copy file between shares if share permissions
     And the OCS status code should be "100"
     When User "user1" copies file "/share/test.txt" to "/re-share/movetest.txt"
     Then the HTTP status code should be "201"
+
+Scenario: Cannot copy files from share without share permission into subfolder of other share
+    Given user "user0" exists
+    Given user "user1" exists
+    Given user "user2" exists
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    When creating a share with
+      | path        | share |
+      | shareType   |     0 |
+      | shareWith   | user1 |
+      | permissions |     7 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And User "user0" uploads file with content "test" to "/share/test.txt"
+    And As an "user1"
+    And user "user1" created a folder "/re-share"
+    And user "user1" created a folder "/re-share/subfolder"
+    When creating a share with
+      | path        | re-share |
+      | shareType   |        0 |
+      | shareWith   |    user2 |
+      | permissions |       31 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
+    Then the HTTP status code should be "403"
+
+Scenario: Cannot move files from share without share permission into subfolder of other share
+    Given user "user0" exists
+    Given user "user1" exists
+    Given user "user2" exists
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    When creating a share with
+      | path        | share |
+      | shareType   |     0 |
+      | shareWith   | user1 |
+      | permissions |     7 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And User "user0" uploads file with content "test" to "/share/test.txt"
+    And As an "user1"
+    And user "user1" created a folder "/re-share"
+    And user "user1" created a folder "/re-share/subfolder"
+    When creating a share with
+      | path        | re-share |
+      | shareType   |        0 |
+      | shareWith   |    user2 |
+      | permissions |       31 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    When User "user1" moves file "/share/test.txt" to "/re-share/subfolder/movetest.txt"
+    Then the HTTP status code should be "403"
+
+Scenario: Cannot move folder containing share without share permission into subfolder of other share
+    Given user "user0" exists
+    Given user "user1" exists
+    Given user "user2" exists
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    When creating a share with
+      | path        | share |
+      | shareType   |     0 |
+      | shareWith   | user1 |
+      | permissions |     7 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And User "user0" uploads file with content "test" to "/share/test.txt"
+    And As an "user1"
+    And user "user1" created a folder "/contains-share"
+    When User "user1" moves file "/share" to "/contains-share/share"
+    Then the HTTP status code should be "201"
+    And user "user1" created a folder "/re-share"
+    And user "user1" created a folder "/re-share/subfolder"
+    When creating a share with
+      | path        | re-share |
+      | shareType   |        0 |
+      | shareWith   |    user2 |
+      | permissions |       31 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    When User "user1" moves file "/contains-share" to "/re-share/subfolder/movetest"
+    Then the HTTP status code should be "403"
+
+Scenario: Can copy file between shares into subfolder if share permissions
+    Given user "user0" exists
+    Given user "user1" exists
+    Given user "user2" exists
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    When creating a share with
+      | path        | share |
+      | shareType   |     0 |
+      | shareWith   | user1 |
+      | permissions |    31 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And User "user0" uploads file with content "test" to "/share/test.txt"
+    And As an "user1"
+    And user "user1" created a folder "/re-share"
+    And user "user1" created a folder "/re-share/subfolder"
+    When creating a share with
+      | path        | re-share |
+      | shareType   |        0 |
+      | shareWith   |    user2 |
+      | permissions |       31 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
+    Then the HTTP status code should be "201"