Merge pull request #5423 from nextcloud/bugfix/noid/comment-deleted

.github/workflows/integration.yml

@@ -70,6 +70,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.28.0
         with:

composer.json

@@ -39,7 +39,8 @@
 			"@test:integration"
 		],
 		"test:unit": "vendor/bin/phpunit -c tests/phpunit.xml",
-		"test:integration": "vendor/bin/phpunit -c tests/phpunit.integration.xml && cd tests/integration && ./run.sh"
+		"test:integration": "vendor/bin/phpunit -c tests/phpunit.integration.xml",
+		"test:api": "cd tests/integration && ./run.sh"
 	},
 	"autoload-dev": {
 		"psr-4": {

lib/Activity/ActivityManager.php

@@ -38,6 +38,7 @@
 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;
@@ -564,4 +565,24 @@ private function findDetailsForAcl($aclId) {
 			'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;
+		}
+	}
 }

lib/Activity/DeckProvider.php

@@ -111,6 +111,9 @@ public function parse($language, IEvent $event, IEvent $previousEvent = null): I
 			$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 @@ public function parse($language, IEvent $event, IEvent $previousEvent = null): I
 		}
 
 		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']);
 			}

lib/Db/Card.php

@@ -39,6 +39,8 @@
  * @method int getLastModified()
  * @method int getCreatedAt()
  * @method bool getArchived()
+ * @method int getDeletedAt()
+ * @method void setDeletedAt(int $deletedAt)
  * @method bool getNotified()
  * @method ?DateTime getDone()
  * @method void setDone(?DateTime $done)

lib/Service/BoardService.php

@@ -446,7 +446,7 @@ public function addAcl($boardId, $type, $participant, $edit, $share, $manage) {
 		$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);
 

lib/Service/CardService.php

@@ -298,7 +298,7 @@ public function delete($id) {
 	public function update($id, $title, $stackId, $type, $owner, $description = '', $order = 0, $duedate = null, $deletedAt = null, $archived = null, ?OptionalNullableValue $done = 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)) {
@@ -310,9 +310,9 @@ public function update($id, $title, $stackId, $type, $owner, $description = '',
 		}
 
 		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.');
 			}
 		}
 

lib/Service/CommentService.php

@@ -94,7 +94,7 @@ private function get(int $cardId, int $commentId): IComment {
 			throw new NotFoundException('No comment found.');
 		}
 		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);
 		}
 
 		return $comment;
@@ -113,24 +113,17 @@ public function getFormatted(int $cardId, int $commentId): array {
 	}
 
 	/**
-	 * @param string $cardId
-	 * @param string $message
-	 * @param string $replyTo
-	 * @return DataResponse
 	 * @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) {
@@ -139,7 +132,7 @@ public function create(string $cardId, string $message, string $replyTo = '0'):
 		}
 
 		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);

lib/Service/PermissionService.php

@@ -28,6 +28,7 @@
 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;
@@ -107,8 +108,9 @@ public function getPermissions($boardId, ?string $userId = null) {
 			return $cached;
 		}
 
+		$board = $this->getBoard($boardId);
 		$owner = $this->userIsBoardOwner($boardId, $userId);
-		$acls = $this->aclMapper->findAll($boardId);
+		$acls = $board->getDeletedAt() === 0 ? $this->aclMapper->findAll($boardId) : [];
 		$permissions = [
 			Acl::PERMISSION_READ => $owner || $this->userCan($acls, Acl::PERMISSION_READ, $userId),
 			Acl::PERMISSION_EDIT => $owner || $this->userCan($acls, Acl::PERMISSION_EDIT, $userId),
@@ -142,13 +144,10 @@ public function matchPermissions(Board $board) {
 	/**
 	 * 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): bool {
+	public function checkPermission(?IPermissionMapper $mapper, $id, int $permission, $userId = null, bool $allowDeletedCard = false): bool {
 		$boardId = $id;
 		if ($mapper instanceof IPermissionMapper && !($mapper instanceof BoardMapper)) {
 			$boardId = $mapper->findBoardId($id);
@@ -160,6 +159,14 @@ public function checkPermission($mapper, $id, $permission, $userId = null): bool
 
 		$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;
 		}
 

lib/Sharing/ShareAPIHelper.php

@@ -115,7 +115,7 @@ private function parseDate(string $expireDate): \DateTime {
 	 */
 	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;
 		}

tests/integration/features/acl.feature

@@ -106,3 +106,37 @@ Feature: acl
 			| 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

tests/integration/features/bootstrap/AttachmentContext.php

@@ -87,4 +87,14 @@ public function fetchingTheAttachmentForTheCard($attachmentReference, $cardRefer
 
 		$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');
+	}
 }

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) {
@@ -204,7 +204,9 @@ public function setTheDescriptionTo($description) {
 			['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);
+		}
 	}
 
 	/**
@@ -216,7 +218,9 @@ public function setCardAttribute($attribute, $value) {
 			[$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);
+		}
 	}
 
 	/**
@@ -227,7 +231,9 @@ public function getCard() {
 			$this->card
 		));
 		$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);
+		}
 	}
 
 	/**
@@ -282,4 +288,38 @@ public function rememberTheLastCardAs($arg1) {
 	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']);
+		$this->card['deletedAt'] = time();
+	}
+
+	/**
+	 * @Given /^delete the board/
+	 */
+	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 ?? []));
+	}
+
+
 }