From f52a871fb5ea6efe1deb28026e98b49542166e9e Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Mon, 7 Sep 2026 16:57:31 +0200 Subject: [PATCH 1/2] fix(dav): allow moving files between subfolders of the same share - group folder storage doesn't implement ISharedStorage, so moves within the same team folder were rejected as cross-share moves. Compare the source's and target's enclosing shares directly instead. - add unit tests coverage in SharesPluginTest.php Assisted-by: ClaudeCode:claude-sonnet-5 Signed-off-by: Maksim Sukharev --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 13 +- .../unit/Connector/Sabre/SharesPluginTest.php | 144 ++++++++++++++++++ 2 files changed, 151 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index 1a9eb88cfe46b..179bd926bcb90 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -267,18 +267,19 @@ public function validateMoveOrCopy(string $source, string $target): bool { return true; } - $sourceStorage = $sourceNode->getStorage(); - if ($sourceStorage->instanceOfStorage(ISharedStorage::class)) { - // source is also a share - check if it is the same share - - /** @var ISharedStorage $sourceStorage */ - $sourceShare = $sourceStorage->getShare(); + // check if source and target are within the same share, e.g. moving a node between + // two subfolders of the same group folder or the same regular share + $sourceShares = $this->getSharesForTarget($sourceNode); + foreach ($sourceShares as $sourceShare) { foreach ($targetShares as $targetShare) { if ($targetShare->getId() === $sourceShare->getId()) { return true; } } + } + $sourceStorage = $sourceNode->getStorage(); + if ($sourceStorage->instanceOfStorage(ISharedStorage::class)) { // if the share recipient is allow to delete from the share, they are allowed to move the file out of the share // the user moving the file out of the share to their home storage would give them share permissions and allow moving into the share // diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php index c90105b3d621f..9352f4afd0654 100644 --- a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php @@ -10,12 +10,15 @@ namespace OCA\DAV\Tests\unit\Connector\Sabre; use OCA\DAV\Connector\Sabre\Directory; +use OCA\DAV\Connector\Sabre\Exception\Forbidden; use OCA\DAV\Connector\Sabre\File; use OCA\DAV\Connector\Sabre\Node; use OCA\DAV\Connector\Sabre\SharesPlugin; use OCA\DAV\Upload\UploadFile; use OCP\Files\Folder; use OCP\Files\IRootFolder; +use OCP\Files\Storage\IStorage; +use OCP\Files\Storage\ISharedStorage; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IManager; @@ -280,4 +283,145 @@ public function testGetPropertiesSkipChunks(): void { $result = $propFind->getResultForMultiStatus(); $this->assertCount(1, $result[404]); } + + public function testValidateMoveOrCopyAllowsShareableSource(): void { + $sourceNode = $this->createMock(Folder::class); + $sourceNode->method('isShareable')->willReturn(true); + + $source = $this->createMock(Node::class); + $source->method('getNode')->willReturn($sourceNode); + $target = $this->createMock(Node::class); + + $this->tree->method('getNodeForPath') + ->willReturnMap([ + ['/target', $target], + ['/source', $source], + ]); + + $this->assertTrue($this->plugin->validateMoveOrCopy('/source', '/target')); + } + + /** + * Stubs getUserFolder() and the nodes' own path so getSharesForTarget() doesn't + * walk up any parents looking for an enclosing share. + */ + private function preventShareLookupFromWalkingUp(MockObject $sourceNode, MockObject $targetNode): void { + $userFolder = $this->createMock(Folder::class); + $userFolder->method('getPath')->willReturn('/user1/files'); + $this->rootFolder->method('getUserFolder')->willReturn($userFolder); + $sourceNode->method('getPath')->willReturn('/user1/files'); + $targetNode->method('getPath')->willReturn('/user1/files'); + } + + public function testValidateMoveOrCopyAllowsWhenTargetNotShared(): void { + $sourceNode = $this->createMock(Folder::class); + $sourceNode->method('isShareable')->willReturn(false); + $targetNode = $this->createMock(Folder::class); + $this->preventShareLookupFromWalkingUp($sourceNode, $targetNode); + + $source = $this->createMock(Node::class); + $source->method('getNode')->willReturn($sourceNode); + $target = $this->createMock(Node::class); + $target->method('getNode')->willReturn($targetNode); + + $this->tree->method('getNodeForPath') + ->willReturnMap([ + ['/target', $target], + ['/source', $source], + ]); + + $this->shareManager->expects($this->any()) + ->method('getSharesBy') + ->willReturn([]); + $this->shareManager->expects($this->any()) + ->method('getSharedWith') + ->willReturn([]); + + $this->assertTrue($this->plugin->validateMoveOrCopy('/source', '/target')); + } + + public function testValidateMoveOrCopyAllowsMoveWithinSameShare(): void { + $sourceNode = $this->createMock(Folder::class); + $sourceNode->method('isShareable')->willReturn(false); + $targetNode = $this->createMock(Folder::class); + $this->preventShareLookupFromWalkingUp($sourceNode, $targetNode); + + $source = $this->createMock(Node::class); + $source->method('getNode')->willReturn($sourceNode); + $target = $this->createMock(Node::class); + $target->method('getNode')->willReturn($targetNode); + + $this->tree->method('getNodeForPath') + ->willReturnMap([ + ['/target', $target], + ['/source', $source], + ]); + + $share = $this->createMock(IShare::class); + $share->method('getId')->willReturn('shared-folder'); + + // both source and target sit inside the same share + $this->shareManager->expects($this->any()) + ->method('getSharesBy') + ->willReturnCallback(function ($userId, $type, $node) use ($sourceNode, $targetNode, $share) { + if ($type !== IShare::TYPE_USER) { + return []; + } + return ($node === $sourceNode || $node === $targetNode) ? [$share] : []; + }); + $this->shareManager->expects($this->any()) + ->method('getSharedWith') + ->willReturn([]); + + $this->assertTrue($this->plugin->validateMoveOrCopy('/source', '/target')); + } + + public function testValidateMoveOrCopyThrowsForCrossShareMove(): void { + $sourceNode = $this->createMock(Folder::class); + $sourceNode->method('isShareable')->willReturn(false); + $sourceNode->method('getId')->willReturn(111); + $targetNode = $this->createMock(Folder::class); + $targetNode->method('getId')->willReturn(222); + + $source = $this->createMock(Node::class); + $source->method('getNode')->willReturn($sourceNode); + $target = $this->createMock(Node::class); + $target->method('getNode')->willReturn($targetNode); + + $this->tree->method('getNodeForPath') + ->willReturnMap([ + ['/target', $target], + ['/source', $source], + ]); + + $sourceShare = $this->createMock(IShare::class); + $sourceShare->method('getId')->willReturn('source-share'); + $targetShare = $this->createMock(IShare::class); + $targetShare->method('getId')->willReturn('target-share'); + + $this->shareManager->expects($this->any()) + ->method('getSharesBy') + ->willReturnCallback(function ($userId, $type, $node) use ($sourceNode, $targetNode, $sourceShare, $targetShare) { + if ($type !== IShare::TYPE_USER) { + return []; + } + if ($node === $sourceNode) { + return [$sourceShare]; + } + if ($node === $targetNode) { + return [$targetShare]; + } + return []; + }); + $this->shareManager->expects($this->any()) + ->method('getSharedWith') + ->willReturn([]); + + $storage = $this->createMock(IStorage::class); + $storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(false); + $sourceNode->method('getStorage')->willReturn($storage); + + $this->expectException(Forbidden::class); + $this->plugin->validateMoveOrCopy('/source', '/target'); + } } From 691a48a1090ef33f013faf491f7659ae930569ed Mon Sep 17 00:00:00 2001 From: Maksim Sukharev Date: Tue, 8 Sep 2026 11:02:35 +0200 Subject: [PATCH 2/2] fixup!: do string comparison against source paths Signed-off-by: Maksim Sukharev --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 20 ++++--- .../unit/Connector/Sabre/SharesPluginTest.php | 58 ++++++++----------- 2 files changed, 37 insertions(+), 41 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index 179bd926bcb90..0d0bc417286f6 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -267,14 +267,18 @@ public function validateMoveOrCopy(string $source, string $target): bool { return true; } - // check if source and target are within the same share, e.g. moving a node between - // two subfolders of the same group folder or the same regular share - $sourceShares = $this->getSharesForTarget($sourceNode); - foreach ($sourceShares as $sourceShare) { - foreach ($targetShares as $targetShare) { - if ($targetShare->getId() === $sourceShare->getId()) { - return true; - } + // Check if source and target are within the same share, e.g. moving a node between + // two subfolders of the same group folder or the same regular share, by comparing + // paths instead of resolving the source's shares too. + $userRoot = $this->rootFolder->getUserFolder($this->userId); + foreach ($targetShares as $targetShare) { + $shareNode = $userRoot->getFirstNodeById($targetShare->getNodeId()); + if ($shareNode === null) { + continue; + } + $sharePath = $shareNode->getPath(); + if ($sourceNode->getPath() === $sharePath || str_starts_with($sourceNode->getPath(), $sharePath . '/')) { + return true; } } diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php index 9352f4afd0654..0b981344fcc64 100644 --- a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php @@ -17,8 +17,8 @@ use OCA\DAV\Upload\UploadFile; use OCP\Files\Folder; use OCP\Files\IRootFolder; -use OCP\Files\Storage\IStorage; use OCP\Files\Storage\ISharedStorage; +use OCP\Files\Storage\IStorage; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IManager; @@ -302,14 +302,13 @@ public function testValidateMoveOrCopyAllowsShareableSource(): void { } /** - * Stubs getUserFolder() and the nodes' own path so getSharesForTarget() doesn't + * Stubs getUserFolder() and the target's own path so getSharesForTarget() doesn't * walk up any parents looking for an enclosing share. */ - private function preventShareLookupFromWalkingUp(MockObject $sourceNode, MockObject $targetNode): void { + private function preventTargetShareLookupFromWalkingUp(MockObject $targetNode): void { $userFolder = $this->createMock(Folder::class); $userFolder->method('getPath')->willReturn('/user1/files'); $this->rootFolder->method('getUserFolder')->willReturn($userFolder); - $sourceNode->method('getPath')->willReturn('/user1/files'); $targetNode->method('getPath')->willReturn('/user1/files'); } @@ -317,7 +316,7 @@ public function testValidateMoveOrCopyAllowsWhenTargetNotShared(): void { $sourceNode = $this->createMock(Folder::class); $sourceNode->method('isShareable')->willReturn(false); $targetNode = $this->createMock(Folder::class); - $this->preventShareLookupFromWalkingUp($sourceNode, $targetNode); + $this->preventTargetShareLookupFromWalkingUp($targetNode); $source = $this->createMock(Node::class); $source->method('getNode')->willReturn($sourceNode); @@ -343,8 +342,8 @@ public function testValidateMoveOrCopyAllowsWhenTargetNotShared(): void { public function testValidateMoveOrCopyAllowsMoveWithinSameShare(): void { $sourceNode = $this->createMock(Folder::class); $sourceNode->method('isShareable')->willReturn(false); + $sourceNode->method('getPath')->willReturn('/user1/files/Shared/Folder A/file.txt'); $targetNode = $this->createMock(Folder::class); - $this->preventShareLookupFromWalkingUp($sourceNode, $targetNode); $source = $this->createMock(Node::class); $source->method('getNode')->willReturn($sourceNode); @@ -357,31 +356,31 @@ public function testValidateMoveOrCopyAllowsMoveWithinSameShare(): void { ['/source', $source], ]); + // target is directly shared, so getSharesForTarget() finds it without walking up $share = $this->createMock(IShare::class); - $share->method('getId')->willReturn('shared-folder'); - - // both source and target sit inside the same share + $share->method('getNodeId')->willReturn(42); $this->shareManager->expects($this->any()) ->method('getSharesBy') - ->willReturnCallback(function ($userId, $type, $node) use ($sourceNode, $targetNode, $share) { - if ($type !== IShare::TYPE_USER) { - return []; - } - return ($node === $sourceNode || $node === $targetNode) ? [$share] : []; - }); + ->willReturnCallback(fn ($userId, $type, $node) => ($node === $targetNode && $type === IShare::TYPE_USER) ? [$share] : []); $this->shareManager->expects($this->any()) ->method('getSharedWith') ->willReturn([]); + // the share's node, resolved in the current user's own tree, is an ancestor of source + $shareNode = $this->createMock(Folder::class); + $shareNode->method('getPath')->willReturn('/user1/files/Shared/Folder A'); + $userFolder = $this->createMock(Folder::class); + $userFolder->method('getFirstNodeById')->with(42)->willReturn($shareNode); + $this->rootFolder->method('getUserFolder')->willReturn($userFolder); + $this->assertTrue($this->plugin->validateMoveOrCopy('/source', '/target')); } public function testValidateMoveOrCopyThrowsForCrossShareMove(): void { $sourceNode = $this->createMock(Folder::class); $sourceNode->method('isShareable')->willReturn(false); - $sourceNode->method('getId')->willReturn(111); + $sourceNode->method('getPath')->willReturn('/user1/files/Elsewhere/file.txt'); $targetNode = $this->createMock(Folder::class); - $targetNode->method('getId')->willReturn(222); $source = $this->createMock(Node::class); $source->method('getNode')->willReturn($sourceNode); @@ -394,29 +393,22 @@ public function testValidateMoveOrCopyThrowsForCrossShareMove(): void { ['/source', $source], ]); - $sourceShare = $this->createMock(IShare::class); - $sourceShare->method('getId')->willReturn('source-share'); $targetShare = $this->createMock(IShare::class); - $targetShare->method('getId')->willReturn('target-share'); - + $targetShare->method('getNodeId')->willReturn(555); $this->shareManager->expects($this->any()) ->method('getSharesBy') - ->willReturnCallback(function ($userId, $type, $node) use ($sourceNode, $targetNode, $sourceShare, $targetShare) { - if ($type !== IShare::TYPE_USER) { - return []; - } - if ($node === $sourceNode) { - return [$sourceShare]; - } - if ($node === $targetNode) { - return [$targetShare]; - } - return []; - }); + ->willReturnCallback(fn ($userId, $type, $node) => ($node === $targetNode && $type === IShare::TYPE_USER) ? [$targetShare] : []); $this->shareManager->expects($this->any()) ->method('getSharedWith') ->willReturn([]); + // source's path is not inside the target share's path + $shareNode = $this->createMock(Folder::class); + $shareNode->method('getPath')->willReturn('/user1/files/Shared/Folder A'); + $userFolder = $this->createMock(Folder::class); + $userFolder->method('getFirstNodeById')->with(555)->willReturn($shareNode); + $this->rootFolder->method('getUserFolder')->willReturn($userFolder); + $storage = $this->createMock(IStorage::class); $storage->method('instanceOfStorage')->with(ISharedStorage::class)->willReturn(false); $sourceNode->method('getStorage')->willReturn($storage);