From 0132dae2157cc00a41c531a6efb783b705e8f677 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Tue, 5 Mar 2019 19:01:55 +0100 Subject: [PATCH] Let ExceptionMiddleware properly return JSON on API related exceptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/AppInfo/Application.php | 7 +-- ...Middleware.php => ExceptionMiddleware.php} | 20 ++++++-- ...reTest.php => ExceptionMiddlewareTest.php} | 48 +++++++++++++------ 3 files changed, 54 insertions(+), 21 deletions(-) rename lib/Middleware/{SharingMiddleware.php => ExceptionMiddleware.php} (81%) rename tests/unit/Middleware/{SharingMiddlewareTest.php => ExceptionMiddlewareTest.php} (56%) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 62fbb19a7..a8826e301 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -28,6 +28,7 @@ use OCA\Deck\Db\Acl; use OCA\Deck\Db\AclMapper; use OCA\Deck\Db\AssignedUsersMapper; use OCA\Deck\Db\CardMapper; +use OCA\Deck\Middleware\ExceptionMiddleware; use OCA\Deck\Notification\Notifier; use OCP\AppFramework\App; use OCA\Deck\Middleware\SharingMiddleware; @@ -52,13 +53,13 @@ class Application extends App { $container = $this->getContainer(); $server = $container->getServer(); - $container->registerService('SharingMiddleware', function() use ($server) { - return new SharingMiddleware( + $container->registerService('ExceptionMiddleware', function() use ($server) { + return new ExceptionMiddleware( $server->getLogger(), $server->getConfig() ); }); - $container->registerMiddleWare('SharingMiddleware'); + $container->registerMiddleWare('ExceptionMiddleware'); $container->registerService('databaseType', function($container) { return $container->getServer()->getConfig()->getSystemValue('dbtype', 'sqlite'); diff --git a/lib/Middleware/SharingMiddleware.php b/lib/Middleware/ExceptionMiddleware.php similarity index 81% rename from lib/Middleware/SharingMiddleware.php rename to lib/Middleware/ExceptionMiddleware.php index ec6eb81f1..96d2a1b65 100644 --- a/lib/Middleware/SharingMiddleware.php +++ b/lib/Middleware/ExceptionMiddleware.php @@ -23,8 +23,8 @@ namespace OCA\Deck\Middleware; +use OCA\Deck\Controller\PageController; use OCA\Deck\StatusException; -use OCA\Deck\BadRequestException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Middleware; use OCP\AppFramework\Http\JSONResponse; @@ -33,7 +33,7 @@ use OCP\Util; use OCP\IConfig; -class SharingMiddleware extends Middleware { +class ExceptionMiddleware extends Middleware { /** @var ILogger */ private $logger; @@ -71,6 +71,20 @@ class SharingMiddleware extends Middleware { ], $exception->getStatus()); } + if (strpos(get_class($controller), 'OCA\\Deck\\Controller\\') === 0) { + $response = [ + 'status' => 500, + 'message' => $exception->getMessage() + ]; + if ($this->config->getSystemValue('loglevel', Util::WARN) === Util::DEBUG) { + $this->logger->logException($exception); + } + if ($this->config->getSystemValue('debug', true) === true) { + $response['exception'] = (array) $exception; + } + return new JSONResponse($response, 500); + } + // uncatched DoesNotExistExceptions will be thrown when the main entity is not found // we return a 403 so we don't leak information over existing entries // TODO: At some point those should properly be catched in the service classes @@ -84,4 +98,4 @@ class SharingMiddleware extends Middleware { throw $exception; } -} \ No newline at end of file +} diff --git a/tests/unit/Middleware/SharingMiddlewareTest.php b/tests/unit/Middleware/ExceptionMiddlewareTest.php similarity index 56% rename from tests/unit/Middleware/SharingMiddlewareTest.php rename to tests/unit/Middleware/ExceptionMiddlewareTest.php index 9757197d9..da21ce622 100644 --- a/tests/unit/Middleware/SharingMiddlewareTest.php +++ b/tests/unit/Middleware/ExceptionMiddlewareTest.php @@ -5,43 +5,51 @@ * @author Julius Härtl * * @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\Middleware; +use OCA\Deck\Controller\BoardController; +use OCA\Deck\Controller\PageController; use OCA\Deck\NoPermissionException; use OCA\Deck\NotFoundException; +use OCA\Deck\Service\BoardService; +use OCA\Deck\Service\PermissionService; +use OCP\AppFramework\Controller; use OCP\AppFramework\Http\JSONResponse; use OCP\ILogger; use OCP\IConfig; +use OCP\IRequest; -class SharingMiddlewareTest extends \Test\TestCase { +class ExceptionMiddlewareTest extends \Test\TestCase { /** @var ILogger */ private $logger; /** @var IConfig */ private $config; - private $sharingMiddleware; + private $controller; + private $exceptionMiddleware; public function setUp() { $this->logger = $this->createMock(ILogger::class); $this->config = $this->createMock(IConfig::class); - $this->sharingMiddleware = new SharingMiddleware( + $this->controller = $this->createMock(Controller::class); + $this->exceptionMiddleware = new ExceptionMiddleware( $this->logger, $this->config ); @@ -58,7 +66,7 @@ class SharingMiddlewareTest extends \Test\TestCase { * @dataProvider dataAfterException */ public function testAfterException($exception, $status, $message) { - $result = $this->sharingMiddleware->afterException('Foo', 'bar', $exception); + $result = $this->exceptionMiddleware->afterException($this->controller, 'bar', $exception); $expected = new JSONResponse([ "status" => $status, "message" => $message @@ -66,12 +74,22 @@ class SharingMiddlewareTest extends \Test\TestCase { $this->assertEquals($expected, $result); } - public function testAfterExceptionFail() { - try { - $result = $this->sharingMiddleware->afterException('Foo', 'bar', new \Exception('failed hard')); - } catch (\Exception $e) { - $this->assertEquals('failed hard', $e->getMessage()); - } + /** + * @expectedException \Exception + * @expectedExceptionMessage failed hard + */ + public function testAfterExceptionNoController() { + $pageController = $this->createMock(PageController::class); + $result = $this->exceptionMiddleware->afterException($pageController, 'bar', new \Exception('failed hard')); } -} \ No newline at end of file + public function testAfterExceptionFail() { + // BoardService $boardService, PermissionService $permissionService, $userId + $boardController = new BoardController('deck', $this->createMock(IRequest::class), $this->createMock(BoardService::class), $this->createMock(PermissionService::class), 'admin'); + $result = $this->exceptionMiddleware->afterException($boardController, 'bar', new \Exception('failed hard')); + $this->assertEquals('failed hard', $result->getData()['message']); + $this->assertEquals(500, $result->getData()['status']); + + } + +}