fix: Properly handle limited scope for remapping users

Signed-off-by: Julius Härtl <jus@bitgrid.net>
2022-03-14 17:25:06 +01:00
parent 7f3a318fc5
commit 7786c86f47
6 changed files with 149 additions and 66 deletions
--- a/lib/Db/AclMapper.php
+++ b/lib/Db/AclMapper.php
@@ -25,6 +25,7 @@ namespace OCA\Deck\Db;

 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;

 class AclMapper extends DeckMapper implements IPermissionMapper {
@@ -59,23 +60,14 @@ class AclMapper extends DeckMapper implements IPermissionMapper {
 	}

 	/**
-	 * @param $ownerId
-	 * @param $newOwnerId
-	 * @return void
+	 * @throws \OCP\DB\Exception
 	 */
-	public function transferOwnership($boardId, $newOwnerId) {
-		$params = [
-			'newOwner' => $newOwnerId,
-			'type' => Acl::PERMISSION_TYPE_USER,
-			'boardId' => $boardId
-		];
-
-		// Drop existing ACL rules for the new owner
-		$sql = "DELETE FROM `{$this->tableName}` 
-                    WHERE `participant` = :newOwner 
-                        AND `type` = :type 
-                        AND `board_id` = :boardId";
-		$stmt = $this->execute($sql, $params);
-		$stmt->closeCursor();
+	public function deleteParticipantFromBoard(int $boardId, int $type, string $participant): void {
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete('deck_board_acl')
+			->where($qb->expr()->eq('type', $qb->createNamedParameter($type, IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('participant', $qb->createNamedParameter($participant, IQueryBuilder::PARAM_STR)))
+			->andWhere($qb->expr()->eq('board_id', $qb->createNamedParameter($boardId, IQueryBuilder::PARAM_INT)));
+		$qb->executeStatement();
 	}
 }
--- a/lib/Db/AssignmentMapper.php
+++ b/lib/Db/AssignmentMapper.php
@@ -29,6 +29,7 @@ use OCA\Deck\NotFoundException;
 use OCA\Deck\Service\CirclesService;
 use OCP\AppFramework\Db\Entity;
 use OCP\AppFramework\Db\QBMapper;
+use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDBConnection;
 use OCP\IGroupManager;
 use OCP\IUserManager;
@@ -147,26 +148,38 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper {
 		return null;
 	}

-	/**
-	 * @psalm-suppress InvalidScalarArgument
-	 * @param $ownerId
-	 * @param $newOwnerId
-	 * @return void
-	 */
-	public function transferOwnership(string $ownerId, string $newOwnerId, int $boardId = null) {
-		$params = [
-			'owner' => $ownerId,
-			'newOwner' => $newOwnerId,
-			'type' => Assignment::TYPE_USER
-		];
-		$qb = $this->db->getQueryBuilder();
-		$sql = "DELETE FROM `*PREFIX*{$this->tableName}`  WHERE `participant` = :newOwner AND `type`= :type AND id IN 
-			(SELECT id FROM `*PREFIX*{$this->tableName}`  WHERE `participant` = :owner)";
-		$stmt = $this->db->executeQuery($sql, $params);
-		$stmt->closeCursor();
+	public function remapAssignedUser(int $boardId, string $userId, string $newUserId): void {
+		$subQuery = $this->db->getQueryBuilder();
+		$subQuery->selectAlias('a.id', 'id')
+			->from('deck_assigned_users', 'a')
+			->innerJoin('a', 'deck_cards', 'c', 'c.id = a.card_id')
+			->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id')
+			->where($subQuery->expr()->eq('a.type', $subQuery->createNamedParameter(Assignment::TYPE_USER, IQueryBuilder::PARAM_INT)))
+			->andWhere($subQuery->expr()->eq('a.participant', $subQuery->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
+			->andWhere($subQuery->expr()->eq('s.board_id', $subQuery->createNamedParameter($boardId, IQueryBuilder::PARAM_INT)))
+			->setMaxResults(1000);

-		$sql = "UPDATE `*PREFIX*{$this->tableName}`  SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type";
-		$stmt = $this->db->executeQuery($sql, $params);
-		$stmt->closeCursor();
+		$qb = $this->db->getQueryBuilder();
+		$qb->update('deck_assigned_users')
+			->set('participant', $qb->createParameter('participant'))
+			->where($qb->expr()->in('id', $qb->createParameter('ids')));
+
+		$moreResults = true;
+		do {
+			$result = $subQuery->executeQuery();
+			$ids = array_map(function ($item) {
+				return $item['id'];
+			}, $result->fetchAll());
+
+			if (count($ids) === 0 || $result->rowCount() === 0) {
+				$moreResults = false;
+			}
+
+			$qb->setParameter('participant', $newUserId, IQueryBuilder::PARAM_STR);
+			$qb->setParameter('ids', $ids, IQueryBuilder::PARAM_INT_ARRAY);
+			$qb->executeStatement();
+		} while ($moreResults === true);
+
+		$result->closeCursor();
 	}
 }
--- a/lib/Db/CardMapper.php
+++ b/lib/Db/CardMapper.php
@@ -596,4 +596,37 @@ class CardMapper extends QBMapper implements IPermissionMapper {
 		$stmt = $this->db->executeQuery($sql, $params);
 		$stmt->closeCursor();
 	}
+
+	public function remapCardOwner(int $boardId, string $userId, string $newUserId): void {
+		$subQuery = $this->db->getQueryBuilder();
+		$subQuery->selectAlias('c.id', 'id')
+			->from('deck_cards', 'c')
+			->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id')
+			->where($subQuery->expr()->eq('c.owner', $subQuery->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
+			->andWhere($subQuery->expr()->eq('s.board_id', $subQuery->createNamedParameter($boardId, IQueryBuilder::PARAM_INT)))
+			->setMaxResults(1000);
+
+		$qb = $this->db->getQueryBuilder();
+		$qb->update('deck_cards')
+			->set('owner', $qb->createParameter('owner'))
+			->where($qb->expr()->in('id', $qb->createParameter('ids')));
+
+		$moreResults = true;
+		do {
+			$result = $subQuery->executeQuery();
+			$ids = array_map(function ($item) {
+				return $item['id'];
+			}, $result->fetchAll());
+
+			if (count($ids) === 0 || $result->rowCount() === 0) {
+				$moreResults = false;
+			}
+
+			$qb->setParameter('owner', $newUserId, IQueryBuilder::PARAM_STR);
+			$qb->setParameter('ids', $ids, IQueryBuilder::PARAM_INT_ARRAY);
+			$qb->executeStatement();
+		} while ($moreResults === true);
+
+		$result->closeCursor();
+	}
 }
--- a/lib/Service/BoardService.php
+++ b/lib/Service/BoardService.php
@@ -685,28 +685,38 @@ class BoardService {
 		$board = $this->boardMapper->find($boardId);
 		$previousOwner = $board->getOwner();
 		$this->clearBoardFromCache($board);
-		$this->aclMapper->transferOwnership($boardId, $newOwner);
+		$this->aclMapper->deleteParticipantFromBoard($boardId, Acl::PERMISSION_TYPE_USER, $newOwner);
 		$this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId);

 		// Optionally also change user assignments and card owner information
 		if ($changeContent) {
-			$this->assignedUsersMapper->transferOwnership($previousOwner, $newOwner, $boardId);
-			$this->cardMapper->transferOwnership($previousOwner, $newOwner, $boardId);
+			$this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner);
+			$this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner);
 		}
 	}

 	public function transferOwnership(string $owner, string $newOwner, $changeContent = false): void {
-		$boards = $this->boardMapper->findAllByUser($owner);
-		foreach ($boards as $board) {
-			$this->clearBoardFromCache($board);
-			$this->aclMapper->transferOwnership($board->getId(), $newOwner);
-		}
-		$this->boardMapper->transferOwnership($owner, $newOwner);
+		\OC::$server->getDatabaseConnection()->beginTransaction();
+		try {
+			$boards = $this->boardMapper->findAllByUser($owner);
+			foreach ($boards as $board) {
+				$this->clearBoardFromCache($board);
+			}

-		// Optionally also change user assignments and card owner information
-		if ($changeContent) {
-			$this->assignedUsersMapper->transferOwnership($owner, $newOwner);
-			$this->cardMapper->transferOwnership($owner, $newOwner);
+			$this->boardMapper->transferOwnership($owner, $newOwner);
+			// Optionally also change user assignments and card owner information
+			if ($changeContent) {
+				foreach ($boards as $board) {
+					$this->clearBoardFromCache($board);
+					$this->aclMapper->deleteParticipantFromBoard($board->getId(), Acl::PERMISSION_TYPE_USER, $newOwner);
+					$this->assignedUsersMapper->remapAssignedUser($board->getId(), $owner, $newOwner);
+					$this->cardMapper->remapCardOwner($board->getId(), $owner, $newOwner);
+				}
+			}
+
+			\OC::$server->getDatabaseConnection()->commit();
+		} catch (\Throwable $e) {
+			\OC::$server->getDatabaseConnection()->rollBack();
 		}
 	}

--- a/tests/integration/config/behat.yml
+++ b/tests/integration/config/behat.yml
@@ -5,7 +5,7 @@ default:
        - '%paths.base%/../features/'
      contexts:
        - ServerContext:
-            baseUrl:  http://localhost:9090/
+            baseUrl:  http://localhost:8080/
        - RequestContext
        - BoardContext
        - CommentContext
--- a/tests/integration/database/TransferOwnershipTest.php
+++ b/tests/integration/database/TransferOwnershipTest.php
@@ -157,11 +157,9 @@ class TransferOwnershipTest extends \Test\TestCase {
 	 */
 	public function testReassignCardToNewOwner() {
 		$this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true);
-		$users = $this->assignmentMapper->findAll($this->cards[0]->getId());
-		$participantsUIDs = [];
-		foreach ($users as $user) {
-			$participantsUIDs[] = $user->getParticipant();
-		}
+		$participantsUIDs = array_map(function ($user) {
+			return $user->getParticipant();
+		}, $this->assignmentMapper->findAll($this->cards[0]->getId()));
 		$this->assertContains(self::TEST_USER_2, $participantsUIDs);
 		$this->assertNotContains(self::TEST_USER_1, $participantsUIDs);
 	}
@@ -171,11 +169,9 @@ class TransferOwnershipTest extends \Test\TestCase {
 	 */
 	public function testNoReassignCardToNewOwner() {
 		$this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false);
-		$users = $this->assignmentMapper->findAll($this->cards[0]->getId());
-		$participantsUIDs = [];
-		foreach ($users as $user) {
-			$participantsUIDs[] = $user->getParticipant();
-		}
+		$participantsUIDs = array_map(function ($user) {
+			return $user->getParticipant();
+		}, $this->assignmentMapper->findAll($this->cards[0]->getId()));
 		$this->assertContains(self::TEST_USER_1, $participantsUIDs);
 		$this->assertNotContains(self::TEST_USER_2, $participantsUIDs);
 	}
@@ -186,11 +182,9 @@ class TransferOwnershipTest extends \Test\TestCase {
 	public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() {
 		$this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, Assignment::TYPE_GROUP);
 		$this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2);
-		$users = $this->assignmentMapper->findAll($this->cards[1]->getId());
-		$participantsUIDs = [];
-		foreach ($users as $user) {
-			$participantsUIDs[] = $user->getParticipant();
-		}
+		$participantsUIDs = array_map(function ($user) {
+			return $user->getParticipant();
+		}, $this->assignmentMapper->findAll($this->cards[1]->getId()));
 		$this->assertContains(self::TEST_USER_1, $participantsUIDs);
 		$this->assertNotContains(self::TEST_USER_2, $participantsUIDs);
 	}
@@ -243,6 +237,47 @@ class TransferOwnershipTest extends \Test\TestCase {
 		$this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3);
 	}

+	/**
+	 * @covers ::transferOwnership
+	 */
+	public function testTransferSingleBoardAssignment() {
+		// Arrange separate board next to the one being transferred
+		$board = $this->boardService->create('Test 2', self::TEST_USER_1, '000000');
+		$id = $board->getId();
+		$this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_USER_1, true, true, true);
+		$this->boardService->addAcl($id, Acl::PERMISSION_TYPE_GROUP, self::TEST_GROUP, true, true, true);
+		$this->boardService->addAcl($id, Acl::PERMISSION_TYPE_USER, self::TEST_USER_3, false, true, false);
+		$stacks[] = $this->stackService->create('Stack A', $id, 1);
+		$stacks[] = $this->stackService->create('Stack B', $id, 1);
+		$stacks[] = $this->stackService->create('Stack C', $id, 1);
+		$cards[] = $this->cardService->create('Card 1', $stacks[0]->getId(), 'text', 0, self::TEST_USER_1);
+		$cards[] = $this->cardService->create('Card 2', $stacks[0]->getId(), 'text', 0, self::TEST_USER_1);
+		$this->assignmentService->assignUser($cards[0]->getId(), self::TEST_USER_1);
+
+		// Act
+		$this->boardService->transferBoardOwnership($this->board->getId(), self::TEST_USER_2, true);
+
+		// Assert that the selected board was transferred
+		$card = $this->cardService->find($this->cards[0]->getId());
+		$this->assertEquals(self::TEST_USER_2, $card->getOwner());
+
+		$participantsUIDs = array_map(function ($assignment) {
+			return $assignment->getParticipant();
+		}, $this->assignmentMapper->findAll($this->cards[0]->getId()));
+		$this->assertContains(self::TEST_USER_2, $participantsUIDs);
+		$this->assertNotContains(self::TEST_USER_1, $participantsUIDs);
+
+		// Assert that other board remained unchanged
+		$card = $this->cardService->find($cards[0]->getId());
+		$this->assertEquals(self::TEST_USER_1, $card->getOwner());
+
+		$participantsUIDs = array_map(function ($assignment) {
+			return $assignment->getParticipant();
+		}, $this->assignmentMapper->findAll($cards[0]->getId()));
+		$this->assertContains(self::TEST_USER_1, $participantsUIDs);
+		$this->assertNotContains(self::TEST_USER_2, $participantsUIDs);
+	}
+
 	public function tearDown(): void {
 		$this->boardService->deleteForce($this->board->getId());
 		parent::tearDown();