ahtlon/deck
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix: Properly handle limited scope for remapping users

Browse Source 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
This commit is contained in:
Julius Härtl
2022-03-14 17:25:06 +01:00
parent a032287cb5
commit c2144373d9
6 changed files with 149 additions and 66 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
lib/Db/AclMapper.php
View File

@@ -25,6 +25,7 @@ namespace OCA\Deck\Db;
use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Db\DoesNotExistException;
use OCP\AppFramework\Db\MultipleObjectsReturnedException; use OCP\AppFramework\Db\MultipleObjectsReturnedException;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection; use OCP\IDBConnection;
class AclMapper extends DeckMapper implements IPermissionMapper { class AclMapper extends DeckMapper implements IPermissionMapper {
@@ -59,23 +60,14 @@ class AclMapper extends DeckMapper implements IPermissionMapper {
} }
/** /**
* @param $ownerId * @throws \OCP\DB\Exception
* @param $newOwnerId
* @return void
*/ */
public function transferOwnership($boardId, $newOwnerId) { public function deleteParticipantFromBoard(int $boardId, int $type, string $participant): void {
$params = [ $qb = $this->db->getQueryBuilder();
'newOwner' => $newOwnerId, $qb->delete('deck_board_acl')
'type' => Acl::PERMISSION_TYPE_USER, ->where($qb->expr()->eq('type', $qb->createNamedParameter($type, IQueryBuilder::PARAM_INT)))
'boardId' => $boardId ->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();
// 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();
} }
} }

53
lib/Db/AssignmentMapper.php
View File

@@ -29,6 +29,7 @@ use OCA\Deck\NotFoundException;
use OCA\Deck\Service\CirclesService; use OCA\Deck\Service\CirclesService;
use OCP\AppFramework\Db\Entity; use OCP\AppFramework\Db\Entity;
use OCP\AppFramework\Db\QBMapper; use OCP\AppFramework\Db\QBMapper;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection; use OCP\IDBConnection;
use OCP\IGroupManager; use OCP\IGroupManager;
use OCP\IUserManager; use OCP\IUserManager;
@@ -147,26 +148,38 @@ class AssignmentMapper extends QBMapper implements IPermissionMapper {
return null; return null;
} }
/** public function remapAssignedUser(int $boardId, string $userId, string $newUserId): void {
* @psalm-suppress InvalidScalarArgument $subQuery = $this->db->getQueryBuilder();
* @param $ownerId $subQuery->selectAlias('a.id', 'id')
* @param $newOwnerId ->from('deck_assigned_users', 'a')
* @return void ->innerJoin('a', 'deck_cards', 'c', 'c.id = a.card_id')
*/ ->innerJoin('c', 'deck_stacks', 's', 's.id = c.stack_id')
public function transferOwnership(string $ownerId, string $newOwnerId, int $boardId = null) { ->where($subQuery->expr()->eq('a.type', $subQuery->createNamedParameter(Assignment::TYPE_USER, IQueryBuilder::PARAM_INT)))
$params = [ ->andWhere($subQuery->expr()->eq('a.participant', $subQuery->createNamedParameter($userId, IQueryBuilder::PARAM_STR)))
'owner' => $ownerId, ->andWhere($subQuery->expr()->eq('s.board_id', $subQuery->createNamedParameter($boardId, IQueryBuilder::PARAM_INT)))
'newOwner' => $newOwnerId, ->setMaxResults(1000);
'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();
$sql = "UPDATE `*PREFIX*{$this->tableName}` SET `participant` = :newOwner WHERE `participant` = :owner AND `type`= :type"; $qb = $this->db->getQueryBuilder();
$stmt = $this->db->executeQuery($sql, $params); $qb->update('deck_assigned_users')
$stmt->closeCursor(); ->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();
} }
} }

33
lib/Db/CardMapper.php
View File

@@ -596,4 +596,37 @@ class CardMapper extends QBMapper implements IPermissionMapper {
$stmt = $this->db->executeQuery($sql, $params); $stmt = $this->db->executeQuery($sql, $params);
$stmt->closeCursor(); $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();
}
} }

36
lib/Service/BoardService.php
View File

