From 29555e448f370a7e46da69b2587c39b3c05d674e Mon Sep 17 00:00:00 2001 From: Hamza Date: Mon, 17 Aug 2026 16:47:52 +0200 Subject: [PATCH] fix(federated-sharing): avoid duplicate shares Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Hamza --- .../lib/FederatedShareProvider.php | 18 ++++-- ...ederatedShareProviderReshareRemoteTest.php | 16 ++--- .../tests/FederatedShareProviderTest.php | 61 +++++++++++++++++++ 3 files changed, 81 insertions(+), 14 deletions(-) diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 84d329e4f93c5..1b6c5589ce888 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -30,6 +30,7 @@ use OCP\IUserManager; use OCP\Security\ISecureRandom; use OCP\Server; +use OCP\Share\Exceptions\AlreadySharedException; use OCP\Share\Exceptions\GenericShareException; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IShare; @@ -101,20 +102,25 @@ public function create(IShare $share): IShare { throw new \Exception($message_t); } + $cloudId = $this->cloudIdManager->resolveCloudId($shareWith); + /* - * Check if file is not already shared with the remote user + * Check if file is not already shared with the remote user. + * Has to be looked up by the normalized cloud ID, because that is what + * gets stored below. Otherwise spellings like "user@server.com/" slip + * past this check and create a duplicate share. */ - $alreadyShared = $this->getSharedWith($shareWith, IShare::TYPE_REMOTE, $share->getNode(), 1, 0); - $alreadySharedGroup = $this->getSharedWith($shareWith, IShare::TYPE_REMOTE_GROUP, $share->getNode(), 1, 0); - if (!empty($alreadyShared) || !empty($alreadySharedGroup)) { + // getSharedWith() ignores its $shareType argument and always + // queries all supported remote types, so a single lookup covers both. + $alreadyShared = $this->getSharedWith($cloudId->getId(), IShare::TYPE_REMOTE, $share->getNode(), 1, 0); + if (!empty($alreadyShared)) { $message = 'Sharing %1$s failed, because this item is already shared with %2$s'; $message_t = $this->l->t('Sharing %1$s failed, because this item is already shared with the account %2$s', [$share->getNode()->getName(), $shareWith]); $this->logger->debug(sprintf($message, $share->getNode()->getName(), $shareWith), ['app' => 'Federated File Sharing']); - throw new \Exception($message_t); + throw new AlreadySharedException($message_t, $alreadyShared[0]); } // don't allow federated shares if source and target server are the same - $cloudId = $this->cloudIdManager->resolveCloudId($shareWith); $currentServer = $this->addressHandler->generateRemoteURL(); $currentUser = $sharedBy; if ($this->addressHandler->compareAddresses($cloudId->getUser(), $cloudId->getRemote(), $currentUser, $currentServer)) { diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php index fff9fb23d4801..783881be8409c 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php @@ -100,7 +100,7 @@ public function testCreateRemoteOwner(): void { $node->method('getName')->willReturn('Share 1'); /* - * Mocks getSharedWith ($alreadyShared and $alreadySharedGroup). + * Mocks getSharedWith ($alreadyShared). * The share we are going to create does not already exist. */ $expr1 = $this->createMock(IExpressionBuilder::class); @@ -247,18 +247,18 @@ public function testCreateRemoteOwner(): void { $qb6->method('createNamedParameter')->willReturn(''); $qb6->method('executeQuery')->willReturn($result6); - $queryBuilderMatcher = $this->exactly(7); + $queryBuilderMatcher = $this->exactly(6); $this->connection ->expects($queryBuilderMatcher) ->method('getQueryBuilder') ->willReturnCallback(function () use ($queryBuilderMatcher, $qb1, $qb2, $qb3, $qb4, $qb5, $qb6) { return match ($queryBuilderMatcher->numberOfInvocations()) { - 1, 2 => $qb1, - 3 => $qb2, - 4 => $qb3, - 5 => $qb4, - 6 => $qb5, - 7 => $qb6, + 1 => $qb1, + 2 => $qb2, + 3 => $qb3, + 4 => $qb4, + 5 => $qb5, + 6 => $qb6, default => throw new LogicException('Unexpected number of invocations for getQueryBuilder') }; }); diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index a4d0dd5695508..8ee7731cfc096 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -32,6 +32,7 @@ use OCP\IUserManager; use OCP\Security\ISecureRandom; use OCP\Server; +use OCP\Share\Exceptions\AlreadySharedException; use OCP\Share\IManager; use OCP\Share\IShare; use PHPUnit\Framework\MockObject\MockObject; @@ -418,6 +419,66 @@ public function testCreateAlreadyShared(): void { } } + public static function dataTestCreateAlreadySharedWithEquivalentCloudId(): array { + return [ + ['user@server.com'], + ['user@server.com/'], + ['user@server.com/index.php'], + ]; + } + + /** + * Sharing a node with a recipient it is already shared with has to be + * rejected with an AlreadySharedException, for every spelling that + * normalizes to the same cloud ID. + */ + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestCreateAlreadySharedWithEquivalentCloudId')] + public function testCreateAlreadySharedWithEquivalentCloudId(string $shareWith): void { + $node = $this->createMock(File::class); + $node->method('getId')->willReturn(42); + $node->method('getName')->willReturn('myFile'); + + $this->addressHandler->expects($this->any())->method('splitUserRemote') + ->willReturn(['user', 'server.com']); + $this->addressHandler->expects($this->any())->method('generateRemoteURL') + ->willReturn('http://localhost/'); + $this->tokenHandler->method('generateToken')->willReturn('token'); + $this->contactsManager->expects($this->any())->method('search') + ->willReturn([]); + $this->notifications->expects($this->once()) + ->method('sendRemoteShare') + ->willReturn(true); + + $share = $this->shareManager->newShare(); + $share->setSharedWith('user@server.com') + ->setSharedBy('sharedBy') + ->setShareOwner('shareOwner') + ->setPermissions(19) + ->setShareType(IShare::TYPE_REMOTE) + ->setNode($node) + ->setTarget(''); + $existingShare = $this->provider->create($share); + + // a recipient of the node re-sharing it with the same account + $duplicate = $this->shareManager->newShare(); + $duplicate->setSharedWith($shareWith) + ->setSharedBy('otherRecipient') + ->setShareOwner('shareOwner') + ->setPermissions(19) + ->setShareType(IShare::TYPE_REMOTE) + ->setNode($node) + ->setTarget(''); + + try { + $this->provider->create($duplicate); + $this->fail('Expected an AlreadySharedException'); + } catch (AlreadySharedException $e) { + $this->assertEquals($existingShare->getId(), $e->getExistingShare()->getId()); + } + + $this->assertCount(1, $this->provider->getSharesByPath($node)); + } + #[\PHPUnit\Framework\Attributes\DataProvider(methodName: 'dataTestUpdate')] public function testUpdate(string $owner, string $sharedBy, ?\DateTime $expirationDate): void { $this->provider = $this->getMockBuilder(FederatedShareProvider::class)