Fix remote group share API interactions

Accepting and declining can now be done repeatedly on both the parent
group share and sub-share with the same effects.

Added unit tests to cover these cases, and also when the same operation
is repeated.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>

2 files changed, 199 insertions, 11 deletions

diff --git a/apps/files_sharing/lib/External/Manager.php b/apps/files_sharing/lib/External/Manager.php
index b67ca675c31..082e1adef8a 100644
--- a/apps/files_sharing/lib/External/Manager.php
+++ b/apps/files_sharing/lib/External/Manager.php
@@ -238,6 +238,20 @@ class Manager {
 		return $share;
 	}
 
+	private function fetchUserShare($parentId, $uid) {
+		$getShare = $this->connection->prepare('
+			SELECT `id`, `remote`, `remote_id`, `share_token`, `name`, `owner`, `user`, `mountpoint`, `accepted`, `parent`, `share_type`, `password`, `mountpoint_hash`
+			FROM  `*PREFIX*share_external`
+			WHERE `parent` = ? AND `user` = ?');
+		$result = $getShare->execute([$parentId, $uid]);
+		$share = $result->fetch();
+		$result->closeCursor();
+		if ($share !== false) {
+			return $share;
+		}
+		return null;
+	}
+
 	/**
 	 * get share
 	 *
@@ -311,9 +325,15 @@ class Manager {
 			} else {
 				$parentId = (int)$share['parent'];
 				if ($parentId !== -1) {
-					// this is the sub-share, simply update it to re-accept
+					// this is the sub-share
+					$subshare = $share;
+				} else {
+					$subshare = $this->fetchUserShare($id, $this->uid);
+				}
+
+				if ($subshare !== null) {
 					try {
-						$this->updateAccepted((int)$share['id'], true);
+						$this->updateAccepted((int)$subshare['id'], true);
 						$result = true;
 					} catch (Exception $e) {
 						$this->logger->logException($e);
@@ -372,13 +392,17 @@ class Manager {
 			$this->processNotification($id);
 			$result = true;
 		} elseif ($share && (int)$share['share_type'] === IShare::TYPE_GROUP) {
-			$parent = (int)$share['parent'];
-			// can only decline an already accepted/mounted group share,
-			// check if this is the sub-share entry
-			if ($parent !== -1) {
+			$parentId = (int)$share['parent'];
+			if ($parentId !== -1) {
+				// this is the sub-share
+				$subshare = $share;
+			} else {
+				$subshare = $this->fetchUserShare($id, $this->uid);
+			}
+
+			if ($subshare !== null) {
 				try {
-					// this is the sub-share, simply update it to decline
-					$this->updateAccepted((int)$share['id'], false);
+					$this->updateAccepted((int)$subshare['id'], false);
 					$result = true;
 				} catch (Exception $e) {
 					$this->logger->logException($e);
@@ -566,6 +590,10 @@ class Manager {
 
 	public function removeShare($mountPoint): bool {
 		$mountPointObj = $this->mountManager->find($mountPoint);
+		if ($mountPointObj === null) {
+			$this->logger->error('Mount point to remove share not found', ['mountPoint' => $mountPoint]);
+			return false;
+		}
 		$id = $mountPointObj->getStorage()->getCache()->getId('');
 
 		$mountPoint = $this->stripPath($mountPoint);
diff --git a/apps/files_sharing/tests/External/ManagerTest.php b/apps/files_sharing/tests/External/ManagerTest.php
index b80b9d04e18..2340d057dd7 100644
--- a/apps/files_sharing/tests/External/ManagerTest.php
+++ b/apps/files_sharing/tests/External/ManagerTest.php
@@ -113,6 +113,9 @@ class ManagerTest extends TestCase {
 			->method('search')
 			->willReturn([]);
 
+		$logger = $this->createMock(ILogger::class);
+		$logger->expects($this->never())->method('logException');
+
 		$this->manager = $this->getMockBuilder(Manager::class)
 			->setConstructorArgs(
 				[
@@ -128,7 +131,7 @@ class ManagerTest extends TestCase {
 					$this->userManager,
 					$this->uid,
 					$this->eventDispatcher,
-					$this->createMock(ILogger::class),
+					$logger,
 				]
 			)->setMethods(['tryOCMEndPoint'])->getMock();
 
@@ -155,6 +158,7 @@ class ManagerTest extends TestCase {
 	}
 
 	private function setupMounts() {
+		$this->mountManager->clear();
 		$mounts = $this->testMountProvider->getMountsForUser($this->user, new StorageFactory());
 		foreach ($mounts as $mount) {
 			$this->mountManager->addMount($mount);
@@ -247,7 +251,7 @@ class ManagerTest extends TestCase {
 		}
 
 		// Accept the first share
-		$this->manager->acceptShare($openShares[0]['id']);
+		$this->assertTrue($this->manager->acceptShare($openShares[0]['id']));
 
 		// Check remaining shares - Accepted
 		$acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]);
@@ -303,7 +307,7 @@ class ManagerTest extends TestCase {
 		}
 
 		// Decline the third share
-		$this->manager->declineShare($openShares[1]['id']);
+		$this->assertTrue($this->manager->declineShare($openShares[1]['id']));
 
 		$this->setupMounts();
 		$this->assertMount($shareData1['name']);
@@ -379,6 +383,162 @@ class ManagerTest extends TestCase {
 		$this->assertNotMount('{{TemporaryMountPointName#' . $shareData1['name'] . '}}-1');
 	}
 
+	private function verifyAcceptedGroupShare($shareData) {
+		$openShares = $this->manager->getOpenShares();
+		$this->assertCount(0, $openShares);
+		$acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]);
+		$this->assertCount(1, $acceptedShares);
+		$shareData['accepted'] = true;
+		$this->assertExternalShareEntry($shareData, $acceptedShares[0], 0, $shareData['name'], $this->uid);
+		$this->setupMounts();
+		$this->assertMount($shareData['name']);
+	}
+
+	private function verifyDeclinedGroupShare($shareData, $tempMount = null) {
+		if ($tempMount === null) {
+			$tempMount = '{{TemporaryMountPointName#/SharedFolder}}';
+		}
+		$openShares = $this->manager->getOpenShares();
+		$this->assertCount(1, $openShares);
+		$acceptedShares = self::invokePrivate($this->manager, 'getShares', [true]);
+		$this->assertCount(0, $acceptedShares);
+		$this->assertExternalShareEntry($shareData, $openShares[0], 0, $tempMount, $this->uid);
+		$this->setupMounts();
+		$this->assertNotMount($shareData['name']);
+		$this->assertNotMount($tempMount);
+	}
+
+	private function createTestGroupShare() {
+		$shareData = [
+			'remote' => 'http://localhost',
+			'token' => 'token1',
+			'password' => '',
+			'name' => '/SharedFolder',
+			'owner' => 'foobar',
+			'shareType' => IShare::TYPE_GROUP,
+			'accepted' => false,
+			'user' => 'group1',
+			'remoteId' => '2342'
+		];
+
+		$this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData));
+
+		$allShares = self::invokePrivate($this->manager, 'getShares', [null]);
+		$this->assertCount(1, $allShares);
+
+		// this will hold the main group entry
+		$groupShare = $allShares[0];
+
+		return [$shareData, $groupShare];
+	}
+
+	public function testAcceptOriginalGroupShare() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// a second time
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+	}
+
+	public function testAcceptGroupShareAgainThroughGroupShare() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// decline again, this keeps the sub-share
+		$this->assertTrue($this->manager->declineShare($groupShare['id']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+
+		// this will return sub-entries
+		$openShares = $this->manager->getOpenShares();
+
+		// accept through sub-share
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData, '/SharedFolder');
+
+		// accept a second time
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData, '/SharedFolder');
+	}
+
+	public function testAcceptGroupShareAgainThroughSubShare() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// decline again, this keeps the sub-share
+		$this->assertTrue($this->manager->declineShare($groupShare['id']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+
+		// this will return sub-entries
+		$openShares = $this->manager->getOpenShares();
+
+		// accept through sub-share
+		$this->assertTrue($this->manager->acceptShare($openShares[0]['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// accept a second time
+		$this->assertTrue($this->manager->acceptShare($openShares[0]['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+	}
+
+	public function testDeclineOriginalGroupShare() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->declineShare($groupShare['id']));
+		$this->verifyDeclinedGroupShare($shareData);
+
+		// a second time
+		$this->assertTrue($this->manager->declineShare($groupShare['id']));
+		$this->verifyDeclinedGroupShare($shareData);
+	}
+
+	public function testDeclineGroupShareAgainThroughGroupShare() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// decline again, this keeps the sub-share
+		$this->assertTrue($this->manager->declineShare($groupShare['id']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+
+		// a second time
+		$this->assertTrue($this->manager->declineShare($groupShare['id']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+	}
+
+	public function testDeclineGroupShareAgainThroughSubshare() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// this will return sub-entries
+		$allShares = self::invokePrivate($this->manager, 'getShares', [null]);
+		$this->assertCount(1, $allShares);
+
+		// decline again through sub-share
+		$this->assertTrue($this->manager->declineShare($allShares[0]['id']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+
+		// a second time
+		$this->assertTrue($this->manager->declineShare($allShares[0]['id']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+	}
+
+	public function testDeclineGroupShareAgainThroughMountPoint() {
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+		$this->verifyAcceptedGroupShare($shareData);
+
+		// decline through mount point name
+		$this->assertTrue($this->manager->removeShare($this->uid . '/files/' . $shareData['name']));
+		$this->verifyDeclinedGroupShare($shareData, '/SharedFolder');
+
+		// second time must fail as the mount point is gone
+		$this->assertFalse($this->manager->removeShare($this->uid . '/files/' . $shareData['name']));
+	}
+
 	/**
 	 * @param array $expected
 	 * @param array $actual