fix(dav): allow moves within own share when sharing is disabled

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

@@ -12,6 +12,7 @@
 use OCA\DAV\Connector\Sabre\Exception\Forbidden;
 use OCA\DAV\Connector\Sabre\Node as DavNode;
 use OCP\Files\Folder;
+use OCP\Files\IHomeStorage;
 use OCP\Files\IRootFolder;
 use OCP\Files\Mount\IShareOwnerlessMount;
 use OCP\Files\Node;
@@ -87,7 +88,7 @@ public function initialize(Server $server) {
 	 * @param Node $node
 	 * @return IShare[]
 	 */
-	private function getShare(Node $node, bool $includeIncoming = true): array {
+	private function getShare(Node $node, bool $includeIncoming = true, bool $reshares = false): array {
 		$result = [];
 		$requestedShareTypes = [
 			IShare::TYPE_USER,
@@ -105,7 +106,7 @@ private function getShare(Node $node, bool $includeIncoming = true): array {
 				$this->userId,
 				$requestedShareType,
 				$node,
-				false,
+				$reshares,
 				-1
 			);
 
@@ -273,7 +274,7 @@ public function validateMoveOrCopy(string $source, string $target): bool {
 
 		$sourceStorage = $sourceNode->getStorage();
 		$sourceIsShare = $sourceStorage->instanceOfStorage(ISharedStorage::class);
-		if (!$sourceIsShare && $sourceNode->getMountPoint() instanceof IShareOwnerlessMount) {
+		if (!$sourceIsShare && ($sourceNode->getMountPoint() instanceof IShareOwnerlessMount || $sourceStorage->instanceOfStorage(IHomeStorage::class))) {
 			// we use the parent because we already know the source is not a share
 			$sourceParent = $sourceNode->getParent();
 			if (!$this->operationAddsShares($sourceParent, $targetNode->getNode())) {
@@ -323,8 +324,8 @@ public function validateMoveOrCopy(string $source, string $target): bool {
 	 * the node where it is now.
 	 *
 	 * NOTE: shares are only looked up on the target and its ancestors. Manager::getSharesBy() returns every share
-	 * on a path only for share-ownerless mounts and otherwise filters by initiator, so the same-mount shortcut is
-	 * only safe on share-ownerless mounts.
+	 * on a path only for share-ownerless mounts, or including reshares for nodes owned by the user, so the same-mount
+	 * shortcut is only safe on share-ownerless mounts and the user's home storage.
 	 */
 	private function operationAddsShares(Folder $sourceParent, Node $targetNode): bool {
 		$targetMountPoint = $targetNode->getMountPoint();
@@ -341,7 +342,7 @@ private function operationAddsShares(Folder $sourceParent, Node $targetNode): bo
 			}
 
 			// note: getShare only returns shares originating from the node itself and not shares that contain the node
-			if ($this->getShare($targetOrAncestor, $includeIncoming) !== []) {
+			if ($this->getShare($targetOrAncestor, $includeIncoming, true) !== []) {
 				return true;
 			}
 		}

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

@@ -16,6 +16,7 @@
 use OCA\DAV\Connector\Sabre\SharesPlugin;
 use OCA\DAV\Upload\UploadFile;
 use OCP\Files\Folder;
+use OCP\Files\IHomeStorage;
 use OCP\Files\IRootFolder;
 use OCP\Files\IUserFolder;
 use OCP\Files\Mount\IMountPoint;
@@ -40,6 +41,7 @@ class SharesPluginTest extends \Test\TestCase {
 	private const TARGET_PARENT = 'files/user1/othersdir';
 	private const USER_ROOT = '/user1/files';
 	private const GROUP_FOLDER_MOUNT = '/user1/files/GF/';
+	private const HOME_MOUNT = '/user1/';
 
 	private \Sabre\DAV\Server $server;
 	private \Sabre\DAV\Tree&MockObject $tree;
@@ -320,6 +322,14 @@ private function mockStorage(?IShare $share = null): IStorage&MockObject {
 		return $storage;
 	}
 
+	private function mockHomeStorage(): IHomeStorage&MockObject {
+		$storage = $this->createMock(IHomeStorage::class);
+		$storage->method('instanceOfStorage')
+			->willReturnCallback(static fn (string $class): bool => $class === IHomeStorage::class);
+
+		return $storage;
+	}
+
 	private function mockSourceNode(
 		IStorage $storage,
 		bool $shareable = false,
@@ -398,15 +408,25 @@ private function mockFolder(string $path, IMountPoint $mountPoint): Folder&MockO
 	/**
 	 * @param array<string, list<string>> $shareIdsByPath ids of the shares returned by getSharesBy() for a node path
 	 * @param array<string, list<string>> $receivedIdsByPath ids of the shares returned by getSharedWith() for a node path
+	 * @param array<string, list<string>> $reshareIdsByPath ids of the shares made by other users on a node of the user,
+	 *                                                      only returned by getSharesBy() when asked for reshares
 	 */
-	private function mockSharesByPath(array $shareIdsByPath, array $receivedIdsByPath): void {
+	private function mockSharesByPath(array $shareIdsByPath, array $receivedIdsByPath, array $reshareIdsByPath = []): void {
 		$sharesByPath = array_map(fn (array $ids): array => array_map($this->mockShare(...), $ids), $shareIdsByPath);
 		$receivedByPath = array_map(fn (array $ids): array => array_map($this->mockShare(...), $ids), $receivedIdsByPath);
+		$resharesByPath = array_map(fn (array $ids): array => array_map($this->mockShare(...), $ids), $reshareIdsByPath);
 
 		$this->shareManager->method('getSharesBy')
-			->willReturnCallback(function (string $userId, int $shareType, FileNode $node) use ($sharesByPath): array {
+			->willReturnCallback(function (string $userId, int $shareType, FileNode $node, bool $reshares) use ($sharesByPath, $resharesByPath): array {
 				$this->sharesRequestedFor[] = $node->getPath();
-				return $shareType === IShare::TYPE_USER ? ($sharesByPath[$node->getPath()] ?? []) : [];
+				if ($shareType !== IShare::TYPE_USER) {
+					return [];
+				}
+
+				return array_merge(
+					$sharesByPath[$node->getPath()] ?? [],
+					$reshares ? ($resharesByPath[$node->getPath()] ?? []) : [],
+				);
 			});
 		$this->shareManager->method('getSharedWith')
 			->willReturnCallback(static fn (string $userId, int $shareType, FileNode $node): array
@@ -644,4 +664,88 @@ public function testValidateMoveOrCopyFromOwnerlessMount(
 			$this->assertNotContains($path, $this->sharesRequestedFor);
 		}
 	}
+
+	public static function validateMoveOrCopyFromHomeStorageProvider(): array {
+		// source parent, target folder, target mount point, share ids by path, received share ids by path,
+		// reshare ids by path, paths whose shares must not be looked up, allowed
+		return [
+			'target not shared' => [
+				'/user1/files/A', '/user1/files/B', self::HOME_MOUNT,
+				[], [], [], [], true,
+			],
+			'target below the same share' => [
+				'/user1/files/Pub/a', '/user1/files/Pub/b', self::HOME_MOUNT,
+				['/user1/files/Pub' => ['1']], [], [], ['/user1/files/Pub'], true,
+			],
+			'target is the shared folder containing the source' => [
+				'/user1/files/Pub/a', '/user1/files/Pub', self::HOME_MOUNT,
+				['/user1/files/Pub' => ['1']], [], [], ['/user1/files/Pub'], true,
+			],
+			'target is a new share' => [
+				'/user1/files/Other', '/user1/files/Pub', self::HOME_MOUNT,
+				['/user1/files/Pub' => ['1']], [], [], [], false,
+			],
+			'target below a new share' => [
+				'/user1/files/Other', '/user1/files/Pub/b', self::HOME_MOUNT,
+				['/user1/files/Pub' => ['1']], [], [], [], false,
+			],
+			'target below a reshare by a recipient' => [
+				'/user1/files/Pub/a', '/user1/files/Pub/b', self::HOME_MOUNT,
+				['/user1/files/Pub' => ['1']], [], ['/user1/files/Pub/b' => ['2']], [], false,
+			],
+			'received share' => [
+				'/user1/files/Pub/a', '/user1/files/Received', '/user1/files/Received/',
+				[], ['/user1/files/Received' => ['1']], [], [], false,
+			],
+		];
+	}
+
+	/**
+	 * A non-shareable node on the user's home storage, e.g. because sharing is disabled for the user
+	 */
+	#[DataProvider(methodName: 'validateMoveOrCopyFromHomeStorageProvider')]
+	public function testValidateMoveOrCopyFromHomeStorage(
+		string $sourceParentPath,
+		string $targetPath,
+		string $targetMountPoint,
+		array $shareIdsByPath,
+		array $receivedIdsByPath,
+		array $reshareIdsByPath,
+		array $notLookedUp,
+		bool $allowed,
+	): void {
+		$this->mockSharesByPath($shareIdsByPath, $receivedIdsByPath, $reshareIdsByPath);
+
+		if (!$allowed) {
+			$this->expectException(Forbidden::class);
+			$this->expectExceptionMessage('You cannot move a non-shareable node into a share');
+		}
+
+		$homeMount = $this->mockMount(self::HOME_MOUNT, false);
+		$targetMount = $targetMountPoint === self::HOME_MOUNT ? $homeMount : $this->mockMount($targetMountPoint, false);
+		$sourceNode = $this->mockSourceNode(
+			$this->mockHomeStorage(),
+			mountPoint: $homeMount,
+			parent: $this->mockFolder($sourceParentPath, $homeMount),
+		);
+
+		$davSource = 'files/user1' . substr($sourceParentPath, strlen(self::USER_ROOT)) . '/source.txt';
+		$davTargetParent = 'files/user1' . substr($targetPath, strlen(self::USER_ROOT));
+		$davTarget = $davTargetParent . '/source.txt';
+
+		$this->mockTree(
+			[
+				$davSource => $sourceNode,
+				$davTargetParent => $this->mockFolder($targetPath, $targetMount),
+			],
+			[$davTarget],
+		);
+
+		$result = $this->plugin->validateMoveOrCopy($davSource, $davTarget);
+		$this->assertTrue($result);
+
+		foreach ($notLookedUp as $path) {
+			$this->assertNotContains($path, $this->sharesRequestedFor);
+		}
+	}
 }

build/integration/features/bootstrap/SharingContext.php

@@ -33,6 +33,8 @@ protected function resetAppConfigs() {
 		$this->deleteServerConfig('files_sharing', 'outgoing_server2server_share_enabled');
 		$this->deleteServerConfig('core', 'shareapi_allow_view_without_download');
 		$this->deleteServerConfig('files_sharing', 'update_cutoff_time');
+		$this->deleteServerConfig('core', 'shareapi_exclude_groups');
+		$this->deleteServerConfig('core', 'shareapi_exclude_groups_list');
 
 		$this->runOcc(['config:system:delete', 'share_folder']);
 	}

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

@@ -427,6 +427,83 @@ Scenario: Can copy file between shares into subfolder if share permissions
     When User "user1" copies file "/share/test.txt" to "/re-share/subfolder/copytest.txt"
     Then the HTTP status code should be "201"
 
+Scenario: Owner without sharing permission can move and copy files within their own share
+    Given user "user0" exists
+    Given user "user1" exists
+    And group "nosharing" exists
+    And user "user0" belongs to group "nosharing"
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    And user "user0" created a folder "/share/a"
+    And user "user0" created a folder "/share/b"
+    And User "user0" uploads file with content "test" to "/share/a/test.txt"
+    When creating a share with
+      | path        | share |
+      | shareType   |     0 |
+      | shareWith   | user1 |
+      | permissions |    15 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And parameter "shareapi_exclude_groups" of app "core" is set to "yes"
+    And parameter "shareapi_exclude_groups_list" of app "core" is set to "nosharing"
+    When User "user0" copies file "/share/a/test.txt" to "/share/b/copytest.txt"
+    Then the HTTP status code should be "201"
+    When User "user0" moves file "/share/a/test.txt" to "/share/b/test.txt"
+    Then the HTTP status code should be "201"
+
+Scenario: Owner without sharing permission cannot move files into their own share
+    Given user "user0" exists
+    Given user "user1" exists
+    And group "nosharing" exists
+    And user "user0" belongs to group "nosharing"
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    And user "user0" created a folder "/not-shared"
+    And User "user0" uploads file with content "test" to "/not-shared/test.txt"
+    When creating a share with
+      | path        | share |
+      | shareType   |     0 |
+      | shareWith   | user1 |
+      | permissions |    15 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And parameter "shareapi_exclude_groups" of app "core" is set to "yes"
+    And parameter "shareapi_exclude_groups_list" of app "core" is set to "nosharing"
+    When User "user0" moves file "/not-shared/test.txt" to "/share/test.txt"
+    Then the HTTP status code should be "403"
+
+Scenario: Owner without sharing permission cannot move files within their own share into a re-shared subfolder
+    Given user "user0" exists
+    Given user "user1" exists
+    Given user "user2" exists
+    And group "nosharing" exists
+    And user "user0" belongs to group "nosharing"
+    And As an "user0"
+    And user "user0" created a folder "/share"
+    And user "user0" created a folder "/share/a"
+    And user "user0" created a folder "/share/b"
+    And User "user0" uploads file with content "test" to "/share/a/test.txt"
+    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 As an "user1"
+    When creating a share with
+      | path        | share/b |
+      | shareType   |       0 |
+      | shareWith   |   user2 |
+      | permissions |      31 |
+    Then the HTTP status code should be "200"
+    And the OCS status code should be "100"
+    And As an "user0"
+    And parameter "shareapi_exclude_groups" of app "core" is set to "yes"
+    And parameter "shareapi_exclude_groups_list" of app "core" is set to "nosharing"
+    When User "user0" moves file "/share/a/test.txt" to "/share/b/test.txt"
+    Then the HTTP status code should be "403"
+
 Scenario: Group deletes removes mount without marking
 	Given As an "admin"
 	And user "user0" exists