Skip to content

Commit 42181c2

Browse files
luka-nextcloudcome-nc
luka-nextcloud
authored and
come-nc
committed
fix: delete re-shares when deleting the parent share
Note: Removed part about fix command from original PR Signed-off-by: Luka Trovic <luka@nextcloud.com> Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
1 parent 3f75c48 commit 42181c2

File tree

4 files changed

+189
-4
lines changed

4 files changed

+189
-4
lines changed

‎apps/files/lib/Service/OwnershipTransferService.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,6 +467,9 @@ private function restoreShares(
467467
}
468468
} catch (\OCP\Files\NotFoundException $e) {
469469
$output->writeln('<error>Share with id ' . $share->getId() . ' points at deleted file, skipping</error>');
470+
} catch (\OCP\Share\Exceptions\GenericShareException $e) {
471+
$output->writeln('<error>Share with id ' . $share->getId() . ' is broken, deleting</error>');
472+
$this->shareManager->deleteShare($share);
470473
} catch (\Throwable $e) {
471474
$output->writeln('<error>Could not restore share with id ' . $share->getId() . ':' . $e->getMessage() . ' : ' . $e->getTraceAsString() . '</error>');
472475
}

‎apps/files_sharing/tests/EtagPropagationTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,8 @@ public function testOwnerUnshares(): void {
277277
self::TEST_FILES_SHARING_API_USER2,
278278
]);
279279

280-
$this->assertAllUnchanged();
280+
$this->assertEtagsNotChanged([self::TEST_FILES_SHARING_API_USER1, self::TEST_FILES_SHARING_API_USER2, self::TEST_FILES_SHARING_API_USER3]);
281+
$this->assertEtagsChanged([self::TEST_FILES_SHARING_API_USER4]);
281282
}
282283

