fix: delete re-shares when deleting the parent share

Note: Removed part about fix command from original PR Signed-off-by: Luka Trovic <[email protected]> Signed-off-by: Côme Chilliet <[email protected]>
nextcloud · Sep 13, 2024 · f6e514d · f6e514d
1 parent c9e4598
commit f6e514d
Show file tree

Hide file tree

Showing 4 changed files with 189 additions and 4 deletions.
diff --git a/apps/files/lib/Service/OwnershipTransferService.php b/apps/files/lib/Service/OwnershipTransferService.php
@@ -465,6 +465,9 @@ private function restoreShares(
 				}
 			} catch (\OCP\Files\NotFoundException $e) {
 				$output->writeln('<error>Share with id ' . $share->getId() . ' points at deleted file, skipping</error>');
+			} catch (\OCP\Share\Exceptions\GenericShareException $e) {
+				$output->writeln('<error>Share with id ' . $share->getId() . ' is broken, deleting</error>');
+				$this->shareManager->deleteShare($share);
 			} catch (\Throwable $e) {
 				$output->writeln('<error>Could not restore share with id ' . $share->getId() . ':' . $e->getMessage() . ' : ' . $e->getTraceAsString() . '</error>');
 			}

diff --git a/apps/files_sharing/tests/EtagPropagationTest.php b/apps/files_sharing/tests/EtagPropagationTest.php
@@ -277,7 +277,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
@@ -18,6 +18,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;
@@ -1010,6 +1011,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
 	 *
@@ -1033,6 +1099,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
@@ -46,6 +46,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;
@@ -227,7 +228,7 @@ public function dataTestDelete() {
 	 */
 	public function testDelete($shareType, $sharedWith) {
 		$manager = $this->createManagerMock()
-			->setMethods(['getShareById', 'deleteChildren'])
+			->setMethods(['getShareById', 'deleteChildren', 'deleteReshare'])
 			->getMock();
 
 		$manager->method('deleteChildren')->willReturn([]);
@@ -245,6 +246,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())
@@ -269,7 +271,7 @@ public function testDelete($shareType, $sharedWith) {
 
 	public function testDeleteLazyShare() {
 		$manager = $this->createManagerMock()
-			->setMethods(['getShareById', 'deleteChildren'])
+			->setMethods(['getShareById', 'deleteChildren', 'deleteReshare'])
 			->getMock();
 
 		$manager->method('deleteChildren')->willReturn([]);
@@ -288,6 +290,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())
@@ -312,7 +315,7 @@ public function testDeleteLazyShare() {
 
 	public function testDeleteNested() {
 		$manager = $this->createManagerMock()
-			->setMethods(['getShareById'])
+			->setMethods(['getShareById', 'deleteReshare'])
 			->getMock();
 
 		$path = $this->createMock(File::class);
@@ -469,6 +472,115 @@ public function testDeleteChildren() {
 		$this->assertSame($shares, $result);
 	}
 
+	public function testDeleteReshareWhenUserHasOneShare() {
+		$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() {
+		$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() {
+		$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() {
 		$share = $this->createMock(IShare::class);