[Pkg-owncloud-commits] [owncloud] 76/199: fix 8757, get rid of service locator antipattern
David Prévot
taffit at moszumanska.debian.org
Sun Jun 1 18:53:11 UTC 2014
This is an automated email from the git hooks/post-receive script.
taffit pushed a commit to branch master
in repository owncloud.
commit 5e9ea2b3659eebf6d21c253771e477abe16496c9
Author: Bernhard Posselt <dev at bernhard-posselt.com>
Date: Wed May 28 02:12:01 2014 +0200
fix 8757, get rid of service locator antipattern
---
.../dependencyinjection/dicontainer.php | 15 +-
.../middleware/security/securitymiddleware.php | 58 ++++---
.../middleware/MiddlewareDispatcherTest.php | 8 +-
.../lib/appframework/middleware/MiddlewareTest.php | 6 +-
.../middleware/security/SecurityMiddlewareTest.php | 176 +++++++++------------
5 files changed, 129 insertions(+), 134 deletions(-)
diff --git a/lib/private/appframework/dependencyinjection/dicontainer.php b/lib/private/appframework/dependencyinjection/dicontainer.php
index ee492b8..61a2333 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 d7e398f..5b56210 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 b1a58e2..b1e221a 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 814efdd..9d952f6 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 6a1bbf7..ae0b05b 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,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);
--
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-owncloud/owncloud.git
More information about the Pkg-owncloud-commits
mailing list