283284
public function testOwnerUnsharesFlatReshares(): void {

‎lib/private/Share20/Manager.php‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
use OCP\Files\IRootFolder;
1919
use OCP\Files\Mount\IMountManager;
2020
use OCP\Files\Node;
21+
use OCP\Files\NotFoundException;
2122
use OCP\HintException;
2223
use OCP\IConfig;
2324
use OCP\IDateTimeZone;
@@ -1039,6 +1040,71 @@ protected function deleteChildren(IShare $share) {
10391040
return $deletedShares;
10401041
}
10411042

1043+
public function deleteReshare(IShare $share) {
1044+
// Skip if node not found
1045+
try {
1046+
$node = $share->getNode();
1047+
} catch (NotFoundException) {
1048+
return;
1049+
}
1050+
1051+
$userIds = [];
1052+
1053+
if ($share->getShareType() === IShare::TYPE_USER) {
1054+
$userIds[] = $share->getSharedWith();
1055+
}
1056+
1057+
if ($share->getShareType() === IShare::TYPE_GROUP) {
1058+
$group = $this->groupManager->get($share->getSharedWith());
1059+
$users = $group->getUsers();
1060+
1061+
foreach ($users as $user) {
1062+
// Skip if share owner is member of shared group
1063+
if ($user->getUID() === $share->getShareOwner()) {
1064+
continue;
1065+
}
1066+
$userIds[] = $user->getUID();
1067+
}
1068+
}
1069+
1070+
$reshareRecords = [];
1071+
$shareTypes = [
1072+
IShare::TYPE_GROUP,
1073+
IShare::TYPE_USER,
1074+
IShare::TYPE_LINK,
1075+
IShare::TYPE_REMOTE,
1076+
IShare::TYPE_EMAIL
1077+
];
1078+
1079+
foreach ($userIds as $userId) {
1080+
foreach ($shareTypes as $shareType) {
1081+
$provider = $this->factory->getProviderForType($shareType);
1082+
$shares = $provider->getSharesBy($userId, $shareType, $node, false, -1, 0);
1083+
foreach ($shares as $child) {
1084+
$reshareRecords[] = $child;
1085+
}
1086+
}
1087+
1088+
if ($share->getNodeType() === 'folder') {
1089+
$sharesInFolder = $this->getSharesInFolder($userId, $node, true);
1090+
1091+
foreach ($sharesInFolder as $shares) {
1092+
foreach ($shares as $child) {
1093+
$reshareRecords[] = $child;
1094+
}
1095+
}
1096+
}
1097+
}
1098+
1099+
foreach ($reshareRecords as $child) {
1100+
try {
1101+
$this->generalCreateChecks($child);
1102+
} catch (GenericShareException $e) {
1103+
$this->deleteShare($child);
1104+
}
1105+
}
1106+
}
1107+
10421108
/**
10431109
* Delete a share
10441110
*
@@ -1062,6 +1128,9 @@ public function deleteShare(IShare $share) {
10621128
$provider = $this->factory->getProviderForType($share->getShareType());
10631129
$provider->delete($share);
10641130

1131+
// Delete shares that shared by the "share with user/group"
1132+
$this->deleteReshare($share);
1133+
10651134
$this->dispatcher->dispatchTyped(new ShareDeletedEvent($share));
10661135
}
10671136

‎tests/lib/Share20/ManagerTest.php‎

Lines changed: 115 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@
4646
use OCP\Share\Events\ShareDeletedEvent;
4747
use OCP\Share\Events\ShareDeletedFromSelfEvent;
4848
use OCP\Share\Exceptions\AlreadySharedException;
49+
use OCP\Share\Exceptions\GenericShareException;
4950
use OCP\Share\Exceptions\ShareNotFound;
5051
use OCP\Share\IManager;
5152
use OCP\Share\IProviderFactory;
@@ -227,7 +228,7 @@ public function dataTestDelete() {
227228
*/
228229
public function testDelete($shareType, $sharedWith): void {
229230
$manager = $this->createManagerMock()
230-
->setMethods(['getShareById', 'deleteChildren'])
231+
->setMethods(['getShareById', 'deleteChildren', 'deleteReshare'])
231232
->getMock();
232233

233234
$manager->method('deleteChildren')->willReturn([]);
@@ -245,6 +246,7 @@ public function testDelete($shareType, $sharedWith): void {
245246
->setTarget('myTarget');
246247

247248
$manager->expects($this->once())->method('deleteChildren')->with($share);
249+
$manager->expects($this->once())->method('deleteReshare')->with($share);
248250

249251
$this->defaultProvider
250252
->expects($this->once())
@@ -269,7 +271,7 @@ public function testDelete($shareType, $sharedWith): void {
269271

270272
public function testDeleteLazyShare(): void {
271273
$manager = $this->createManagerMock()
272-
->setMethods(['getShareById', 'deleteChildren'])
274+
->setMethods(['getShareById', 'deleteChildren', 'deleteReshare'])
273275
->getMock();
274276

275277
$manager->method('deleteChildren')->willReturn([]);
@@ -288,6 +290,7 @@ public function testDeleteLazyShare(): void {
288290
$this->rootFolder->expects($this->never())->method($this->anything());
289291

290292
$manager->expects($this->once())->method('deleteChildren')->with($share);
293+
$manager->expects($this->once())->method('deleteReshare')->with($share);
291294

292295
$this->defaultProvider
293296
->expects($this->once())
@@ -312,7 +315,7 @@ public function testDeleteLazyShare(): void {
312315

313316
public function testDeleteNested(): void {
314317
$manager = $this->createManagerMock()
315-
->setMethods(['getShareById'])
318+
->setMethods(['getShareById', 'deleteReshare'])
316319
->getMock();
317320

318321
$path = $this->createMock(File::class);
@@ -469,6 +472,115 @@ public function testDeleteChildren(): void {
469472
$this->assertSame($shares, $result);
470473
}
471474

475+
public function testDeleteReshareWhenUserHasOneShare(): void {
476+
$manager = $this->createManagerMock()
477+
->setMethods(['deleteShare', 'getSharesInFolder', 'generalCreateChecks'])
478+
->getMock();
479+
480+
$folder = $this->createMock(Folder::class);
481+
482+
$share = $this->createMock(IShare::class);
483+
$share->method('getShareType')->willReturn(IShare::TYPE_USER);
484+
$share->method('getNodeType')->willReturn('folder');
485+
$share->method('getSharedWith')->willReturn('UserB');
486+
$share->method('getNode')->willReturn($folder);
487+
488+
$reShare = $this->createMock(IShare::class);
489+
$reShare->method('getSharedBy')->willReturn('UserB');
490+
$reShare->method('getSharedWith')->willReturn('UserC');
491+
$reShare->method('getNode')->willReturn($folder);
492+
493+
$reShareInSubFolder = $this->createMock(IShare::class);
494+
$reShareInSubFolder->method('getSharedBy')->willReturn('UserB');
495+
496+
$manager->method('getSharesInFolder')->willReturn([$reShareInSubFolder]);
497+
$manager->method('generalCreateChecks')->willThrowException(new GenericShareException());
498+
499+
$this->defaultProvider->method('getSharesBy')
500+
->willReturn([$reShare]);
501+
502+
$manager->expects($this->atLeast(2))->method('deleteShare')->withConsecutive([$reShare], [$reShareInSubFolder]);
503+
504+
$manager->deleteReshare($share);
505+
}
506+
507+
public function testDeleteReshareWhenUserHasAnotherShare(): void {
508+
$manager = $this->createManagerMock()
509+
->setMethods(['deleteShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks'])
510+
->getMock();
511+
512+
$folder = $this->createMock(Folder::class);
513+
514+
$share = $this->createMock(IShare::class);
515+
$share->method('getShareType')->willReturn(IShare::TYPE_USER);
516+
$share->method('getNodeType')->willReturn('folder');
517+
$share->method('getSharedWith')->willReturn('UserB');
518+
$share->method('getNode')->willReturn($folder);
519+
520+
$reShare = $this->createMock(IShare::class);
521+
$reShare->method('getShareType')->willReturn(IShare::TYPE_USER);
522+
$reShare->method('getNodeType')->willReturn('folder');
523+
$reShare->method('getSharedBy')->willReturn('UserB');
524+
$reShare->method('getNode')->willReturn($folder);
525+
526+
$this->defaultProvider->method('getSharesBy')->willReturn([$reShare]);
527+
$manager->method('getSharesInFolder')->willReturn([]);
528+
$manager->method('generalCreateChecks')->willReturn(true);
529+
530+
$manager->expects($this->never())->method('deleteShare');
531+
532+
$manager->deleteReshare($share);
533+
}
534+
535+
public function testDeleteReshareOfUsersInGroupShare(): void {
536+
$manager = $this->createManagerMock()
537+
->setMethods(['deleteShare', 'getSharesInFolder', 'getSharedWith', 'generalCreateChecks'])
538+
->getMock();
539+
540+
$folder = $this->createMock(Folder::class);
541+
542+
$userA = $this->createMock(IUser::class);
543+
$userA->method('getUID')->willReturn('userA');
544+
545+
$share = $this->createMock(IShare::class);
546+
$share->method('getShareType')->willReturn(IShare::TYPE_GROUP);
547+
$share->method('getNodeType')->willReturn('folder');
548+
$share->method('getSharedWith')->willReturn('Group');
549+
$share->method('getNode')->willReturn($folder);
550+
$share->method('getShareOwner')->willReturn($userA);
551+
552+
$reShare1 = $this->createMock(IShare::class);
553+
$reShare1->method('getShareType')->willReturn(IShare::TYPE_USER);
554+
$reShare1->method('getNodeType')->willReturn('folder');
555+
$reShare1->method('getSharedBy')->willReturn('UserB');
556+
$reShare1->method('getNode')->willReturn($folder);
557+
558+
$reShare2 = $this->createMock(IShare::class);
559+
$reShare2->method('getShareType')->willReturn(IShare::TYPE_USER);
560+
$reShare2->method('getNodeType')->willReturn('folder');
561+
$reShare2->method('getSharedBy')->willReturn('UserC');
562+
$reShare2->method('getNode')->willReturn($folder);
563+
564+
$userB = $this->createMock(IUser::class);
565+
$userB->method('getUID')->willReturn('userB');
566+
$userC = $this->createMock(IUser::class);
567+
$userC->method('getUID')->willReturn('userC');
568+
$group = $this->createMock(IGroup::class);
569+
$group->method('getUsers')->willReturn([$userB, $userC]);
570+
$this->groupManager->method('get')->with('Group')->willReturn($group);
571+
572+
$this->defaultProvider->method('getSharesBy')
573+
->willReturn([]);
574+
$manager->method('generalCreateChecks')->willThrowException(new GenericShareException());
575+
576+
$manager->method('getSharedWith')->willReturn([]);
577+
$manager->expects($this->exactly(2))->method('getSharesInFolder')->willReturnOnConsecutiveCalls([[$reShare1]], [[$reShare2]]);
578+
579+
$manager->expects($this->exactly(2))->method('deleteShare')->withConsecutive([$reShare1], [$reShare2]);
580+
581+
$manager->deleteReshare($share);
582+
}
583+
472584
public function testGetShareById(): void {
473585
$share = $this->createMock(IShare::class);
474586

0 commit comments

Comments
 (0)