fix 8757, get rid of service locator antipattern

diff --git a/lib/private/appframework/dependencyinjection/dicontainer.php b/lib/private/appframework/dependencyinjection/dicontainer.php
index ee492b8a9e5e13ddc229d291de4b940d2bd7a58d..61a2333eceeb38d4ddad732ebd50098b869d2fc5 100644 (file)
--- 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 d7e398fe4454a6274c6c54817b6145a998ed18ec..5b56210024dbe2eef3c81b760002c291d03b875d 100644 (file)
--- 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 b1a58e212893d13de8934d67846ae982ddf38d56..b1e221aab99d8a6e6bfd55332c7ed145616612a3 100644 (file)
--- 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 814efdd8118bc64aadff0933babcad847addb4b2..9d952f615734afbc0cb586bbc1fac22ed06c0ef4 100644 (file)
--- 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 6a1bbf72c1393fe0725cc043f8553a76aaf6fad8..ae0b05bdb3c43af4538b3d82de5e1287ae5faa06 100644 (file)
--- 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,24 +103,19 @@ 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());
                }
@@ -178,22 +185,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,19 +201,16 @@ 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');
@@ -222,8 +218,8 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
                        $this->setExpectedException(null);
                }
 
-               $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 +228,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 +242,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 +255,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 +269,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
         * @NoAdminRequired
         */
        public function testLoggedInCheck(){
-               $this->securityCheck('testLoggedInCheck', 'isLoggedIn');
+               $this->securityCheck(__FUNCTION__, 'isLoggedIn');
        }
 
 
@@ -291,7 +278,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
         * @NoAdminRequired
         */
        public function testFailLoggedInCheck(){
-               $this->securityCheck('testFailLoggedInCheck', 'isLoggedIn', true);
+               $this->securityCheck(__FUNCTION__, 'isLoggedIn', true);
        }
 
 
@@ -299,7 +286,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
         * @NoCSRFRequired
         */
        public function testIsAdminCheck(){
-               $this->securityCheck('testIsAdminCheck', 'isAdminUser');
+               $this->securityCheck(__FUNCTION__, 'isAdminUser');
        }
 
 
@@ -307,7 +294,7 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
         * @NoCSRFRequired
         */
        public function testFailIsAdminCheck(){
-               $this->securityCheck('testFailIsAdminCheck', 'isAdminUser', true);
+               $this->securityCheck(__FUNCTION__, 'isAdminUser', true);
        }
 
 
@@ -319,17 +306,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);