From 22a3efe445a0851a8b3356a73ad32fb4cc8f4a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 4 Jan 2024 00:30:41 +0100 Subject: [PATCH 01/11] tests: Add integration tests for deleted boards/cards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- tests/integration/features/acl.feature | 50 ++++++++++ .../features/bootstrap/AttachmentContext.php | 10 ++ .../features/bootstrap/BoardContext.php | 35 ++++++- .../features/bootstrap/CommentContext.php | 31 ++++++ .../features/bootstrap/ServerContext.php | 8 +- tests/integration/features/decks.feature | 98 +++++++++++++++++++ 6 files changed, 226 insertions(+), 6 deletions(-) diff --git a/tests/integration/features/acl.feature b/tests/integration/features/acl.feature index 2937a377f..1a9677fc1 100644 --- a/tests/integration/features/acl.feature +++ b/tests/integration/features/acl.feature @@ -90,3 +90,53 @@ Feature: acl And the current user should not have "edit" permissions on the board And the current user should have "share" permissions on the board And the current user should not have "manage" permissions on the board + + Scenario: Share a board multiple times + Given Logging in using web as "user0" + And creates a board named "Double shared board" with color "ff0000" + And shares the board with user "user1" + And shares the board with group "group1" + And creates a board named "Single shared board" with color "00ff00" + And shares the board with user "user1" + When Logging in using web as "user1" + And fetching the board list + Then the response should have a status code "200" + And the response should be a list of objects + And the response should contain an element with the properties + | property | value | + | title | Double shared board | + + + Scenario: Deleted board is inaccessible to share recipients + Given acting as user "user0" + When creates a board with example content + And remember the last card as "user0-card" + When post a comment with content "hello comment" on the card + And uploads an attachment to the last used card + And remember the last attachment as "user0-attachment" + And shares the board with user "user1" + Then the HTTP status code should be "200" + And delete the board + + Given acting as user "user1" + When fetching the attachments for the card "user0-card" + Then the response should have a status code 403 + + When get the comments on the card + Then the response should have a status code 403 + + When update a comment with content "hello deleted" on the card + Then the response should have a status code 403 + + When delete the comment on the card + Then the response should have a status code 403 + # 644 + When post a comment with content "hello deleted" on the card + Then the response should have a status code 403 + + When get the card details + Then the response should have a status code 403 + When fetching the attachment "user0-attachment" for the card "user0-card" + Then the response should have a status code 403 + When deleting the attachment "user0-attachment" for the card "user0-card" + Then the response should have a status code 403 diff --git a/tests/integration/features/bootstrap/AttachmentContext.php b/tests/integration/features/bootstrap/AttachmentContext.php index 051789aec..84ef35048 100644 --- a/tests/integration/features/bootstrap/AttachmentContext.php +++ b/tests/integration/features/bootstrap/AttachmentContext.php @@ -87,4 +87,14 @@ class AttachmentContext implements Context { $this->requestContext->sendPlainRequest('GET', '/index.php/apps/deck/cards/' . $cardId . '/attachment/file:' . $attachmentId); } + + /** + * @When fetching the attachments for the card :cardReference + */ + public function fetchingTheAttachmentsForTheCard($cardReference) { + $cardId = $this->boardContext->getRememberedCard($cardReference)['id'] ?? null; + Assert::assertNotNull($cardId, 'Card needs to be available'); + + $this->requestContext->sendPlainRequest('GET', '/index.php/apps/deck/cards/' . $cardId . '/attachments'); + } } diff --git a/tests/integration/features/bootstrap/BoardContext.php b/tests/integration/features/bootstrap/BoardContext.php index 420a7d410..fdd985d27 100644 --- a/tests/integration/features/bootstrap/BoardContext.php +++ b/tests/integration/features/bootstrap/BoardContext.php @@ -186,7 +186,9 @@ class BoardContext implements Context { ['description' => $description] )); $this->requestContext->getResponse()->getBody()->seek(0); - $this->card = json_decode((string)$this->getResponse()->getBody(), true); + if ($this->requestContext->getResponse()->getStatusCode() === 200) { + $this->card = json_decode((string)$this->getResponse()->getBody(), true); + } } /** @@ -198,7 +200,22 @@ class BoardContext implements Context { [$attribute => $value] )); $this->requestContext->getResponse()->getBody()->seek(0); - $this->card = json_decode((string)$this->getResponse()->getBody(), true); + if ($this->requestContext->getResponse()->getStatusCode() === 200) { + $this->card = json_decode((string)$this->getResponse()->getBody(), true); + } + } + + /** + * @Given /^get the card details$/ + */ + public function getCard() { + $this->requestContext->sendJSONrequest('GET', '/index.php/apps/deck/cards/' . $this->card['id'], array_merge( + $this->card + )); + $this->requestContext->getResponse()->getBody()->seek(0); + if ($this->requestContext->getResponse()->getStatusCode() === 200) { + $this->card = json_decode((string)$this->getResponse()->getBody(), true); + } } /** @@ -253,4 +270,18 @@ class BoardContext implements Context { public function getRememberedCard($arg1) { return $this->storedCards[$arg1] ?? null; } + + /** + * @Given /^delete the card$/ + */ + public function deleteTheCard() { + $this->requestContext->sendJSONrequest('DELETE', '/index.php/apps/deck/cards/' . $this->card['id']); + } + + /** + * @Given /^delete the board/ + */ + public function deleteTheBoard() { + $this->requestContext->sendJSONrequest('DELETE', '/index.php/apps/deck/boards/' . $this->board['id']); + } } diff --git a/tests/integration/features/bootstrap/CommentContext.php b/tests/integration/features/bootstrap/CommentContext.php index 40b6f4e45..92bc9a347 100644 --- a/tests/integration/features/bootstrap/CommentContext.php +++ b/tests/integration/features/bootstrap/CommentContext.php @@ -11,6 +11,8 @@ class CommentContext implements Context { /** @var BoardContext */ protected $boardContext; + private $lastComment = null; + /** @BeforeScenario */ public function gatherContexts(BeforeScenarioScope $scope) { $environment = $scope->getEnvironment(); @@ -27,5 +29,34 @@ class CommentContext implements Context { 'message' => $content, 'parentId' => null ]); + $this->lastComment = $this->requestContext->getResponseBodyFromJson()['ocs']['data'] ?? null; } + + /** + * @Given /^get the comments on the card$/ + */ + public function getCommentsOnTheCard() { + $card = $this->boardContext->getLastUsedCard(); + $this->requestContext->sendOCSRequest('GET', '/apps/deck/api/v1.0/cards/' . $card['id'] . '/comments'); + } + + /** + * @When /^update a comment with content "([^"]*)" on the card$/ + */ + public function updateACommentWithContentOnTheCard($content) { + $card = $this->boardContext->getLastUsedCard(); + $this->requestContext->sendOCSRequest('PUT', '/apps/deck/api/v1.0/cards/' . $card['id'] . '/comments/'. $this->lastComment['id'], [ + 'message' => $content, + 'parentId' => null + ]); + } + + /** + * @When /^delete the comment on the card$/ + */ + public function deleteTheCommentOnTheCard() { + $card = $this->boardContext->getLastUsedCard(); + $this->requestContext->sendOCSRequest('DELETE', '/apps/deck/api/v1.0/cards/' . $card['id'] . '/comments/'. $this->lastComment['id']); + } + } diff --git a/tests/integration/features/bootstrap/ServerContext.php b/tests/integration/features/bootstrap/ServerContext.php index 87b068ec8..6ed36f320 100644 --- a/tests/integration/features/bootstrap/ServerContext.php +++ b/tests/integration/features/bootstrap/ServerContext.php @@ -10,15 +10,15 @@ class ServerContext implements Context { WebDav::__construct as private __tConstruct; } + private string $rawBaseUrl; + private string $mappedUserId; + private array $lastInsertIds = []; + public function __construct($baseUrl) { $this->rawBaseUrl = $baseUrl; $this->__tConstruct($baseUrl . '/index.php/ocs/', ['admin', 'admin'], '123456'); } - /** @var string */ - private $mappedUserId; - - private $lastInsertIds = []; /** * @BeforeSuite diff --git a/tests/integration/features/decks.feature b/tests/integration/features/decks.feature index 22a74baec..3582af430 100644 --- a/tests/integration/features/decks.feature +++ b/tests/integration/features/decks.feature @@ -32,3 +32,101 @@ Feature: decks And creates a board named "MyBoard" with color "000000" And create a stack named "ToDo" When create a card named "This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters" + + Scenario: Setting a duedate on a card + Given acting as user "user0" + And creates a board named "MyBoard" with color "000000" + And create a stack named "ToDo" + And create a card named "Overdue task" + When get the card details + And the response should be a JSON array with the following mandatory values + |key|value| + |title|Overdue task| + |duedate|| + |overdue|0| + And set the card attribute "duedate" to "2020-12-12 13:37:00" + When get the card details + And the response should be a JSON array with the following mandatory values + |key|value| + |title|Overdue task| + |duedate|2020-12-12T13:37:00+00:00| + |overdue|3| + And set the card attribute "duedate" to "" + When get the card details + And the response should be a JSON array with the following mandatory values + |key|value| + |title|Overdue task| + |duedate|| + |overdue|0| + + Scenario: Cannot access card on a deleted board + Given acting as user "user0" + And creates a board named "MyBoard" with color "000000" + And create a stack named "ToDo" + And create a card named "Overdue task" + And remember the last card as "deletedCard" + And uploads an attachment to the last used card + And remember the last attachment as "my-attachment" + And post a comment with content "My first comment" on the card + And delete the board + + When fetching the attachment "my-attachment" for the card "deletedCard" + Then the response should have a status code 403 + + When get the comments on the card + Then the response should have a status code 403 + + When post a comment with content "My second comment" on the card + Then the response should have a status code 403 + + When uploads an attachment to the last used card + Then the response should have a status code 403 + + When set the description to "Update some text" + Then the response should have a status code 403 + + When get the card details + Then the response should have a status code 403 + + When create a card named "Overdue task" + Then the response should have a status code 403 + + When create a stack named "ToDo" + Then the response should have a status code 403 + + Scenario: Cannot access card on a deleted card + Given acting as user "user0" + And creates a board named "MyBoard" with color "000000" + And create a stack named "ToDo" + And create a card named "Overdue task" + And remember the last card as "deletedCard" + And uploads an attachment to the last used card + And remember the last attachment as "my-attachment" + And post a comment with content "My first comment" on the card + And delete the card + + When fetching the attachment "my-attachment" for the card "deletedCard" + Then the response should have a status code 403 + + When get the comments on the card + Then the response should have a status code 403 + + When post a comment with content "My second comment" on the card + Then the response should have a status code 403 + + When deleting the attachment "my-attachment" for the card "deletedCard" + Then the response should have a status code 403 + + When uploads an attachment to the last used card + Then the response should have a status code 403 + + When get the card details + Then the response should have a status code 403 + + # We currently still expect to be able to update the card as this is used to undo deletion + When set the description to "Update some text" + Then the response should have a status code 403 + #When set the card attribute "deletedAt" to "0" + #Then the response should have a status code 200 + #When set the description to "Update some text" + #Then the response should have a status code 200 From 71948d670ea32a2f43e85d4a99fd6f685fd31907 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 3 Jan 2024 15:32:40 +0100 Subject: [PATCH 02/11] fix: Consider a deleted board inaccessible to share recipients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only the owner can delete/undo a board deletion so there is no reason other users should have any permission on a board marked as deleted Signed-off-by: Julius Härtl --- lib/Service/PermissionService.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index c42ca8045..62a61dc6a 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -102,8 +102,9 @@ class PermissionService { return $cached; } - $owner = $this->userIsBoardOwner($boardId); - $acls = $this->aclMapper->findAll($boardId); + $board = $this->getBoard($boardId); + $owner = $this->userIsBoardOwner($boardId, $userId); + $acls = $board->getDeletedAt() === 0 ? $this->aclMapper->findAll($boardId) : []; $permissions = [ Acl::PERMISSION_READ => $owner || $this->userCan($acls, Acl::PERMISSION_READ), Acl::PERMISSION_EDIT => $owner || $this->userCan($acls, Acl::PERMISSION_EDIT), From aef06d833de56760b6b8b6a643b82e6127e4dbf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 4 Jan 2024 10:53:11 +0100 Subject: [PATCH 03/11] fix: limit to non-deleted cards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Service/BoardService.php | 2 +- lib/Service/CommentService.php | 9 +++------ lib/Service/PermissionService.php | 19 +++++++++++++------ lib/Sharing/ShareAPIHelper.php | 2 +- 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index f1cf2b55f..09130d7cf 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -471,7 +471,7 @@ class BoardService { $newAcl = $this->aclMapper->insert($acl); $this->activityManager->triggerEvent(ActivityManager::DECK_OBJECT_BOARD, $newAcl, ActivityManager::SUBJECT_BOARD_SHARE, [], $this->userId); - $this->notificationHelper->sendBoardShared((int)$boardId, $acl); + $this->notificationHelper->sendBoardShared($boardId, $acl); $this->boardMapper->mapAcl($newAcl); $this->changeHelper->boardChanged($boardId); diff --git a/lib/Service/CommentService.php b/lib/Service/CommentService.php index c389a9ad2..d15c7cc88 100644 --- a/lib/Service/CommentService.php +++ b/lib/Service/CommentService.php @@ -90,17 +90,14 @@ class CommentService { * @throws BadRequestException * @throws NotFoundException|NoPermissionException */ - public function create(string $cardId, string $message, string $replyTo = '0'): DataResponse { - if (!is_numeric($cardId)) { - throw new BadRequestException('A valid card id must be provided'); - } + public function create(int $cardId, string $message, string $replyTo = '0'): DataResponse { $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_READ); // Check if parent is a comment on the same card if ($replyTo !== '0') { try { $comment = $this->commentsManager->get($replyTo); - if ($comment->getObjectType() !== Application::COMMENT_ENTITY_TYPE || $comment->getObjectId() !== $cardId) { + if ($comment->getObjectType() !== Application::COMMENT_ENTITY_TYPE || (int)$comment->getObjectId() !== $cardId) { throw new CommentNotFoundException(); } } catch (CommentNotFoundException $e) { @@ -109,7 +106,7 @@ class CommentService { } try { - $comment = $this->commentsManager->create('users', $this->userId, Application::COMMENT_ENTITY_TYPE, $cardId); + $comment = $this->commentsManager->create('users', $this->userId, Application::COMMENT_ENTITY_TYPE, (string)$cardId); $comment->setMessage($message); $comment->setVerb('comment'); $comment->setParentId($replyTo); diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 62a61dc6a..5b0f3ccdd 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -29,6 +29,7 @@ use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; +use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\IPermissionMapper; use OCA\Deck\Db\User; use OCA\Deck\NoPermissionException; @@ -138,13 +139,10 @@ class PermissionService { /** * check permissions for replacing dark magic middleware * - * @param $mapper IPermissionMapper|null null if $id is a boardId - * @param $id int unique identifier of the Entity - * @param $permission int - * @return bool + * @param numeric $id * @throws NoPermissionException */ - public function checkPermission($mapper, $id, $permission, $userId = null) { + public function checkPermission($mapper, $id, $permission, $userId = null, bool $allowDeletedCard = false) { $boardId = $id; if ($mapper instanceof IPermissionMapper && !($mapper instanceof BoardMapper)) { $boardId = $mapper->findBoardId($id); @@ -158,7 +156,16 @@ class PermissionService { throw new NoPermissionException('Permission denied'); } - if ($this->userIsBoardOwner($boardId, $userId)) { + $permissions = $this->getPermissions($boardId, $userId); + if ($permissions[$permission] === true) { + + if (!$allowDeletedCard && $mapper instanceof CardMapper) { + $card = $mapper->find($id); + if ($card->getDeletedAt() > 0) { + throw new NoPermissionException('Card is deleted'); + } + } + return true; } diff --git a/lib/Sharing/ShareAPIHelper.php b/lib/Sharing/ShareAPIHelper.php index 5528b6a92..41a5dfc6d 100644 --- a/lib/Sharing/ShareAPIHelper.php +++ b/lib/Sharing/ShareAPIHelper.php @@ -115,7 +115,7 @@ class ShareAPIHelper { */ public function canAccessShare(IShare $share, string $user): bool { try { - $this->permissionService->checkPermission($this->cardMapper, $share->getSharedWith(), Acl::PERMISSION_READ, $user); + $this->permissionService->checkPermission($this->cardMapper, (int)$share->getSharedWith(), Acl::PERMISSION_READ, $user); } catch (NoPermissionException $e) { return false; } From 272da5406a7c7750aa421ac469092a2ae32f7d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 4 Jan 2024 14:01:24 +0100 Subject: [PATCH 04/11] fix: Further limit updating cards 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 | 8 ++++---- tests/integration/features/bootstrap/BoardContext.php | 1 + tests/integration/features/decks.feature | 8 ++++---- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index 717b0b15d..dc74a2fb2 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -1,4 +1,4 @@ - * @@ -264,7 +264,7 @@ class CardService { public function update($id, $title, $stackId, $type, $owner, $description = '', $order = 0, $duedate = null, $deletedAt = null, $archived = null) { $this->cardServiceValidator->check(compact('id', 'title', 'stackId', 'type', 'owner', 'order')); - $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT); + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT, allowDeletedCard: true); $this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_EDIT); if ($this->boardService->isArchived($this->cardMapper, $id)) { @@ -276,9 +276,9 @@ class CardService { } if ($card->getDeletedAt() !== 0) { - if ($deletedAt === null) { + if ($deletedAt === null || $deletedAt > 0) { // Only allow operations when restoring the card - throw new StatusException('Operation not allowed. This card was deleted.'); + throw new NoPermissionException('Operation not allowed. This card was deleted.'); } } diff --git a/tests/integration/features/bootstrap/BoardContext.php b/tests/integration/features/bootstrap/BoardContext.php index fdd985d27..20b6188c6 100644 --- a/tests/integration/features/bootstrap/BoardContext.php +++ b/tests/integration/features/bootstrap/BoardContext.php @@ -276,6 +276,7 @@ class BoardContext implements Context { */ public function deleteTheCard() { $this->requestContext->sendJSONrequest('DELETE', '/index.php/apps/deck/cards/' . $this->card['id']); + $this->card['deletedAt'] = time(); } /** diff --git a/tests/integration/features/decks.feature b/tests/integration/features/decks.feature index 3582af430..9656c2a23 100644 --- a/tests/integration/features/decks.feature +++ b/tests/integration/features/decks.feature @@ -126,7 +126,7 @@ Feature: decks # We currently still expect to be able to update the card as this is used to undo deletion When set the description to "Update some text" Then the response should have a status code 403 - #When set the card attribute "deletedAt" to "0" - #Then the response should have a status code 200 - #When set the description to "Update some text" - #Then the response should have a status code 200 + When set the card attribute "deletedAt" to "0" + Then the response should have a status code 200 + When set the description to "Update some text" + Then the response should have a status code 200 From 0954d4d8a02f5e2a3e9504c7b434c9e16898e949 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 4 Jan 2024 14:01:49 +0100 Subject: [PATCH 05/11] fix: Limit card activities for deleted cards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Activity/ActivityManager.php | 21 +++++++++++++++++ lib/Activity/DeckProvider.php | 6 +++++ .../features/bootstrap/BoardContext.php | 23 +++++++++++++++++-- tests/integration/features/decks.feature | 5 ++++ tests/unit/Activity/DeckProviderTest.php | 3 +++ 5 files changed, 56 insertions(+), 2 deletions(-) diff --git a/lib/Activity/ActivityManager.php b/lib/Activity/ActivityManager.php index baedf109d..a433e4fbb 100644 --- a/lib/Activity/ActivityManager.php +++ b/lib/Activity/ActivityManager.php @@ -38,6 +38,7 @@ use OCA\Deck\Db\CardMapper; use OCA\Deck\Db\Label; use OCA\Deck\Db\Stack; use OCA\Deck\Db\StackMapper; +use OCA\Deck\NoPermissionException; use OCA\Deck\Service\PermissionService; use OCP\Activity\IEvent; use OCP\Activity\IManager; @@ -549,4 +550,24 @@ class ActivityManager { 'board' => $board ]; } + + public function canSeeCardActivity(int $cardId): bool { + try { + $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_READ); + $card = $this->cardMapper->find($cardId); + return $card->getDeletedAt() === 0; + } catch (NoPermissionException $e) { + return false; + } + } + + public function canSeeBoardActivity(int $boardId): bool { + try { + $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ); + $board = $this->boardMapper->find($boardId); + return $board->getDeletedAt() === 0; + } catch (NoPermissionException $e) { + return false; + } + } } diff --git a/lib/Activity/DeckProvider.php b/lib/Activity/DeckProvider.php index 4235816e5..7779f5757 100644 --- a/lib/Activity/DeckProvider.php +++ b/lib/Activity/DeckProvider.php @@ -111,6 +111,9 @@ class DeckProvider implements IProvider { $event->setAuthor($author); } if ($event->getObjectType() === ActivityManager::DECK_OBJECT_BOARD) { + if (!$this->activityManager->canSeeBoardActivity($event->getObjectId())) { + throw new \InvalidArgumentException(); + } if (isset($subjectParams['board']) && $event->getObjectName() === '') { $event->setObject($event->getObjectType(), $event->getObjectId(), $subjectParams['board']['title']); } @@ -125,6 +128,9 @@ class DeckProvider implements IProvider { } if (isset($subjectParams['card']) && $event->getObjectType() === ActivityManager::DECK_OBJECT_CARD) { + if (!$this->activityManager->canSeeCardActivity($event->getObjectId())) { + throw new \InvalidArgumentException(); + } if ($event->getObjectName() === '') { $event->setObject($event->getObjectType(), $event->getObjectId(), $subjectParams['card']['title']); } diff --git a/tests/integration/features/bootstrap/BoardContext.php b/tests/integration/features/bootstrap/BoardContext.php index 20b6188c6..b6fb28d16 100644 --- a/tests/integration/features/bootstrap/BoardContext.php +++ b/tests/integration/features/bootstrap/BoardContext.php @@ -17,9 +17,9 @@ class BoardContext implements Context { /** @var array last card response */ private $card = null; private array $storedCards = []; + private ?array $activities = null; - /** @var ServerContext */ - private $serverContext; + private ServerContext $serverContext; /** @BeforeScenario */ public function gatherContexts(BeforeScenarioScope $scope) { @@ -285,4 +285,23 @@ class BoardContext implements Context { public function deleteTheBoard() { $this->requestContext->sendJSONrequest('DELETE', '/index.php/apps/deck/boards/' . $this->board['id']); } + + + /** + * @Given /^get the activities for the last card$/ + */ + public function getActivitiesForTheLastCard() { + $card = $this->getLastUsedCard(); + $this->requestContext->sendOCSRequest('GET', '/apps/activity/api/v2/activity/filter?format=json&type=deck&since=0&object_type=deck_card&object_id=' . $card['id'] . '&limit=50'); + $this->activities = json_decode((string)$this->getResponse()->getBody(), true)['ocs']['data'] ?? null; + } + + /** + * @Then the fetched activities should have :count entries + */ + public function theFetchedActivitiesShouldHaveEntries($count) { + Assert::assertEquals($count, count($this->activities ?? [])); + } + + } diff --git a/tests/integration/features/decks.feature b/tests/integration/features/decks.feature index 9656c2a23..0351f6269 100644 --- a/tests/integration/features/decks.feature +++ b/tests/integration/features/decks.feature @@ -103,8 +103,13 @@ Feature: decks And uploads an attachment to the last used card And remember the last attachment as "my-attachment" And post a comment with content "My first comment" on the card + When get the activities for the last card + Then the fetched activities should have 3 entries And delete the card + When get the activities for the last card + Then the fetched activities should have 0 entries + When fetching the attachment "my-attachment" for the card "deletedCard" Then the response should have a status code 403 diff --git a/tests/unit/Activity/DeckProviderTest.php b/tests/unit/Activity/DeckProviderTest.php index 4302834cb..13cdf691e 100644 --- a/tests/unit/Activity/DeckProviderTest.php +++ b/tests/unit/Activity/DeckProviderTest.php @@ -74,6 +74,9 @@ class DeckProviderTest extends TestCase { $this->config = $this->createMock(IConfig::class); $this->cardService = $this->createMock(CardService::class); $this->provider = new DeckProvider($this->urlGenerator, $this->activityManager, $this->userManager, $this->commentsManager, $this->l10nFactory, $this->config, $this->userId, $this->cardService); + + $this->activityManager->method('canSeeCardActivity')->willReturn(true); + $this->activityManager->method('canSeeBoardActivity')->willReturn(true); } private function mockEvent($objectType, $objectId, $objectName, $subject, $subjectParameters = []) { From 0c0bb4051507396f4a1cb0f7cc5dc0fed7bb6cd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 4 Jan 2024 16:01:01 +0100 Subject: [PATCH 06/11] chore: Fix ci setup for activity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .github/workflows/integration.yml | 7 +++++++ composer.json | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index fe62ad337..909473d22 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -61,6 +61,13 @@ jobs: with: path: apps/${{ env.APP_NAME }} + - name: Checkout activity + uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 + with: + repository: nextcloud/activity + ref: ${{ matrix.server-versions }} + path: apps/activity + - name: Set up php ${{ matrix.php-versions }} uses: shivammathur/setup-php@2.18.0 with: diff --git a/composer.json b/composer.json index 38e2fa46d..a84c8a7d1 100644 --- a/composer.json +++ b/composer.json @@ -42,7 +42,8 @@ "@test:integration" ], "test:unit": "phpunit -c tests/phpunit.xml", - "test:integration": "phpunit -c tests/phpunit.integration.xml && cd tests/integration && ./run.sh" + "test:integration": "phpunit -c tests/phpunit.integration.xml", + "test:api": "cd tests/integration && ./run.sh" }, "autoload-dev": { "psr-4": { From 9119017ce8ff954e9785a91445c537475317e592 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 9 Jan 2024 17:52:41 +0100 Subject: [PATCH 07/11] tests: Fix missing behat context methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Service/PermissionService.php | 1 - .../features/bootstrap/BoardContext.php | 2 -- .../features/bootstrap/CommentContext.php | 1 - .../features/bootstrap/RequestContext.php | 25 +++++++++++++++++++ 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 5b0f3ccdd..48d6af96d 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -158,7 +158,6 @@ class PermissionService { $permissions = $this->getPermissions($boardId, $userId); if ($permissions[$permission] === true) { - if (!$allowDeletedCard && $mapper instanceof CardMapper) { $card = $mapper->find($id); if ($card->getDeletedAt() > 0) { diff --git a/tests/integration/features/bootstrap/BoardContext.php b/tests/integration/features/bootstrap/BoardContext.php index b6fb28d16..2a7f2bc7c 100644 --- a/tests/integration/features/bootstrap/BoardContext.php +++ b/tests/integration/features/bootstrap/BoardContext.php @@ -302,6 +302,4 @@ class BoardContext implements Context { public function theFetchedActivitiesShouldHaveEntries($count) { Assert::assertEquals($count, count($this->activities ?? [])); } - - } diff --git a/tests/integration/features/bootstrap/CommentContext.php b/tests/integration/features/bootstrap/CommentContext.php index 92bc9a347..aba5a6065 100644 --- a/tests/integration/features/bootstrap/CommentContext.php +++ b/tests/integration/features/bootstrap/CommentContext.php @@ -58,5 +58,4 @@ class CommentContext implements Context { $card = $this->boardContext->getLastUsedCard(); $this->requestContext->sendOCSRequest('DELETE', '/apps/deck/api/v1.0/cards/' . $card['id'] . '/comments/'. $this->lastComment['id']); } - } diff --git a/tests/integration/features/bootstrap/RequestContext.php b/tests/integration/features/bootstrap/RequestContext.php index 9df6be205..ebf01e80a 100644 --- a/tests/integration/features/bootstrap/RequestContext.php +++ b/tests/integration/features/bootstrap/RequestContext.php @@ -166,4 +166,29 @@ class RequestContext implements Context { $this->getResponse()->getBody()->seek(0); return json_decode((string)$this->getResponse()->getBody(), true); } + + /** + * @Given /^the response should be a list of objects$/ + */ + public function theResponseShouldBeAListOfObjects() { + $jsonResponse = $this->getResponseBodyFromJson(); + Assert::assertEquals(array_keys($jsonResponse), range(0, count($jsonResponse) - 1)); + } + + /** + * @When /^the response should contain an element with the properties$/ + */ + public function responseContainsElement(TableNode $element) { + $json = $this->getResponseBodyFromJson(); + $found = array_filter($json, function ($board) use ($element) { + foreach ($element as $row) { + if ($row['value'] !== $board[$row['property']]) { + return false; + } + } + + return true; + }); + Assert::assertEquals(1, count($found)); + } } From 1f71d1ab781c6286fa0cc29b2d2e2a87798c1325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 9 Jan 2024 18:08:53 +0100 Subject: [PATCH 08/11] fix: PHP 7.4 compatibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .github/workflows/integration.yml | 11 ++++++----- lib/Service/CardService.php | 4 ++-- lib/Service/CommentService.php | 6 +----- lib/Service/PermissionService.php | 10 +++++++--- 4 files changed, 16 insertions(+), 15 deletions(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 909473d22..508ed1481 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -69,16 +69,17 @@ jobs: path: apps/activity - name: Set up php ${{ matrix.php-versions }} - uses: shivammathur/setup-php@2.18.0 + uses: shivammathur/setup-php@2.25.4 with: php-version: ${{ matrix.php-versions }} - tools: phpunit - extensions: mbstring, iconv, fileinfo, intl, sqlite, pdo_sqlite, mysql, pdo_mysql, pgsql, pdo_pgsql, + extensions: bz2, ctype, curl, dom, fileinfo, gd, iconv, intl, json, libxml, mbstring, openssl, pcntl, posix, session, simplexml, xmlreader, xmlwriter, zip, zlib, sqlite, pdo_sqlite, mysql, pdo_mysql, pgsql, pdo_pgsql, apcu + ini-values: + apc.enable_cli=on coverage: none - - name: Set up PHPUnit + - name: Set up dependencies working-directory: apps/${{ env.APP_NAME }} - run: composer i + run: composer i --no-dev - name: Set up Nextcloud run: | diff --git a/lib/Service/CardService.php b/lib/Service/CardService.php index dc74a2fb2..62a9b55ab 100644 --- a/lib/Service/CardService.php +++ b/lib/Service/CardService.php @@ -1,4 +1,4 @@ -lib/Service/CardService.php * @@ -264,7 +264,7 @@ class CardService { public function update($id, $title, $stackId, $type, $owner, $description = '', $order = 0, $duedate = null, $deletedAt = null, $archived = null) { $this->cardServiceValidator->check(compact('id', 'title', 'stackId', 'type', 'owner', 'order')); - $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT, allowDeletedCard: true); + $this->permissionService->checkPermission($this->cardMapper, $id, Acl::PERMISSION_EDIT, null, true); $this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_EDIT); if ($this->boardService->isArchived($this->cardMapper, $id)) { diff --git a/lib/Service/CommentService.php b/lib/Service/CommentService.php index d15c7cc88..5f47e0893 100644 --- a/lib/Service/CommentService.php +++ b/lib/Service/CommentService.php @@ -83,10 +83,6 @@ class CommentService { } /** - * @param string $cardId - * @param string $message - * @param string $replyTo - * @return DataResponse * @throws BadRequestException * @throws NotFoundException|NoPermissionException */ @@ -142,7 +138,7 @@ class CommentService { throw new NoPermissionException('Only authors are allowed to edit their comment.'); } if ($comment->getParentId() !== '0') { - $this->permissionService->checkPermission($this->cardMapper, $comment->getParentId(), Acl::PERMISSION_READ); + $this->permissionService->checkPermission($this->cardMapper, (int)$comment->getParentId(), Acl::PERMISSION_READ); } $comment->setMessage($message); diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 48d6af96d..5fd657c37 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -98,7 +98,11 @@ class PermissionService { * @param $boardId * @return bool|array */ - public function getPermissions($boardId) { + public function getPermissions($boardId, ?string $userId = null) { + if ($userId === null) { + $userId = $this->userId; + } + if ($cached = $this->permissionCache->get($boardId)) { return $cached; } @@ -113,7 +117,7 @@ class PermissionService { Acl::PERMISSION_SHARE => ($owner || $this->userCan($acls, Acl::PERMISSION_SHARE)) && (!$this->shareManager->sharingDisabledForUser($this->userId)) ]; - $this->permissionCache->set($boardId, $permissions); + $this->permissionCache->set((string)$boardId, $permissions); return $permissions; } @@ -169,7 +173,7 @@ class PermissionService { } try { - $acls = $this->getBoard($boardId)->getAcl() ?? []; + $acls = $this->getBoard((int)$boardId)->getAcl() ?? []; $result = $this->userCan($acls, $permission, $userId); if ($result) { return true; From 52c5020ba49e8dde9dc9199871d33cff6a795eab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 9 Jan 2024 23:45:30 +0100 Subject: [PATCH 09/11] chore: update actions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .github/workflows/fixup.yml | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/.github/workflows/fixup.yml b/.github/workflows/fixup.yml index 6092cc3a5..9548d19f2 100644 --- a/.github/workflows/fixup.yml +++ b/.github/workflows/fixup.yml @@ -3,18 +3,31 @@ # https://github.com/nextcloud/.github # https://docs.github.com/en/actions/learn-github-actions/sharing-workflows-with-your-organization -name: Pull request checks +name: Block fixup and squash commits -on: pull_request +on: + pull_request: + types: [opened, ready_for_review, reopened, synchronize] + +permissions: + contents: read + +concurrency: + group: fixup-${{ github.head_ref || github.run_id }} + cancel-in-progress: true jobs: commit-message-check: + if: github.event.pull_request.draft == false + + permissions: + pull-requests: write name: Block fixup and squash commits runs-on: ubuntu-latest steps: - name: Run check - uses: xt0rted/block-autosquash-commits-action@v2 + uses: skjnldsv/block-fixup-merge-action@42d26e1b536ce61e5cf467d65fb76caf4aa85acf # v1 with: repo-token: ${{ secrets.GITHUB_TOKEN }} From f70b31c449a2aedfa9f12b3c873e508cba19536c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 9 Jan 2024 23:48:51 +0100 Subject: [PATCH 10/11] ci: Remove unused test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- tests/integration/features/decks.feature | 26 ------------------------ 1 file changed, 26 deletions(-) diff --git a/tests/integration/features/decks.feature b/tests/integration/features/decks.feature index 0351f6269..1913c8420 100644 --- a/tests/integration/features/decks.feature +++ b/tests/integration/features/decks.feature @@ -33,32 +33,6 @@ Feature: decks And create a stack named "ToDo" When create a card named "This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters This is a very ong name that exceeds the maximum length of a deck board created which is longer than 255 characters" - Scenario: Setting a duedate on a card - Given acting as user "user0" - And creates a board named "MyBoard" with color "000000" - And create a stack named "ToDo" - And create a card named "Overdue task" - When get the card details - And the response should be a JSON array with the following mandatory values - |key|value| - |title|Overdue task| - |duedate|| - |overdue|0| - And set the card attribute "duedate" to "2020-12-12 13:37:00" - When get the card details - And the response should be a JSON array with the following mandatory values - |key|value| - |title|Overdue task| - |duedate|2020-12-12T13:37:00+00:00| - |overdue|3| - And set the card attribute "duedate" to "" - When get the card details - And the response should be a JSON array with the following mandatory values - |key|value| - |title|Overdue task| - |duedate|| - |overdue|0| - Scenario: Cannot access card on a deleted board Given acting as user "user0" And creates a board named "MyBoard" with color "000000" From 86751c4b89defd29a8f9b76261545cf20ec233b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Thu, 11 Jan 2024 14:21:59 +0100 Subject: [PATCH 11/11] test: Adapt unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- .github/workflows/phpunit.yml | 2 +- lib/Service/PermissionService.php | 20 ++++++++++---------- tests/unit/Service/PermissionServiceTest.php | 4 ++++ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index a7a97f9ea..e1d317f8b 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -18,7 +18,7 @@ jobs: strategy: fail-fast: false matrix: - php-versions: ['7.4', '8.0', '8.1'] + php-versions: ['7.4', '8.0'] databases: ['sqlite', 'mysql', 'pgsql'] server-versions: ['stable24'] diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 5fd657c37..32c0ed078 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -160,19 +160,19 @@ class PermissionService { throw new NoPermissionException('Permission denied'); } - $permissions = $this->getPermissions($boardId, $userId); - if ($permissions[$permission] === true) { - if (!$allowDeletedCard && $mapper instanceof CardMapper) { - $card = $mapper->find($id); - if ($card->getDeletedAt() > 0) { - throw new NoPermissionException('Card is deleted'); + try { + $permissions = $this->getPermissions($boardId, $userId); + if ($permissions[$permission] === true) { + if (!$allowDeletedCard && $mapper instanceof CardMapper) { + $card = $mapper->find($id); + if ($card->getDeletedAt() > 0) { + throw new NoPermissionException('Card is deleted'); + } } + + return true; } - return true; - } - - try { $acls = $this->getBoard((int)$boardId)->getAcl() ?? []; $result = $this->userCan($acls, $permission, $userId); if ($result) { diff --git a/tests/unit/Service/PermissionServiceTest.php b/tests/unit/Service/PermissionServiceTest.php index 0994a778e..1bf00c5f3 100644 --- a/tests/unit/Service/PermissionServiceTest.php +++ b/tests/unit/Service/PermissionServiceTest.php @@ -240,6 +240,8 @@ class PermissionServiceTest extends \Test\TestCase { ->method('sharingDisabledForUser') ->willReturn(false); + $this->aclMapper->method('findAll')->willReturn([]); + if ($result) { $actual = $this->service->checkPermission($mapper, 1234, $permission); $this->assertTrue($actual); @@ -262,6 +264,8 @@ class PermissionServiceTest extends \Test\TestCase { $this->boardMapper->expects($this->any())->method('find')->willReturn($board); } + $this->aclMapper->method('findAll')->willReturn([]); + if ($result) { $actual = $this->service->checkPermission($mapper, 1234, $permission); $this->assertTrue($actual);