Merge pull request #2118 from nextcloud/bugfix/noid/attachment-id-check

nextcloud · Jul 13, 2020 · 06aad4b · 06aad4b
2 parents e544550 + 6114c28
commit 06aad4b
Show file tree

Hide file tree

Showing 8 changed files with 72 additions and 96 deletions.
diff --git 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
@@ -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
@@ -213,7 +213,6 @@ public function create($cardId, $type, $data) {
 	/**
 	 * Display the attachment
 	 *
-	 * @param $cardId
 	 * @param $attachmentId
 	 * @return Response
 	 * @throws BadRequestException
@@ -222,17 +221,17 @@ 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');
 		}
 
-		$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());
@@ -245,7 +244,6 @@ public function display($cardId, $attachmentId) {
 	/**
 	 * Update an attachment with custom data
 	 *
-	 * @param $cardId
 	 * @param $attachmentId
 	 * @param $request
 	 * @return mixed
@@ -254,23 +252,23 @@ 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');
 		}
 
 		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->cache->clear('card-' . $cardId);
+		$this->permissionService->checkPermission($this->cardMapper, $attachment->getCardId(), Acl::PERMISSION_EDIT);
+		$this->cache->clear('card-' . $attachment->getCardId());
 
-		$attachment = $this->attachmentMapper->find($attachmentId);
 		$attachment->setData($data);
 		try {
 			$service = $this->getService($attachment->getType());
@@ -296,27 +294,27 @@ 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
 	 * @throws \OCP\AppFramework\Db\DoesNotExistException
 	 * @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');
 		}
 
-		$this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT);
-		$this->cache->clear('card-' . $cardId);
+		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-' . $attachment->getCardId());
 
-		$attachment = $this->attachmentMapper->find($attachmentId);
 		try {
 			$service = $this->getService($attachment->getType());
 			if ($service->allowUndo()) {
@@ -334,19 +332,20 @@ 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');
 		}
 
-		$this->permissionService->checkPermission($this->cardMapper, $cardId, Acl::PERMISSION_EDIT);
-		$this->cache->clear('card-' . $cardId);
+		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-' . $attachment->getCardId());
 
-		$attachment = $this->attachmentMapper->find($attachmentId);
 		try {
 			$service = $this->getService($attachment->getType());
 			if ($service->allowUndo()) {

diff --git 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
@@ -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
@@ -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
@@ -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();