From 024f98a8a15e5ebb6bca8fb7b9a611ed1e3c9c31 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Wed, 7 Aug 2024 11:13:35 +0200 Subject: [PATCH 01/10] fix: delete re-shares when deleting the parent share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Note: Removed part about fix command from original PR Signed-off-by: Luka Trovic Signed-off-by: Côme Chilliet (cherry picked from commit 42181c2f490025860e22907255b6917583c798af) --- .../lib/Service/OwnershipTransferService.php | 3 + .../tests/EtagPropagationTest.php | 3 +- lib/private/Share20/Manager.php | 69 ++++++++++ tests/lib/Share20/ManagerTest.php | 120 +++++++++++++++++- 4 files changed, 190 insertions(+), 5 deletions(-) diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php index d4bc3033b1b50..c47e51923ce52 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -469,6 +469,9 @@ private function restoreShares( } } catch (\OCP\Files\NotFoundException $e) { $output->writeln('Share with id ' . $share->getId() . ' points at deleted file, skipping'); + } catch (\OCP\Share\Exceptions\GenericShareException $e) { + $output->writeln('Share with id ' . $share->getId() . ' is broken, deleting'); + $this->shareManager->deleteShare($share); } catch (\Throwable $e) { $output->writeln('Could not restore share with id ' . $share->getId() . ':' . $e->getMessage() . ' : ' . $e->getTraceAsString() . ''); } diff --git a/apps/files_sharing/tests/EtagPropagationTest.php b/apps/files_sharing/tests/EtagPropagationTest.php index 3f9ddfc413dc7..327726acc9774 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -277,7 +277,8 @@ public function testOwnerUnshares() { self::TEST_FILES_SHARING_API_USER2, ]); - $this->assertAllUnchanged(); + $this->assertEtagsNotChanged([self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3]); + $this->assertEtagsChanged([self::TEST_FILES_SHARING_API_USER4]); } public function testOwnerUnsharesFlatReshares() { diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 2cc11c2c6346d..c4761aa81cfdc 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -18,6 +18,7 @@ use OCP\Files\IRootFolder; use OCP\Files\Mount\IMountManager; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\HintException; use OCP\IConfig; use OCP\IDateTimeZone; @@ -1031,6 +1032,71 @@ protected function deleteChildren(IShare $share) { return $deletedShares; } + public function deleteReshare(IShare $share) { + // Skip if node not found + try { + $node = $share->getNode(); + } catch (NotFoundException) { + return; + } + + $userIds = []; + + if ($share->getShareType() === IShare::TYPE_USER) { + $userIds[] = $share->getSharedWith(); + } + + if ($share->getShareType() === IShare::TYPE_GROUP) { + $group = $this->groupManager->get($share->getSharedWith()); + $users = $group->getUsers(); + + foreach ($users as $user) { + // Skip if share owner is member of shared group + if ($user->getUID() === $share->getShareOwner()) { + continue; + } + $userIds[] = $user->getUID(); + } + } + + $reshareRecords = []; + $shareTypes = [ + IShare::TYPE_GROUP, + IShare::TYPE_USER, + IShare::TYPE_LINK, + IShare::TYPE_REMOTE, + IShare::TYPE_EMAIL + ]; + + foreach ($userIds as $userId) { + foreach ($shareTypes as $shareType) { + $provider = $this->factory->getProviderForType($shareType); + $shares = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0); + foreach ($shares as $child) { + $reshareRecords[] = $child; + } + } + + if ($share->getNodeType() === 'folder') { + $sharesInFolder = $this->getSharesInFolder($userId, $node, true); + + foreach ($sharesInFolder as $shares) { + foreach ($shares as $child) { + $reshareRecords[] = $child; + } + } + } + } + + foreach ($reshareRecords as $child) { + try { + $this->generalCreateChecks($child); + } catch (GenericShareException $e) { + $this->deleteShare($child); + } + } + } + /** * Delete a share * @@ -1054,6 +1120,9 @@ public function deleteShare(IShare $share) { $provider = $this->factory->getProviderForType($share->getShareType()); $provider->delete($share); + // Delete shares that shared by the "share with user/group" + $this->deleteReshare($share); + $this->dispatcher->dispatchTyped(new ShareDeletedEvent($share)); } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index c0cf0ca9c129b..4acdfc8bc12fe 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -46,6 +46,7 @@ use OCP\Share\Events\ShareDeletedEvent; use OCP\Share\Events\ShareDeletedFromSelfEvent; use OCP\Share\Exceptions\AlreadySharedException; +use OCP\Share\Exceptions\GenericShareException; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; use OCP\Share\IProviderFactory; @@ -227,7 +228,7 @@ public function dataTestDelete() { */ public function testDelete($shareType, $sharedWith) { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -245,6 +246,7 @@ public function testDelete($shareType, $sharedWith) { ->setTarget('myTarget'); $manager->expects($this->once())->method('deleteChildren')->with($share); + $manager->expects($this->once())->method('deleteReshare')->with($share); $this->defaultProvider ->expects($this->once()) @@ -269,7 +271,7 @@ public function testDelete($shareType, $sharedWith) { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -288,6 +290,7 @@ public function testDeleteLazyShare() { $this->rootFolder->expects($this->never())->method($this->anything()); $manager->expects($this->once())->method('deleteChildren')->with($share); + $manager->expects($this->once())->method('deleteReshare')->with($share); $this->defaultProvider ->expects($this->once()) @@ -312,7 +315,7 @@ public function testDeleteLazyShare() { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById']) + ->setMethods(['getShareById', 'deleteReshare']) ->getMock(); $path = $this->createMock(File::class); @@ -469,7 +472,116 @@ public function testDeleteChildren() { $this->assertSame($shares, $result); } - public function testGetShareById() { + public function testDeleteReshareWhenUserHasOneShare(): void { + $manager = $this->createManagerMock() + ->setMethods(['deleteShare', 'getSharesInFolder', 'generalCreateChecks']) + ->getMock(); + + $folder = $this->createMock(Folder::class); + + $share = $this->createMock(IShare::class); + $share->method('getShareType')->willReturn(IShare::TYPE_USER); + $share->method('getNodeType')->willReturn('folder'); + $share->method('getSharedWith')->willReturn('UserB'); + $share->method('getNode')->willReturn($folder); + + $reShare = $this->createMock(IShare::class); + $reShare->method('getSharedBy')->willReturn('UserB'); + $reShare->method('getSharedWith')->willReturn('UserC'); + $reShare->method('getNode')->willReturn($folder); + + $reShareInSubFolder = $this->createMock(IShare::class); + $reShareInSubFolder->method('getSharedBy')->willReturn('UserB'); + + $manager->method('getSharesInFolder')->willReturn([$reShareInSubFolder]); + $manager->method('generalCreateChecks')->willThrowException(new GenericShareException()); + + $this->defaultProvider->method('getSharesBy') + ->willReturn([$reShare]); + + $manager->expects($this->atLeast(2))->method('deleteShare')->withConsecutive([$reShare], [$reShareInSubFolder]); + + $manager->deleteReshare($share); + } + + public function testDeleteReshareWhenUserHasAnotherShare(): void { + $manager = $this->createManagerMock() + ->setMethods(['deleteShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) + ->getMock(); + + $folder = $this->createMock(Folder::class); + + $share = $this->createMock(IShare::class); + $share->method('getShareType')->willReturn(IShare::TYPE_USER); + $share->method('getNodeType')->willReturn('folder'); + $share->method('getSharedWith')->willReturn('UserB'); + $share->method('getNode')->willReturn($folder); + + $reShare = $this->createMock(IShare::class); + $reShare->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShare->method('getNodeType')->willReturn('folder'); + $reShare->method('getSharedBy')->willReturn('UserB'); + $reShare->method('getNode')->willReturn($folder); + + $this->defaultProvider->method('getSharesBy')->willReturn([$reShare]); + $manager->method('getSharesInFolder')->willReturn([]); + $manager->method('generalCreateChecks')->willReturn(true); + + $manager->expects($this->never())->method('deleteShare'); + + $manager->deleteReshare($share); + } + + public function testDeleteReshareOfUsersInGroupShare(): void { + $manager = $this->createManagerMock() + ->setMethods(['deleteShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) + ->getMock(); + + $folder = $this->createMock(Folder::class); + + $userA = $this->createMock(IUser::class); + $userA->method('getUID')->willReturn('userA'); + + $share = $this->createMock(IShare::class); + $share->method('getShareType')->willReturn(IShare::TYPE_GROUP); + $share->method('getNodeType')->willReturn('folder'); + $share->method('getSharedWith')->willReturn('Group'); + $share->method('getNode')->willReturn($folder); + $share->method('getShareOwner')->willReturn($userA); + + $reShare1 = $this->createMock(IShare::class); + $reShare1->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShare1->method('getNodeType')->willReturn('folder'); + $reShare1->method('getSharedBy')->willReturn('UserB'); + $reShare1->method('getNode')->willReturn($folder); + + $reShare2 = $this->createMock(IShare::class); + $reShare2->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShare2->method('getNodeType')->willReturn('folder'); + $reShare2->method('getSharedBy')->willReturn('UserC'); + $reShare2->method('getNode')->willReturn($folder); + + $userB = $this->createMock(IUser::class); + $userB->method('getUID')->willReturn('userB'); + $userC = $this->createMock(IUser::class); + $userC->method('getUID')->willReturn('userC'); + $group = $this->createMock(IGroup::class); + $group->method('getUsers')->willReturn([$userB, $userC]); + $this->groupManager->method('get')->with('Group')->willReturn($group); + + $this->defaultProvider->method('getSharesBy') + ->willReturn([]); + $manager->method('generalCreateChecks')->willThrowException(new GenericShareException()); + + $manager->method('getSharedWith')->willReturn([]); + $manager->expects($this->exactly(2))->method('getSharesInFolder')->willReturnOnConsecutiveCalls([[$reShare1]], [[$reShare2]]); + + $manager->expects($this->exactly(2))->method('deleteShare')->withConsecutive([$reShare1], [$reShare2]); + + $manager->deleteReshare($share); + } + + public function testGetShareById(): void { $share = $this->createMock(IShare::class); $this->defaultProvider From 16decdede7cbdebb3ac2b9455dc6640a66bcbd06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 22 Aug 2024 17:03:07 +0200 Subject: [PATCH 02/10] fix: Tidy up code for reshare deletion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 30 ++++++++++++++++-------------- tests/lib/Share20/ManagerTest.php | 16 ++++++++-------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index c4761aa81cfdc..063a1349b8d78 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1032,31 +1032,32 @@ protected function deleteChildren(IShare $share) { return $deletedShares; } - public function deleteReshare(IShare $share) { - // Skip if node not found + protected function deleteReshares(IShare $share): void { try { $node = $share->getNode(); } catch (NotFoundException) { + /* Skip if node not found */ return; } $userIds = []; - if ($share->getShareType() === IShare::TYPE_USER) { + if ($share->getShareType() === IShare::TYPE_USER) { $userIds[] = $share->getSharedWith(); - } - - if ($share->getShareType() === IShare::TYPE_GROUP) { + } elseif ($share->getShareType() === IShare::TYPE_GROUP) { $group = $this->groupManager->get($share->getSharedWith()); - $users = $group->getUsers(); + $users = $group?->getUsers() ?? []; foreach ($users as $user) { - // Skip if share owner is member of shared group + /* Skip share owner */ if ($user->getUID() === $share->getShareOwner()) { continue; } $userIds[] = $user->getUID(); } + } else { + /* We only support user and group shares */ + return; } $reshareRecords = []; @@ -1065,7 +1066,7 @@ public function deleteReshare(IShare $share) { IShare::TYPE_USER, IShare::TYPE_LINK, IShare::TYPE_REMOTE, - IShare::TYPE_EMAIL + IShare::TYPE_EMAIL, ]; foreach ($userIds as $userId) { @@ -1077,8 +1078,8 @@ public function deleteReshare(IShare $share) { } } - if ($share->getNodeType() === 'folder') { - $sharesInFolder = $this->getSharesInFolder($userId, $node, true); + if ($node instanceof Folder) { + $sharesInFolder = $this->getSharesInFolder($userId, $node, false); foreach ($sharesInFolder as $shares) { foreach ($shares as $child) { @@ -1092,6 +1093,7 @@ public function deleteReshare(IShare $share) { try { $this->generalCreateChecks($child); } catch (GenericShareException $e) { + $this->logger->debug('Delete reshare because of exception '.$e->getMessage(), ['exception' => $e]); $this->deleteShare($child); } } @@ -1120,10 +1122,10 @@ public function deleteShare(IShare $share) { $provider = $this->factory->getProviderForType($share->getShareType()); $provider->delete($share); - // Delete shares that shared by the "share with user/group" - $this->deleteReshare($share); - $this->dispatcher->dispatchTyped(new ShareDeletedEvent($share)); + + // Delete reshares of the deleted share + $this->deleteReshares($share); } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 4acdfc8bc12fe..d51b1bd06ad78 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -228,7 +228,7 @@ public function dataTestDelete() { */ public function testDelete($shareType, $sharedWith) { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshares']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -246,7 +246,7 @@ public function testDelete($shareType, $sharedWith) { ->setTarget('myTarget'); $manager->expects($this->once())->method('deleteChildren')->with($share); - $manager->expects($this->once())->method('deleteReshare')->with($share); + $manager->expects($this->once())->method('deleteReshares')->with($share); $this->defaultProvider ->expects($this->once()) @@ -271,7 +271,7 @@ public function testDelete($shareType, $sharedWith) { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshares']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -290,7 +290,7 @@ public function testDeleteLazyShare() { $this->rootFolder->expects($this->never())->method($this->anything()); $manager->expects($this->once())->method('deleteChildren')->with($share); - $manager->expects($this->once())->method('deleteReshare')->with($share); + $manager->expects($this->once())->method('deleteReshares')->with($share); $this->defaultProvider ->expects($this->once()) @@ -315,7 +315,7 @@ public function testDeleteLazyShare() { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteReshare']) + ->setMethods(['getShareById', 'deleteReshares']) ->getMock(); $path = $this->createMock(File::class); @@ -501,7 +501,7 @@ public function testDeleteReshareWhenUserHasOneShare(): void { $manager->expects($this->atLeast(2))->method('deleteShare')->withConsecutive([$reShare], [$reShareInSubFolder]); - $manager->deleteReshare($share); + self::invokePrivate($manager, 'deleteReshares', [$share]); } public function testDeleteReshareWhenUserHasAnotherShare(): void { @@ -529,7 +529,7 @@ public function testDeleteReshareWhenUserHasAnotherShare(): void { $manager->expects($this->never())->method('deleteShare'); - $manager->deleteReshare($share); + self::invokePrivate($manager, 'deleteReshares', [$share]); } public function testDeleteReshareOfUsersInGroupShare(): void { @@ -578,7 +578,7 @@ public function testDeleteReshareOfUsersInGroupShare(): void { $manager->expects($this->exactly(2))->method('deleteShare')->withConsecutive([$reShare1], [$reShare2]); - $manager->deleteReshare($share); + self::invokePrivate($manager, 'deleteReshares', [$share]); } public function testGetShareById(): void { From f0e10eaeed50e2b899701f289e2b2bd0407bed35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 26 Aug 2024 11:31:39 +0200 Subject: [PATCH 03/10] fix: Transfer incomming shares first, do not delete non-migratable ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Canceling the previous add of deletion of invalid shares in transferownership because in some cases it deletes valid reshares, if incoming shares are not transfered on purpose. Inverting the order of transfer between incoming and outgoing so that reshare can be migrated when incoming shares are transfered. Signed-off-by: Côme Chilliet --- .../lib/Service/OwnershipTransferService.php | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php index c47e51923ce52..08c9f3f719fee 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -150,16 +150,6 @@ public function transfer( $output ); - $destinationPath = $finalTarget . '/' . $path; - // restore the shares - $this->restoreShares( - $sourceUid, - $destinationUid, - $destinationPath, - $shares, - $output - ); - // transfer the incoming shares if ($transferIncomingShares === true) { $sourceShares = $this->collectIncomingShares( @@ -184,6 +174,16 @@ public function transfer( $move ); } + + $destinationPath = $finalTarget . '/' . $path; + // restore the shares + $this->restoreShares( + $sourceUid, + $destinationUid, + $destinationPath, + $shares, + $output + ); } private function sanitizeFolderName(string $name): string { @@ -469,9 +469,6 @@ private function restoreShares( } } catch (\OCP\Files\NotFoundException $e) { $output->writeln('Share with id ' . $share->getId() . ' points at deleted file, skipping'); - } catch (\OCP\Share\Exceptions\GenericShareException $e) { - $output->writeln('Share with id ' . $share->getId() . ' is broken, deleting'); - $this->shareManager->deleteShare($share); } catch (\Throwable $e) { $output->writeln('Could not restore share with id ' . $share->getId() . ':' . $e->getMessage() . ' : ' . $e->getTraceAsString() . ''); } From c7dbe7e71c773587b71d283e341f7c2aaf938028 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Fri, 13 Sep 2024 10:00:53 +0200 Subject: [PATCH 04/10] fix(shares): Promote reshares into direct shares when share is deleted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 063a1349b8d78..cb4e54b47327e 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1032,7 +1032,8 @@ protected function deleteChildren(IShare $share) { return $deletedShares; } - protected function deleteReshares(IShare $share): void { + /* Promote reshares into direct shares so that target user keeps access */ + protected function promoteReshares(IShare $share): void { try { $node = $share->getNode(); } catch (NotFoundException) { @@ -1050,7 +1051,7 @@ protected function deleteReshares(IShare $share): void { foreach ($users as $user) { /* Skip share owner */ - if ($user->getUID() === $share->getShareOwner()) { + if ($user->getUID() === $share->getShareOwner() || $user->getUID() === $share->getSharedBy()) { continue; } $userIds[] = $user->getUID(); @@ -1093,8 +1094,14 @@ protected function deleteReshares(IShare $share): void { try { $this->generalCreateChecks($child); } catch (GenericShareException $e) { - $this->logger->debug('Delete reshare because of exception '.$e->getMessage(), ['exception' => $e]); - $this->deleteShare($child); + /* The check is invalid, promote it to a direct share from the sharer of parent share */ + $this->logger->debug('Promote reshare because of exception ' . $e->getMessage(), ['exception' => $e, 'fullId' => $child->getFullId()]); + try { + $child->setSharedBy($share->getSharedBy()); + $this->updateShare($child); + } catch (GenericShareException|\InvalidArgumentException $e) { + $this->logger->warning('Failed to promote reshare because of exception ' . $e->getMessage(), ['exception' => $e, 'fullId' => $child->getFullId()]); + } } } } @@ -1124,8 +1131,8 @@ public function deleteShare(IShare $share) { $this->dispatcher->dispatchTyped(new ShareDeletedEvent($share)); - // Delete reshares of the deleted share - $this->deleteReshares($share); + // Promote reshares of the deleted share + $this->promoteReshares($share); } From 04b6eef2468116df74908dec9bf94ba95ee29ccd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= <91878298+come-nc@users.noreply.github.com> Date: Sun, 15 Sep 2024 17:16:55 +0200 Subject: [PATCH 05/10] chore: Turn method description into phpdoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Ferdinand Thiessen Signed-off-by: Côme Chilliet <91878298+come-nc@users.noreply.github.com> --- lib/private/Share20/Manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index cb4e54b47327e..ebcebb714a9b1 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1032,7 +1032,7 @@ protected function deleteChildren(IShare $share) { return $deletedShares; } - /* Promote reshares into direct shares so that target user keeps access */ + /** Promote re-shares into direct shares so that target user keeps access */ protected function promoteReshares(IShare $share): void { try { $node = $share->getNode(); From ec85a49cadafda7cbcf244692374e96993ce9c66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 16 Sep 2024 18:13:54 +0200 Subject: [PATCH 06/10] chore: Add comment to make code clearer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index ebcebb714a9b1..807f3b3de45e5 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1092,6 +1092,7 @@ protected function promoteReshares(IShare $share): void { foreach ($reshareRecords as $child) { try { + /* Check if the share is still valid (means the resharer still has access to the file through another mean) */ $this->generalCreateChecks($child); } catch (GenericShareException $e) { /* The check is invalid, promote it to a direct share from the sharer of parent share */ From 10c7e100d13b5e72490246fd7d1a40d59106545c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 3 Oct 2024 10:26:30 +0200 Subject: [PATCH 07/10] fix(tests): Fix share tests to test new reshare promotion system MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- tests/lib/Share20/ManagerTest.php | 34 +++++++++++++++---------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index d51b1bd06ad78..49286dfc94874 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -228,7 +228,7 @@ public function dataTestDelete() { */ public function testDelete($shareType, $sharedWith) { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren', 'deleteReshares']) + ->setMethods(['getShareById', 'deleteChildren', 'promoteReshares']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -246,7 +246,7 @@ public function testDelete($shareType, $sharedWith) { ->setTarget('myTarget'); $manager->expects($this->once())->method('deleteChildren')->with($share); - $manager->expects($this->once())->method('deleteReshares')->with($share); + $manager->expects($this->once())->method('promoteReshares')->with($share); $this->defaultProvider ->expects($this->once()) @@ -271,7 +271,7 @@ public function testDelete($shareType, $sharedWith) { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren', 'deleteReshares']) + ->setMethods(['getShareById', 'deleteChildren', 'promoteReshares']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -290,7 +290,7 @@ public function testDeleteLazyShare() { $this->rootFolder->expects($this->never())->method($this->anything()); $manager->expects($this->once())->method('deleteChildren')->with($share); - $manager->expects($this->once())->method('deleteReshares')->with($share); + $manager->expects($this->once())->method('promoteReshares')->with($share); $this->defaultProvider ->expects($this->once()) @@ -315,7 +315,7 @@ public function testDeleteLazyShare() { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteReshares']) + ->setMethods(['getShareById', 'promoteReshares']) ->getMock(); $path = $this->createMock(File::class); @@ -472,9 +472,9 @@ public function testDeleteChildren() { $this->assertSame($shares, $result); } - public function testDeleteReshareWhenUserHasOneShare(): void { + public function testPromoteReshareWhenUserHasOneShare(): void { $manager = $this->createManagerMock() - ->setMethods(['deleteShare', 'getSharesInFolder', 'generalCreateChecks']) + ->setMethods(['updateShare', 'getSharesInFolder', 'generalCreateChecks']) ->getMock(); $folder = $this->createMock(Folder::class); @@ -499,14 +499,14 @@ public function testDeleteReshareWhenUserHasOneShare(): void { $this->defaultProvider->method('getSharesBy') ->willReturn([$reShare]); - $manager->expects($this->atLeast(2))->method('deleteShare')->withConsecutive([$reShare], [$reShareInSubFolder]); + $manager->expects($this->atLeast(2))->method('updateShare')->withConsecutive([$reShare], [$reShareInSubFolder]); - self::invokePrivate($manager, 'deleteReshares', [$share]); + self::invokePrivate($manager, 'promoteReshares', [$share]); } - public function testDeleteReshareWhenUserHasAnotherShare(): void { + public function testPromoteReshareWhenUserHasAnotherShare(): void { $manager = $this->createManagerMock() - ->setMethods(['deleteShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) + ->setMethods(['updateShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) ->getMock(); $folder = $this->createMock(Folder::class); @@ -527,14 +527,14 @@ public function testDeleteReshareWhenUserHasAnotherShare(): void { $manager->method('getSharesInFolder')->willReturn([]); $manager->method('generalCreateChecks')->willReturn(true); - $manager->expects($this->never())->method('deleteShare'); + $manager->expects($this->never())->method('updateShare'); - self::invokePrivate($manager, 'deleteReshares', [$share]); + self::invokePrivate($manager, 'promoteReshares', [$share]); } - public function testDeleteReshareOfUsersInGroupShare(): void { + public function testPromoteReshareOfUsersInGroupShare(): void { $manager = $this->createManagerMock() - ->setMethods(['deleteShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) + ->setMethods(['updateShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) ->getMock(); $folder = $this->createMock(Folder::class); @@ -576,9 +576,9 @@ public function testDeleteReshareOfUsersInGroupShare(): void { $manager->method('getSharedWith')->willReturn([]); $manager->expects($this->exactly(2))->method('getSharesInFolder')->willReturnOnConsecutiveCalls([[$reShare1]], [[$reShare2]]); - $manager->expects($this->exactly(2))->method('deleteShare')->withConsecutive([$reShare1], [$reShare2]); + $manager->expects($this->exactly(2))->method('updateShare')->withConsecutive([$reShare1], [$reShare2]); - self::invokePrivate($manager, 'deleteReshares', [$share]); + self::invokePrivate($manager, 'promoteReshares', [$share]); } public function testGetShareById(): void { From ba76378e3224a4484792af80dfdee2adca6f042d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 8 Oct 2024 17:41:03 +0200 Subject: [PATCH 08/10] fix(tests): Revert changes to tests now that reshares are not deleted but promoted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/files_sharing/tests/EtagPropagationTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/files_sharing/tests/EtagPropagationTest.php b/apps/files_sharing/tests/EtagPropagationTest.php index 327726acc9774..3f9ddfc413dc7 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -277,8 +277,7 @@ public function testOwnerUnshares() { self::TEST_FILES_SHARING_API_USER2, ]); - $this->assertEtagsNotChanged([self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3]); - $this->assertEtagsChanged([self::TEST_FILES_SHARING_API_USER4]); + $this->assertAllUnchanged(); } public function testOwnerUnsharesFlatReshares() { From e30cd2f03f81d805bcb3c80f6c1f9d04984f7ebd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 14 Oct 2024 11:56:58 +0200 Subject: [PATCH 09/10] fix: Fix promotion of reshares from subsubfolders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 27 +++++--- tests/lib/Share20/ManagerTest.php | 100 +++++++++++++++++++++++++----- 2 files changed, 101 insertions(+), 26 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 807f3b3de45e5..1919c7bc13baf 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1073,16 +1073,23 @@ protected function promoteReshares(IShare $share): void { foreach ($userIds as $userId) { foreach ($shareTypes as $shareType) { $provider = $this->factory->getProviderForType($shareType); - $shares = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0); - foreach ($shares as $child) { - $reshareRecords[] = $child; - } - } - - if ($node instanceof Folder) { - $sharesInFolder = $this->getSharesInFolder($userId, $node, false); - - foreach ($sharesInFolder as $shares) { + if ($node instanceof Folder) { + /* We need to get all shares by this user to get subshares */ + $shares = $provider->getSharesBy($userId, $shareType, null, false, -1, 0); + + foreach ($shares as $share) { + try { + $path = $share->getNode()->getPath(); + } catch (NotFoundException) { + /* Ignore share of non-existing node */ + continue; + } + if (str_starts_with($path, $node->getPath() . '/') || ($path === $node->getPath())) { + $reshareRecords[] = $share; + } + } + } else { + $shares = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0); foreach ($shares as $child) { $reshareRecords[] = $child; } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 49286dfc94874..d21ac89fd0345 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -472,34 +472,92 @@ public function testDeleteChildren() { $this->assertSame($shares, $result); } - public function testPromoteReshareWhenUserHasOneShare(): void { + public function testPromoteReshareFile(): void { + $manager = $this->createManagerMock() + ->setMethods(['updateShare', 'getSharesInFolder', 'generalCreateChecks']) + ->getMock(); + + $file = $this->createMock(File::class); + + $share = $this->createMock(IShare::class); + $share->method('getShareType')->willReturn(IShare::TYPE_USER); + $share->method('getNodeType')->willReturn('folder'); + $share->method('getSharedWith')->willReturn('userB'); + $share->method('getNode')->willReturn($file); + + $reShare = $this->createMock(IShare::class); + $reShare->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShare->method('getSharedBy')->willReturn('userB'); + $reShare->method('getSharedWith')->willReturn('userC'); + $reShare->method('getNode')->willReturn($file); + + $this->defaultProvider->method('getSharesBy') + ->willReturnCallback(function ($userId, $shareType, $node, $reshares, $limit, $offset) use ($reShare, $file) { + $this->assertEquals($file, $node); + if ($shareType === IShare::TYPE_USER) { + return match($userId) { + 'userB' => [$reShare], + }; + } else { + return []; + } + }); + $manager->method('generalCreateChecks')->willThrowException(new GenericShareException()); + + $manager->expects($this->exactly(1))->method('updateShare')->with($reShare); + + self::invokePrivate($manager, 'promoteReshares', [$share]); + } + + public function testPromoteReshare(): void { $manager = $this->createManagerMock() ->setMethods(['updateShare', 'getSharesInFolder', 'generalCreateChecks']) ->getMock(); $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn('/path/to/folder'); + + $subFolder = $this->createMock(Folder::class); + $subFolder->method('getPath')->willReturn('/path/to/folder/sub'); + + $otherFolder = $this->createMock(Folder::class); + $otherFolder->method('getPath')->willReturn('/path/to/otherfolder/'); $share = $this->createMock(IShare::class); $share->method('getShareType')->willReturn(IShare::TYPE_USER); $share->method('getNodeType')->willReturn('folder'); - $share->method('getSharedWith')->willReturn('UserB'); + $share->method('getSharedWith')->willReturn('userB'); $share->method('getNode')->willReturn($folder); $reShare = $this->createMock(IShare::class); - $reShare->method('getSharedBy')->willReturn('UserB'); - $reShare->method('getSharedWith')->willReturn('UserC'); + $reShare->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShare->method('getSharedBy')->willReturn('userB'); + $reShare->method('getSharedWith')->willReturn('userC'); $reShare->method('getNode')->willReturn($folder); $reShareInSubFolder = $this->createMock(IShare::class); - $reShareInSubFolder->method('getSharedBy')->willReturn('UserB'); + $reShareInSubFolder->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShareInSubFolder->method('getSharedBy')->willReturn('userB'); + $reShareInSubFolder->method('getNode')->willReturn($subFolder); - $manager->method('getSharesInFolder')->willReturn([$reShareInSubFolder]); - $manager->method('generalCreateChecks')->willThrowException(new GenericShareException()); + $reShareInOtherFolder = $this->createMock(IShare::class); + $reShareInOtherFolder->method('getShareType')->willReturn(IShare::TYPE_USER); + $reShareInOtherFolder->method('getSharedBy')->willReturn('userB'); + $reShareInOtherFolder->method('getNode')->willReturn($otherFolder); $this->defaultProvider->method('getSharesBy') - ->willReturn([$reShare]); + ->willReturnCallback(function ($userId, $shareType, $node, $reshares, $limit, $offset) use ($reShare, $reShareInSubFolder, $reShareInOtherFolder) { + if ($shareType === IShare::TYPE_USER) { + return match($userId) { + 'userB' => [$reShare,$reShareInSubFolder,$reShareInOtherFolder], + }; + } else { + return []; + } + }); + $manager->method('generalCreateChecks')->willThrowException(new GenericShareException()); - $manager->expects($this->atLeast(2))->method('updateShare')->withConsecutive([$reShare], [$reShareInSubFolder]); + $manager->expects($this->exactly(2))->method('updateShare')->withConsecutive([$reShare], [$reShareInSubFolder]); self::invokePrivate($manager, 'promoteReshares', [$share]); } @@ -510,23 +568,24 @@ public function testPromoteReshareWhenUserHasAnotherShare(): void { ->getMock(); $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn('/path/to/folder'); $share = $this->createMock(IShare::class); $share->method('getShareType')->willReturn(IShare::TYPE_USER); $share->method('getNodeType')->willReturn('folder'); - $share->method('getSharedWith')->willReturn('UserB'); + $share->method('getSharedWith')->willReturn('userB'); $share->method('getNode')->willReturn($folder); $reShare = $this->createMock(IShare::class); $reShare->method('getShareType')->willReturn(IShare::TYPE_USER); $reShare->method('getNodeType')->willReturn('folder'); - $reShare->method('getSharedBy')->willReturn('UserB'); + $reShare->method('getSharedBy')->willReturn('userB'); $reShare->method('getNode')->willReturn($folder); $this->defaultProvider->method('getSharesBy')->willReturn([$reShare]); - $manager->method('getSharesInFolder')->willReturn([]); $manager->method('generalCreateChecks')->willReturn(true); + /* No share is promoted because generalCreateChecks does not throw */ $manager->expects($this->never())->method('updateShare'); self::invokePrivate($manager, 'promoteReshares', [$share]); @@ -538,6 +597,7 @@ public function testPromoteReshareOfUsersInGroupShare(): void { ->getMock(); $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn('/path/to/folder'); $userA = $this->createMock(IUser::class); $userA->method('getUID')->willReturn('userA'); @@ -552,13 +612,13 @@ public function testPromoteReshareOfUsersInGroupShare(): void { $reShare1 = $this->createMock(IShare::class); $reShare1->method('getShareType')->willReturn(IShare::TYPE_USER); $reShare1->method('getNodeType')->willReturn('folder'); - $reShare1->method('getSharedBy')->willReturn('UserB'); + $reShare1->method('getSharedBy')->willReturn('userB'); $reShare1->method('getNode')->willReturn($folder); $reShare2 = $this->createMock(IShare::class); $reShare2->method('getShareType')->willReturn(IShare::TYPE_USER); $reShare2->method('getNodeType')->willReturn('folder'); - $reShare2->method('getSharedBy')->willReturn('UserC'); + $reShare2->method('getSharedBy')->willReturn('userC'); $reShare2->method('getNode')->willReturn($folder); $userB = $this->createMock(IUser::class); @@ -570,11 +630,19 @@ public function testPromoteReshareOfUsersInGroupShare(): void { $this->groupManager->method('get')->with('Group')->willReturn($group); $this->defaultProvider->method('getSharesBy') - ->willReturn([]); + ->willReturnCallback(function ($userId, $shareType, $node, $reshares, $limit, $offset) use ($reShare1, $reShare2) { + if ($shareType === IShare::TYPE_USER) { + return match($userId) { + 'userB' => [$reShare1], + 'userC' => [$reShare2], + }; + } else { + return []; + } + }); $manager->method('generalCreateChecks')->willThrowException(new GenericShareException()); $manager->method('getSharedWith')->willReturn([]); - $manager->expects($this->exactly(2))->method('getSharesInFolder')->willReturnOnConsecutiveCalls([[$reShare1]], [[$reShare2]]); $manager->expects($this->exactly(2))->method('updateShare')->withConsecutive([$reShare1], [$reShare2]); From 28c885dbf23732aa104b31fa5d1c956cf287f210 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 14 Oct 2024 17:23:29 +0200 Subject: [PATCH 10/10] fix: Use getRelativePath method to check if node is inside folder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/Share20/Manager.php | 3 ++- tests/lib/Share20/ManagerTest.php | 24 ++++++++++++++---------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 1919c7bc13baf..1006a0f24bf76 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1084,7 +1084,8 @@ protected function promoteReshares(IShare $share): void { /* Ignore share of non-existing node */ continue; } - if (str_starts_with($path, $node->getPath() . '/') || ($path === $node->getPath())) { + if ($node->getRelativePath($path) !== null) { + /* If relative path is not null it means the shared node is the same or in a subfolder */ $reshareRecords[] = $share; } } diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index d21ac89fd0345..c780d29bfed66 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -9,6 +9,7 @@ use DateTimeZone; use OC\Files\Mount\MoveableMount; +use OC\Files\Utils\PathHelper; use OC\KnownUser\KnownUserService; use OC\Share20\DefaultShareProvider; use OC\Share20\Exception; @@ -199,6 +200,14 @@ private function createManagerMock() { ]); } + private function createFolderMock(string $folderPath): MockObject&Folder { + $folder = $this->createMock(Folder::class); + $folder->method('getPath')->willReturn($folderPath); + $folder->method('getRelativePath')->willReturnCallback( + fn (string $path): ?string => PathHelper::getRelativePath($folderPath, $path) + ); + return $folder; + } public function testDeleteNoShareId() { $this->expectException(\InvalidArgumentException::class); @@ -514,14 +523,11 @@ public function testPromoteReshare(): void { ->setMethods(['updateShare', 'getSharesInFolder', 'generalCreateChecks']) ->getMock(); - $folder = $this->createMock(Folder::class); - $folder->method('getPath')->willReturn('/path/to/folder'); + $folder = $this->createFolderMock('/path/to/folder'); - $subFolder = $this->createMock(Folder::class); - $subFolder->method('getPath')->willReturn('/path/to/folder/sub'); + $subFolder = $this->createFolderMock('/path/to/folder/sub'); - $otherFolder = $this->createMock(Folder::class); - $otherFolder->method('getPath')->willReturn('/path/to/otherfolder/'); + $otherFolder = $this->createFolderMock('/path/to/otherfolder/'); $share = $this->createMock(IShare::class); $share->method('getShareType')->willReturn(IShare::TYPE_USER); @@ -567,8 +573,7 @@ public function testPromoteReshareWhenUserHasAnotherShare(): void { ->setMethods(['updateShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) ->getMock(); - $folder = $this->createMock(Folder::class); - $folder->method('getPath')->willReturn('/path/to/folder'); + $folder = $this->createFolderMock('/path/to/folder'); $share = $this->createMock(IShare::class); $share->method('getShareType')->willReturn(IShare::TYPE_USER); @@ -596,8 +601,7 @@ public function testPromoteReshareOfUsersInGroupShare(): void { ->setMethods(['updateShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks']) ->getMock(); - $folder = $this->createMock(Folder::class); - $folder->method('getPath')->willReturn('/path/to/folder'); + $folder = $this->createFolderMock('/path/to/folder'); $userA = $this->createMock(IUser::class); $userA->method('getUID')->willReturn('userA');