From 7a3a7794266b615192cb2ccab9163d0d7683e5cf 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 283a49932f723..fdbc4b141d332 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 9deaf94029279..06f796f7916ca 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; @@ -1155,6 +1156,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 * @@ -1178,6 +1244,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 7ed3c06361112..f0916c4df0e92 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 257b516f4efe95dc85c639dda987b9290754b906 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 06f796f7916ca..5c04979176804 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1156,31 +1156,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 = []; @@ -1189,7 +1190,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) { @@ -1201,8 +1202,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) { @@ -1216,6 +1217,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); } } @@ -1244,10 +1246,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 f0916c4df0e92..3227168696e73 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 58b64ad5672098334fbb6cdffec8efbbab0abb80 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 fdbc4b141d332..e28fbfe19bd56 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 c647d6c6613ab47db20cef937d4eb934915e9ef9 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 5c04979176804..b3bea14de8d0a 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1156,7 +1156,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) { @@ -1174,7 +1175,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(); @@ -1217,8 +1218,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()]); + } } } } @@ -1248,8 +1255,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 fd95d1e3638961d286712da4732c08ccbddf257f 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 b3bea14de8d0a..70ed23062089f 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1156,7 +1156,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 a24460d8da3340ac37d7852d89e677c6853f96d7 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 70ed23062089f..2fb080d07dc5e 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1216,6 +1216,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 8bc34e99f6fd76cc6303bd226fef0ee620e67b99 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 3227168696e73..0aace03a99a4f 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 69e5ae7bee3979d48522eb316a239c76e110df7e 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 30fdc6c114b3462418fd712dd069f16a17131f73 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 2fb080d07dc5e..2bc639607d864 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1197,16 +1197,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 0aace03a99a4f..517f236ef2816 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 3f695c6209a718ee989809120c689fd7a7ec3475 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 2bc639607d864..e9ac9166f2642 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1208,7 +1208,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 517f236ef2816..f18816eec6a17 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 4c10a793d95810761a87a94e240a38e3a9871b4b 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 f18816eec6a17..4f4cd22185849 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( @@ -4772,7 +4775,7 @@ public function testCurrentUserCanEnumerateTargetUser(bool $currentUserIsGuest, 'limitEnumerationToPhone', 'limitEnumerationToGroups', ]) - ->getMock(); + ->getMock(); $manager->method('allowEnumerationFullMatch') ->willReturn($allowEnumerationFullMatch);