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; } } 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