From 379f1144b3624616e36f45e661b1e815b7f5007c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 21 Mar 2022 13:42:15 +0100 Subject: [PATCH] Cover case where the owner is preserved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Controller/BoardController.php | 3 +- lib/Service/BoardService.php | 10 +++- src/components/board/SharingTabSidebar.vue | 2 +- .../database/TransferOwnershipTest.php | 58 ++++++++++++++----- 4 files changed, 56 insertions(+), 17 deletions(-) diff --git a/lib/Controller/BoardController.php b/lib/Controller/BoardController.php index b82629723..c5e2c62a0 100644 --- a/lib/Controller/BoardController.php +++ b/lib/Controller/BoardController.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Controller; use OCA\Deck\Db\Acl; +use OCA\Deck\Db\Board; use OCA\Deck\Service\BoardService; use OCA\Deck\Service\PermissionService; use OCP\AppFramework\ApiController; @@ -152,7 +153,7 @@ class BoardController extends ApiController { /** * @NoAdminRequired * @param $boardId - * @return \OCP\Deck\DB\Board + * @return Board */ public function clone($boardId) { return $this->boardService->clone($boardId, $this->userId); diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 556408551..ee0da6720 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Service; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use OCA\Deck\Activity\ActivityManager; use OCA\Deck\Activity\ChangeSet; use OCA\Deck\AppInfo\Application; @@ -694,6 +695,11 @@ class BoardService { if ($changeContent) { $this->assignedUsersMapper->remapAssignedUser($boardId, $previousOwner, $newOwner); $this->cardMapper->remapCardOwner($boardId, $previousOwner, $newOwner); + } else { + try { + $this->addAcl($boardId, Acl::PERMISSION_TYPE_USER, $previousOwner, true, true, true); + } catch (UniqueConstraintViolationException $e) { + } } \OC::$server->getDatabaseConnection()->commit(); return $this->boardMapper->find($boardId); @@ -706,7 +712,9 @@ class BoardService { public function transferOwnership(string $owner, string $newOwner, bool $changeContent = false): \Generator { $boards = $this->boardMapper->findAllByUser($owner); foreach ($boards as $board) { - yield $this->transferBoardOwnership($board->getId(), $newOwner, $changeContent); + if ($board->getOwner() === $owner) { + yield $this->transferBoardOwnership($board->getId(), $newOwner, $changeContent); + } } } diff --git a/src/components/board/SharingTabSidebar.vue b/src/components/board/SharingTabSidebar.vue index b51089eb6..0aa2136d2 100644 --- a/src/components/board/SharingTabSidebar.vue +++ b/src/components/board/SharingTabSidebar.vue @@ -53,7 +53,7 @@ {{ t('deck', 'Can manage') }} - + {{ t('deck', 'Owner') }} diff --git a/tests/integration/database/TransferOwnershipTest.php b/tests/integration/database/TransferOwnershipTest.php index c7e9662d5..a88178c06 100644 --- a/tests/integration/database/TransferOwnershipTest.php +++ b/tests/integration/database/TransferOwnershipTest.php @@ -64,7 +64,6 @@ class TransferOwnershipTest extends \Test\TestCase { $stacks = []; $board = $this->boardService->create('Test', 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); @@ -110,13 +109,20 @@ class TransferOwnershipTest extends \Test\TestCase { * @covers ::transferOwnership */ public function testTransferACLOwnership() { - iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true)); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); - // Check if old owner is no longer in ACL - $this->assertTrue((bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_USER_1 && $item->getType() === Acl::PERMISSION_TYPE_USER; - })); + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_1); + } + + /** + * @covers ::transferOwnership + */ + public function testTransferACLOwnershipPreserveOwner() { + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, false)); + $board = $this->boardService->find($this->board->getId()); + $acl = $board->getAcl(); + $this->assertBoardHasAclUser($board, self::TEST_USER_1); } /** @@ -196,17 +202,41 @@ class TransferOwnershipTest extends \Test\TestCase { iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3)); } + private function assertBoardHasAclUser($board, $userId) { + $hasUser = (bool)array_filter($board->getAcl(), function ($item) use ($userId) { + return $item->getParticipant() === $userId && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + self::assertTrue($hasUser, 'user ' . $userId . ' should be in the board acl list'); + } + + private function assertBoardDoesNotHaveAclUser($board, $userId) { + $hasUser = (bool)array_filter($board->getAcl(), function ($item) use ($userId) { + return $item->getParticipant() === $userId && $item->getType() === Acl::PERMISSION_TYPE_USER; + }); + self::assertFalse($hasUser, 'user ' . $userId . ' should not be in the board acl list'); + } + /** * @covers ::transferOwnership */ - public function testDontRemoveTargetFromAcl() { - iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3)); + public function testDontRemoveOldOwnerFromAcl() { + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2)); $board = $this->boardService->find($this->board->getId()); - $acl = $board->getAcl(); - $isOwnerInAcl = (bool)array_filter($acl, function ($item) { - return $item->getParticipant() === self::TEST_USER_3 && $item->getType() === Acl::PERMISSION_TYPE_USER; - }); - $this->assertTrue($isOwnerInAcl); + + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_2); + $this->assertBoardHasAclUser($board, self::TEST_USER_3); + $this->assertBoardHasAclUser($board, self::TEST_USER_1); + } + + /** + * @covers ::transferOwnership + */ + public function testRemoveOldOwnerFromAclForChange() { + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_2, true)); + $board = $this->boardService->find($this->board->getId()); + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_2); + $this->assertBoardHasAclUser($board, self::TEST_USER_3); + $this->assertBoardDoesNotHaveAclUser($board, self::TEST_USER_1); } /** @@ -214,7 +244,7 @@ class TransferOwnershipTest extends \Test\TestCase { */ public function testMergePermissions() { $this->boardService->addAcl($this->board->getId(), Acl::PERMISSION_TYPE_USER, self::TEST_USER_2, true, false, true); - iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_2, self::TEST_USER_3)); + iterator_to_array($this->boardService->transferOwnership(self::TEST_USER_1, self::TEST_USER_3)); $board = $this->boardService->find($this->board->getId()); $acl = $board->getAcl(); $isMerged = (bool)array_filter($acl, function ($item) {