From a92dc282a88e7af94201a396b0a0cc3e19b3677f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 2 Sep 2024 14:32:13 +0200 Subject: [PATCH 1/3] fix: Limit label actions to labels of the cards board MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Service/CardService.php | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 639fa078e..3c6742e1e 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -609,8 +609,9 @@ class CardService { public function assignLabel($cardId, $labelId) { $this->cardServiceValidator->check(compact('cardId', 'labelId')); - $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); + $this->permissionService->checkPermission($this->labelMapper, $labelId, Acl::PERMISSION_READ); + if ($this->boardService->isArchived($this->cardMapper, $cardId)) { throw new StatusException('Operation not allowed. This board is archived.'); } @@ -619,6 +620,9 @@ class CardService { throw new StatusException('Operation not allowed. This card is archived.'); } $label = $this->labelMapper->find($labelId); + if ($label->getBoardId() !== $this->cardMapper->findBoardId($card->getId())) { + throw new StatusException('Operation not allowed. Label does not exist.'); + } $this->cardMapper->assignLabel($cardId, $labelId); $this->changeHelper->cardChanged($cardId); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_LABEL_ASSIGN, ['label' => $label]); @@ -640,6 +644,8 @@ class CardService { $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); + $this->permissionService->checkPermission($this->labelMapper, $labelId, Acl::PERMISSION_READ); + if ($this->boardService->isArchived($this->cardMapper, $cardId)) { throw new StatusException('Operation not allowed. This board is archived.'); } @@ -648,6 +654,9 @@ class CardService { throw new StatusException('Operation not allowed. This card is archived.'); } $label = $this->labelMapper->find($labelId); + if ($label->getBoardId() !== $this->cardMapper->findBoardId($card->getId())) { + throw new StatusException('Operation not allowed. Label does not exist.'); + } $this->cardMapper->removeLabel($cardId, $labelId); $this->changeHelper->cardChanged($cardId); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_CARD, $card, ActivityManager::SUBJECT_LABEL_UNASSING, ['label' => $label]); From 438a149304cb5b5eec7796d27806c6815e4ab46a Mon Sep 17 00:00:00 2001 From: Julius Knorr Date: Mon, 20 Jan 2025 15:47:44 +0100 Subject: [PATCH 2/3] chore: Add migration step for wrong label mapping Signed-off-by: Julius Knorr --- appinfo/info.xml | 3 + lib/Migration/LabelMismatchCleanup.php | 78 ++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 lib/Migration/LabelMismatchCleanup.php diff --git a/appinfo/info.xml b/appinfo/info.xml index eee5a9ad7..49da0f238 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -54,6 +54,9 @@ OCA\Deck\Migration\DeletedCircleCleanup + + OCA\Deck\Migration\LabelMismatchCleanup + OCA\Deck\Command\UserExport diff --git a/lib/Migration/LabelMismatchCleanup.php b/lib/Migration/LabelMismatchCleanup.php new file mode 100644 index 000000000..bc1da01e8 --- /dev/null +++ b/lib/Migration/LabelMismatchCleanup.php @@ -0,0 +1,78 @@ +db->getQueryBuilder(); + $qb->select('al.id', 'al.label_id', 'al.card_id', 's.board_id as actual_board_id', 'l.board_id as wrong_id', 'l.color', 'l.title') + ->from('deck_assigned_labels', 'al') + ->innerJoin('al', 'deck_cards', 'c', 'c.id = al.card_id') + ->innerJoin('c', 'deck_stacks', 's', 'c.stack_id = s.id') + ->innerJoin('al', 'deck_labels', 'l', 'l.id = al.label_id') + ->where($qb->expr()->neq('l.board_id', 's.board_id')); + + $labels = $qb->executeQUery()->fetchAll(); + if (count($labels) === 0) { + return; + } + + $output->info('Found ' . count($labels) . ' labels with wrong board mapping'); + + foreach ($labels as $label) { + // Select existing label on the correct board + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from('deck_labels') + ->where($qb->expr()->eq('title', $qb->createNamedParameter($label['title']))) + ->andWhere($qb->expr()->eq('color', $qb->createNamedParameter($label['color']))) + ->andWhere($qb->expr()->eq('board_id', $qb->createNamedParameter($label['actual_board_id']))); + $result = $qb->executeQuery(); + $newLabel = $result->fetchOne(); + $result->closeCursor(); + + if (!$newLabel) { + // Create a new label with the same title and color on the correct board + $qb = $this->db->getQueryBuilder(); + $qb->insert('deck_labels') + ->values([ + 'title' => $qb->createNamedParameter($label['title']), + 'color' => $qb->createNamedParameter($label['color']), + 'board_id' => $qb->createNamedParameter($label['actual_board_id']), + ]); + $qb->executeStatement(); + $newLabel = $qb->getLastInsertId(); + $output->debug('Created new label ' . $label['title'] . ' on board ' . $label['actual_board_id']); + } else { + $output->debug('Found existing label ' . $label['title'] . ' on board ' . $label['actual_board_id']); + } + + // Update the assignment to use the new label + $qb = $this->db->getQueryBuilder(); + $qb->update('deck_assigned_labels') + ->set('label_id', $qb->createNamedParameter($newLabel)) + ->where($qb->expr()->eq('id', $qb->createNamedParameter($label['id']))); + $qb->executeStatement(); + $output->debug('Updated label assignment ' . $label['id'] . ' to use label ' . $newLabel); + } + } +} From 60b34d190a245007514513113fb5548a5e29b305 Mon Sep 17 00:00:00 2001 From: Julius Knorr Date: Tue, 22 Apr 2025 16:26:04 +0200 Subject: [PATCH 3/3] tests: Fix unit test mocking around label checks Signed-off-by: Julius Knorr --- tests/unit/Service/CardServiceTest.php | 29 ++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/unit/Service/CardServiceTest.php b/tests/unit/Service/CardServiceTest.php index f7e8b3a99..73e29093f 100644 --- a/tests/unit/Service/CardServiceTest.php +++ b/tests/unit/Service/CardServiceTest.php @@ -258,6 +258,17 @@ class CardServiceTest extends TestCase { ->method('find') ->willReturn($card, $clonedCard); + $this->cardMapper->expects($this->any()) + ->method('findBoardId') + ->willReturn(1234); + + $this->labelMapper->expects($this->any()) + ->method('find') + ->willReturn(Label::fromRow([ + 'id' => 1, + 'boardId' => 1234, + ])); + // check if users are assigned $this->assignmentService->expects($this->once()) ->method('assignUser') @@ -433,8 +444,17 @@ class CardServiceTest extends TestCase { public function testAssignLabel() { $card = new Card(); $card->setArchived(false); + $card->setId(123); + $label = new Label(); + $label->setBoardId(1); $this->cardMapper->expects($this->once())->method('find')->willReturn($card); $this->cardMapper->expects($this->once())->method('assignLabel'); + $this->cardMapper->expects($this->once()) + ->method('findBoardId') + ->willReturn(1); + $this->labelMapper->expects($this->once()) + ->method('find') + ->willReturn($label); $this->cardService->assignLabel(123, 999); } @@ -450,8 +470,17 @@ class CardServiceTest extends TestCase { public function testRemoveLabel() { $card = new Card(); $card->setArchived(false); + $card->setId(123); + $label = new Label(); + $label->setBoardId(1); $this->cardMapper->expects($this->once())->method('find')->willReturn($card); $this->cardMapper->expects($this->once())->method('removeLabel'); + $this->cardMapper->expects($this->once()) + ->method('findBoardId') + ->willReturn(1); + $this->labelMapper->expects($this->once()) + ->method('find') + ->willReturn($label); $this->cardService->removeLabel(123, 999); }