@@ -685,28 +685,38 @@ class BoardService {
$board = $this->boardMapper->find($boardId); $board = $this->boardMapper->find($boardId);
$previousOwner = $board->getOwner(); $previousOwner = $board->getOwner();
$this->clearBoardFromCache($board); $this->clearBoardFromCache($board);
$this->aclMapper->transferOwnership($boardId, $newOwner); $this->aclMapper->deleteParticipantFromBoard($boardId, Acl::PERMISSION_TYPE_USER, $newOwner);
$this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId); $this->boardMapper->transferOwnership($previousOwner, $newOwner, $boardId);
// Optionally also change user assignments and card owner information // Optionally also change user assignments and card owner information
if ($changeContent) { if ($changeContent) {
$this->assignedUsersMapper->transferOwnership($previousOwner, $newOwner, $boardId); $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner);
$this->cardMapper->transferOwnership($previousOwner, $newOwner, $boardId); $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner);
} }
} }
public function transferOwnership(string $owner, string $newOwner, $changeContent = false): void { public function transferOwnership(string $owner, string $newOwner, $changeContent = false): void {
$boards = $this->boardMapper->findAllByUser($owner); \OC::$server->getDatabaseConnection()->beginTransaction();
foreach ($boards as $board) { try {
$this->clearBoardFromCache($board); $boards = $this->boardMapper->findAllByUser($owner);
$this->aclMapper->transferOwnership($board->getId(), $newOwner); foreach ($boards as $board) {
} $this->clearBoardFromCache($board);
$this->boardMapper->transferOwnership($owner, $newOwner); }
// Optionally also change user assignments and card owner information $this->boardMapper->transferOwnership($owner, $newOwner);
if ($changeContent) { // Optionally also change user assignments and card owner information
$this->assignedUsersMapper->transferOwnership($owner, $newOwner); if ($changeContent) {
$this->cardMapper->transferOwnership($owner, $newOwner); 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();
} }
} }

2
tests/integration/config/behat.yml
View File

@@ -5,7 +5,7 @@ default:
- '%paths.base%/../features/' - '%paths.base%/../features/'
contexts: contexts:
- ServerContext: - ServerContext:
baseUrl: http://localhost:9090/ baseUrl: http://localhost:8080/
- RequestContext - RequestContext
- BoardContext - BoardContext
- CommentContext - CommentContext

65
tests/integration/database/TransferOwnershipTest.php
View File

@@ -157,11 +157,9 @@ class TransferOwnershipTest extends \Test\TestCase {
*/ */
public function testReassignCardToNewOwner() { public function testReassignCardToNewOwner() {
$this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true); $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true);
$users = $this->assignmentMapper->findAll($this->cards[0]->getId()); $participantsUIDs = array_map(function ($user) {
$participantsUIDs = []; return $user->getParticipant();
foreach ($users as $user) { }, $this->assignmentMapper->findAll($this->cards[0]->getId()));
$participantsUIDs[] = $user->getParticipant();
}
$this->assertContains(self::TEST_USER_2, $participantsUIDs); $this->assertContains(self::TEST_USER_2, $participantsUIDs);
$this->assertNotContains(self::TEST_USER_1, $participantsUIDs); $this->assertNotContains(self::TEST_USER_1, $participantsUIDs);
} }
@@ -171,11 +169,9 @@ class TransferOwnershipTest extends \Test\TestCase {
*/ */
public function testNoReassignCardToNewOwner() { public function testNoReassignCardToNewOwner() {
$this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false); $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false);
$users = $this->assignmentMapper->findAll($this->cards[0]->getId()); $participantsUIDs = array_map(function ($user) {
$participantsUIDs = []; return $user->getParticipant();
foreach ($users as $user) { }, $this->assignmentMapper->findAll($this->cards[0]->getId()));
$participantsUIDs[] = $user->getParticipant();
}
$this->assertContains(self::TEST_USER_1, $participantsUIDs); $this->assertContains(self::TEST_USER_1, $participantsUIDs);
$this->assertNotContains(self::TEST_USER_2, $participantsUIDs); $this->assertNotContains(self::TEST_USER_2, $participantsUIDs);
} }
@@ -186,11 +182,9 @@ class TransferOwnershipTest extends \Test\TestCase {
public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() { public function testReassignCardToNewParticipantOnlyIfParticipantHasUserType() {
$this->assignmentService->assignUser($this->cards[1]->getId(), self::TEST_USER_1, Assignment::TYPE_GROUP); $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); $this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2);
$users = $this->assignmentMapper->findAll($this->cards[1]->getId()); $participantsUIDs = array_map(function ($user) {
$participantsUIDs = []; return $user->getParticipant();
foreach ($users as $user) { }, $this->assignmentMapper->findAll($this->cards[1]->getId()));
$participantsUIDs[] = $user->getParticipant();
}
$this->assertContains(self::TEST_USER_1, $participantsUIDs); $this->assertContains(self::TEST_USER_1, $participantsUIDs);
$this->assertNotContains(self::TEST_USER_2, $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); $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 { public function tearDown(): void {
$this->boardService->deleteForce($this->board->getId()); $this->boardService->deleteForce($this->board->getId());
parent::tearDown(); parent::tearDown();