From 42181c2f490025860e22907255b6917583c798af Mon Sep 17 00:00:00 2001 From: Luka Trovic Date: Wed, 7 Aug 2024 11:13:35 +0200 Subject: 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 --- tests/lib/Share20/ManagerTest.php | 118 +++++++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 1dfe43410df..743b68d24d5 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): void { $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(): void { $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(): void { $manager = $this->createManagerMock() - ->setMethods(['getShareById']) + ->setMethods(['getShareById', 'deleteReshare']) ->getMock(); $path = $this->createMock(File::class); @@ -469,6 +472,115 @@ class ManagerTest extends \Test\TestCase { $this->assertSame($shares, $result); } + 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); -- cgit v1.2.3 From 7e9bc7c8cf981b4931cd4a75b674c662eddc4bb9 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Thu, 22 Aug 2024 17:03:07 +0200 Subject: 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(-) (limited to 'tests') diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 96dd88c6793..e5e5166b4a9 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1040,31 +1040,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 = []; @@ -1073,7 +1074,7 @@ class Manager implements IManager { IShare::TYPE_USER, IShare::TYPE_LINK, IShare::TYPE_REMOTE, - IShare::TYPE_EMAIL + IShare::TYPE_EMAIL, ]; foreach ($userIds as $userId) { @@ -1085,8 +1086,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) { @@ -1100,6 +1101,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); } } @@ -1128,10 +1130,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 743b68d24d5..46a10108617 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): void { $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(): void { $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(): void { $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 { -- cgit v1.2.3 From 3ee5bf6af1dfbc752b8b471b77787bdea79588f1 Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Thu, 3 Oct 2024 10:26:30 +0200 Subject: 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(-) (limited to 'tests') diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 46a10108617..c5d0a763f9b 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): void { $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(): void { $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(): void { $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 { -- cgit v1.2.3 From e584e9baf744149649147cf45bab3c5218a31d4e Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Mon, 14 Oct 2024 11:56:58 +0200 Subject: 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(-) (limited to 'tests') diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 52618d4e03e..5730f4f9635 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1082,16 +1082,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 c5d0a763f9b..c739e2e8851 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]); -- cgit v1.2.3 From d6ba52426351e10abb2b34c80c8f00da63afc32b Mon Sep 17 00:00:00 2001 From: Côme Chilliet Date: Mon, 14 Oct 2024 17:23:29 +0200 Subject: 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(-) (limited to 'tests') diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index 5730f4f9635..4856c051307 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -1093,7 +1093,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 c739e2e8851..a4c1dd3803d 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(): void { $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'); -- cgit v1.2.3