From b735ee56605afe00f22587ed867d774f8a939450 Mon Sep 17 00:00:00 2001 From: Daniel Kesselberg Date: Mon, 1 Dec 2025 14:09:28 +0100 Subject: [PATCH] fix(sharing): do not notify remote if owner and sharee are local users If both the owner and the sharee are local users, no remote notification is required. If either the owner or the sharee is not a local user, the change must be propagated. Based on https://github.com/owncloud/core/pull/37534 Signed-off-by: Daniel Kesselberg Co-authored-by: Victor Dubiniuk --- .../lib/FederatedShareProvider.php | 26 +- ...ederatedShareProviderReshareRemoteTest.php | 417 ++++++++++++++++++ .../tests/FederatedShareProviderTest.php | 6 +- 3 files changed, 436 insertions(+), 13 deletions(-) create mode 100644 apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php diff --git a/apps/federatedfilesharing/lib/FederatedShareProvider.php b/apps/federatedfilesharing/lib/FederatedShareProvider.php index 1d1c00772c5bf..59087efaca544 100644 --- a/apps/federatedfilesharing/lib/FederatedShareProvider.php +++ b/apps/federatedfilesharing/lib/FederatedShareProvider.php @@ -329,14 +329,29 @@ public function update(IShare $share): IShare { ->set('expiration', $qb->createNamedParameter($share->getExpirationDate(), IQueryBuilder::PARAM_DATETIME_MUTABLE)) ->executeStatement(); - // send the updated permission to the owner/initiator, if they are not the same - if ($share->getShareOwner() !== $share->getSharedBy()) { + /* + * If the share owner and share initiator are on the same instance, + * then we're done here as the share was just updated above. + * + * However, if the share owner/sharee is on a remote instance (and thus we're dealing with a federated share), + * then we are supposed to let the share owner/ sharee on the remote instance know. + */ + if ($this->shouldNotifyRemote($share)) { $this->sendPermissionUpdate($share); } return $share; } + /** + * Notify owner/sharee if they are not the same and ANY of them is a remote user. + */ + protected function shouldNotifyRemote(IShare $share): bool { + $ownerOrSharerIsRemoteUser = !$this->userManager->userExists($share->getShareOwner()) + || !$this->userManager->userExists($share->getSharedBy()); + return $ownerOrSharerIsRemoteUser && $share->getShareOwner() !== $share->getSharedBy(); + } + /** * Send the updated permission to the owner/initiator, if they are not the same. * @@ -466,13 +481,8 @@ public function delete(IShare $share) { * @throws HintException */ protected function revokeShare($share, $isOwner) { - if ($this->userManager->userExists($share->getShareOwner()) && $this->userManager->userExists($share->getSharedBy())) { - // If both the owner and the initiator of the share are local users we don't have to notify anybody else - return; - } - // also send a unShare request to the initiator, if this is a different user than the owner - if ($share->getShareOwner() !== $share->getSharedBy()) { + if ($this->shouldNotifyRemote($share)) { if ($isOwner) { [, $remote] = $this->addressHandler->splitUserRemote($share->getSharedBy()); } else { diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php new file mode 100644 index 0000000000000..65cecaf1580c1 --- /dev/null +++ b/apps/federatedfilesharing/tests/FederatedShareProviderReshareRemoteTest.php @@ -0,0 +1,417 @@ +connection = $this->createMock(IDBConnection::class); + $this->addressHandler = $this->createMock(AddressHandler::class); + $this->notifications = $this->createMock(Notifications::class); + $this->tokenHandler = $this->createMock(TokenHandler::class); + $this->l10n = $this->createMock(IL10N::class); + $this->rootFolder = $this->createMock(IRootFolder::class); + $this->config = $this->createMock(IConfig::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->cloudIdManager = $this->createMock(ICloudIdManager::class); + $this->gsConfig = $this->createMock(GlobalScaleConfig::class); + $this->cloudFederationProviderManager = $this->createMock(ICloudFederationProviderManager::class); + $this->logger = new NullLogger(); + + $this->shareProvider = new FederatedShareProvider( + $this->connection, + $this->addressHandler, + $this->notifications, + $this->tokenHandler, + $this->l10n, + $this->rootFolder, + $this->config, + $this->userManager, + $this->cloudIdManager, + $this->gsConfig, + $this->cloudFederationProviderManager, + $this->logger, + ); + } + + /** + * This test case validates that requestReShare is called when creating a federated share. + * + * We have three actors: + * + * jane@https://origin.test + * alice@https://local.test + * bob@https://destination.test + * + * Jane shared the folder with Alice which re-shares the folder with Bob. + * + * The expected outcome is, that Alice sends a request to Jane to share the folder with Bob. + */ + public function testCreateRemoteOwner(): void { + $permissions = Constants::PERMISSION_READ | Constants::PERMISSION_CREATE | Constants::PERMISSION_UPDATE | Constants::PERMISSION_DELETE; + + $node = $this->createMock(Folder::class); + $node->method('getId')->willReturn(1000); + $node->method('getName')->willReturn('Share 1'); + + /* + * Mocks getSharedWith ($alreadyShared and $alreadySharedGroup). + * The share we are going to create does not already exist. + */ + $expr1 = $this->createMock(IExpressionBuilder::class); + $expr1->method('in')->willReturn(''); + $expr1->method('eq')->willReturn(''); + + $result1 = $this->createMock(IResult::class); + $result1->method('fetchAssociative')->willReturn(false); + + $qb1 = $this->createMock(IQueryBuilder::class); + $qb1->method('select')->willReturnSelf(); + $qb1->method('from')->willReturnSelf(); + $qb1->method('where')->willReturnSelf(); + $qb1->method('expr')->willReturn($expr1); + $qb1->method('createNamedParameter')->willReturn(''); + $qb1->method('executeQuery')->willReturn($result1); + + /* + * Mocks for getShareFromExternalShareTable. + * The share we are going to create is an external share. + */ + $expr2 = $this->createMock(IExpressionBuilder::class); + $expr2->method('eq')->willReturn(''); + + $result2 = $this->createMock(IResult::class); + $result2->method('fetchAllAssociative')->willReturn([ + [ + 'id' => 100000, + 'parent' => -1, + 'share_type' => 0, + 'remote' => 'https://origin.test/', + 'remote_id' => '10', + 'share_token' => 'share_token1', + 'password' => '', + 'name' => '/Share1', + 'owner' => 'jane', // owner in share_external is the user on the remote instance + 'user' => 'alice', // user in share_external is the receiver on the current instance + 'mountpoint' => '/Share1', + 'mountpoint_hash' => '94ee935396a30e27953838d0f65d1e17', // md5(mountpoint) + 'accepted' => 1, + ], + ]); + + $qb2 = $this->createMock(IQueryBuilder::class); + $qb2->method('select')->willReturnSelf(); + $qb2->method('from')->willReturnSelf(); + $qb2->method('where')->willReturnSelf(); + $qb2->method('expr')->willReturn($expr2); + $qb2->method('createNamedParameter')->willReturn(''); + $qb2->method('executeQuery')->willReturn($result2); + + /* + * Mocks for addShareToDB. + * The record on the local instance for the outgoing share. + */ + $expr3 = $this->createMock(IExpressionBuilder::class); + $expr3->method('eq')->willReturn(''); + + $result3 = $this->createMock(IResult::class); + $result3->method('fetchAllAssociative')->willReturn([ + [ + 'id' => 100000, + 'parent' => -1, + 'share_type' => 0, + 'remote' => 'https://origin.test/', + 'remote_id' => '10', + 'share_token' => 'share_token2', + 'password' => '', + 'name' => '/Share1', + 'owner' => 'jane', // owner in share_external is the user on the remote instance + 'user' => 'alice', // user in share_external is the receiver on the current instance + 'mountpoint' => '/Share1', + 'mountpoint_hash' => '94ee935396a30e27953838d0f65d1e17', // md5(mountpoint) + 'accepted' => 1, + ], + ]); + + $qb3 = $this->createMock(IQueryBuilder::class); + $qb3->method('insert')->willReturnSelf(); + $qb3->method('setValue')->willReturnSelf(); + $qb3->method('getLastInsertId')->willReturn(2000); + + /* + * Mocks for updateSuccessfulReShare + */ + $expr4 = $this->createMock(IExpressionBuilder::class); + $expr4->method('eq')->willReturn(''); + + $qb4 = $this->createMock(IQueryBuilder::class); + $qb4->method('update')->willReturnSelf(); + $qb4->method('where')->willReturnSelf(); + $qb4->method('expr')->willReturn($expr4); + $qb4->method('set')->willReturnSelf(); + $qb4->method('createNamedParameter')->willReturn(''); + + /* + * Mocks for storeRemoteId. + */ + $qb5 = $this->createMock(IQueryBuilder::class); + $qb5->method('insert')->willReturnSelf(); + $qb5->method('values')->willReturnSelf(); + + /* + * Mocks for getRawShare. + */ + $expr6 = $this->createMock(IExpressionBuilder::class); + $expr6->method('eq')->willReturn(''); + + $result6 = $this->createMock(IResult::class); + $result6->method('fetchAssociative')->willReturn([ + 'id' => 20000, + 'share_type' => IShare::TYPE_REMOTE, + 'share_with' => 'bob@https://destination.test', + 'password' => null, + 'uid_owner' => 'jane@origin.test', + 'uid_initiator' => 'alice', + 'parent' => null, + 'item_type' => 'folder', + 'item_source' => (string)$node->getId(), + 'item_target' => null, + 'file_source' => $node->getId(), + 'file_target' => '', + 'permissions' => $permissions, + 'stime' => 0, + 'accepted' => 0, + 'expiration' => null, + 'token' => 'share_token3', + 'mail_send' => 0, + 'share_name' => null, + 'password_by_talk' => 0, + 'note' => null, + 'hide_download' => 0, + 'label' => null, + 'attributes' => null, + 'password_expiration_time' => null, + 'reminder_sent' => 0, + ]); + + $qb6 = $this->createMock(IQueryBuilder::class); + $qb6->method('select')->willReturnSelf(); + $qb6->method('from')->willReturnSelf(); + $qb6->method('where')->willReturnSelf(); + $qb6->method('expr')->willReturn($expr6); + $qb6->method('createNamedParameter')->willReturn(''); + $qb6->method('executeQuery')->willReturn($result6); + + + $queryBuilderMatcher = $this->exactly(8); + $this->connection + ->expects($queryBuilderMatcher) + ->method('getQueryBuilder') + ->willReturnCallback(function () use ($queryBuilderMatcher, $qb1, $qb2, $qb3, $qb4, $qb5, $qb6) { + return match ($queryBuilderMatcher->numberOfInvocations()) { + 1, 2 => $qb1, + 3, 5 => $qb2, + 4 => $qb3, + 6 => $qb4, + 7 => $qb5, + 8 => $qb6, + default => throw new LogicException('Unexpected number of invocations for getQueryBuilder') + }; + }); + + // the cloud id for the recipient + $this->cloudIdManager->method('resolveCloudId') + ->with('bob@https://destination.test') + ->willReturn(new CloudId( + 'bob@https://destination.test', + 'bob', + 'https://destination.test', + 'Bob', // is usually null in prod, setting it here to avoid additional mocking + )); + + $this->addressHandler->method('generateRemoteURL') + ->willReturn('https://local.test'); + $this->addressHandler->method('compareAddresses') + ->willReturn(false); + + // the cloud id of the actual owner + $this->cloudIdManager->method('getCloudId') + ->willReturn(new CloudId( + 'jane@https://origin.test', + 'jane', + 'https://origin.test', + 'Jane', // is usually null in prod, setting it here to avoid additional mocking + )); + + $this->notifications->expects($this->once()) + ->method('requestReShare') + ->with( + $this->equalTo('share_token1'), + $this->equalTo('10'), + $this->equalTo('2000'), + $this->equalTo('https://origin.test/'), + $this->equalTo('bob@https://destination.test'), + $this->equalTo($permissions), + $this->equalTo('Share 1'), + $this->equalTo(IShare::TYPE_REMOTE), + ) + ->willReturn(['share_token2', '20']); + + $share = new Share($this->rootFolder, $this->userManager); + $share + ->setSharedWith('bob@https://destination.test') + ->setShareOwner('alice') + ->setSharedBy('alice') + ->setPermissions($permissions) + ->setShareType(IShare::TYPE_REMOTE) + ->setNode($node) + ->setTarget('/Share1'); + + $this->shareProvider->create($share); + } + + /** + * This test case validates that sendPermission is called when updating a federated share. + * + * We have three actors: + * + * jane@https://origin.test + * alice@https://local.test + * bob@https://destination.test + * + * Jane shared the folder with Alice which re-shared the folder with Bob. + * Alice is now changing the permissions for the share. + * + * The expected outcome is, that Alice sends a request to Jane to change the share. + */ + public function testUpdateRemoteOwner(): void { + $permissions = Constants::PERMISSION_READ; + + $node = $this->createMock(Folder::class); + $node->method('getId')->willReturn(1000); + $node->method('getName')->willReturn('Share 1'); + + /* + * Mocks update share. + */ + $expr1 = $this->createMock(IExpressionBuilder::class); + $expr1->method('eq')->willReturn(''); + + $qb1 = $this->createMock(IQueryBuilder::class); + $qb1->method('update')->willReturnSelf(); + $qb1->method('where')->willReturnSelf(); + $qb1->method('expr')->willReturn($expr1); + $qb1->method('createNamedParameter')->willReturn(''); + $qb1->method('set')->willReturnSelf(); + + /* + * Mocks getRemoteId. + */ + $expr2 = $this->createMock(IExpressionBuilder::class); + $expr2->method('eq')->willReturn(''); + + $result2 = $this->createMock(IResult::class); + $result2->method('fetchAssociative')->willReturn([ + 'share_id' => 3000, + 'remote_id' => '10', + ]); + + $qb2 = $this->createMock(IQueryBuilder::class); + $qb2->method('select')->willReturnSelf(); + $qb2->method('from')->willReturnSelf(); + $qb2->method('where')->willReturnSelf(); + $qb2->method('expr')->willReturn($expr2); + $qb2->method('createNamedParameter')->willReturn(''); + $qb2->method('executeQuery')->willReturn($result2); + + $queryBuilderMatcher = $this->exactly(2); + $this->connection + ->expects($queryBuilderMatcher) + ->method('getQueryBuilder') + ->willReturnCallback(function () use ($queryBuilderMatcher, $qb1, $qb2) { + return match ($queryBuilderMatcher->numberOfInvocations()) { + 1 => $qb1, + 2 => $qb2, + default => throw new LogicException('Unexpected number of invocations for getQueryBuilder') + }; + }); + + $this->userManager->method('userExists') + ->willReturnMap([ + ['jane@https://origin.test', false], + ['alice', true], + ]); + + $this->addressHandler->method('splitUserRemote') + ->willReturn(['jane', 'https://origin.test']); + + $this->notifications->expects($this->once()) + ->method('sendPermissionChange') + ->with( + $this->equalTo('https://origin.test'), + $this->equalTo('10'), + $this->equalTo('share_token3'), + $this->equalTo($permissions), + ); + + $share = new Share($this->rootFolder, $this->userManager); + $share + ->setId('3000') + ->setToken('share_token3') + ->setSharedWith('bob@https://destination.test') + ->setShareOwner('jane@https://origin.test') + ->setSharedBy('alice') + ->setPermissions($permissions) + ->setShareType(IShare::TYPE_REMOTE) + ->setNode($node) + ->setTarget('/Share1'); + + $this->shareProvider->update($share); + } +} diff --git a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php index 0852a4b548904..b5cb8a85a5351 100644 --- a/apps/federatedfilesharing/tests/FederatedShareProviderTest.php +++ b/apps/federatedfilesharing/tests/FederatedShareProviderTest.php @@ -450,11 +450,7 @@ public function testUpdate(string $owner, string $sharedBy, ?\DateTime $expirati $sharedBy . '@http://localhost' )->willReturn(true); - if ($owner === $sharedBy) { - $this->provider->expects($this->never())->method('sendPermissionUpdate'); - } else { - $this->provider->expects($this->once())->method('sendPermissionUpdate'); - } + $this->provider->expects($this->never())->method('sendPermissionUpdate'); $this->rootFolder->expects($this->never())->method($this->anything());