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 d4bc3033b1b..c47e51923ce 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -469,6 +469,9 @@ class OwnershipTransferService { } } 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 3f9ddfc413d..327726acc97 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -277,7 +277,8 @@ class EtagPropagationTest extends PropagationTestCase { 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 2cc11c2c634..c4761aa81cf 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -18,6 +18,7 @@ use OCP\Files\Folder; 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 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { $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 c0cf0ca9c12..4acdfc8bc12 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -46,6 +46,7 @@ use OCP\Share\Events\ShareCreatedEvent; 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 @@ class ManagerTest extends \Test\TestCase { */ public function testDelete($shareType, $sharedWith) { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -245,6 +246,7 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -288,6 +290,7 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById']) + ->setMethods(['getShareById', 'deleteReshare']) ->getMock(); $path = $this->createMock(File::class); @@ -469,7 +472,116 @@ class ManagerTest extends \Test\TestCase { $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 c4761aa81cf..063a1349b8d 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1032,31 +1032,32 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { IShare::TYPE_USER, IShare::TYPE_LINK, IShare::TYPE_REMOTE, - IShare::TYPE_EMAIL + IShare::TYPE_EMAIL, ]; foreach ($userIds as $userId) { @@ -1077,8 +1078,8 @@ class Manager implements IManager { } } - 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 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { $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 4acdfc8bc12..d51b1bd06ad 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -228,7 +228,7 @@ class ManagerTest extends \Test\TestCase { */ 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 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren', 'deleteReshare']) + ->setMethods(['getShareById', 'deleteChildren', 'deleteReshares']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -290,7 +290,7 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteReshare']) + ->setMethods(['getShareById', 'deleteReshares']) ->getMock(); $path = $this->createMock(File::class); @@ -501,7 +501,7 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { $manager->expects($this->never())->method('deleteShare'); - $manager->deleteReshare($share); + self::invokePrivate($manager, 'deleteReshares', [$share]); } public function testDeleteReshareOfUsersInGroupShare(): void { @@ -578,7 +578,7 @@ class ManagerTest extends \Test\TestCase { $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 c47e51923ce..08c9f3f719f 100644 --- a/apps/files/lib/Service/OwnershipTransferService.php +++ b/apps/files/lib/Service/OwnershipTransferService.php @@ -150,16 +150,6 @@ class OwnershipTransferService { $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 @@ class OwnershipTransferService { $move ); } + + $destinationPath = $finalTarget . '/' . $path; + // restore the shares + $this->restoreShares( + $sourceUid, + $destinationUid, + $destinationPath, + $shares, + $output + ); } private function sanitizeFolderName(string $name): string { @@ -469,9 +469,6 @@ class OwnershipTransferService { } } 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 063a1349b8d..cb4e54b4732 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1032,7 +1032,8 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { 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 @@ class Manager implements IManager { $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 cb4e54b4732..ebcebb714a9 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1032,7 +1032,7 @@ class Manager implements IManager { 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 ebcebb714a9..807f3b3de45 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1092,6 +1092,7 @@ class Manager implements IManager { 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 d51b1bd06ad..49286dfc948 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -228,7 +228,7 @@ class ManagerTest extends \Test\TestCase { */ 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 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { public function testDeleteLazyShare() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteChildren', 'deleteReshares']) + ->setMethods(['getShareById', 'deleteChildren', 'promoteReshares']) ->getMock(); $manager->method('deleteChildren')->willReturn([]); @@ -290,7 +290,7 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { public function testDeleteNested() { $manager = $this->createManagerMock() - ->setMethods(['getShareById', 'deleteReshares']) + ->setMethods(['getShareById', 'promoteReshares']) ->getMock(); $path = $this->createMock(File::class); @@ -472,9 +472,9 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { $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 327726acc97..3f9ddfc413d 100644 --- a/apps/files_sharing/tests/EtagPropagationTest.php +++ b/apps/files_sharing/tests/EtagPropagationTest.php @@ -277,8 +277,7 @@ class EtagPropagationTest extends PropagationTestCase { 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 | 25 +++++--- tests/lib/Share20/ManagerTest.php | 100 +++++++++++++++++++++++++----- 2 files changed, 100 insertions(+), 25 deletions(-) diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 807f3b3de45..1919c7bc13b 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1073,16 +1073,23 @@ class Manager implements IManager { 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) { + /* We need to get all shares by this user to get subshares */ + $shares = $provider->getSharesBy($userId, $shareType, null, false, -1, 0); - if ($node instanceof Folder) { - $sharesInFolder = $this->getSharesInFolder($userId, $node, false); - - foreach ($sharesInFolder as $shares) { + 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 49286dfc948..d21ac89fd03 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -472,34 +472,92 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { $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 @@ class ManagerTest extends \Test\TestCase { $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 1919c7bc13b..1006a0f24bf 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1084,7 +1084,8 @@ class Manager implements IManager { /* 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 d21ac89fd03..c780d29bfed 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -9,6 +9,7 @@ namespace Test\Share20; 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 @@ class ManagerTest extends \Test\TestCase { ]); } + 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 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { ->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 @@ class ManagerTest extends \Test\TestCase { ->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');