Merge pull request #5444 from nextcloud/backport/5443/stable25

[stable25] Fix deleted card/board issues

.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/[email protected]
         with:

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": {

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;
@@ -559,4 +560,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) {
 			$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) {
 		}
 
 		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

@@ -36,6 +36,8 @@
  * @method int getLastModified()
  * @method int getCreatedAt()
  * @method bool getArchived()
+ * @method int getDeletedAt()
+ * @method void setDeletedAt(int $deletedAt)
  * @method bool getNotified()
  *
  * @method void setLabels(Label[] $labels)

lib/Service/BoardService.php

@@ -473,7 +473,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

@@ -267,7 +267,7 @@ public function delete($id) {
 	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, null, true);
 		$this->permissionService->checkPermission($this->stackMapper, $stackId, Acl::PERMISSION_EDIT);
 
 		if ($this->boardService->isArchived($this->cardMapper, $id)) {
@@ -279,9 +279,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

@@ -77,24 +77,17 @@ public function list(string $cardId, int $limit = 20, int $offset = 0): DataResp
 	}
 
 	/**
-	 * @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) {
@@ -103,7 +96,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);
@@ -139,7 +132,7 @@ public function update(string $cardId, string $commentId, string $message): Data
 			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);

lib/Service/PermissionService.php

@@ -29,6 +29,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;
@@ -97,21 +98,26 @@ public function __construct(
 	 * @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;
 		}
 
-		$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),
 			Acl::PERMISSION_MANAGE => $owner || $this->userCan($acls, Acl::PERMISSION_MANAGE),
 			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;
 	}
 
@@ -137,13 +143,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) {
+	public function checkPermission($mapper, $id, $permission, $userId = null, bool $allowDeletedCard = false) {
 		$boardId = $id;
 		if ($mapper instanceof IPermissionMapper && !($mapper instanceof BoardMapper)) {
 			$boardId = $mapper->findBoardId($id);
@@ -157,12 +160,20 @@ public function checkPermission($mapper, $id, $permission, $userId = null) {
 			throw new NoPermissionException('Permission denied');
 		}
 
-		if ($this->userIsBoardOwner($boardId, $userId)) {
-			return true;
-		}
-
 		try {
-			$acls = $this->getBoard($boardId)->getAcl() ?? [];
+			$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;
+			}
+
+			$acls = $this->getBoard((int)$boardId)->getAcl() ?? [];
 			$result = $this->userCan($acls, $permission, $userId);
 			if ($result) {
 				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

@@ -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

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');
+	}
 }