ahtlon/deck
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

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

Browse Source
This commit is contained in:
Julius Härtl
2024-01-05 13:34:07 +01:00
committed by GitHub
parent a8625f2efb 569ff6f8e2
commit 532e21f168
17 changed files with 264 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.github/workflows/integration.yml vendored
View File

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

3
composer.json
View File

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

21
lib/Activity/ActivityManager.php
View File

@@ -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;
@@ -564,4 +565,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;
}
}
}

6
lib/Activity/DeckProvider.php
View File

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

2
lib/Db/Card.php
View File

@@ -39,6 +39,8 @@ use Sabre\VObject\Component\VCalendar;
* @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)

2
lib/Service/BoardService.php
View File

@@ -446,7 +446,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);

6
lib/Service/CardService.php
View File

@@ -298,7 +298,7 @@ class CardService {
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 @@ 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.');
}
}

15
lib/Service/CommentService.php
View File

@@ -94,7 +94,7 @@ class CommentService {
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 @@ class CommentService {
}
/**
* @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 @@ 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);

19
lib/Service/PermissionService.php
View File

@@ -28,6 +28,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;
@@ -107,8 +108,9 @@ class PermissionService {
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 @@ 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): 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 @@ class PermissionService {
$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;
}

2
lib/Sharing/ShareAPIHelper.php
View File

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

34
tests/integration/features/acl.feature
View File

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

10
tests/integration/features/bootstrap/AttachmentContext.php
View File

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

50
tests/integration/features/bootstrap/BoardContext.php
View File

@@ -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 @@ 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);
}
}
/**
@@ -216,7 +218,9 @@ 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);
}
}
/**
@@ -227,7 +231,9 @@ class BoardContext implements Context {
$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 @@ 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']);
$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 ?? []));
}
}

31
tests/integration/features/bootstrap/CommentContext.php
View File

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

8
tests/integration/features/bootstrap/ServerContext.php
View File

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

77
tests/integration/features/decks.feature
View File

@@ -58,3 +58,80 @@ Feature: decks
|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
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
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

3
tests/unit/Activity/DeckProviderTest.php
View File

@@ -78,6 +78,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 = []) {