diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index 1a9eb88cfe46b..0d0bc417286f6 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -267,18 +267,23 @@ 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(); - 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; + } + } + $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..0b981344fcc64 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\ISharedStorage; +use OCP\Files\Storage\IStorage; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IManager; @@ -280,4 +283,137 @@ 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 target's own path so getSharesForTarget() doesn't + * walk up any parents looking for an enclosing share. + */ + private function preventTargetShareLookupFromWalkingUp(MockObject $targetNode): void { + $userFolder = $this->createMock(Folder::class); + $userFolder->method('getPath')->willReturn('/user1/files'); + $this->rootFolder->method('getUserFolder')->willReturn($userFolder); + $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->preventTargetShareLookupFromWalkingUp($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); + $sourceNode->method('getPath')->willReturn('/user1/files/Shared/Folder A/file.txt'); + $targetNode = $this->createMock(Folder::class); + + $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], + ]); + + // target is directly shared, so getSharesForTarget() finds it without walking up + $share = $this->createMock(IShare::class); + $share->method('getNodeId')->willReturn(42); + $this->shareManager->expects($this->any()) + ->method('getSharesBy') + ->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('getPath')->willReturn('/user1/files/Elsewhere/file.txt'); + $targetNode = $this->createMock(Folder::class); + + $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], + ]); + + $targetShare = $this->createMock(IShare::class); + $targetShare->method('getNodeId')->willReturn(555); + $this->shareManager->expects($this->any()) + ->method('getSharesBy') + ->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); + + $this->expectException(Forbidden::class); + $this->plugin->validateMoveOrCopy('/source', '/target'); + } }