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/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)