From dfbaa103f95ca58df0680e47bde10c72e51ca200 Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Wed, 7 Aug 2024 11:13:35 +0200 Subject: [PATCH 01/11] 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 75f67767ac919..d000133e13fe7 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -488,6 +488,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 77149ae388e9b..de274de6c1097 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -299,7 +299,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 ada5e44740088..6722de547da9b 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -52,6 +52,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; @@ -1163,6 +1164,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 * @@ -1186,6 +1252,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 8094f57333242..3a2206926938c 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -60,6 +60,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; @@ -241,7 +242,7 @@ public function dataTestDelete() { */ public function testDelete($shareType, $sharedWith) { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -259,6 +260,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()) @@ -283,7 +285,7 @@ public function testDelete($shareType, $sharedWith) { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -302,6 +304,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()) @@ -326,7 +329,7 @@ public function testDeleteLazyShare() { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById']) + ->setMethods(['getShareById', 'deleteReshare']) ->getMock(); $path = $this->createMock(File::class); @@ -483,7 +486,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 786424ef09de0eaade7bc4db15c235afc1b0a46f 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/11] 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 6722de547da9b..894e785bc624a 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1164,31 +1164,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 = []; @@ -1197,7 +1198,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) { @@ -1209,8 +1210,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) { @@ -1224,6 +1225,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); } } @@ -1252,10 +1254,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 3a2206926938c..1d74b36d31901 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -242,7 +242,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([]); @@ -260,7 +260,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()) @@ -285,7 +285,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([]); @@ -304,7 +304,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()) @@ -329,7 +329,7 @@ public function testDeleteLazyShare() { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteReshare']) + ->setMethods(['getShareById', 'deleteReshares']) ->getMock(); $path = $this->createMock(File::class); @@ -515,7 +515,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 { @@ -543,7 +543,7 @@ public function testDeleteReshareWhenUserHasAnotherShare(): void { $manager->expects($this->never())->method('deleteShare'); - $manager->deleteReshare($share); + self::invokePrivate($manager, 'deleteReshares', [$share]); } public function testDeleteReshareOfUsersInGroupShare(): void { @@ -592,7 +592,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 8ae7c031b3297b52ebbe4ed080adb458918e67d7 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/11] 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 d000133e13fe7..b805814448d86 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -176,16 +176,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( @@ -210,6 +200,16 @@ public function transfer( $move ); } + + $destinationPath = $finalTarget . '/' . $path; + // restore the shares + $this->restoreShares( + $sourceUid, + $destinationUid, + $destinationPath, + $shares, + $output + ); } private function walkFiles(View $view, $path, Closure $callBack) { @@ -488,9 +488,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 0132840c9b1ec40fa3c4d432c95a2e3da153dd40 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/11] 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 894e785bc624a..6498cea20da59 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1164,7 +1164,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) { @@ -1182,7 +1183,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(); @@ -1225,8 +1226,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()]); + } } } } @@ -1256,8 +1263,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 1b9896f7a99eab306fef6a7252a1ee3eb213567a 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/11] 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 6498cea20da59..428890999bd5d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1164,7 +1164,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 7370a585c17482b82695ce356e2f72b0e0750c7b 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/11] 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 428890999bd5d..bd6456122e10a 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1224,6 +1224,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 0ee5c781f7d04d368ecb29de68427fb0b4c0917e 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/11] 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 1d74b36d31901..555babf6e2eb3 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -242,7 +242,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([]); @@ -260,7 +260,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()) @@ -285,7 +285,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([]); @@ -304,7 +304,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()) @@ -329,7 +329,7 @@ public function testDeleteLazyShare() { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteReshares']) + ->setMethods(['getShareById', 'promoteReshares']) ->getMock(); $path = $this->createMock(File::class); @@ -486,9 +486,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); @@ -513,14 +513,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); @@ -541,14 +541,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); @@ -590,9 +590,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 6582128d48b1eb50f7bbaa0360050fa40d28f40e 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/11] 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 de274de6c1097..77149ae388e9b 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -299,8 +299,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 e1a23cf0a74273acd57eb03789c67a5b0cc6905b 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/11] 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 bd6456122e10a..6b255394f0b02 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1205,16 +1205,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 555babf6e2eb3..dcb4307403747 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -486,34 +486,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]); } @@ -524,23 +582,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]); @@ -552,6 +611,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'); @@ -566,13 +626,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); @@ -584,11 +644,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 97800c555d96ad8936fe04341af8ee28e67c32c9 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/11] 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 6b255394f0b02..4f02e45c73588 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1216,7 +1216,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 dcb4307403747..b46a7d768f55d 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -23,6 +23,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; @@ -213,6 +214,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); @@ -528,14 +537,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); @@ -581,8 +587,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); @@ -610,8 +615,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'); From 56a5350806282b0a7d0aa76fa9f7fd16c11bf9a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 19 Dec 2024 15:50:23 +0100 Subject: [PATCH 11/11] chore: Remove syntax incompatible with PHP 8.0 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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index b46a7d768f55d..edc072f4695b9 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -214,7 +214,10 @@ private function createManagerMock() { ]); } - private function createFolderMock(string $folderPath): MockObject&Folder { + /** + * @return MockObject&Folder + */ + private function createFolderMock(string $folderPath) { $folder = $this->createMock(Folder::class); $folder->method('getPath')->willReturn($folderPath); $folder->method('getRelativePath')->willReturnCallback( @@ -4784,7 +4787,7 @@ public function testCurrentUserCanEnumerateTargetUser(bool $currentUserIsGuest, 'limitEnumerationToPhone', 'limitEnumerationToGroups', ]) - ->getMock(); + ->getMock(); $manager->method('allowEnumerationFullMatch') ->willReturn($allowEnumerationFullMatch);