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

.github/workflows/integration-sqlite.yml

@@ -129,6 +129,7 @@ jobs:
           extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, imagick, intl, json, ldap, libxml, mbstring, openssl, pcntl, posix, redis, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite
           coverage: none
           ini-file: development
+          ini-values: disable_functions=""
         env:
           GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
 

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

@@ -185,7 +185,8 @@ public function createServer(
 				$server->addPlugin(new SharesPlugin(
 					$tree,
 					$this->userSession,
-					\OCP\Server::get(\OCP\Share\IManager::class)
+					\OCP\Server::get(\OCP\Share\IManager::class),
+					\OCP\Server::get(IRootFolder::class),
 				));
 				$server->addPlugin(new CommentPropertiesPlugin(\OCP\Server::get(ICommentsManager::class), $this->userSession));
 				$server->addPlugin(new 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;
@@ -51,6 +52,7 @@ public function __construct(
 		private Tree $tree,
 		IUserSession $userSession,
 		private IManager $shareManager,
+		private IRootFolder $rootFolder,
 	) {
 		$this->userId = $userSession->getUser()->getUID();
 	}
@@ -82,7 +84,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,
@@ -104,6 +106,10 @@ private function getShare(Node $node): array {
 				-1
 			);
 
+			if (!$includeIncoming) {
+				continue;
+			}
+
 			// Also check for shares where the user is the recipient
 			try {
 				$result[] = $this->shareManager->getSharedWith(
@@ -120,6 +126,24 @@ private function getShare(Node $node): array {
 		return array_merge(...$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[][]
@@ -235,8 +259,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;
 		}
@@ -257,7 +281,7 @@ public function validateMoveOrCopy(string $source, string $target): bool {
 			// the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share
 			//
 			// since the 2-step move is allowed, we also allow both steps at once
-			if ($sourceNode->isDeletable()) {
+			if ($sourceNode->getInternalPath() !== '' && $sourceNode->isDeletable()) {
 				return true;
 			}
 		}

apps/dav/lib/Server.php

@@ -340,6 +340,7 @@ public function __construct(
 					$this->server->tree,
 					$userSession,
 					$shareManager,
+					\OCP\Server::get(IRootFolder::class),
 				));
 				$this->server->addPlugin(new CommentPropertiesPlugin(
 					\OCP\Server::get(ICommentsManager::class),

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

@@ -14,6 +14,7 @@
 use OCA\DAV\Connector\Sabre\SharesPlugin;
 use OCA\DAV\Upload\UploadFile;
 use OCP\Files\Folder;
+use OCP\Files\IRootFolder;
 use OCP\IUser;
 use OCP\IUserSession;
 use OCP\Share\IManager;
@@ -27,13 +28,15 @@ class SharesPluginTest extends \Test\TestCase {
 	private \Sabre\DAV\Server $server;
 	private \Sabre\DAV\Tree&MockObject $tree;
 	private \OCP\Share\IManager&MockObject $shareManager;
+	private IRootFolder&MockObject $rootFolder;
 	private SharesPlugin $plugin;
 
 	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')
@@ -46,7 +49,8 @@ protected function setUp(): void {
 		$this->plugin = new SharesPlugin(
 			$this->tree,
 			$userSession,
-			$this->shareManager
+			$this->shareManager,
+			$this->rootFolder,
 		);
 		$this->plugin->initialize($this->server);
 	}

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

@@ -316,6 +316,117 @@ Scenario: Can copy file between shares if share permissions
     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"
+
 Scenario: Group deletes removes mount without marking
 	Given As an "admin"
 	And user "user0" exists