Merge pull request #8759 from owncloud/fix-phpunit

fix 8757, get rid of service locator antipattern

lib/private/appframework/dependencyinjection/dicontainer.php

@@ -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['ControllerMethodReflector'],
+				$app->getServer()->getNavigationManager(),
+				$app->getServer()->getURLGenerator(),
+				$app->getServer()->getLogger(),
+				$c['AppName'],
+				$app->isLoggedIn(),
+				$app->isAdminUser()
 			);
 		});
 

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 {
 
-	/**
+	private $navigationManager;
-	 * @var \OCP\AppFramework\IAppContainer
-	 */
-	private $app;
-
-	/**
-	 * @var \OCP\IRequest
-	 */
 	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,
+	public function __construct(IRequest $request,
-	                            ControllerMethodReflector $reflector){
+	                            ControllerMethodReflector $reflector,
-		$this->app = $app;
+	                            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;

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'))));
 	}
 
 

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',
+		$this->api = $this->getMockBuilder(
-					array(), array('test'));
+				'OC\AppFramework\DependencyInjection\DIContainer')
+				->disableOriginalConstructor()
+				->getMock();
 
 		$this->controller = $this->getMock('OCP\AppFramework\Controller',
 				array(), array($this->api, new Request()));

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->getMockBuilder('OCP\AppFramework\Controller')
-		$this->controller = $this->getMock('OCP\AppFramework\Controller',
+			->disableOriginalConstructor()
-				array(), array($api, new Request()));
+				->getMock();
 		$this->reader = new ControllerMethodReflector();
-
+		$this->logger = $this->getMockBuilder(
-		$this->request = new Request();
+				'OCP\ILogger')
-		$this->middleware = new SecurityMiddleware($api, $this->request, $this->reader);
+				->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(){
+	private function getMiddleware($isLoggedIn, $isAdminUser){
-		return $this->getMock('OC\AppFramework\DependencyInjection\DIContainer',
+		return new SecurityMiddleware(
-					array('isLoggedIn', 'passesCSRFCheck', 'isAdminUser',
+			$this->request,
-							'isSubAdminUser', 'getUserId'),
+			$this->reader,
-					array('app'));
+			$this->navigationManager,
-	}
+			$this->urlGenerator,
-
+			$this->logger,
-
+			'test',
-	/**
+			$isLoggedIn,
-	 * @param string $method
+			$isAdminUser
-	 */
+		);
-	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);
 	}
 
 
@@ -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();
+		$isLoggedIn = false;
-		$api->expects($this->any())
+		$isAdminUser = false;
-				->method($test)
-				->will($this->returnValue(false));
 
 		// isAdminUser requires isLoggedIn call to return true
 		if ($test === 'isAdminUser') {
-			$api->expects($this->any())
+			$isLoggedIn = true;
-				->method('isLoggedIn')
-				->will($this->returnValue(true));
 		}
 
-		$sec = new SecurityMiddleware($api, $this->request, $this->reader);
+		$sec = $this->getMiddleware($isLoggedIn, $isAdminUser);
 
 		try {
-			$controller = '\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest';
+			$this->reader->reflect(__CLASS__, $method);
-			$this->reader->reflect($controller, $method);
+			$sec->beforeController(__CLASS__, $method);
-			$sec->beforeController($controller,	$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();
+		$this->request->expects($this->never())
-		$api->expects($this->never())
 				->method('passesCSRFCheck')
-				->will($this->returnValue(true));
+				->will($this->returnValue(false));
-		$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);
+		$sec = $this->getMiddleware(false, false);
-		$this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest',
+
-				'testNoChecks');
+		$this->reader->reflect(__CLASS__, __FUNCTION__);
-		$sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest',
+		$sec->beforeController(__CLASS__, __FUNCTION__);
-				'testNoChecks');
 	}
 
 
@@ -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())
+			$isLoggedIn = true;
-				->method('isLoggedIn')
+			$isAdminUser = !$shouldFail;
-				->will($this->returnValue(true));
+		} 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);
+		$this->reader->reflect(__CLASS__, $method);
-		$sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', $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();
+		$this->request->expects($this->once())
-		$request = $this->getMock('OC\AppFramework\Http\Request', array('passesCSRFCheck'));
-		$request->expects($this->once())
 			->method('passesCSRFCheck')
 			->will($this->returnValue(false));
 
-		$sec = new SecurityMiddleware($api, $request, $this->reader);
+		$this->reader->reflect(__CLASS__, __FUNCTION__);
-		$this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testCsrfCheck');
+		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
-		$sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testCsrfCheck');
 	}
 
 
@@ -249,15 +247,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 	 * @NoCSRFRequired
 	 */
 	public function testNoCsrfCheck(){
-		$api = $this->getAPI();
+		$this->request->expects($this->never())
-		$request = $this->getMock('OC\AppFramework\Http\Request', array('passesCSRFCheck'));
-		$request->expects($this->never())
 			->method('passesCSRFCheck')
 			->will($this->returnValue(false));
 
-		$sec = new SecurityMiddleware($api, $request, $this->reader);
+		$this->reader->reflect(__CLASS__, __FUNCTION__);
-		$this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testNoCsrfCheck');
+		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
-		$sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testNoCsrfCheck');
 	}
 
 
@@ -265,15 +260,12 @@ class SecurityMiddlewareTest extends \PHPUnit_Framework_TestCase {
 	 * @PublicPage
 	 */
 	public function testFailCsrfCheck(){
-		$api = $this->getAPI();
+		$this->request->expects($this->once())
-		$request = $this->getMock('OC\AppFramework\Http\Request', array('passesCSRFCheck'));
-		$request->expects($this->once())
 			->method('passesCSRFCheck')
 			->will($this->returnValue(true));
 
-		$sec = new SecurityMiddleware($api, $request, $this->reader);
+		$this->reader->reflect(__CLASS__, __FUNCTION__);
-		$this->reader->reflect('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testFailCsrfCheck');
+		$this->middleware->beforeController(__CLASS__, __FUNCTION__);
-		$sec->beforeController('\OC\AppFramework\Middleware\Security\SecurityMiddlewareTest', 'testFailCsrfCheck');
 	}
 
 
@@ -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')));
+			array('server' =>
-		$this->middleware = new SecurityMiddleware($api, $this->request, $this->reader);
+				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);