From 7da08121868ce6922151e13246f82e8935a6cc51 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Wed, 28 Feb 2018 20:26:03 +0100 Subject: [PATCH 1/3] Do not throw AppNotEnabledException for app public pages - refs #6962, refs #5309 It allows non-logged user to access public pages of applications restricted to a group Signed-off-by: Julien Veyssier --- .../AppFramework/Middleware/Security/SecurityMiddleware.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index bb3083c835c4f..4af39c99db3de 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -195,8 +195,9 @@ public function beforeController($controller, $methodName) { * Checks if app is enabled (also includes a check whether user is allowed to access the resource) * The getAppPath() check is here since components such as settings also use the AppFramework and * therefore won't pass this check. + * If page is public, app does not need to be enabled for current user/visitor */ - if(\OC_App::getAppPath($this->appName) !== false && !$this->appManager->isEnabledForUser($this->appName)) { + if(\OC_App::getAppPath($this->appName) !== false && !$isPublicPage && !$this->appManager->isEnabledForUser($this->appName)) { throw new AppNotEnabledException(); } From 340e8ef16ced722ae97e6175b82f3010772a2550 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 8 Mar 2018 10:11:47 +0100 Subject: [PATCH 2/3] Make SecurityMiddleware strict Signed-off-by: Roeland Jago Douma --- .../Security/SecurityMiddleware.php | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 4af39c99db3de..38ce08dd09ab8 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -1,4 +1,5 @@ reflector->hasAnnotation('NoAdminRequired')) { - if(!$this->isAdminUser) { - throw new NotAdminException($this->l10n->t('Logged in user must be an admin')); - } + if(!$this->reflector->hasAnnotation('NoAdminRequired') && !$this->isAdminUser) { + throw new NotAdminException($this->l10n->t('Logged in user must be an admin')); } } @@ -212,7 +211,7 @@ public function beforeController($controller, $methodName) { * @param Response $response * @return Response */ - public function afterController($controller, $methodName, Response $response) { + public function afterController($controller, $methodName, Response $response): Response { $policy = !is_null($response->getContentSecurityPolicy()) ? $response->getContentSecurityPolicy() : new ContentSecurityPolicy(); if (get_class($policy) === EmptyContentSecurityPolicy::class) { @@ -241,14 +240,14 @@ public function afterController($controller, $methodName, Response $response) { * @throws \Exception the passed in exception if it can't handle it * @return Response a Response object or null in case that the exception could not be handled */ - public function afterException($controller, $methodName, \Exception $exception) { + public function afterException($controller, $methodName, \Exception $exception): Response { if($exception instanceof SecurityException) { if($exception instanceof StrictCookieMissingException) { return new RedirectResponse(\OC::$WEBROOT); } if (stripos($this->request->getHeader('Accept'),'html') === false) { $response = new JSONResponse( - array('message' => $exception->getMessage()), + ['message' => $exception->getMessage()], $exception->getCode() ); } else { From 3ad7daeda5a320276021e72684bfed4469cbae37 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Thu, 8 Mar 2018 11:05:18 +0100 Subject: [PATCH 3/3] Add tests Signed-off-by: Roeland Jago Douma --- .../Security/SecurityMiddleware.php | 26 ++---- .../Security/SecurityMiddlewareTest.php | 88 ++++++++++++++++--- 2 files changed, 86 insertions(+), 28 deletions(-) diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 38ce08dd09ab8..7eb730ac2a3f4 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -40,6 +40,7 @@ use OC\Security\CSP\ContentSecurityPolicyManager; use OC\Security\CSP\ContentSecurityPolicyNonceManager; use OC\Security\CSRF\CsrfTokenManager; +use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; use OCP\AppFramework\Http\ContentSecurityPolicy; use OCP\AppFramework\Http\EmptyContentSecurityPolicy; @@ -92,21 +93,6 @@ class SecurityMiddleware extends Middleware { /** @var IL10N */ private $l10n; - /** - * @param IRequest $request - * @param ControllerMethodReflector $reflector - * @param INavigationManager $navigationManager - * @param IURLGenerator $urlGenerator - * @param ILogger $logger - * @param string $appName - * @param bool $isLoggedIn - * @param bool $isAdminUser - * @param ContentSecurityPolicyManager $contentSecurityPolicyManager - * @param CSRFTokenManager $csrfTokenManager - * @param ContentSecurityPolicyNonceManager $cspNonceManager - * @param IAppManager $appManager - * @param IL10N $l10n - */ public function __construct(IRequest $request, ControllerMethodReflector $reflector, INavigationManager $navigationManager, @@ -190,16 +176,20 @@ public function beforeController($controller, $methodName) { } /** - * FIXME: Use DI once available * Checks if app is enabled (also includes a check whether user is allowed to access the resource) * The getAppPath() check is here since components such as settings also use the AppFramework and * therefore won't pass this check. * If page is public, app does not need to be enabled for current user/visitor */ - if(\OC_App::getAppPath($this->appName) !== false && !$isPublicPage && !$this->appManager->isEnabledForUser($this->appName)) { - throw new AppNotEnabledException(); + try { + $appPath = $this->appManager->getAppPath($this->appName); + } catch (AppPathNotFoundException $e) { + $appPath = false; } + if ($appPath !== false && !$isPublicPage && !$this->appManager->isEnabledForUser($this->appName)) { + throw new AppNotEnabledException(); + } } /** diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index a631fe59a6007..f51f7e9a1c60e 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -95,22 +95,19 @@ protected function setUp() { $this->contentSecurityPolicyManager = $this->createMock(ContentSecurityPolicyManager::class); $this->csrfTokenManager = $this->createMock(CsrfTokenManager::class); $this->cspNonceManager = $this->createMock(ContentSecurityPolicyNonceManager::class); - $this->appManager = $this->createMock(IAppManager::class); $this->l10n = $this->createMock(IL10N::class); - $this->appManager->expects($this->any()) - ->method('isEnabledForUser') - ->willReturn(true); $this->middleware = $this->getMiddleware(true, true); $this->secException = new SecurityException('hey', false); $this->secAjaxException = new SecurityException('hey', true); } - /** - * @param bool $isLoggedIn - * @param bool $isAdminUser - * @return SecurityMiddleware - */ - private function getMiddleware($isLoggedIn, $isAdminUser) { + private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isAppEnabledForUser = true): SecurityMiddleware { + + $this->appManager = $this->createMock(IAppManager::class); + $this->appManager->expects($this->any()) + ->method('isEnabledForUser') + ->willReturn($isAppEnabledForUser); + return new SecurityMiddleware( $this->request, $this->reader, @@ -667,4 +664,75 @@ public function testAfterControllerWithContentSecurityPolicy3Support() { $this->assertEquals($response, $this->middleware->afterController($this->controller, 'test', $response)); } + + public function dataRestrictedApp() { + return [ + [false, false, false,], + [false, false, true,], + [false, true, false,], + [false, true, true,], + [ true, false, false,], + [ true, false, true,], + [ true, true, false,], + [ true, true, true,], + ]; + } + + /** + * @PublicPage + * @NoAdminRequired + * @NoCSRFRequired + */ + public function testRestrictedAppLoggedInPublicPage() { + $middleware = $this->getMiddleware(true, false); + $this->reader->reflect(__CLASS__,__FUNCTION__); + + $this->appManager->method('getAppPath') + ->with('files') + ->willReturn('foo'); + + $this->appManager->method('isEnabledForUser') + ->with('files') + ->willReturn(false); + + $middleware->beforeController($this->controller, __FUNCTION__); + $this->addToAssertionCount(1); + } + + /** + * @PublicPage + * @NoAdminRequired + * @NoCSRFRequired + */ + public function testRestrictedAppNotLoggedInPublicPage() { + $middleware = $this->getMiddleware(false, false); + $this->reader->reflect(__CLASS__,__FUNCTION__); + + $this->appManager->method('getAppPath') + ->with('files') + ->willReturn('foo'); + + $this->appManager->method('isEnabledForUser') + ->with('files') + ->willReturn(false); + + $middleware->beforeController($this->controller, __FUNCTION__); + $this->addToAssertionCount(1); + } + + /** + * @NoAdminRequired + * @NoCSRFRequired + */ + public function testRestrictedAppLoggedIn() { + $middleware = $this->getMiddleware(true, false, false); + $this->reader->reflect(__CLASS__,__FUNCTION__); + + $this->appManager->method('getAppPath') + ->with('files') + ->willReturn('foo'); + + $this->expectException(AppNotEnabledException::class); + $middleware->beforeController($this->controller, __FUNCTION__); + } }