From cd0b3b29f1977af87cda9b562da057eb09aefae9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 7 Jul 2020 15:33:52 +0200 Subject: [PATCH 1/2] Check for the card permissions based on attachment id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Service/AttachmentService.php | 34 +++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 945dd5bd0..6c174b335 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -231,8 +231,12 @@ public function display($cardId, $attachmentId) { throw new BadRequestException('attachment id must be a number'); } - $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_READ); - $attachment = $this->attachmentMapper->find($attachmentId); + try { + $attachment = $this->attachmentMapper->find($attachmentId); + } catch (\Exception $e) { + throw new NoPermissionException('Permission denied'); + } + $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_READ); try { $service = $this->getService($attachment->getType()); @@ -266,11 +270,15 @@ public function update($cardId, $attachmentId, $data) { if ($data === false || $data === null) { //throw new BadRequestException('data must be provided'); } + try { + $attachment = $this->attachmentMapper->find($attachmentId); + } catch (\Exception $e) { + throw new NoPermissionException('Permission denied'); + } - $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); + $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); $this->cache->clear('card-' . $cardId); - $attachment = $this->attachmentMapper->find($attachmentId); $attachment->setData($data); try { $service = $this->getService($attachment->getType()); @@ -313,10 +321,15 @@ public function delete($cardId, $attachmentId) { throw new BadRequestException('attachment id must be a number'); } - $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); + try { + $attachment = $this->attachmentMapper->find($attachmentId); + } catch (\Exception $e) { + throw new NoPermissionException('Permission denied'); + } + + $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); $this->cache->clear('card-' . $cardId); - $attachment = $this->attachmentMapper->find($attachmentId); try { $service = $this->getService($attachment->getType()); if ($service->allowUndo()) { @@ -343,10 +356,15 @@ public function restore($cardId, $attachmentId) { throw new BadRequestException('attachment id must be a number'); } - $this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT); + try { + $attachment = $this->attachmentMapper->find($attachmentId); + } catch (\Exception $e) { + throw new NoPermissionException('Permission denied'); + } + + $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); $this->cache->clear('card-' . $cardId); - $attachment = $this->attachmentMapper->find($attachmentId); try { $service = $this->getService($attachment->getType()); if ($service->allowUndo()) { From 6114c28b1029cd591be3a53aeacf5635cc0f4d5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Sat, 11 Jul 2020 11:58:53 +0200 Subject: [PATCH 2/2] Remove unneeded cardId parameters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Controller/AttachmentApiController.php | 9 +++-- lib/Controller/AttachmentController.php | 16 ++++----- lib/Service/AttachmentService.php | 33 ++++------------- lib/Service/PermissionService.php | 2 +- lib/Service/StackService.php | 4 +-- tests/unit/Service/AttachmentServiceTest.php | 15 ++++---- .../AttachmentApiControllerTest.php | 36 +++++-------------- .../controller/AttachmentControllerTest.php | 19 +++++----- 8 files changed, 46 insertions(+), 88 deletions(-) diff --git a/lib/Controller/AttachmentApiController.php b/lib/Controller/AttachmentApiController.php index 821d870c0..54741613b 100644 --- a/lib/Controller/AttachmentApiController.php +++ b/lib/Controller/AttachmentApiController.php @@ -54,8 +54,7 @@ public function getAll() { * */ public function display() { - $attachment = $this->attachmentService->display($this->request->getParam('cardId'), $this->request->getParam('attachmentId')); - return $attachment; + return $this->attachmentService->display($this->request->getParam('attachmentId')); } /** @@ -76,7 +75,7 @@ public function create($type, $data) { * */ public function update($data) { - $attachment = $this->attachmentService->update($this->request->getParam('cardId'), $this->request->getParam('attachmentId'), $data); + $attachment = $this->attachmentService->update($this->request->getParam('attachmentId'), $data); return new DataResponse($attachment, HTTP::STATUS_OK); } @@ -87,7 +86,7 @@ public function update($data) { * */ public function delete() { - $attachment = $this->attachmentService->delete($this->request->getParam('cardId'), $this->request->getParam('attachmentId')); + $attachment = $this->attachmentService->delete($this->request->getParam('attachmentId')); return new DataResponse($attachment, HTTP::STATUS_OK); } @@ -98,7 +97,7 @@ public function delete() { * */ public function restore() { - $attachment = $this->attachmentService->restore($this->request->getParam('cardId'), $this->request->getParam('attachmentId')); + $attachment = $this->attachmentService->restore($this->request->getParam('attachmentId')); return new DataResponse($attachment, HTTP::STATUS_OK); } } diff --git a/lib/Controller/AttachmentController.php b/lib/Controller/AttachmentController.php index daec5cf2d..18cab5761 100644 --- a/lib/Controller/AttachmentController.php +++ b/lib/Controller/AttachmentController.php @@ -52,8 +52,8 @@ public function getAll($cardId) { * @return \OCP\AppFramework\Http\Response * @throws \OCA\Deck\NotFoundException */ - public function display($cardId, $attachmentId) { - return $this->attachmentService->display($cardId, $attachmentId); + public function display($attachmentId) { + return $this->attachmentService->display($attachmentId); } /** @@ -70,21 +70,21 @@ public function create($cardId) { /** * @NoAdminRequired */ - public function update($cardId, $attachmentId) { - return $this->attachmentService->update($cardId, $attachmentId, $this->request->getParam('data')); + public function update($attachmentId) { + return $this->attachmentService->update($attachmentId, $this->request->getParam('data')); } /** * @NoAdminRequired */ - public function delete($cardId, $attachmentId) { - return $this->attachmentService->delete($cardId, $attachmentId); + public function delete($attachmentId) { + return $this->attachmentService->delete($attachmentId); } /** * @NoAdminRequired */ - public function restore($cardId, $attachmentId) { - return $this->attachmentService->restore($cardId, $attachmentId); + public function restore($attachmentId) { + return $this->attachmentService->restore($attachmentId); } } diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php index 6c174b335..e80210329 100644 --- a/lib/Service/AttachmentService.php +++ b/lib/Service/AttachmentService.php @@ -213,7 +213,6 @@ public function create($cardId, $type, $data) { /** * Display the attachment * - * @param $cardId * @param $attachmentId * @return Response * @throws BadRequestException @@ -222,11 +221,7 @@ public function create($cardId, $type, $data) { * @throws \OCP\AppFramework\Db\DoesNotExistException * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException */ - public function display($cardId, $attachmentId) { - if (is_numeric($cardId) === false) { - throw new BadRequestException('card id must be a number'); - } - + public function display($attachmentId) { if (is_numeric($attachmentId) === false) { throw new BadRequestException('attachment id must be a number'); } @@ -249,7 +244,6 @@ public function display($cardId, $attachmentId) { /** * Update an attachment with custom data * - * @param $cardId * @param $attachmentId * @param $request * @return mixed @@ -258,11 +252,7 @@ public function display($cardId, $attachmentId) { * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException * @throws BadRequestException */ - public function update($cardId, $attachmentId, $data) { - if (is_numeric($cardId) === false) { - throw new BadRequestException('card id must be a number'); - } - + public function update($attachmentId, $data) { if (is_numeric($attachmentId) === false) { throw new BadRequestException('attachment id must be a number'); } @@ -277,7 +267,7 @@ public function update($cardId, $attachmentId, $data) { } $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $cardId); + $this->cache->clear('card-' . $attachment->getCardId()); $attachment->setData($data); try { @@ -304,7 +294,6 @@ public function update($cardId, $attachmentId, $data) { * Either mark an attachment as deleted for later removal or just remove it depending * on the IAttachmentService implementation * - * @param $cardId * @param $attachmentId * @return \OCP\AppFramework\Db\Entity * @throws \OCA\Deck\NoPermissionException @@ -312,11 +301,7 @@ public function update($cardId, $attachmentId, $data) { * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException * @throws BadRequestException */ - public function delete($cardId, $attachmentId) { - if (is_numeric($cardId) === false) { - throw new BadRequestException('card id must be a number'); - } - + public function delete($attachmentId) { if (is_numeric($attachmentId) === false) { throw new BadRequestException('attachment id must be a number'); } @@ -328,7 +313,7 @@ public function delete($cardId, $attachmentId) { } $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $cardId); + $this->cache->clear('card-' . $attachment->getCardId()); try { $service = $this->getService($attachment->getType()); @@ -347,11 +332,7 @@ public function delete($cardId, $attachmentId) { return $attachment; } - public function restore($cardId, $attachmentId) { - if (is_numeric($cardId) === false) { - throw new BadRequestException('card id must be a number'); - } - + public function restore($attachmentId) { if (is_numeric($attachmentId) === false) { throw new BadRequestException('attachment id must be a number'); } @@ -363,7 +344,7 @@ public function restore($cardId, $attachmentId) { } $this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT); - $this->cache->clear('card-' . $cardId); + $this->cache->clear('card-' . $attachment->getCardId()); try { $service = $this->getService($attachment->getType()); diff --git a/lib/Service/PermissionService.php b/lib/Service/PermissionService.php index 8d528f2e4..ce0088258 100644 --- a/lib/Service/PermissionService.php +++ b/lib/Service/PermissionService.php @@ -133,7 +133,7 @@ public function matchPermissions(Board $board) { */ public function checkPermission($mapper, $id, $permission, $userId = null) { $boardId = $id; - if ($mapper instanceof IPermissionMapper) { + if ($mapper instanceof IPermissionMapper && !($mapper instanceof BoardMapper)) { $boardId = $mapper->findBoardId($id); } if ($boardId === null) { diff --git a/lib/Service/StackService.php b/lib/Service/StackService.php index 6e01ee9e7..7df627e06 100644 --- a/lib/Service/StackService.php +++ b/lib/Service/StackService.php @@ -147,9 +147,7 @@ public function findAll($boardId, $since = -1) { } public function fetchDeleted($boardId) { - $this->permissionService->checkPermission( - $this->boardMapper, $boardId, Acl::PERMISSION_READ - ); + $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ); $stacks = $this->stackMapper->findDeleted($boardId); $this->enrichStacksWithCards($stacks); diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php index 29b2ccb31..75a861f1d 100644 --- a/tests/unit/Service/AttachmentServiceTest.php +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -155,6 +155,7 @@ private function createAttachment($type, $data) { $attachment = new Attachment(); $attachment->setType($type); $attachment->setData($data); + $attachment->setCardId(123); return $attachment; } @@ -255,7 +256,7 @@ public function testDisplay() { ->method('display') ->with($attachment) ->willReturn($response); - $actual = $this->attachmentService->display(123, 1); + $actual = $this->attachmentService->display(1); $this->assertEquals($response, $actual); } @@ -272,7 +273,7 @@ public function testDisplayInvalid() { ->method('display') ->with($attachment) ->will($this->throwException(new InvalidAttachmentType('deck_file'))); - $this->attachmentService->display(123, 1); + $this->attachmentService->display(1); } public function testUpdate() { $attachment = $this->createAttachment('deck_file', 'file_name.jpg'); @@ -294,7 +295,7 @@ public function testUpdate() { $a->setExtendedData(['mime' => 'image/jpeg']); }); - $actual = $this->attachmentService->update(123, 1, 'file_name.jpg'); + $actual = $this->attachmentService->update(1, 'file_name.jpg'); $expected->setExtendedData(['mime' => 'image/jpeg']); $expected->setLastModified($attachment->getLastModified()); @@ -318,7 +319,7 @@ public function testDelete() { $this->attachmentMapper->expects($this->once()) ->method('delete') ->willReturn($attachment); - $actual = $this->attachmentService->delete(123, 1); + $actual = $this->attachmentService->delete(1); $this->assertEquals($expected, $actual); } @@ -343,7 +344,7 @@ public function testDeleteWithUndo() { ->method('update') ->willReturn($attachment); $expected->setDeletedAt(23); - $actual = $this->attachmentService->delete(123, 1); + $actual = $this->attachmentService->delete(1); $this->assertEquals($expected, $actual); } @@ -363,7 +364,7 @@ public function testRestore() { ->method('update') ->willReturn($attachment); $expected->setDeletedAt(0); - $actual = $this->attachmentService->restore(123, 1); + $actual = $this->attachmentService->restore(1); $this->assertEquals($expected, $actual); } @@ -380,6 +381,6 @@ public function testRestoreNotAllowed() { $this->attachmentServiceImpl->expects($this->once()) ->method('allowUndo') ->willReturn(false); - $actual = $this->attachmentService->restore(123, 1); + $actual = $this->attachmentService->restore(1); } } diff --git a/tests/unit/controller/AttachmentApiControllerTest.php b/tests/unit/controller/AttachmentApiControllerTest.php index a7e5c2a4b..c92878d6b 100644 --- a/tests/unit/controller/AttachmentApiControllerTest.php +++ b/tests/unit/controller/AttachmentApiControllerTest.php @@ -73,14 +73,9 @@ public function testDisplay() { ->method('display') ->willReturn($this->attachmentExample); - $this->request->expects($this->exactly(2)) + $this->request->expects($this->once()) ->method('getParam') - ->withConsecutive( - ['cardId'], - ['attachmentId'] - )->willReturnonConsecutiveCalls( - $this->cardId, - $this->attachmentExample->getId()); + ->willReturn($this->attachmentExample->getId()); $expected = $this->attachmentExample; $actual = $this->controller->display(); @@ -114,14 +109,9 @@ public function testUpdate() { ->method('update') ->willReturn($this->attachmentExample); - $this->request->expects($this->exactly(2)) + $this->request->expects($this->once()) ->method('getParam') - ->withConsecutive( - ['cardId'], - ['attachmentId'] - )->willReturnonConsecutiveCalls( - $this->cardId, - $this->attachmentExample->getId()); + ->willReturn($this->attachmentExample->getId()); $expected = new DataResponse($this->attachmentExample, HTTP::STATUS_OK); $actual = $this->controller->update($data); @@ -133,14 +123,9 @@ public function testDelete() { ->method('delete') ->willReturn($this->attachmentExample); - $this->request->expects($this->exactly(2)) + $this->request->expects($this->once()) ->method('getParam') - ->withConsecutive( - ['cardId'], - ['attachmentId'] - )->willReturnonConsecutiveCalls( - $this->cardId, - $this->attachmentExample->getId()); + ->willReturn($this->attachmentExample->getId()); $expected = new DataResponse($this->attachmentExample, HTTP::STATUS_OK); $actual = $this->controller->delete(); @@ -152,14 +137,9 @@ public function testRestore() { ->method('restore') ->willReturn($this->attachmentExample); - $this->request->expects($this->exactly(2)) + $this->request->expects($this->once()) ->method('getParam') - ->withConsecutive( - ['cardId'], - ['attachmentId'] - )->willReturnonConsecutiveCalls( - $this->cardId, - $this->attachmentExample->getId()); + ->willReturn($this->attachmentExample->getId()); $expected = new DataResponse($this->attachmentExample, HTTP::STATUS_OK); $actual = $this->controller->restore(); diff --git a/tests/unit/controller/AttachmentControllerTest.php b/tests/unit/controller/AttachmentControllerTest.php index 5bdc29bdb..79b20e63d 100644 --- a/tests/unit/controller/AttachmentControllerTest.php +++ b/tests/unit/controller/AttachmentControllerTest.php @@ -44,8 +44,7 @@ public function setUp(): void { $this->controller = new AttachmentController( 'deck', $this->request, - $this->attachmentService, - $this->userId + $this->attachmentService ); } @@ -55,8 +54,8 @@ public function testGetAll() { } public function testDisplay() { - $this->attachmentService->expects($this->once())->method('display')->with(1, 2); - $this->controller->display(1, 2); + $this->attachmentService->expects($this->once())->method('display')->with(2); + $this->controller->display(2); } public function testCreate() { @@ -76,25 +75,25 @@ public function testUpdate() { ->will($this->onConsecutiveCalls('data')); $this->attachmentService->expects($this->once()) ->method('update') - ->with(1, 2, 'data') + ->with(2, 'data') ->willReturn(1); - $this->assertEquals(1, $this->controller->update(1, 2)); + $this->assertEquals(1, $this->controller->update(2)); } public function testDelete() { $this->attachmentService->expects($this->once()) ->method('delete') - ->with(123, 234) + ->with(234) ->willReturn(1); - $this->assertEquals(1, $this->controller->delete(123, 234)); + $this->assertEquals(1, $this->controller->delete(234)); } public function testRestore() { $this->attachmentService->expects($this->once()) ->method('restore') - ->with(123, 234) + ->with(234) ->willReturn(1); - $this->assertEquals(1, $this->controller->restore(123, 234)); + $this->assertEquals(1, $this->controller->restore(234)); } }