diff --git a/lib/BadRequestException.php b/lib/BadRequestException.php new file mode 100644 index 000000000..894721a35 --- /dev/null +++ b/lib/BadRequestException.php @@ -0,0 +1,36 @@ + + * + * @author Ryan Fletcher + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Deck; + +use OCP\AppFramework\Http; +class BadRequestException extends \Exception { + + public function __construct($message) { + parent::__construct($message); + } + + public function getStatus() { + return HTTP::STATUS_BAD_REQUEST; + } +} \ No newline at end of file diff --git a/lib/Controller/BoardApiController.php b/lib/Controller/BoardApiController.php index 30296e150..0d4770d7a 100644 --- a/lib/Controller/BoardApiController.php +++ b/lib/Controller/BoardApiController.php @@ -63,7 +63,6 @@ class BoardApiController extends ApiController { */ public function index() { $boards = $this->service->findAll(); - return new DataResponse($boards, HTTP::STATUS_OK); } @@ -75,18 +74,8 @@ class BoardApiController extends ApiController { * * Return the board specified by $this->request->getParam('boardId'). */ - public function get() { - - if (is_numeric($this->request->getParam('boardId')) === false) { - return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - } - + public function get() { $board = $this->service->find($this->request->getParam('boardId')); - - if ($board === false || $board === null) { - return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - } - return new DataResponse($board, HTTP::STATUS_OK); } @@ -100,22 +89,8 @@ class BoardApiController extends ApiController { * * Create a board with the specified title and color. */ - public function create($title, $color) { - - if ($title === false || $title === null) { - return new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST); - } - - if ($color === false || $color === null) { - return new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST); - } - - $board = $this->service->create($title, $this->userId, $color); - - if ($board === false || $board === null) { - return new DataResponse('Internal Server Error', HTTP::STATUS_INTERNAL_SERVER_ERROR); - } - + public function create($title, $color) { + $board = $this->service->create($title, $this->userId, $color); return new DataResponse($board, HTTP::STATUS_OK); } @@ -131,29 +106,7 @@ class BoardApiController extends ApiController { * Update a board with the specified boardId, title and color, and archived state. */ public function update($title, $color, $archived = false) { - - if (is_numeric($this->request->getParam('boardId')) === false) { - return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - } - - if (is_bool($archived) === false) { - return new DataResponse('archived must be a boolean', HTTP::STATUS_BAD_REQUEST); - } - - if ($title === false || $title === null) { - return new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST); - } - - if ($color === false || $color === null) { - return new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST); - } - $board = $this->service->update($this->request->getParam('boardId'), $title, $color, $archived); - - if ($board === false || $board === null) { - return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - } - return new DataResponse($board, HTTP::STATUS_OK); } @@ -166,17 +119,7 @@ class BoardApiController extends ApiController { * Delete the board specified by $boardId. Return the board that was deleted. */ public function delete() { - - if (is_numeric($this->request->getParam('boardId')) === false) { - return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - } - - $board = $this->service->delete($this->request->getParam('boardId')); - - if ($board === false || $board === null) { - return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - } - + $board = $this->service->delete($this->request->getParam('boardId')); return new DataResponse($board, HTTP::STATUS_OK); } @@ -188,18 +131,8 @@ class BoardApiController extends ApiController { * * Undo the deletion of the board specified by $boardId. */ - public function undoDelete() { - - if (is_numeric($this->request->getParam('boardId')) === false) { - return new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - } - - $board = $this->service->deleteUndo($this->request->getParam('boardId')); - - if ($board === false || $board === null) { - return new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - } - + public function undoDelete() { + $board = $this->service->deleteUndo($this->request->getParam('boardId')); return new DataResponse($board, HTTP::STATUS_OK); } diff --git a/lib/Middleware/SharingMiddleware.php b/lib/Middleware/SharingMiddleware.php index 13fed3cb9..5e53a69df 100644 --- a/lib/Middleware/SharingMiddleware.php +++ b/lib/Middleware/SharingMiddleware.php @@ -24,6 +24,7 @@ namespace OCA\Deck\Middleware; use OCA\Deck\StatusException; +use OCA\Deck\BadRequestException; use OCP\AppFramework\Middleware; use OCP\AppFramework\Http\JSONResponse; use OCP\ILogger; @@ -59,7 +60,7 @@ class SharingMiddleware extends Middleware { * @throws \Exception */ public function afterException($controller, $methodName, \Exception $exception) { - if ($exception instanceof StatusException) { + if ($exception instanceof StatusException || $exception instanceof BadRequestException) { if ($this->config->getSystemValue('loglevel', Util::WARN) === Util::DEBUG) { $this->logger->logException($exception); } diff --git a/lib/Service/BoardService.php b/lib/Service/BoardService.php index 6b250fb4a..b6d3da122 100644 --- a/lib/Service/BoardService.php +++ b/lib/Service/BoardService.php @@ -36,6 +36,7 @@ use OCA\Deck\Db\Board; use OCA\Deck\Db\BoardMapper; use OCA\Deck\Db\LabelMapper; use OCP\IUserManager; +use OCA\Deck\BadRequestException; class BoardService { @@ -111,8 +112,14 @@ class BoardService { * @throws DoesNotExistException * @throws \OCA\Deck\NoPermissionException * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException + * @throws BadRequestException */ public function find($boardId) { + + if ( is_numeric($boardId) === false ) { + throw new BadRequestException('board id must be a number'); + } + $this->permissionService->checkPermission($this->boardMapper, $boardId, Acl::PERMISSION_READ); /** @var Board $board */ $board = $this->boardMapper->find($boardId, true, true); @@ -201,13 +208,27 @@ class BoardService { * @param $userId * @param $color * @return \OCP\AppFramework\Db\Entity + * @throws BadRequestException */ public function create($title, $userId, $color) { + + if ($title === false || $title === null) { + throw new BadRequestException('title must be provided'); + } + + if ($userId === false || $userId === null) { + throw new BadRequestException('userId must be provided'); + } + + if ($color === false || $color === null) { + throw new BadRequestException('color must be provided'); + } + $board = new Board(); $board->setTitle($title); $board->setOwner($userId); $board->setColor($color); - $new_board = $this->boardMapper->insert($board); + $new_board = $this->boardMapper->insert($board); // create new labels $default_labels = [ @@ -243,8 +264,14 @@ class BoardService { * @throws DoesNotExistException * @throws \OCA\Deck\NoPermissionException * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException + * @throws BadRequestException */ public function delete($id) { + + if (is_numeric($id) === false) { + throw new BadRequestException('board id must be a number'); + } + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_READ); $board = $this->find($id); $board->setDeletedAt(time()); @@ -260,6 +287,11 @@ class BoardService { * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException */ public function deleteUndo($id) { + + if (is_numeric($id) === false) { + throw new BadRequestException('board id must be a number'); + } + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_READ); $board = $this->find($id); $board->setDeletedAt(0); @@ -288,8 +320,26 @@ class BoardService { * @throws DoesNotExistException * @throws \OCA\Deck\NoPermissionException * @throws \OCP\AppFramework\Db\MultipleObjectsReturnedException + * @throws BadRequestException */ public function update($id, $title, $color, $archived) { + + if (is_numeric($id) === false) { + throw new BadRequestException('board id must be a number'); + } + + if ($title === false || $title === null) { + throw new BadRequestException('color must be provided'); + } + + if ($color === false || $color === null) { + throw new BadRequestException('color must be provided'); + } + + if ( is_bool($archived) === false ) { + throw new BadRequestException('archived must be a boolean'); + } + $this->permissionService->checkPermission($this->boardMapper, $id, Acl::PERMISSION_MANAGE); $board = $this->find($id); $board->setTitle($title); diff --git a/tests/unit/controller/BoardApiControllerTest.php b/tests/unit/controller/BoardApiControllerTest.php index df3783f1a..6a2ebf2d7 100644 --- a/tests/unit/controller/BoardApiControllerTest.php +++ b/tests/unit/controller/BoardApiControllerTest.php @@ -92,31 +92,6 @@ class BoardApiControllerTest extends \Test\TestCase { $expected = new DataResponse($board, HTTP::STATUS_OK); $actual = $this->controller->get(); - $this->assertEquals($expected, $actual); - } - - public function testGetBadRequest() { - - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue('hello')); - - $expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->get(); - - $this->assertEquals($expected, $actual); - } - - public function testGetNotFound() { - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue('999')); - - $expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - $actual = $this->controller->get(); - $this->assertEquals($expected, $actual); } @@ -132,20 +107,8 @@ class BoardApiControllerTest extends \Test\TestCase { $expected = new DataResponse($board, HTTP::STATUS_OK); $actual = $this->controller->create($this->exampleBoard['title'], $this->exampleBoard['color']); $this->assertEquals($expected, $actual); - } + } - public function testCreateBadTitle() { - $expected = new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->create(null, $this->exampleBoard['color']); - $this->assertEquals($expected, $actual); - } - - public function testCreateBadColor() { - $expected = new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->create($this->exampleBoard['title'], null); - $this->assertEquals($expected, $actual); - } - public function testUpdate() { $board = new Board(); $board->setId($this->exampleBoard['id']); @@ -163,61 +126,9 @@ class BoardApiControllerTest extends \Test\TestCase { $expected = new DataResponse($board, HTTP::STATUS_OK); $actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color']); $this->assertEquals($expected, $actual); - } - - public function testUpdateBadId() { - $expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color']); - $this->assertEquals($expected, $actual); - } - - public function testUpdateBadArchived() { - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue($this->exampleBoard['id'])); - - $expected = new DataResponse('archived must be a boolean', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color'], 'Not a boolean value'); - $this->assertEquals($expected, $actual); - } - - public function testUpdateBadTitle() { - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue($this->exampleBoard['id'])); - - $expected = new DataResponse('title must be provided', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->update(null, $this->exampleBoard['color']); - $this->assertEquals($expected, $actual); - } - - public function testUpdateBadColor() { - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue($this->exampleBoard['id'])); - - $expected = new DataResponse('color must be provided', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->update($this->exampleBoard['title'], null); - $this->assertEquals($expected, $actual); - } - - public function testUpdateBoardNotFound() { - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue($this->exampleBoard['id'])); - - $expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - $actual = $this->controller->update($this->exampleBoard['title'], $this->exampleBoard['color']); - $this->assertEquals($expected, $actual); - } - - // TODO: Write testDelete() - public function testDelete() { - + } + + public function testDelete() { $board = new Board(); $board->setId($this->exampleBoard['id']); $board->setTitle($this->exampleBoard['title']); @@ -235,33 +146,7 @@ class BoardApiControllerTest extends \Test\TestCase { $actual = $this->controller->delete(); $this->assertEquals($expected, $actual); - } - - public function testDeleteBadId() { - - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue('bad id')); - - $expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->delete(); - - $this->assertEquals($expected, $actual); - } - - public function testDeleteNotFound() { - - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue('85')); - - $expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - $actual = $this->controller->delete(); - - $this->assertEquals($expected, $actual); - } + } public function testUndoDelete() { $board = new board(); @@ -281,28 +166,4 @@ class BoardApiControllerTest extends \Test\TestCase { $actual = $this->controller->undoDelete(); $this->assertEquals($expected, $actual); } - - public function testUndoDeleteBadId() { - - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue('bad id')); - - $expected = new DataResponse('board id must be a number', HTTP::STATUS_BAD_REQUEST); - $actual = $this->controller->undoDelete(); - $this->assertEquals($expected, $actual); - } - - public function testUndoDeleteNotFound() { - - $this->request->expects($this->any()) - ->method('getParam') - ->with('boardId') - ->will($this->returnValue(189)); - - $expected = new DataResponse('board not found', HTTP::STATUS_NOT_FOUND); - $actual = $this->controller->undoDelete(); - $this->assertEquals($expected, $actual); - } } \ No newline at end of file