diff --git a/lib/base.php b/lib/base.php index d779e8a01991..a5cee29d3a16 100644 --- a/lib/base.php +++ b/lib/base.php @@ -898,7 +898,6 @@ public static function handleRequest() { } else { // For guests: Load only filesystem and logging OC_App::loadApps(['filesystem', 'logging']); - self::handleLogin($request); } } diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index e5c84b8e1771..cd2d2c5c349b 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -353,8 +353,8 @@ public function __construct($appName, $urlParams = []){ $app->getServer()->getNavigationManager(), $app->getServer()->getURLGenerator(), $app->getServer()->getLogger(), + $app->getServer()->getUserSession(), $c['AppName'], - $app->isLoggedIn(), $app->isAdminUser(), $app->getServer()->getContentSecurityPolicyManager() ); diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index a9a3dc6a44c9..afb3ca8f8b33 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -46,6 +46,7 @@ use OCP\IRequest; use OCP\ILogger; use OCP\AppFramework\Controller; +use OCP\IUserSession; use OCP\Util; use OC\AppFramework\Middleware\Security\Exceptions\SecurityException; @@ -69,11 +70,11 @@ class SecurityMiddleware extends Middleware { /** @var ILogger */ private $logger; /** @var bool */ - private $isLoggedIn; - /** @var bool */ private $isAdminUser; /** @var ContentSecurityPolicyManager */ private $contentSecurityPolicyManager; + /** @var IUserSession */ + private $session; /** * @param IRequest $request @@ -81,8 +82,8 @@ class SecurityMiddleware extends Middleware { * @param INavigationManager $navigationManager * @param IURLGenerator $urlGenerator * @param ILogger $logger + * @param IUserSession $session * @param string $appName - * @param bool $isLoggedIn * @param bool $isAdminUser * @param ContentSecurityPolicyManager $contentSecurityPolicyManager */ @@ -91,8 +92,8 @@ public function __construct(IRequest $request, INavigationManager $navigationManager, IURLGenerator $urlGenerator, ILogger $logger, + IUserSession $session, $appName, - $isLoggedIn, $isAdminUser, ContentSecurityPolicyManager $contentSecurityPolicyManager) { $this->navigationManager = $navigationManager; @@ -101,7 +102,7 @@ public function __construct(IRequest $request, $this->appName = $appName; $this->urlGenerator = $urlGenerator; $this->logger = $logger; - $this->isLoggedIn = $isLoggedIn; + $this->session = $session; $this->isAdminUser = $isAdminUser; $this->contentSecurityPolicyManager = $contentSecurityPolicyManager; } @@ -124,7 +125,7 @@ public function beforeController($controller, $methodName) { // security checks $isPublicPage = $this->reflector->hasAnnotation('PublicPage'); if(!$isPublicPage) { - if(!$this->isLoggedIn) { + if(!$this->isLoggedIn()) { throw new NotLoggedInException(); } @@ -165,7 +166,7 @@ public function beforeController($controller, $methodName) { * @return Response */ public function afterController($controller, $methodName, Response $response) { - $policy = !is_null($response->getContentSecurityPolicy()) ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy(); + $policy = $response->getContentSecurityPolicy() !== null ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy(); $defaultPolicy = $this->contentSecurityPolicyManager->getDefaultPolicy(); $defaultPolicy = $this->contentSecurityPolicyManager->mergePolicies($defaultPolicy, $policy); @@ -229,4 +230,13 @@ public function afterException($controller, $methodName, \Exception $exception) throw $exception; } + private function isLoggedIn() { + static $loginCalled = false; + if (!$loginCalled && !$this->session->isLoggedIn()) { + \OC::handleLogin($this->request); + $loginCalled = true; + } + return $this->session->isLoggedIn(); + } + } diff --git a/lib/private/AppFramework/Utility/ControllerMethodReflector.php b/lib/private/AppFramework/Utility/ControllerMethodReflector.php index f6e195b382a3..da34317e8e5e 100644 --- a/lib/private/AppFramework/Utility/ControllerMethodReflector.php +++ b/lib/private/AppFramework/Utility/ControllerMethodReflector.php @@ -46,8 +46,9 @@ public function __construct() { /** - * @param object $object an object or classname + * @param object|string $object an object or classname * @param string $method the method which we want to inspect + * @throws \ReflectionException */ public function reflect($object, $method){ $reflection = new \ReflectionMethod($object, $method); diff --git a/lib/private/legacy/json.php b/lib/private/legacy/json.php index 8d4f62c7402b..65f1460d1d52 100644 --- a/lib/private/legacy/json.php +++ b/lib/private/legacy/json.php @@ -65,6 +65,12 @@ public static function checkAppEnabled($app) { * @deprecated Use annotation based ACLs from the AppFramework instead */ public static function checkLoggedIn() { + static $loginCalled = false; + if (!$loginCalled && !OC_User::isLoggedIn()) { + \OC::handleLogin(\OC::$server->getRequest()); + $loginCalled = true; + } + $twoFactorAuthManger = \OC::$server->getTwoFactorAuthManager(); if( !OC_User::isLoggedIn() || $twoFactorAuthManger->needsSecondFactor()) { diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 06e0f59f145d..a53b5968ede0 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -39,46 +39,67 @@ use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; use OCP\ISession; - - -class SecurityMiddlewareTest extends \Test\TestCase { - +use OCP\AppFramework\Controller; +use OCP\IUserSession; +use Test\TestCase; +use OCP\AppFramework\Http\Response; +use OCP\IConfig; +use OCP\Security\ISecureRandom; +use OC\Security\CSP\ContentSecurityPolicyManager; +use OCP\IRequest; +use OCP\IURLGenerator; +use OCP\INavigationManager; +use OCP\ILogger; + + +class SecurityMiddlewareTest extends TestCase { + + /** @var SecurityMiddleware */ private $middleware; + /** @var Controller | \PHPUnit_Framework_MockObject_MockObject */ private $controller; private $secException; private $secAjaxException; + /** @var IRequest | \PHPUnit_Framework_MockObject_MockObject */ private $request; + /** @var ControllerMethodReflector | \PHPUnit_Framework_MockObject_MockObject */ private $reader; + /** @var ILogger | \PHPUnit_Framework_MockObject_MockObject */ private $logger; + /** @var INavigationManager | \PHPUnit_Framework_MockObject_MockObject */ private $navigationManager; + /** @var IURLGenerator | \PHPUnit_Framework_MockObject_MockObject */ private $urlGenerator; + /** @var ContentSecurityPolicyManager | \PHPUnit_Framework_MockObject_MockObject */ private $contentSecurityPolicyManager; + /** @var IUserSession | \PHPUnit_Framework_MockObject_MockObject */ + private $session; protected function setUp() { parent::setUp(); - $this->controller = $this->getMockBuilder('OCP\AppFramework\Controller') + $this->controller = $this->getMockBuilder(Controller::class) ->disableOriginalConstructor() ->getMock(); $this->reader = new ControllerMethodReflector(); $this->logger = $this->getMockBuilder( - 'OCP\ILogger') + ILogger::class) ->disableOriginalConstructor() ->getMock(); $this->navigationManager = $this->getMockBuilder( - 'OCP\INavigationManager') + INavigationManager::class) ->disableOriginalConstructor() ->getMock(); $this->urlGenerator = $this->getMockBuilder( - 'OCP\IURLGenerator') + IURLGenerator::class) ->disableOriginalConstructor() ->getMock(); $this->request = $this->getMockBuilder( - 'OCP\IRequest') + IRequest::class) ->disableOriginalConstructor() ->getMock(); $this->contentSecurityPolicyManager = $this->getMockBuilder( - 'OC\Security\CSP\ContentSecurityPolicyManager') + ContentSecurityPolicyManager::class) ->disableOriginalConstructor() ->getMock(); $this->middleware = $this->getMiddleware(true, true); @@ -92,14 +113,16 @@ protected function setUp() { * @return SecurityMiddleware */ private function getMiddleware($isLoggedIn, $isAdminUser) { + $this->session = $this->createMock(IUserSession::class); + $this->session->expects($this->any())->method('isLoggedIn')->willReturn($isLoggedIn); return new SecurityMiddleware( $this->request, $this->reader, $this->navigationManager, $this->urlGenerator, $this->logger, + $this->session, 'files', - $isLoggedIn, $isAdminUser, $this->contentSecurityPolicyManager ); @@ -109,6 +132,8 @@ private function getMiddleware($isLoggedIn, $isAdminUser) { /** * @PublicPage * @NoCSRFRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testSetNavigationEntry(){ $this->navigationManager->expects($this->once()) @@ -123,6 +148,8 @@ public function testSetNavigationEntry(){ /** * @param string $method * @param string $test + * @param $status + * @throws \ReflectionException */ private function ajaxExceptionStatus($method, $test, $status) { $isLoggedIn = false; @@ -149,6 +176,9 @@ private function ajaxExceptionStatus($method, $test, $status) { } } + /** + * @throws \ReflectionException + */ public function testAjaxStatusLoggedInCheck() { $this->ajaxExceptionStatus( __FUNCTION__, @@ -159,6 +189,7 @@ public function testAjaxStatusLoggedInCheck() { /** * @NoCSRFRequired + * @throws \ReflectionException */ public function testAjaxNotAdminCheck() { $this->ajaxExceptionStatus( @@ -170,6 +201,7 @@ public function testAjaxNotAdminCheck() { /** * @PublicPage + * @throws \ReflectionException */ public function testAjaxStatusCSRFCheck() { $this->ajaxExceptionStatus( @@ -182,6 +214,10 @@ public function testAjaxStatusCSRFCheck() { /** * @PublicPage * @NoCSRFRequired + * @throws \ReflectionException + * @throws \ReflectionException + * @throws \ReflectionException + * @throws \ReflectionException */ public function testAjaxStatusAllGood() { $this->ajaxExceptionStatus( @@ -210,6 +246,8 @@ public function testAjaxStatusAllGood() { /** * @PublicPage * @NoCSRFRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testNoChecks(){ $this->request->expects($this->never()) @@ -226,6 +264,9 @@ public function testNoChecks(){ /** * @param string $method * @param string $expects + * @param bool $shouldFail + * @throws SecurityException + * @throws \ReflectionException */ private function securityCheck($method, $expects, $shouldFail=false){ // admin check requires login @@ -240,7 +281,7 @@ private function securityCheck($method, $expects, $shouldFail=false){ $sec = $this->getMiddleware($isLoggedIn, $isAdminUser); if($shouldFail) { - $this->setExpectedException('\OC\AppFramework\Middleware\Security\Exceptions\SecurityException'); + $this->expectException(SecurityException::class); } else { $this->assertTrue(true); } @@ -253,6 +294,8 @@ private function securityCheck($method, $expects, $shouldFail=false){ /** * @PublicPage * @expectedException \OC\AppFramework\Middleware\Security\Exceptions\CrossSiteRequestForgeryException + * @throws SecurityException + * @throws \ReflectionException */ public function testCsrfCheck(){ $this->request->expects($this->once()) @@ -267,6 +310,8 @@ public function testCsrfCheck(){ /** * @PublicPage * @NoCSRFRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testNoCsrfCheck(){ $this->request->expects($this->never()) @@ -280,6 +325,8 @@ public function testNoCsrfCheck(){ /** * @PublicPage + * @throws SecurityException + * @throws \ReflectionException */ public function testFailCsrfCheck(){ $this->request->expects($this->once()) @@ -294,6 +341,8 @@ public function testFailCsrfCheck(){ /** * @NoCSRFRequired * @NoAdminRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testLoggedInCheck(){ $this->securityCheck(__FUNCTION__, 'isLoggedIn'); @@ -303,6 +352,8 @@ public function testLoggedInCheck(){ /** * @NoCSRFRequired * @NoAdminRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testFailLoggedInCheck(){ $this->securityCheck(__FUNCTION__, 'isLoggedIn', true); @@ -311,6 +362,8 @@ public function testFailLoggedInCheck(){ /** * @NoCSRFRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testIsAdminCheck(){ $this->securityCheck(__FUNCTION__, 'isAdminUser'); @@ -319,18 +372,26 @@ public function testIsAdminCheck(){ /** * @NoCSRFRequired + * @throws SecurityException + * @throws \ReflectionException */ public function testFailIsAdminCheck(){ $this->securityCheck(__FUNCTION__, 'isAdminUser', true); } + /** + * @throws \Exception + */ public function testAfterExceptionNotCaughtThrowsItAgain(){ $ex = new \Exception(); - $this->setExpectedException('\Exception'); + $this->expectException(\Exception::class); $this->middleware->afterException($this->controller, 'test', $ex); } + /** + * @throws \Exception + */ public function testAfterExceptionReturnsRedirectForNotLoggedInUser() { $this->request = new Request( [ @@ -340,8 +401,8 @@ public function testAfterExceptionReturnsRedirectForNotLoggedInUser() { 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' ] ], - $this->createMock('\OCP\Security\ISecureRandom'), - $this->createMock('\OCP\IConfig') + $this->createMock(ISecureRandom::class), + $this->createMock(IConfig::class) ); $this->middleware = $this->getMiddleware(false, false); $this->urlGenerator @@ -388,6 +449,7 @@ public function exceptionProvider() { /** * @dataProvider exceptionProvider * @param SecurityException $exception + * @throws \Exception */ public function testAfterExceptionReturnsTemplateResponse(SecurityException $exception) { $this->request = new Request( @@ -398,8 +460,8 @@ public function testAfterExceptionReturnsTemplateResponse(SecurityException $exc 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' ] ], - $this->createMock('\OCP\Security\ISecureRandom'), - $this->createMock('\OCP\IConfig') + $this->createMock(ISecureRandom::class), + $this->createMock(IConfig::class) ); $this->middleware = $this->getMiddleware(false, false); $this->logger @@ -417,6 +479,9 @@ public function testAfterExceptionReturnsTemplateResponse(SecurityException $exc $this->assertEquals($expected , $response); } + /** + * @throws \Exception + */ public function testAfterExceptionReturnsLoginPageForCsrfErrorOnLogin() { $this->request = new Request( [ @@ -426,8 +491,8 @@ public function testAfterExceptionReturnsLoginPageForCsrfErrorOnLogin() { 'REQUEST_URI' => 'owncloud/index.php/apps/specialapp' ] ], - $this->createMock('\OCP\Security\ISecureRandom'), - $this->createMock('\OCP\IConfig') + $this->createMock(ISecureRandom::class), + $this->createMock(IConfig::class) ); $exception = new CrossSiteRequestForgeryException(); @@ -462,6 +527,9 @@ public function testAfterExceptionReturnsLoginPageForCsrfErrorOnLogin() { } + /** + * @throws \Exception + */ public function testAfterAjaxExceptionReturnsJSONError(){ $response = $this->middleware->afterException($this->controller, 'test', $this->secAjaxException); @@ -470,7 +538,8 @@ public function testAfterAjaxExceptionReturnsJSONError(){ } public function testAfterController() { - $response = $this->getMockBuilder('\OCP\AppFramework\Http\Response')->disableOriginalConstructor()->getMock(); + /** @var Response | \PHPUnit_Framework_MockObject_MockObject $response */ + $response = $this->getMockBuilder(Response::class)->disableOriginalConstructor()->getMock(); $defaultPolicy = new ContentSecurityPolicy(); $defaultPolicy->addAllowedImageDomain('defaultpolicy'); $currentPolicy = new ContentSecurityPolicy();