Otherwise this leads to an endless redirection in case of a CSRF exception. Also sets user expectation right.tags/v9.0beta1
@@ -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; |
@@ -0,0 +1,36 @@ | |||
<?php | |||
/** | |||
* @author Lukas Reschke <lukas@owncloud.com> | |||
* | |||
* @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 <http://www.gnu.org/licenses/> | |||
* | |||
*/ | |||
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); | |||
} | |||
} |
@@ -0,0 +1,36 @@ | |||
<?php | |||
/** | |||
* @author Lukas Reschke <lukas@owncloud.com> | |||
* | |||
* @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 <http://www.gnu.org/licenses/> | |||
* | |||
*/ | |||
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); | |||
} | |||
} |
@@ -0,0 +1,36 @@ | |||
<?php | |||
/** | |||
* @author Lukas Reschke <lukas@owncloud.com> | |||
* | |||
* @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 <http://www.gnu.org/licenses/> | |||
* | |||
*/ | |||
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); | |||
} | |||
} |
@@ -0,0 +1,36 @@ | |||
<?php | |||
/** | |||
* @author Lukas Reschke <lukas@owncloud.com> | |||
* | |||
* @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 <http://www.gnu.org/licenses/> | |||
* | |||
*/ | |||
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); | |||
} | |||
} |
@@ -1,7 +1,6 @@ | |||
<?php | |||
/** | |||
* @author Morris Jobke <hey@morrisjobke.de> | |||
* @author Thomas Müller <thomas.mueller@tmit.eu> | |||
* @author Lukas Reschke <lukas@owncloud.com> | |||
* | |||
* @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 {} |
@@ -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; |
@@ -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( |
@@ -1,34 +1,40 @@ | |||
<?php | |||
/** | |||
* ownCloud - App Framework | |||
* @author Bernhard Posselt <dev@bernhard-posselt.com> | |||
* @author Lukas Reschke <lukas@owncloud.com> | |||
* | |||
* @author Bernhard Posselt | |||
* @copyright 2012 Bernhard Posselt <dev@bernhard-posselt.com> | |||
* @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 <http://www.gnu.org/licenses/>. | |||
* You should have received a copy of the GNU Affero General Public License, version 3, | |||
* along with this program. If not, see <http://www.gnu.org/licenses/> | |||
* | |||
*/ | |||
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' | |||
] | |||
'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() | |||
); | |||
$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(), | |||
], | |||
$this->getMock('\OCP\Security\ISecureRandom'), | |||
$this->getMock('\OCP\IConfig') | |||
[ | |||
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 | |||
); | |||
$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 TemplateResponse('core', '403', ['file' => $exception->getMessage()], 'guest'); | |||
$expected->setStatus($exception->getCode()); | |||
$this->assertEquals($expected , $response); | |||
} | |||