From f4eb15d34023c8d524b286d137d175f98d70ef9c Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Sat, 28 Nov 2015 11:06:46 +0100 Subject: [PATCH] Show error template Otherwise this leads to an endless redirection in case of a CSRF exception. Also sets user expectation right. --- .../middleware/security/corsmiddleware.php | 1 + .../exceptions/appnotenabledexception.php | 36 +++++ .../crosssiterequestforgeryexception.php | 36 +++++ .../security/exceptions/notadminexception.php | 36 +++++ .../exceptions/notloggedinexception.php | 36 +++++ .../{ => exceptions}/securityexception.php | 23 +-- .../security/securitymiddleware.php | 47 +++--- .../security/CORSMiddlewareTest.php | 6 +- .../security/SecurityMiddlewareTest.php | 136 +++++++++++++----- 9 files changed, 284 insertions(+), 73 deletions(-) create mode 100644 lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php create mode 100644 lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php create mode 100644 lib/private/appframework/middleware/security/exceptions/notadminexception.php create mode 100644 lib/private/appframework/middleware/security/exceptions/notloggedinexception.php rename lib/private/appframework/middleware/security/{ => exceptions}/securityexception.php (63%) diff --git a/lib/private/appframework/middleware/security/corsmiddleware.php b/lib/private/appframework/middleware/security/corsmiddleware.php index 74b0dd0997..0e37e81c5b 100644 --- a/lib/private/appframework/middleware/security/corsmiddleware.php +++ b/lib/private/appframework/middleware/security/corsmiddleware.php @@ -23,6 +23,7 @@ namespace OC\AppFramework\Middleware\Security; +use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; diff --git a/lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php b/lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php new file mode 100644 index 0000000000..54cb08f438 --- /dev/null +++ b/lib/private/appframework/middleware/security/exceptions/appnotenabledexception.php @@ -0,0 +1,36 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Appframework\Middleware\Security\Exceptions; + +use OCP\AppFramework\Http; + +/** + * Class AppNotEnabledException is thrown when a resource for an application is + * requested that is not enabled. + * + * @package OC\Appframework\Middleware\Security\Exceptions + */ +class AppNotEnabledException extends SecurityException { + public function __construct() { + parent::__construct('App is not enabled', Http::STATUS_PRECONDITION_FAILED); + } +} diff --git a/lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php b/lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php new file mode 100644 index 0000000000..c59c921f5e --- /dev/null +++ b/lib/private/appframework/middleware/security/exceptions/crosssiterequestforgeryexception.php @@ -0,0 +1,36 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Appframework\Middleware\Security\Exceptions; + +use OCP\AppFramework\Http; + +/** + * Class CrossSiteRequestForgeryException is thrown when a CSRF exception has + * been encountered. + * + * @package OC\Appframework\Middleware\Security\Exceptions + */ +class CrossSiteRequestForgeryException extends SecurityException { + public function __construct() { + parent::__construct('CSRF check failed', Http::STATUS_PRECONDITION_FAILED); + } +} diff --git a/lib/private/appframework/middleware/security/exceptions/notadminexception.php b/lib/private/appframework/middleware/security/exceptions/notadminexception.php new file mode 100644 index 0000000000..3fa03cae89 --- /dev/null +++ b/lib/private/appframework/middleware/security/exceptions/notadminexception.php @@ -0,0 +1,36 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Appframework\Middleware\Security\Exceptions; + +use OCP\AppFramework\Http; + +/** + * Class NotAdminException is thrown when a resource has been requested by a + * non-admin user that is not accessible to non-admin users. + * + * @package OC\Appframework\Middleware\Security\Exceptions + */ +class NotAdminException extends SecurityException { + public function __construct() { + parent::__construct('Logged in user must be an admin', Http::STATUS_FORBIDDEN); + } +} diff --git a/lib/private/appframework/middleware/security/exceptions/notloggedinexception.php b/lib/private/appframework/middleware/security/exceptions/notloggedinexception.php new file mode 100644 index 0000000000..5f27625aa5 --- /dev/null +++ b/lib/private/appframework/middleware/security/exceptions/notloggedinexception.php @@ -0,0 +1,36 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * 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, version 3, + * along with this program. If not, see + * + */ + +namespace OC\Appframework\Middleware\Security\Exceptions; + +use OCP\AppFramework\Http; + +/** + * Class NotLoggedInException is thrown when a resource has been requested by a + * guest user that is not accessible to the public. + * + * @package OC\Appframework\Middleware\Security\Exceptions + */ +class NotLoggedInException extends SecurityException { + public function __construct() { + parent::__construct('Current user is not logged in', Http::STATUS_UNAUTHORIZED); + } +} diff --git a/lib/private/appframework/middleware/security/securityexception.php b/lib/private/appframework/middleware/security/exceptions/securityexception.php similarity index 63% rename from lib/private/appframework/middleware/security/securityexception.php rename to lib/private/appframework/middleware/security/exceptions/securityexception.php index 6f96b9a1d8..9b99282ce8 100644 --- a/lib/private/appframework/middleware/security/securityexception.php +++ b/lib/private/appframework/middleware/security/exceptions/securityexception.php @@ -1,7 +1,6 @@ - * @author Thomas Müller + * @author Lukas Reschke * * @copyright Copyright (c) 2015, ownCloud, Inc. * @license AGPL-3.0 @@ -20,20 +19,12 @@ * */ - -namespace OC\AppFramework\Middleware\Security; - +namespace OC\AppFramework\Middleware\Security\Exceptions; /** - * Thrown when the security middleware encounters a security problem + * Class SecurityException is the base class for security exceptions thrown by + * the security middleware. + * + * @package OC\AppFramework\Middleware\Security\Exceptions */ -class SecurityException extends \Exception { - - /** - * @param string $msg the security error message - */ - public function __construct($msg, $code = 0) { - parent::__construct($msg, $code); - } - -} +class SecurityException extends \Exception {} diff --git a/lib/private/appframework/middleware/security/securitymiddleware.php b/lib/private/appframework/middleware/security/securitymiddleware.php index 34c626ce8b..d0b7202a36 100644 --- a/lib/private/appframework/middleware/security/securitymiddleware.php +++ b/lib/private/appframework/middleware/security/securitymiddleware.php @@ -28,8 +28,13 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Http; +use OC\Appframework\Middleware\Security\Exceptions\AppNotEnabledException; +use OC\Appframework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; +use OC\Appframework\Middleware\Security\Exceptions\NotAdminException; +use OC\Appframework\Middleware\Security\Exceptions\NotLoggedInException; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Http\RedirectResponse; +use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\JSONResponse; @@ -39,7 +44,7 @@ use OCP\IRequest; use OCP\ILogger; use OCP\AppFramework\Controller; use OCP\Util; - +use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; /** * Used to do all the authentication and checking stuff for a controller method @@ -75,7 +80,7 @@ class SecurityMiddleware extends Middleware { ILogger $logger, $appName, $isLoggedIn, - $isAdminUser){ + $isAdminUser) { $this->navigationManager = $navigationManager; $this->request = $request; $this->reflector = $reflector; @@ -95,7 +100,7 @@ class SecurityMiddleware extends Middleware { * @param string $methodName the name of the method * @throws SecurityException when a security check fails */ - public function beforeController($controller, $methodName){ + public function beforeController($controller, $methodName) { // this will set the current navigation entry of the app, use this only // for normal HTML requests and not for AJAX requests @@ -105,12 +110,12 @@ class SecurityMiddleware extends Middleware { $isPublicPage = $this->reflector->hasAnnotation('PublicPage'); if(!$isPublicPage) { if(!$this->isLoggedIn) { - throw new SecurityException('Current user is not logged in', Http::STATUS_UNAUTHORIZED); + throw new NotLoggedInException(); } if(!$this->reflector->hasAnnotation('NoAdminRequired')) { if(!$this->isAdminUser) { - throw new SecurityException('Logged in user must be an admin', Http::STATUS_FORBIDDEN); + throw new NotAdminException(); } } } @@ -119,18 +124,18 @@ class SecurityMiddleware extends Middleware { Util::callRegister(); if(!$this->reflector->hasAnnotation('NoCSRFRequired')) { if(!$this->request->passesCSRFCheck()) { - throw new SecurityException('CSRF check failed', Http::STATUS_PRECONDITION_FAILED); + throw new CrossSiteRequestForgeryException(); } } /** * FIXME: Use DI once available - * Checks if app is enabled (also inclues a check whether user is allowed to access the resource) + * Checks if app is enabled (also includes a check whether user is allowed to access the resource) * The getAppPath() check is here since components such as settings also use the AppFramework and * therefore won't pass this check. */ if(\OC_App::getAppPath($this->appName) !== false && !\OC_App::isEnabled($this->appName)) { - throw new SecurityException('App is not enabled', Http::STATUS_PRECONDITION_FAILED); + throw new AppNotEnabledException(); } } @@ -146,28 +151,28 @@ class SecurityMiddleware extends Middleware { * @throws \Exception the passed in exception if it cant handle it * @return Response a Response object or null in case that the exception could not be handled */ - public function afterException($controller, $methodName, \Exception $exception){ - if($exception instanceof SecurityException){ - - if (stripos($this->request->getHeader('Accept'),'html')===false) { + public function afterException($controller, $methodName, \Exception $exception) { + if($exception instanceof SecurityException) { + if (stripos($this->request->getHeader('Accept'),'html') === false) { $response = new JSONResponse( array('message' => $exception->getMessage()), $exception->getCode() ); - $this->logger->debug($exception->getMessage()); } else { - - // TODO: replace with link to route - $url = $this->urlGenerator->getAbsoluteURL('index.php'); - // add redirect URL to redirect to the previous page after login - $url .= '?redirect_url=' . urlencode($this->request->server['REQUEST_URI']); - $response = new RedirectResponse($url); - $this->logger->debug($exception->getMessage()); + if($exception instanceof NotLoggedInException) { + // TODO: replace with link to route + $url = $this->urlGenerator->getAbsoluteURL('index.php'); + $url .= '?redirect_url=' . urlencode($this->request->server['REQUEST_URI']); + $response = new RedirectResponse($url); + } else { + $response = new TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest'); + $response->setStatus($exception->getCode()); + } } + $this->logger->debug($exception->getMessage()); return $response; - } throw $exception; diff --git a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php index ca526fb859..cf5f97a046 100644 --- a/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/CORSMiddlewareTest.php @@ -14,7 +14,7 @@ namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Http\Request; use OC\AppFramework\Utility\ControllerMethodReflector; - +use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\Response; @@ -91,7 +91,7 @@ class CORSMiddlewareTest extends \Test\TestCase { /** * @CORS - * @expectedException \OC\AppFramework\Middleware\Security\SecurityException + * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\SecurityException */ public function testCorsIgnoredIfWithCredentialsHeaderPresent() { $request = new Request( @@ -160,7 +160,7 @@ class CORSMiddlewareTest extends \Test\TestCase { /** * @CORS - * @expectedException \OC\AppFramework\Middleware\Security\SecurityException + * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\SecurityException */ public function testCORSShouldNotAllowCookieAuth() { $request = new Request( diff --git a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php index 347a0423ea..62223bbc2d 100644 --- a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php @@ -1,34 +1,40 @@ + * @author Lukas Reschke * - * @author Bernhard Posselt - * @copyright 2012 Bernhard Posselt + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 * - * This library is free software; you can redistribute it and/or - * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE - * License as published by the Free Software Foundation; either - * version 3 of the License, or any later version. + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. * - * This library is distributed in the hope that it will be useful, + * 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. + * 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 library. If not, see . + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see * */ + namespace OC\AppFramework\Middleware\Security; use OC\AppFramework\Http; use OC\AppFramework\Http\Request; +use OC\Appframework\Middleware\Security\Exceptions\AppNotEnabledException; +use OC\Appframework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException; +use OC\Appframework\Middleware\Security\Exceptions\NotAdminException; +use OC\Appframework\Middleware\Security\Exceptions\NotLoggedInException; +use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; use OC\AppFramework\Utility\ControllerMethodReflector; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\Http\TemplateResponse; class SecurityMiddlewareTest extends \Test\TestCase { @@ -71,8 +77,12 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->secAjaxException = new SecurityException('hey', true); } - - private function getMiddleware($isLoggedIn, $isAdminUser){ + /** + * @param bool $isLoggedIn + * @param bool $isAdminUser + * @return SecurityMiddleware + */ + private function getMiddleware($isLoggedIn, $isAdminUser) { return new SecurityMiddleware( $this->request, $this->reader, @@ -219,8 +229,8 @@ class SecurityMiddlewareTest extends \Test\TestCase { $sec = $this->getMiddleware($isLoggedIn, $isAdminUser); - if($shouldFail){ - $this->setExpectedException('\OC\AppFramework\Middleware\Security\SecurityException'); + if($shouldFail) { + $this->setExpectedException('\OC\AppFramework\Middleware\Security\Exceptions\SecurityException'); } else { $this->assertTrue(true); } @@ -232,7 +242,7 @@ class SecurityMiddlewareTest extends \Test\TestCase { /** * @PublicPage - * @expectedException \OC\AppFramework\Middleware\Security\SecurityException + * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException */ public function testCsrfCheck(){ $this->request->expects($this->once()) @@ -311,25 +321,85 @@ class SecurityMiddlewareTest extends \Test\TestCase { $this->middleware->afterException($this->controller, 'test', $ex); } - - public function testAfterExceptionReturnsRedirect(){ + public function testAfterExceptionReturnsRedirectForNotLoggedInUser() { $this->request = new Request( - [ - 'server' => [ - 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', - 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' - ] - ], - $this->getMock('\OCP\Security\ISecureRandom'), - $this->getMock('\OCP\IConfig') + 'server' => + [ + 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' + ] + ], + $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\IConfig') + ); + $this->middleware = $this->getMiddleware(false, false); + $this->urlGenerator + ->expects($this->once()) + ->method('getAbsoluteURL') + ->with('index.php') + ->will($this->returnValue('http://localhost/index.php')); + $this->logger + ->expects($this->once()) + ->method('debug') + ->with('Current user is not logged in'); + $response = $this->middleware->afterException( + $this->controller, + 'test', + new NotLoggedInException() ); - $this->middleware = $this->getMiddleware(true, true); - $response = $this->middleware->afterException($this->controller, 'test', - $this->secException); - $this->assertTrue($response instanceof RedirectResponse); - $this->assertEquals('?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp', $response->getRedirectURL()); + $expected = new RedirectResponse('http://localhost/index.php?redirect_url=owncloud%2Findex.php%2Fapps%2Fspecialapp'); + $this->assertEquals($expected , $response); + } + + /** + * @return array + */ + public function exceptionProvider() { + return [ + [ + new AppNotEnabledException(), + ], + [ + new CrossSiteRequestForgeryException(), + ], + [ + new NotAdminException(), + ], + ]; + } + + /** + * @dataProvider exceptionProvider + * @param SecurityException $exception + */ + public function testAfterExceptionReturnsTemplateResponse(SecurityException $exception) { + $this->request = new Request( + [ + 'server' => + [ + 'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' + ] + ], + $this->getMock('\OCP\Security\ISecureRandom'), + $this->getMock('\OCP\IConfig') + ); + $this->middleware = $this->getMiddleware(false, false); + $this->logger + ->expects($this->once()) + ->method('debug') + ->with($exception->getMessage()); + $response = $this->middleware->afterException( + $this->controller, + 'test', + $exception + ); + + $expected = new TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest'); + $expected->setStatus($exception->getCode()); + $this->assertEquals($expected , $response); }