Merge pull request #922 from nextcloud/enhancement/noid/api-exception

Let ExceptionMiddleware properly return JSON on API related exceptions

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) {
+		$container->registerService('ExceptionMiddleware', function() use ($server) {
-			return new SharingMiddleware(
+			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');

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

tests/unit/Middleware/ExceptionMiddlewareTest.php

@@ -23,25 +23,33 @@
 
 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 {
+	 * @expectedException \Exception
-			$result = $this->sharingMiddleware->afterException('Foo', 'bar', new \Exception('failed hard'));
+	 * @expectedExceptionMessage failed hard
-		} catch (\Exception $e) {
+	 */
-			$this->assertEquals('failed hard', $e->getMessage());
+	public function testAfterExceptionNoController() {
+		$pageController = $this->createMock(PageController::class);
+		$result = $this->exceptionMiddleware->afterException($pageController, 'bar', new \Exception('failed hard'));
 	}
+
+	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']);
+
 	}
 
 }