fix: Tidy up code for reshare deletion

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

2 files changed, 24 insertions, 22 deletions

diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php
index 6722de547da..894e785bc62 100644
--- a/lib/private/Share20/Manager.php
+++ b/lib/private/Share20/Manager.php
@@ -1164,31 +1164,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 = [];
@@ -1197,7 +1198,7 @@ class Manager implements IManager {
 			IShare::TYPE_USER,
 			IShare::TYPE_LINK,
 			IShare::TYPE_REMOTE,
-			IShare::TYPE_EMAIL
+			IShare::TYPE_EMAIL,
 		];
 
 		foreach ($userIds as $userId) {
@@ -1209,8 +1210,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) {
@@ -1224,6 +1225,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);
 			}
 		}
@@ -1252,10 +1254,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 3a220692693..1d74b36d319 100644
--- a/tests/lib/Share20/ManagerTest.php
+++ b/tests/lib/Share20/ManagerTest.php
@@ -242,7 +242,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([]);
@@ -260,7 +260,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())
@@ -285,7 +285,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([]);
@@ -304,7 +304,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())
@@ -329,7 +329,7 @@ class ManagerTest extends \Test\TestCase {
 
 	public function testDeleteNested() {
 		$manager = $this->createManagerMock()
-			->setMethods(['getShareById', 'deleteReshare'])
+			->setMethods(['getShareById', 'deleteReshares'])
 			->getMock();
 
 		$path = $this->createMock(File::class);
@@ -515,7 +515,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 {
@@ -543,7 +543,7 @@ class ManagerTest extends \Test\TestCase {
 
 		$manager->expects($this->never())->method('deleteShare');
 
-		$manager->deleteReshare($share);
+		self::invokePrivate($manager, 'deleteReshares', [$share]);
 	}
 
 	public function testDeleteReshareOfUsersInGroupShare(): void {
@@ -592,7 +592,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 {