From 91bfe94d96dc1fa11957dfaa50b702bc0a1ba43f Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:46:55 +0200 Subject: [PATCH 1/2] refactor: remove array_merge in loop Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index 905f118a6e4e9..632bc2754f9b2 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -137,13 +137,15 @@ private function getSharesForTarget(Node $node): array { return $shares; } - // also check the owner side + // also check outgoing shares made by the user (or anyone for IShareOwnerlessMount) $userRoot = $this->rootFolder->getUserFolder($this->userId); + $outgoingAncestorShares = []; while (str_starts_with($node->getPath(), $userRoot->getPath() . '/')) { - $shares = array_merge($shares, $this->getShare($node, false)); + $outgoingAncestorShares[] = $this->getShare($node, false); $node = $node->getParent(); } - return $shares; + + return array_merge($shares, ...$outgoingAncestorShares); } /** @@ -270,7 +272,7 @@ public function validateMoveOrCopy(string $source, string $target): bool { $targetShares = $this->getSharesForTarget($targetNode->getNode()); if ($targetShares === []) { - // Target is not a share so no re-sharing inprogress + // Target is not a share so no re-sharing in progress return true; } From 03c06c0be8c3b16b79338142ad6897ce774b6eca Mon Sep 17 00:00:00 2001 From: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:48:13 +0200 Subject: [PATCH 2/2] fix: allow move or copy on ownerless mounts in some cases This commit allows the move or copy operation inside ownerless mounts, when the user performing the operation has no sharing permissions and the operation would not result in exposing the files into additional shares. Signed-off-by: Salvatore Martire <4652631+salmart-dev@users.noreply.github.com> Assisted-by: ClaudeCode:claude-opus-5.5 --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 69 +++- .../unit/Connector/Sabre/SharesPluginTest.php | 340 +++++++++++++++++- 2 files changed, 405 insertions(+), 4 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index 632bc2754f9b2..54385ed97cedf 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/apps/dav/lib/Connector/Sabre/SharesPlugin.php @@ -13,6 +13,7 @@ use OCA\DAV\Connector\Sabre\Node as DavNode; use OCP\Files\Folder; use OCP\Files\IRootFolder; +use OCP\Files\Mount\IShareOwnerlessMount; use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\Files\Storage\ISharedStorage; @@ -270,14 +271,25 @@ public function validateMoveOrCopy(string $source, string $target): bool { return true; } + $sourceStorage = $sourceNode->getStorage(); + $sourceIsShare = $sourceStorage->instanceOfStorage(ISharedStorage::class); + if (!$sourceIsShare && $sourceNode->getMountPoint() instanceof IShareOwnerlessMount) { + // we use the parent because we already know the source is not a share + $sourceParent = $sourceNode->getParent(); + if (!$this->operationAddsShares($sourceParent, $targetNode->getNode())) { + return true; + } + + throw new Forbidden('You cannot move a non-shareable node into a share'); + } + $targetShares = $this->getSharesForTarget($targetNode->getNode()); if ($targetShares === []) { // Target is not a share so no re-sharing in progress return true; } - $sourceStorage = $sourceNode->getStorage(); - if ($sourceStorage->instanceOfStorage(ISharedStorage::class)) { + if ($sourceIsShare) { // source is also a share - check if it is the same share /** @var ISharedStorage $sourceStorage */ @@ -299,4 +311,57 @@ public function validateMoveOrCopy(string $source, string $target): bool { throw new Forbidden('You cannot move a non-shareable node into a share'); } + + /** + * Whether moving or copying a node from $sourceParent into $targetNode would expose it to recipients + * that cannot already see it. + * + * Walks the target and its ancestors up to the target's mount root, or the user root if it comes + * first, since a share is a jail over a single storage subtree. + * When target and source are within the same mount the walk ends at the lowest ancestor that is or contains + * $sourceParent: any share at or above it covers the source too, so its recipients can already see + * 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. + */ + private function operationAddsShares(Node $sourceParent, Node $targetNode): bool { + $targetMountPoint = $targetNode->getMountPoint(); + $sameMount = $sourceParent->getMountPoint()->getMountPoint() === $targetMountPoint->getMountPoint(); + $sourceParentPath = $sourceParent->getPath(); + // on share-ownerless mounts the shares of a node already include the ones received by the user + $includeIncoming = !($targetMountPoint instanceof IShareOwnerlessMount); + + foreach ($this->getNodeAndAncestorsInMount($targetNode) as $node) { + $path = $node->getPath(); + // check that the source path is below the target path and in the same mount shares containing the target path + // also cover the source path + if ($sameMount && ($sourceParentPath === $path || str_starts_with($sourceParentPath, $path . '/'))) { + return false; + } + + // note: getShare only returns shares originating from the node itself and not shares that contain the node + if ($this->getShare($node, $includeIncoming) !== []) { + return true; + } + } + + return false; + } + + /** + * @return \Generator $node and its ancestors, up to the root of the mount of $node or the user folder + */ + private function getNodeAndAncestorsInMount(Node $node): \Generator { + $mountRoot = rtrim($node->getMountPoint()->getMountPoint(), '/'); + $userRootPath = $this->rootFolder->getUserFolder($this->userId)->getPath(); + while (str_starts_with($node->getPath(), $userRootPath . '/')) { + yield $node; + if ($node->getPath() === $mountRoot) { + return; + } + $node = $node->getParent(); + } + } } diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php index c90105b3d621f..3028de572244d 100644 --- a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php @@ -10,27 +10,45 @@ 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\IUserFolder; +use OCP\Files\Mount\IMountPoint; +use OCP\Files\Mount\IShareOwnerlessMount; +use OCP\Files\Node as FileNode; +use OCP\Files\Storage\ISharedStorage; +use OCP\Files\Storage\IStorage; use OCP\IUser; use OCP\IUserSession; use OCP\Share\IManager; use OCP\Share\IShare; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\MockObject\MockObject; +use Sabre\DAV\Exception\NotFound; use Sabre\DAV\Tree; class SharesPluginTest extends \Test\TestCase { public const SHARETYPES_PROPERTYNAME = SharesPlugin::SHARETYPES_PROPERTYNAME; + private const SOURCE = 'files/user1/somedir/source.txt'; + private const TARGET = 'files/user1/othersdir/source.txt'; + private const TARGET_PARENT = 'files/user1/othersdir'; + private const USER_ROOT = '/user1/files'; + private const GROUP_FOLDER_MOUNT = '/user1/files/GF/'; + private \Sabre\DAV\Server $server; private \Sabre\DAV\Tree&MockObject $tree; private \OCP\Share\IManager&MockObject $shareManager; private IRootFolder&MockObject $rootFolder; private SharesPlugin $plugin; + private ?IUserFolder $userRoot = null; + /** @var list paths of the nodes getSharesBy() was called for */ + private array $sharesRequestedFor = []; protected function setUp(): void { parent::setUp(); @@ -56,7 +74,7 @@ protected function setUp(): void { $this->plugin->initialize($this->server); } - #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'sharesGetPropertiesDataProvider')] + #[DataProvider(methodName: 'sharesGetPropertiesDataProvider')] public function testGetProperties(array $shareTypes): void { $sabreNode = $this->createMock(Node::class); $sabreNode->expects($this->any()) @@ -119,7 +137,7 @@ public function testGetProperties(array $shareTypes): void { $this->assertEquals($shareTypes, $result[200][self::SHARETYPES_PROPERTYNAME]->getShareTypes()); } - #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'sharesGetPropertiesDataProvider')] + #[DataProvider(methodName: 'sharesGetPropertiesDataProvider')] public function testPreloadThenGetProperties(array $shareTypes): void { $sabreNode1 = $this->createMock(File::class); $sabreNode1->method('getId') @@ -280,4 +298,322 @@ public function testGetPropertiesSkipChunks(): void { $result = $propFind->getResultForMultiStatus(); $this->assertCount(1, $result[404]); } + + private function mockShare(string $id): IShare&MockObject { + $share = $this->createMock(IShare::class); + $share->method('getId') + ->willReturn($id); + + return $share; + } + + private function mockStorage(?IShare $share = null): (IStorage&MockObject)|(ISharedStorage&MockObject) { + $storage = $this->createMock($share === null ? IStorage::class : ISharedStorage::class); + + if ($share !== null) { + $storage->method('getShare') + ->willReturn($share); + $storage->method('instanceOfStorage') + ->willReturnCallback(static fn (string $class): bool => $class === ISharedStorage::class); + } + + return $storage; + } + + private function mockSourceNode( + IStorage $storage, + bool $shareable = false, + bool $deletable = false, + string $internalPath = 'source.txt', + ?IMountPoint $mountPoint = null, + ?Folder $parent = null, + ): FileNode&MockObject { + $node = $this->createMock(FileNode::class); + $node->method('getStorage') + ->willReturn($storage); + $node->method('isShareable') + ->willReturn($shareable); + $node->method('isDeletable') + ->willReturn($deletable); + $node->method('getInternalPath') + ->willReturn($internalPath); + $node->method('getMountPoint') + ->willReturn($mountPoint); + if ($parent !== null) { + $node->method('getParent') + ->willReturn($parent); + } + + return $node; + } + + private function mockMount(string $mountPoint, bool $shareOwnerless): IMountPoint&MockObject { + /** @var IMountPoint&MockObject $mount */ + $mount = $shareOwnerless + ? $this->createMockForIntersectionOfInterfaces([IMountPoint::class, IShareOwnerlessMount::class]) + : $this->createMock(IMountPoint::class); + $mount->method('getMountPoint') + ->willReturn($mountPoint); + + return $mount; + } + + private function mockUserRoot(): IUserFolder { + if ($this->userRoot === null) { + $userRoot = $this->createMock(IUserFolder::class); + $userRoot->method('getPath') + ->willReturn(self::USER_ROOT); + $this->rootFolder->method('getUserFolder') + ->with('user1') + ->willReturn($userRoot); + $this->userRoot = $userRoot; + } + + return $this->userRoot; + } + + /** + * Mocks the folder at $path, with its ancestors up to the user root as parents + */ + private function mockFolder(string $path, IMountPoint $mountPoint): Folder&MockObject { + $parent = $this->mockUserRoot(); + $currentPath = self::USER_ROOT; + foreach (explode('/', substr($path, strlen(self::USER_ROOT) + 1)) as $name) { + $currentPath .= '/' . $name; + $folder = $this->createMock(Folder::class); + $folder->method('getPath') + ->willReturn($currentPath); + $folder->method('getParent') + ->willReturn($parent); + $folder->method('getMountPoint') + ->willReturn($mountPoint); + $parent = $folder; + } + + return $folder; + } + + /** + * @param array> $shareIdsByPath ids of the shares returned by getSharesBy() for a node path + * @param array> $receivedIdsByPath ids of the shares returned by getSharedWith() for a node path + */ + private function mockSharesByPath(array $shareIdsByPath, array $receivedIdsByPath): 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); + + $this->shareManager->method('getSharesBy') + ->willReturnCallback(function (string $userId, int $shareType, FileNode $node) use ($sharesByPath): array { + $this->sharesRequestedFor[] = $node->getPath(); + return $shareType === IShare::TYPE_USER ? ($sharesByPath[$node->getPath()] ?? []) : []; + }); + $this->shareManager->method('getSharedWith') + ->willReturnCallback(static fn (string $userId, int $shareType, FileNode $node): array + => $shareType === IShare::TYPE_USER ? ($receivedByPath[$node->getPath()] ?? []) : []); + } + + /** + * @param ?IShare $share the share the target is part of, or null when the target is not shared + */ + private function mockTargetNode(?IShare $share): FileNode&MockObject { + $node = $this->createMock(FileNode::class); + $node->method('getPath') + ->willReturn(self::USER_ROOT . '/othersdir'); + + $this->shareManager->method('getSharesBy') + ->willReturn([]); + $this->shareManager->method('getSharedWith') + ->willReturnCallback(static fn (string $userId, int $shareType): array + => ($share !== null && $shareType === IShare::TYPE_USER) ? [$share] : []); + + if ($share === null) { + // when the target itself is not shared, getSharesForNode() walks up to the user root + $node->method('getParent') + ->willReturn($this->mockUserRoot()); + } + + return $node; + } + + /** + * @param array $nodes nodes keyed by their dav path + * @param list $missing paths the tree should report as not found + */ + private function mockTree(array $nodes, array $missing = []): void { + $this->tree->method('getNodeForPath') + ->willReturnCallback(function (string $path) use ($nodes, $missing): Node { + if (in_array($path, $missing, true)) { + throw new NotFound($path); + } + + $this->assertArrayHasKey($path, $nodes, 'unexpected tree lookup'); + $sabreNode = $this->createMock(Node::class); + $sabreNode->method('getNode') + ->willReturn($nodes[$path]); + return $sabreNode; + }); + } + + public function testValidateMoveOrCopyAllowsRenameInSameFolder(): void { + $this->tree->expects($this->never()) + ->method('getNodeForPath'); + + $this->assertTrue($this->plugin->validateMoveOrCopy(self::SOURCE, 'files/user1/somedir/renamed.txt')); + } + + public function testValidateMoveOrCopyAllowsShareableSource(): void { + $this->shareManager->expects($this->never()) + ->method('getSharesBy'); + $this->shareManager->expects($this->never()) + ->method('getSharedWith'); + + $this->mockTree([ + self::SOURCE => $this->mockSourceNode($this->mockStorage(), shareable: true), + self::TARGET => $this->createMock(FileNode::class), + ]); + + $result = $this->plugin->validateMoveOrCopy(self::SOURCE, self::TARGET); + $this->assertTrue($result); + } + + public function testValidateMoveOrCopyResolvesParentOfMissingTarget(): void { + $this->mockTree( + [ + self::SOURCE => $this->mockSourceNode($this->mockStorage()), + self::TARGET_PARENT => $this->mockTargetNode($this->mockShare('1')), + ], + [self::TARGET], + ); + + $this->expectException(Forbidden::class); + $this->expectExceptionMessage('You cannot move a non-shareable node into a share'); + + $this->plugin->validateMoveOrCopy(self::SOURCE, self::TARGET); + } + + public static function validateMoveOrCopyProvider(): array { + // source share id (null: not a share), target share id (null: not shared), source deletable, + // source internal path, source on a share-ownerless mount, allowed + return [ + 'target not shared' => [null, null, false, 'source.txt', false, true], + 'non-shareable source into a share' => [null, '1', false, 'source.txt', false, false], + 'within the same share' => ['42', '42', false, 'source.txt', false, true], + 'deletable share source into another share' => ['1', '2', true, 'source.txt', false, true], + 'non-deletable share source into another share' => ['1', '2', false, 'source.txt', false, false], + 'share root into another share' => ['1', '2', true, '', false, false], + // a received share on a share-ownerless mount is handled as a share + 'share root on a share-ownerless mount into another share' => ['1', '2', true, '', true, false], + ]; + } + + #[DataProvider(methodName: 'validateMoveOrCopyProvider')] + public function testValidateMoveOrCopy( + ?string $sourceShareId, + ?string $targetShareId, + bool $deletable, + string $internalPath, + bool $ownerlessMount, + bool $allowed, + ): void { + $sourceNode = $this->mockSourceNode( + $sourceShareId === null ? $this->mockStorage() : $this->mockStorage($this->mockShare($sourceShareId)), + deletable: $deletable, + internalPath: $internalPath, + mountPoint: $ownerlessMount ? $this->mockMount(self::GROUP_FOLDER_MOUNT, true) : null, + ); + $targetNode = $this->mockTargetNode($targetShareId === null ? null : $this->mockShare($targetShareId)); + + if (!$allowed) { + $this->expectException(Forbidden::class); + $this->expectExceptionMessage('You cannot move a non-shareable node into a share'); + } + + $this->mockTree([ + self::SOURCE => $sourceNode, + self::TARGET => $targetNode, + ]); + + $result = $this->plugin->validateMoveOrCopy(self::SOURCE, self::TARGET); + + $this->assertTrue($result); + } + + public static function validateMoveOrCopyFromOwnerlessMountProvider(): array { + // source parent, target, target mount point, target mount is share-ownerless, share ids by path, + // received share ids by path, paths whose shares must not be looked up, allowed + return [ + 'target below the same shares' => [ + '/user1/files/GF/A/B', '/user1/files/GF/A/C', self::GROUP_FOLDER_MOUNT, true, + ['/user1/files/GF/A' => ['1']], [], ['/user1/files/GF/A'], true, + ], + 'target is a new share' => [ + '/user1/files/GF/B', '/user1/files/GF/A', self::GROUP_FOLDER_MOUNT, true, + ['/user1/files/GF/A' => ['1']], [], [], false, + ], + 'target below a new share' => [ + '/user1/files/GF/B', '/user1/files/GF/A/L/deeper', self::GROUP_FOLDER_MOUNT, true, + ['/user1/files/GF/A' => ['1']], [], [], false, + ], + 'share above the target mount' => [ + '/user1/files/GF/B', '/user1/files/Projects/ext/sub', '/user1/files/Projects/ext/', true, + ['/user1/files/Projects' => ['1']], [], ['/user1/files/Projects'], true, + ], + 'shared folder on another mount' => [ + '/user1/files/GF/B', '/user1/files/Public/sub', '/user1/', false, + ['/user1/files/Public' => ['1']], [], [], false, + ], + 'received share' => [ + '/user1/files/GF/B', '/user1/files/Received', '/user1/files/Received/', false, + [], ['/user1/files/Received' => ['1']], [], false, + ], + 'below a received share' => [ + '/user1/files/GF/B', '/user1/files/Received/sub', '/user1/files/Received/', false, + [], ['/user1/files/Received' => ['1']], [], false, + ], + ]; + } + + #[DataProvider(methodName: 'validateMoveOrCopyFromOwnerlessMountProvider')] + public function testValidateMoveOrCopyFromOwnerlessMount( + string $sourceParentPath, + string $targetPath, + string $targetMountPoint, + bool $targetMountIsOwnerless, + array $shareIdsByPath, + array $receivedIdsByPath, + array $notLookedUp, + bool $allowed, + ): void { + $this->mockSharesByPath($shareIdsByPath, $receivedIdsByPath); + + if (!$allowed) { + $this->expectException(Forbidden::class); + $this->expectExceptionMessage('You cannot move a non-shareable node into a share'); + } + + $targetMount = $this->mockMount($targetMountPoint, $targetMountIsOwnerless); + $groupFolder = $this->mockMount(self::GROUP_FOLDER_MOUNT, true); + $sourceNode = $this->mockSourceNode( + $this->mockStorage(), + mountPoint: $groupFolder, + parent: $this->mockFolder($sourceParentPath, $groupFolder), + ); + + $davSource = 'files/user1' . substr($sourceParentPath, strlen(self::USER_ROOT)) . '/source.txt'; + $davTargetParent = 'files/user1' . substr($targetPath, strlen(self::USER_ROOT)); + $this->mockTree( + [ + $davSource => $sourceNode, + $davTargetParent => $this->mockFolder($targetPath, $targetMount), + ], + [$davTargetParent . '/source.txt'], + ); + + $result = $this->plugin->validateMoveOrCopy($davSource, $davTargetParent . '/source.txt'); + + $this->assertTrue($result); + + foreach ($notLookedUp as $path) { + $this->assertNotContains($path, $this->sharesRequestedFor); + } + } }