Fix remote share deletion when deleting user

When deleting a user, we should only delete the direct remote user shares or the remote group based subshares. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
4 years ago · f6f2f63016
parent 1125832623
commit f6f2f63016
2 changed files with 201 additions and 51 deletions
--- a/apps/files_sharing/lib/External/Manager.php
+++ b/apps/files_sharing/lib/External/Manager.php
@ -675,38 +675,69 @@ class Manager {
 			$getShare = $this->connection->prepare('
 				SELECT `id`, `remote`, `share_type`, `share_token`, `remote_id`
 				FROM  `*PREFIX*share_external`
-				WHERE `user` = ?');
-			$result = $getShare->execute([$uid]);
+				WHERE `user` = ?
+				AND `share_type` = ?');
+			$result = $getShare->execute([$uid, IShare::TYPE_USER]);
 			$shares = $result->fetchAll();
 			$result->closeCursor();
-			$deletedGroupShares = [];
+
 			foreach ($shares as $share) {
-				if ((int)$share['share_type'] === IShare::TYPE_GROUP) {
-					$deletedGroupShares[] = $share['id'];
-				} else {
-					$this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline');
-				}
+				$this->sendFeedbackToRemote($share['remote'], $share['share_token'], $share['remote_id'], 'decline');
 			}

-			$query = $this->connection->prepare('
-				DELETE FROM `*PREFIX*share_external`
+			$qb = $this->connection->getQueryBuilder();
+			$qb->delete('share_external')
+				// user field can specify a user or a group
+				->where($qb->expr()->eq('user', $qb->createNamedParameter($uid)))
+				->andWhere(
+					$qb->expr()->orX(
+						// delete direct shares
+						$qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_USER)),
+						// delete sub-shares of group shares for that user
+						$qb->expr()->andX(
+							$qb->expr()->eq('share_type', $qb->expr()->literal(IShare::TYPE_GROUP)),
+							$qb->expr()->neq('parent', $qb->expr()->literal(-1)),
+						)
+					)
+				);
+			$qb->execute();
+		} catch (\Doctrine\DBAL\Exception $ex) {
+			$this->logger->emergency('Could not delete user shares', ['exception' => $ex]);
+			return false;
+		}
+
+		return true;
+	}
+
+	public function removeGroupShares($gid): bool {
+		try {
+			$getShare = $this->connection->prepare('
+				SELECT `id`, `remote`, `share_type`, `share_token`, `remote_id`
+				FROM  `*PREFIX*share_external`
 				WHERE `user` = ?
-			');
-			$deleteResult = $query->execute([$uid]);
-			$deleteResult->closeCursor();
+				AND `share_type` = ?');
+			$result = $getShare->execute([$gid, IShare::TYPE_GROUP]);
+			$shares = $result->fetchAll();
+			$result->closeCursor();

-			// delete sub-entries from deleted parents
-			foreach ($deletedGroupShares as $deletedId) {
-				// TODO: batch this with query builder
-				$query = $this->connection->prepare('
-					DELETE FROM `*PREFIX*share_external`
-					WHERE `parent` = ?
-				');
-				$deleteResult = $query->execute([$deletedId]);
-				$deleteResult->closeCursor();
+			$deletedGroupShares = [];
+			$qb = $this->connection->getQueryBuilder();
+			// delete group share entry and matching sub-entries
+			$qb->delete('share_external')
+			   ->where(
+				   $qb->expr()->orX(
+					   $qb->expr()->eq('id', $qb->createParameter('share_id')),
+					   $qb->expr()->eq('parent', $qb->createParameter('share_parent_id'))
+				   )
+			   );
+
+			foreach ($shares as $share) {
+				$qb->setParameter('share_id', $share['id']);
+				$qb->setParameter('share_parent_id', $share['id']);
+				$qb->execute();
 			}
 		} catch (\Doctrine\DBAL\Exception $ex) {
-			$this->logger->emergency('Could not get shares', ['exception' => $ex]);
+			$this->logger->emergency('Could not delete user shares', ['exception' => $ex]);
 			return false;
 		}

--- a/apps/files_sharing/tests/External/ManagerTest.php
+++ b/apps/files_sharing/tests/External/ManagerTest.php
@ -83,6 +83,9 @@ class ManagerTest extends TestCase {
 	/** @var \PHPUnit\Framework\MockObject\MockObject|IUserManager */
 	private $userManager;

+	/** @var LoggerInterface */
+	private $logger;
+
 	private $uid;

 	/**
@ -114,27 +117,10 @@ class ManagerTest extends TestCase {
 			->method('search')
 			->willReturn([]);

-		$logger = $this->createMock(LoggerInterface::class);
-		$logger->expects($this->never())->method('emergency');
+		$this->logger = $this->createMock(LoggerInterface::class);
+		$this->logger->expects($this->never())->method('emergency');

-		$this->manager = $this->getMockBuilder(Manager::class)
-			->setConstructorArgs(
-				[
-					\OC::$server->getDatabaseConnection(),
-					$this->mountManager,
-					new StorageFactory(),
-					$this->clientService,
-					\OC::$server->getNotificationManager(),
-					\OC::$server->query(\OCP\OCS\IDiscoveryService::class),
-					$this->cloudFederationProviderManager,
-					$this->cloudFederationFactory,
-					$this->groupManager,
-					$this->userManager,
-					$this->uid,
-					$this->eventDispatcher,
-					$logger,
-				]
-			)->setMethods(['tryOCMEndPoint'])->getMock();
+		$this->manager = $this->createManagerForUser($this->uid);

 		$this->testMountProvider = new MountProvider(\OC::$server->getDatabaseConnection(), function () {
 			return $this->manager;
@ -166,6 +152,27 @@ class ManagerTest extends TestCase {
 		parent::tearDown();
 	}

+	private function createManagerForUser($userId) {
+		return $this->getMockBuilder(Manager::class)
+			->setConstructorArgs(
+				[
+					\OC::$server->getDatabaseConnection(),
+					$this->mountManager,
+					new StorageFactory(),
+					$this->clientService,
+					\OC::$server->getNotificationManager(),
+					\OC::$server->query(\OCP\OCS\IDiscoveryService::class),
+					$this->cloudFederationProviderManager,
+					$this->cloudFederationFactory,
+					$this->groupManager,
+					$this->userManager,
+					$userId,
+					$this->eventDispatcher,
+					$this->logger,
+				]
+			)->setMethods(['tryOCMEndPoint'])->getMock();
+	}
+
 	private function setupMounts() {
 		$this->mountManager->clear();
 		$mounts = $this->testMountProvider->getMountsForUser($this->user, new StorageFactory());
@ -349,7 +356,7 @@ class ManagerTest extends TestCase {

 		if ($isGroup) {
 			// no http requests here
-			$this->manager->removeUserShares('group1');
+			$this->manager->removeGroupShares('group1');
 		} else {
 			$client1 = $this->getMockBuilder('OCP\Http\Client\IClient')
 				->disableOriginalConstructor()->getMock();
@ -417,7 +424,24 @@ class ManagerTest extends TestCase {
 		$this->assertNotMount($tempMount);
 	}

-	private function createTestGroupShare() {
+	private function createTestUserShare($userId = 'user1') {
+		$shareData = [
+			'remote' => 'http://localhost',
+			'token' => 'token1',
+			'password' => '',
+			'name' => '/SharedFolder',
+			'owner' => 'foobar',
+			'shareType' => IShare::TYPE_USER,
+			'accepted' => false,
+			'user' => $userId,
+			'remoteId' => '2342'
+		];
+
+		$this->assertSame(null, call_user_func_array([$this->manager, 'addShare'], $shareData));
+
+		return $shareData;
+	}
+	private function createTestGroupShare($groupId = 'group1') {
 		$shareData = [
 			'remote' => 'http://localhost',
 			'token' => 'token1',
@ -426,17 +450,20 @@ class ManagerTest extends TestCase {
 			'owner' => 'foobar',
 			'shareType' => IShare::TYPE_GROUP,
 			'accepted' => false,
-			'user' => 'group1',
+			'user' => $groupId,
 			'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];
+		foreach ($allShares as $share) {
+			if ($share['user'] === $groupId) {
+				// this will hold the main group entry
+				$groupShare = $share;
+				break;
+			}
+		}

 		return [$shareData, $groupShare];
 	}
@ -462,8 +489,9 @@ class ManagerTest extends TestCase {

 		// this will return sub-entries
 		$openShares = $this->manager->getOpenShares();
+		$this->assertCount(1, $openShares);

-		// accept through sub-share
+		// accept through group share
 		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
 		$this->verifyAcceptedGroupShare($shareData, '/SharedFolder');

@ -483,6 +511,7 @@ class ManagerTest extends TestCase {

 		// this will return sub-entries
 		$openShares = $this->manager->getOpenShares();
+		$this->assertCount(1, $openShares);

 		// accept through sub-share
 		$this->assertTrue($this->manager->acceptShare($openShares[0]['id']));
@ -584,6 +613,96 @@ class ManagerTest extends TestCase {
 		$this->verifyAcceptedGroupShare($shareData);
 	}

+	public function testDeleteUserShares() {
+		// user 1 shares
+
+		$shareData = $this->createTestUserShare($this->uid);
+
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+
+		$shares = $this->manager->getOpenShares();
+		$this->assertCount(2, $shares);
+
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+
+		// user 2 shares
+		$manager2 = $this->createManagerForUser('user2');
+		$shareData2 = [
+			'remote' => 'http://localhost',
+			'token' => 'token1',
+			'password' => '',
+			'name' => '/SharedFolder',
+			'owner' => 'foobar',
+			'shareType' => IShare::TYPE_USER,
+			'accepted' => false,
+			'user' => 'user2',
+			'remoteId' => '2342'
+		];
+		$this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2));
+
+		$user2Shares = $manager2->getOpenShares();
+		$this->assertCount(2, $user2Shares);
+
+		$this->manager->expects($this->at(0))->method('tryOCMEndPoint')->with('http://localhost', 'token1', '2342', 'decline')->willReturn([]);
+		$this->manager->removeUserShares($this->uid);
+
+		$user1Shares = $this->manager->getOpenShares();
+		// user share is gone, group is still there
+		$this->assertCount(1, $user1Shares);
+		$this->assertEquals($user1Shares[0]['share_type'], IShare::TYPE_GROUP);
+
+		// user 2 shares untouched
+		$user2Shares = $manager2->getOpenShares();
+		$this->assertCount(2, $user2Shares);
+		$this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_GROUP);
+		$this->assertEquals($user2Shares[0]['user'], 'group1');
+		$this->assertEquals($user2Shares[1]['share_type'], IShare::TYPE_USER);
+		$this->assertEquals($user2Shares[1]['user'], 'user2');
+	}
+
+	public function testDeleteGroupShares() {
+		$shareData = $this->createTestUserShare($this->uid);
+
+		[$shareData, $groupShare] = $this->createTestGroupShare();
+
+		$shares = $this->manager->getOpenShares();
+		$this->assertCount(2, $shares);
+
+		$this->assertTrue($this->manager->acceptShare($groupShare['id']));
+
+		// user 2 shares
+		$manager2 = $this->createManagerForUser('user2');
+		$shareData2 = [
+			'remote' => 'http://localhost',
+			'token' => 'token1',
+			'password' => '',
+			'name' => '/SharedFolder',
+			'owner' => 'foobar',
+			'shareType' => IShare::TYPE_USER,
+			'accepted' => false,
+			'user' => 'user2',
+			'remoteId' => '2342'
+		];
+		$this->assertSame(null, call_user_func_array([$manager2, 'addShare'], $shareData2));
+
+		$user2Shares = $manager2->getOpenShares();
+		$this->assertCount(2, $user2Shares);
+
+		$this->manager->expects($this->never())->method('tryOCMEndPoint');
+		$this->manager->removeGroupShares('group1');
+
+		$user1Shares = $this->manager->getOpenShares();
+		// user share is gone, group is still there
+		$this->assertCount(1, $user1Shares);
+		$this->assertEquals($user1Shares[0]['share_type'], IShare::TYPE_USER);
+
+		// user 2 shares untouched
+		$user2Shares = $manager2->getOpenShares();
+		$this->assertCount(1, $user2Shares);
+		$this->assertEquals($user2Shares[0]['share_type'], IShare::TYPE_USER);
+		$this->assertEquals($user2Shares[0]['user'], 'user2');
+	}
+
 	/**
 	 * @param array $expected
 	 * @param array $actual