diff options
5 files changed, 143 insertions, 143 deletions
diff --git a/lib/private/appframework/dependencyinjection/dicontainer.php b/lib/private/appframework/dependencyinjection/dicontainer.php index ee492b8a9e5..61a2333ecee 100644 --- a/lib/private/appframework/dependencyinjection/dicontainer.php +++ b/lib/private/appframework/dependencyinjection/dicontainer.php @@ -83,8 +83,8 @@ class DIContainer extends SimpleContainer implements IAppContainer{ $this['Dispatcher'] = $this->share(function($c) { return new Dispatcher( - $c['Protocol'], - $c['MiddlewareDispatcher'], + $c['Protocol'], + $c['MiddlewareDispatcher'], $c['ControllerMethodReflector'], $c['Request'] ); @@ -97,9 +97,14 @@ class DIContainer extends SimpleContainer implements IAppContainer{ $app = $this; $this['SecurityMiddleware'] = $this->share(function($c) use ($app){ return new SecurityMiddleware( - $app, - $c['Request'], - $c['ControllerMethodReflector'] + $c['Request'], + $c['ControllerMethodReflector'], + $app->getServer()->getNavigationManager(), + $app->getServer()->getURLGenerator(), + $app->getServer()->getLogger(), + $c['AppName'], + $app->isLoggedIn(), + $app->isAdminUser() ); }); diff --git a/lib/private/appframework/middleware/security/securitymiddleware.php b/lib/private/appframework/middleware/security/securitymiddleware.php index d7e398fe445..5b56210024d 100644 --- a/lib/private/appframework/middleware/security/securitymiddleware.php +++ b/lib/private/appframework/middleware/security/securitymiddleware.php @@ -30,8 +30,10 @@ use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Middleware; use OCP\AppFramework\Http\Response; use OCP\AppFramework\Http\JSONResponse; -use OCP\AppFramework\IAppContainer; +use OCP\INavigationManager; +use OCP\IURLGenerator; use OCP\IRequest; +use OCP\ILogger; /** @@ -42,31 +44,41 @@ use OCP\IRequest; */ class SecurityMiddleware extends Middleware { - /** - * @var \OCP\AppFramework\IAppContainer - */ - private $app; - - /** - * @var \OCP\IRequest - */ + private $navigationManager; private $request; - - /** - * @var OC\AppFramework\Utility\ControllerMethodReflector - */ private $reflector; + private $appName; + private $urlGenerator; + private $logger; + private $isLoggedIn; + private $isAdminUser; /** - * @param IAppContainer $app * @param IRequest $request * @param ControllerMethodReflector $reflector + * @param INavigationManager $navigationManager + * @param IURLGenerator $urlGenerator + * @param ILogger $logger + * @param string $appName + * @param bool $isLoggedIn + * @param bool $isAdminUser */ - public function __construct(IAppContainer $app, IRequest $request, - ControllerMethodReflector $reflector){ - $this->app = $app; + public function __construct(IRequest $request, + ControllerMethodReflector $reflector, + INavigationManager $navigationManager, + IURLGenerator $urlGenerator, + ILogger $logger, + $appName, + $isLoggedIn, + $isAdminUser){ + $this->navigationManager = $navigationManager; $this->request = $request; $this->reflector = $reflector; + $this->appName = $appName; + $this->urlGenerator = $urlGenerator; + $this->logger = $logger; + $this->isLoggedIn = $isLoggedIn; + $this->isAdminUser = $isAdminUser; } @@ -82,17 +94,17 @@ class SecurityMiddleware extends Middleware { // this will set the current navigation entry of the app, use this only // for normal HTML requests and not for AJAX requests - $this->app->getServer()->getNavigationManager()->setActiveEntry($this->app->getAppName()); + $this->navigationManager->setActiveEntry($this->appName); // security checks $isPublicPage = $this->reflector->hasAnnotation('PublicPage'); if(!$isPublicPage) { - if(!$this->app->isLoggedIn()) { + if(!$this->isLoggedIn) { throw new SecurityException('Current user is not logged in', Http::STATUS_UNAUTHORIZED); } if(!$this->reflector->hasAnnotation('NoAdminRequired')) { - if(!$this->app->isAdminUser()) { + if(!$this->isAdminUser) { throw new SecurityException('Logged in user must be an admin', Http::STATUS_FORBIDDEN); } } @@ -126,13 +138,13 @@ class SecurityMiddleware extends Middleware { array('message' => $exception->getMessage()), $exception->getCode() ); - $this->app->log($exception->getMessage(), 'debug'); + $this->logger->debug($exception->getMessage()); } else { // TODO: replace with link to route - $url = $this->app->getServer()->getURLGenerator()->getAbsoluteURL('index.php'); + $url = $this->urlGenerator->getAbsoluteURL('index.php'); $response = new RedirectResponse($url); - $this->app->log($exception->getMessage(), 'debug'); + $this->logger->debug($exception->getMessage()); } return $response; diff --git a/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php b/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php index b1a58e21289..b1e221aab99 100644 --- a/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php +++ b/tests/lib/appframework/middleware/MiddlewareDispatcherTest.php @@ -124,15 +124,9 @@ class MiddlewareDispatcherTest extends \PHPUnit_Framework_TestCase { } - private function getAPIMock(){ - return $this->getMock('OC\AppFramework\DependencyInjection\DIContainer', - array('getAppName'), array('app')); - } - - private function getControllerMock(){ return $this->getMock('OCP\AppFramework\Controller', array('method'), - array($this->getAPIMock(), new Request(array('method' => 'GET')))); + array('app', new Request(array('method' => 'GET')))); } diff --git a/tests/lib/appframework/middleware/MiddlewareTest.php b/tests/lib/appframework/middleware/MiddlewareTest.php index 814efdd8118..9d952f61573 100644 --- a/tests/lib/appframework/middleware/MiddlewareTest.php +++ b/tests/lib/appframework/middleware/MiddlewareTest.php @@ -44,8 +44,10 @@ class MiddlewareTest extends \PHPUnit_Framework_TestCase { protected function setUp(){ $this->middleware = new ChildMiddleware(); - $this->api = $this->getMock('OC\AppFramework\DependencyInjection\DIContainer', - array(), array('test')); + $this->api = $this->getMockBuilder( + 'OC\AppFramework\DependencyInjection\DIContainer') + ->disableOriginalConstructor() + ->getMock(); $this->controller = $this->getMock('OCP\AppFramework\Controller', array(), array($this->api, new Request())); diff --git a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php index 6a1bbf72c13..47556ca9542 100644 --- a/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php +++ b/tests/lib/appframework/middleware/security/SecurityMiddlewareTest.php @@ -39,41 +39,48 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { private $secAjaxException; private $request; private $reader; + private $logger; + private $navigationManager; + private $urlGenerator; public function setUp() { - $api = $this->getMock('OC\AppFramework\DependencyInjection\DIContainer', array(), array('test')); - $this->controller = $this->getMock('OCP\AppFramework\Controller', - array(), array($api, new Request())); + $this->controller = $this->getMockBuilder('OCP\AppFramework\Controller') + ->disableOriginalConstructor() + ->getMock(); $this->reader = new ControllerMethodReflector(); - - $this->request = new Request(); - $this->middleware = new SecurityMiddleware($api, $this->request, $this->reader); + $this->logger = $this->getMockBuilder( + 'OCP\ILogger') + ->disableOriginalConstructor() + ->getMock(); + $this->navigationManager = $this->getMockBuilder( + 'OCP\INavigationManager') + ->disableOriginalConstructor() + ->getMock(); + $this->urlGenerator = $this->getMockBuilder( + 'OCP\IURLGenerator') + ->disableOriginalConstructor() + ->getMock(); + $this->request = $this->getMockBuilder( + 'OCP\IRequest') + ->disableOriginalConstructor() + ->getMock(); + $this->middleware = $this->getMiddleware(true, true); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); } - private function getAPI(){ - return $this->getMock('OC\AppFramework\DependencyInjection\DIContainer', - array('isLoggedIn', 'passesCSRFCheck', 'isAdminUser', - 'isSubAdminUser', 'getUserId'), - array('app')); - } - - - /** - * @param string $method - */ - private function checkNavEntry($method){ - $api = $this->getAPI(); - - $serverMock = $this->getMock('\OC\Server', array()); - $api->expects($this->any())->method('getServer') - ->will($this->returnValue($serverMock)); - - $sec = new SecurityMiddleware($api, $this->request, $this->reader); - $this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', $method); - $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', $method); + private function getMiddleware($isLoggedIn, $isAdminUser){ + return new SecurityMiddleware( + $this->request, + $this->reader, + $this->navigationManager, + $this->urlGenerator, + $this->logger, + 'test', + $isLoggedIn, + $isAdminUser + ); } @@ -82,7 +89,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoCSRFRequired */ public function testSetNavigationEntry(){ - $this->checkNavEntry('testSetNavigationEntry'); + $this->navigationManager->expects($this->once()) + ->method('setActiveEntry') + ->with($this->equalTo('test')); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); } @@ -91,32 +103,33 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @param string $test */ private function ajaxExceptionStatus($method, $test, $status) { - $api = $this->getAPI(); - $api->expects($this->any()) - ->method($test) - ->will($this->returnValue(false)); + $isLoggedIn = false; + $isAdminUser = false; // isAdminUser requires isLoggedIn call to return true if ($test === 'isAdminUser') { - $api->expects($this->any()) - ->method('isLoggedIn') - ->will($this->returnValue(true)); + $isLoggedIn = true; } - $sec = new SecurityMiddleware($api, $this->request, $this->reader); + $sec = $this->getMiddleware($isLoggedIn, $isAdminUser); try { - $controller = '\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest'; - $this->reader->reflect($controller, $method); - $sec->beforeController($controller, $method); + $this->reader->reflect(__CLASS__, $method); + $sec->beforeController(__CLASS__, $method); } catch (SecurityException $ex){ $this->assertEquals($status, $ex->getCode()); } + + // add assertion if everything should work fine otherwise phpunit will + // complain + if ($status === 0) { + $this->assertTrue(true); + } } public function testAjaxStatusLoggedInCheck() { $this->ajaxExceptionStatus( - 'testAjaxStatusLoggedInCheck', + __FUNCTION__, 'isLoggedIn', Http::STATUS_UNAUTHORIZED ); @@ -124,11 +137,10 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { /** * @NoCSRFRequired - * @NoAdminRequired */ public function testAjaxNotAdminCheck() { $this->ajaxExceptionStatus( - 'testAjaxNotAdminCheck', + __FUNCTION__, 'isAdminUser', Http::STATUS_FORBIDDEN ); @@ -139,7 +151,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { */ public function testAjaxStatusCSRFCheck() { $this->ajaxExceptionStatus( - 'testAjaxStatusCSRFCheck', + __FUNCTION__, 'passesCSRFCheck', Http::STATUS_PRECONDITION_FAILED ); @@ -151,22 +163,22 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { */ public function testAjaxStatusAllGood() { $this->ajaxExceptionStatus( - 'testAjaxStatusAllGood', + __FUNCTION__, 'isLoggedIn', 0 ); $this->ajaxExceptionStatus( - 'testAjaxStatusAllGood', + __FUNCTION__, 'isAdminUser', 0 ); $this->ajaxExceptionStatus( - 'testAjaxStatusAllGood', + __FUNCTION__, 'isSubAdminUser', 0 ); $this->ajaxExceptionStatus( - 'testAjaxStatusAllGood', + __FUNCTION__, 'passesCSRFCheck', 0 ); @@ -178,22 +190,14 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoCSRFRequired */ public function testNoChecks(){ - $api = $this->getAPI(); - $api->expects($this->never()) + $this->request->expects($this->never()) ->method('passesCSRFCheck') - ->will($this->returnValue(true)); - $api->expects($this->never()) - ->method('isAdminUser') - ->will($this->returnValue(true)); - $api->expects($this->never()) - ->method('isLoggedIn') - ->will($this->returnValue(true)); - - $sec = new SecurityMiddleware($api, $this->request, $this->reader); - $this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', - 'testNoChecks'); - $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', - 'testNoChecks'); + ->will($this->returnValue(false)); + + $sec = $this->getMiddleware(false, false); + + $this->reader->reflect(__CLASS__, __FUNCTION__); + $sec->beforeController(__CLASS__, __FUNCTION__); } @@ -202,28 +206,25 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @param string $expects */ private function securityCheck($method, $expects, $shouldFail=false){ - $api = $this->getAPI(); - $api->expects($this->once()) - ->method($expects) - ->will($this->returnValue(!$shouldFail)); - // admin check requires login if ($expects === 'isAdminUser') { - $api->expects($this->once()) - ->method('isLoggedIn') - ->will($this->returnValue(true)); + $isLoggedIn = true; + $isAdminUser = !$shouldFail; + } else { + $isLoggedIn = !$shouldFail; + $isAdminUser = false; } - $sec = new SecurityMiddleware($api, $this->request, $this->reader); + $sec = $this->getMiddleware($isLoggedIn, $isAdminUser); if($shouldFail){ $this->setExpectedException('\OC\AppFramework\Middleware\Security\SecurityException'); } else { - $this->setExpectedException(null); + $this->assertTrue(true); } - $this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', $method); - $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', $method); + $this->reader->reflect(__CLASS__, $method); + $sec->beforeController(__CLASS__, $method); } @@ -232,15 +233,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @expectedException \OC\AppFramework\Middleware\Security\SecurityException */ public function testCsrfCheck(){ - $api = $this->getAPI(); - $request = $this->getMock('OC\AppFramework\Http\Request', array('passesCSRFCheck')); - $request->expects($this->once()) + $this->request->expects($this->once()) ->method('passesCSRFCheck') ->will($this->returnValue(false)); - $sec = new SecurityMiddleware($api, $request, $this->reader); - $this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testCsrfCheck'); - $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testCsrfCheck'); + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); } @@ -249,15 +247,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoCSRFRequired */ public function testNoCsrfCheck(){ - $api = $this->getAPI(); - $request = $this->getMock('OC\AppFramework\Http\Request', array('passesCSRFCheck')); - $request->expects($this->never()) + $this->request->expects($this->never()) ->method('passesCSRFCheck') ->will($this->returnValue(false)); - $sec = new SecurityMiddleware($api, $request, $this->reader); - $this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testNoCsrfCheck'); - $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testNoCsrfCheck'); + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); } @@ -265,15 +260,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @PublicPage */ public function testFailCsrfCheck(){ - $api = $this->getAPI(); - $request = $this->getMock('OC\AppFramework\Http\Request', array('passesCSRFCheck')); - $request->expects($this->once()) + $this->request->expects($this->once()) ->method('passesCSRFCheck') ->will($this->returnValue(true)); - $sec = new SecurityMiddleware($api, $request, $this->reader); - $this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testFailCsrfCheck'); - $sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testFailCsrfCheck'); + $this->reader->reflect(__CLASS__, __FUNCTION__); + $this->middleware->beforeController(__CLASS__, __FUNCTION__); } @@ -282,7 +274,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoAdminRequired */ public function testLoggedInCheck(){ - $this->securityCheck('testLoggedInCheck', 'isLoggedIn'); + $this->securityCheck(__FUNCTION__, 'isLoggedIn'); } @@ -291,7 +283,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoAdminRequired */ public function testFailLoggedInCheck(){ - $this->securityCheck('testFailLoggedInCheck', 'isLoggedIn', true); + $this->securityCheck(__FUNCTION__, 'isLoggedIn', true); } @@ -299,7 +291,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoCSRFRequired */ public function testIsAdminCheck(){ - $this->securityCheck('testIsAdminCheck', 'isAdminUser'); + $this->securityCheck(__FUNCTION__, 'isAdminUser'); } @@ -307,7 +299,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { * @NoCSRFRequired */ public function testFailIsAdminCheck(){ - $this->securityCheck('testFailIsAdminCheck', 'isAdminUser', true); + $this->securityCheck(__FUNCTION__, 'isAdminUser', true); } @@ -319,17 +311,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase { public function testAfterExceptionReturnsRedirect(){ - $api = $this->getMock('OC\AppFramework\DependencyInjection\DIContainer', array(), array('test')); - $serverMock = $this->getMock('\OC\Server', array('getNavigationManager')); - $api->expects($this->once())->method('getServer') - ->will($this->returnValue($serverMock)); - - $this->controller = $this->getMock('OCP\AppFramework\Controller', - array(), array($api, new Request())); - $this->request = new Request( - array('server' => array('HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8'))); - $this->middleware = new SecurityMiddleware($api, $this->request, $this->reader); + array('server' => + array('HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8') + ) + ); + $this->middleware = $this->getMiddleware(true, true); $response = $this->middleware->afterException($this->controller, 'test', $this->secException); |