From f6d7a62fbdd1b032372a6fea10c18bfd9e95be74 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 12 Aug 2016 15:15:24 +0200 Subject: [PATCH 1/3] Move OCS Middleware before security middleware This is required to be able to catch the NotLoggedIn exceptions etc in the OCSMiddleware and convert them to proper OCS Responses. --- lib/private/AppFramework/DependencyInjection/DIContainer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 66ca59d26e2da..b60ce6432499c 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -396,6 +396,7 @@ public function __construct($appName, $urlParams = array()){ $this->registerService('MiddlewareDispatcher', function($c) use (&$middleWares) { $dispatcher = new MiddlewareDispatcher(); $dispatcher->registerMiddleware($c['CORSMiddleware']); + $dispatcher->registerMiddleware($c['OCSMiddleware']); $dispatcher->registerMiddleware($c['SecurityMiddleware']); $dispatcher->registerMiddleWare($c['TwoFactorMiddleware']); @@ -404,7 +405,6 @@ public function __construct($appName, $urlParams = array()){ } $dispatcher->registerMiddleware($c['SessionMiddleware']); - $dispatcher->registerMiddleware($c['OCSMiddleware']); return $dispatcher; }); From aacb68c9a5458d29b74dcc7889920a4bcf11b57d Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 11 Aug 2016 09:44:12 +0200 Subject: [PATCH 2/3] Extend OCSMiddleware * Always set 401 (v1.php and v2.php) * Set proper error codes for v2.php * Proper OCS output on unhandled exceptions --- .../AppFramework/Middleware/OCSMiddleware.php | 62 ++++++++++++++++++- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/lib/private/AppFramework/Middleware/OCSMiddleware.php b/lib/private/AppFramework/Middleware/OCSMiddleware.php index e07d100d8ac4b..68445bbcc5196 100644 --- a/lib/private/AppFramework/Middleware/OCSMiddleware.php +++ b/lib/private/AppFramework/Middleware/OCSMiddleware.php @@ -23,8 +23,14 @@ namespace OC\AppFramework\Middleware; use OC\AppFramework\Http; +use OCP\API; +use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\OCSResponse; +use OCP\AppFramework\Http\Response; use OCP\AppFramework\OCS\OCSException; +use OCP\AppFramework\OCS\OCSForbiddenException; +use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; use OCP\IRequest; use OCP\AppFramework\Middleware; @@ -54,12 +60,35 @@ public function afterException($controller, $methodName, \Exception $exception) $code = $exception->getCode(); if ($code === 0) { - $code = Http::STATUS_INTERNAL_SERVER_ERROR; + $code = API::RESPOND_UNKNOWN_ERROR; } + + // Build the response $response = new OCSResponse($format, $code, $exception->getMessage()); + // Forbidden always sets 401 (even on v1.php) + if ($exception instanceof OCSForbiddenException || $code === API::RESPOND_UNAUTHORISED) { + $response->setStatus(Http::STATUS_UNAUTHORIZED); + } + + // On v2.php we set actual HTTP error codes if (substr_compare($this->request->getScriptName(), '/ocs/v2.php', -strlen('/ocs/v2.php')) === 0) { - $response->setStatus($code); + if ($code === API::RESPOND_NOT_FOUND) { + $response->setStatus(Http::STATUS_NOT_FOUND); + } else if ($code === API::RESPOND_SERVER_ERROR) { + $response->setStatus(Http::STATUS_INTERNAL_SERVER_ERROR); + } else if ($code === API::RESPOND_UNKNOWN_ERROR) { + $response->setStatus(Http::STATUS_INTERNAL_SERVER_ERROR); + } else if ($code === API::RESPOND_UNAUTHORISED) { + // Already set + } + // 4xx and 5xx codes are forwarded as is. + else if ($code >= 400 && $code < 600) { + $response->setStatus($code); + } else { + // All other codes get a bad request + $response->setStatus(Http::STATUS_BAD_REQUEST); + } } return $response; } @@ -67,6 +96,35 @@ public function afterException($controller, $methodName, \Exception $exception) throw $exception; } + /** + * @param \OCP\AppFramework\Controller $controller + * @param string $methodName + * @param Response $response + * @return \OCP\AppFramework\Http\Response + */ + public function afterController($controller, $methodName, Response $response) { + /* + * If a different middleware has detected that a request unauthorized or forbidden + * we need to catch the response and convert it to a proper OCS response. + */ + if ($controller instanceof OCSController && !($response instanceof OCSResponse)) { + if ($response->getStatus() === Http::STATUS_UNAUTHORIZED || + $response->getStatus() === Http::STATUS_FORBIDDEN) { + $format = $this->getFormat($controller); + + $message = ''; + if ($response instanceof JSONResponse) { + /** @var DataResponse $response */ + $message = $response->getData()['message']; + } + $response = new OCSResponse($format, \OCP\API::RESPOND_UNAUTHORISED, $message); + $response->setStatus(Http::STATUS_UNAUTHORIZED); + } + } + + return $response; + } + /** * @param \OCP\AppFramework\Controller $controller * @return string From a98376f38cb2f2933495b16eaf5ba096fd1d1ebe Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 12 Aug 2016 15:42:26 +0200 Subject: [PATCH 3/3] Add tests --- .../Middleware/OCSMiddlewareTest.php | 76 +++++++++++++++++-- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/tests/lib/AppFramework/Middleware/OCSMiddlewareTest.php b/tests/lib/AppFramework/Middleware/OCSMiddlewareTest.php index 7d8cadc677f37..b2295fdc26d91 100644 --- a/tests/lib/AppFramework/Middleware/OCSMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/OCSMiddlewareTest.php @@ -27,14 +27,14 @@ use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\OCS\OCSNotFoundException; -use OC\AppFramework\Http\Request; +use OCP\IRequest; use OC\AppFramework\Middleware\OCSMiddleware; class OCSMiddlewareTest extends \Test\TestCase { /** - * @var Request + * @var IRequest */ private $request; @@ -101,8 +101,18 @@ public function testAfterExceptionOCSv1($controller, $exception, $forward, $mess $this->assertInstanceOf('OCP\AppFramework\Http\OCSResponse', $result); $this->assertSame($message, $this->invokePrivate($result, 'message')); - $this->assertSame($code, $this->invokePrivate($result, 'statuscode')); - $this->assertSame(200, $result->getStatus()); + + if ($exception->getCode() === 0) { + $this->assertSame(\OCP\API::RESPOND_UNKNOWN_ERROR, $this->invokePrivate($result, 'statuscode')); + } else { + $this->assertSame($code, $this->invokePrivate($result, 'statuscode')); + } + + if ($exception instanceof OCSForbiddenException) { + $this->assertSame(Http::STATUS_UNAUTHORIZED, $result->getStatus()); + } else { + $this->assertSame(200, $result->getStatus()); + } } catch (\Exception $e) { $this->assertTrue($forward); $this->assertEquals($exception, $e); @@ -131,7 +141,11 @@ public function testAfterExceptionOCSv2($controller, $exception, $forward, $mess $this->assertInstanceOf('OCP\AppFramework\Http\OCSResponse', $result); $this->assertSame($message, $this->invokePrivate($result, 'message')); - $this->assertSame($code, $this->invokePrivate($result, 'statuscode')); + if ($exception->getCode() === 0) { + $this->assertSame(\OCP\API::RESPOND_UNKNOWN_ERROR, $this->invokePrivate($result, 'statuscode')); + } else { + $this->assertSame($code, $this->invokePrivate($result, 'statuscode')); + } $this->assertSame($code, $result->getStatus()); } catch (\Exception $e) { $this->assertTrue($forward); @@ -161,7 +175,11 @@ public function testAfterExceptionOCSv2SubFolder($controller, $exception, $forwa $this->assertInstanceOf('OCP\AppFramework\Http\OCSResponse', $result); $this->assertSame($message, $this->invokePrivate($result, 'message')); - $this->assertSame($code, $this->invokePrivate($result, 'statuscode')); + if ($exception->getCode() === 0) { + $this->assertSame(\OCP\API::RESPOND_UNKNOWN_ERROR, $this->invokePrivate($result, 'statuscode')); + } else { + $this->assertSame($code, $this->invokePrivate($result, 'statuscode')); + } $this->assertSame($code, $result->getStatus()); } catch (\Exception $e) { $this->assertTrue($forward); @@ -169,4 +187,50 @@ public function testAfterExceptionOCSv2SubFolder($controller, $exception, $forwa } } + public function dataAfterController() { + $OCSController = $this->getMockBuilder('OCP\AppFramework\OCSController') + ->disableOriginalConstructor() + ->getMock(); + $controller = $this->getMockBuilder('OCP\AppFramework\Controller') + ->disableOriginalConstructor() + ->getMock(); + + return [ + [$OCSController, new Http\Response(), false], + [$OCSController, new Http\JSONResponse(), false], + [$OCSController, new Http\JSONResponse(['message' => 'foo']), false], + [$OCSController, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_UNAUTHORIZED), true], + [$OCSController, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_FORBIDDEN), true], + + [$controller, new Http\Response(), false], + [$controller, new Http\JSONResponse(), false], + [$controller, new Http\JSONResponse(['message' => 'foo']), false], + [$controller, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_UNAUTHORIZED), false], + [$controller, new Http\JSONResponse(['message' => 'foo'], Http::STATUS_FORBIDDEN), false], + + ]; + } + + /** + * @dataProvider dataAfterController + * + * @param Controller $controller + * @param Http\Response $response + * @param bool $converted + */ + public function testAfterController($controller, $response, $converted) { + $OCSMiddleware = new OCSMiddleware($this->request); + $newResponse = $OCSMiddleware->afterController($controller, 'foo', $response); + + if ($converted === false) { + $this->assertSame($response, $newResponse); + } else { + $this->assertInstanceOf('\OCP\AppFramework\Http\OCSResponse', $newResponse); + /** @var Http\OCSResponse $newResponse */ + $this->assertSame($response->getData()['message'], $this->invokePrivate($newResponse, 'message')); + $this->assertSame(\OCP\API::RESPOND_UNAUTHORISED, $this->invokePrivate($newResponse, 'statuscode')); + $this->assertSame(Http::STATUS_UNAUTHORIZED, $newResponse->getStatus()); + } + } + }