fix 8757, get rid of service locator antipattern
This commit is contained in:
parent
4e2cdb2f8a
commit
5e9ea2b365
|
@ -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()
|
||||
);
|
||||
});
|
||||
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -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'))));
|
||||
}
|
||||
|
||||
|
||||
|
|
|
@ -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()));
|
||||
|
|
|
@ -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));
|
||||
->will($this->returnValue(false));
|
||||
|
||||
$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');
|
||||
$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);
|
||||
|
||||
|
|
Loading…
Reference in New Issue