From 63001237cc66de7a9e6e7026f3d2734ea7c2e140 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 29 Oct 2026 17:27:46 +0100 Subject: [PATCH 1/2] test(dav): cover moves within own share when sharing is disabled When sharing is disabled for a user, their home storage loses the share permission, so validateMoveOrCopy() refuses to move or copy any of their files into a folder that has a shared ancestor, even when source and target are covered by the same share and no new recipient gains access. The new unit and integration tests for moves within such a share fail until this is fixed. Tests for moves that do expose the file to new recipients (a share not covering the source, a reshare by a recipient, a received share) document the behavior that has to be kept. Refs: https://github.com/nextcloud/server/issues/65433 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen --- .../unit/Connector/Sabre/SharesPluginTest.php | 110 +++++++++++++++++- .../features/bootstrap/SharingContext.php | 2 + .../sharing_features/sharing-v1-part4.feature | 77 ++++++++++++ 3 files changed, 186 insertions(+), 3 deletions(-) diff --git a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php b/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php index 20c76e165e738..29e2a0ada5db4 100644 --- a/apps/dav/tests/unit/Connector/Sabre/SharesPluginTest.php +++ b/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> $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 + * @param array> $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); + } + } } diff --git a/build/integration/features/bootstrap/SharingContext.php b/build/integration/features/bootstrap/SharingContext.php index 4bf6918650eba..cedab3548cb78 100644 --- a/build/integration/features/bootstrap/SharingContext.php +++ b/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']); } diff --git a/build/integration/sharing_features/sharing-v1-part4.feature b/build/integration/sharing_features/sharing-v1-part4.feature index 7ba1234a6500e..b2bf6aa17f24c 100644 --- a/build/integration/sharing_features/sharing-v1-part4.feature +++ b/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 From a32e66419d97045b97dd34a8c19b4da8cbbbbb92 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 29 Oct 2026 17:32:52 +0100 Subject: [PATCH 2/2] fix(dav): allow moves within own share when sharing is disabled When sharing is disabled for a user, the sharing_mask wrapper strips the share permission from their home storage. validateMoveOrCopy() then refused every move or copy of their files into a folder with a shared ancestor, even when source and target are below the same share. Handle sources on the user's home storage like share-ownerless mounts: only refuse the operation when it exposes the node to new recipients. Look up shares including reshares, so shares created by recipients on subfolders of the user's own share are still taken into account. Fixes: https://github.com/nextcloud/server/issues/65433 Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: Ferdinand Thiessen --- apps/dav/lib/Connector/Sabre/SharesPlugin.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/SharesPlugin.php b/apps/dav/lib/Connector/Sabre/SharesPlugin.php index d0119bfc577fc..c3968f3c29780 100644 --- a/apps/dav/lib/Connector/Sabre/SharesPlugin.php +++ b/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; } }