From 7da08121868ce6922151e13246f82e8935a6cc51 Mon Sep 17 00:00:00 2001 From: Julien Veyssier <eneiluj@posteo.net> Date: Wed, 28 Feb 2018 20:26:03 +0100 Subject: 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 <eneiluj@posteo.net> --- lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'lib/private') diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index bb3083c835c..4af39c99db3 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -195,8 +195,9 @@ class SecurityMiddleware extends Middleware { * 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(); } -- cgit v1.2.3 From 340e8ef16ced722ae97e6175b82f3010772a2550 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma <roeland@famdouma.nl> Date: Thu, 8 Mar 2018 10:11:47 +0100 Subject: Make SecurityMiddleware strict Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl> --- .../Middleware/Security/SecurityMiddleware.php | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) (limited to 'lib/private') diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 4af39c99db3..38ce08dd09a 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -1,4 +1,5 @@ <?php +declare(strict_types=1); /** * @copyright Copyright (c) 2016, ownCloud, Inc. * @@ -111,9 +112,9 @@ class SecurityMiddleware extends Middleware { INavigationManager $navigationManager, IURLGenerator $urlGenerator, ILogger $logger, - $appName, - $isLoggedIn, - $isAdminUser, + string $appName, + bool $isLoggedIn, + bool $isAdminUser, ContentSecurityPolicyManager $contentSecurityPolicyManager, CsrfTokenManager $csrfTokenManager, ContentSecurityPolicyNonceManager $cspNonceManager, @@ -156,10 +157,8 @@ class SecurityMiddleware extends Middleware { throw new NotLoggedInException(); } - if(!$this->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 @@ class SecurityMiddleware extends Middleware { * @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 @@ class SecurityMiddleware extends Middleware { * @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 { -- cgit v1.2.3 From 3ad7daeda5a320276021e72684bfed4469cbae37 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma <roeland@famdouma.nl> Date: Thu, 8 Mar 2018 11:05:18 +0100 Subject: Add tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl> --- .../Middleware/Security/SecurityMiddleware.php | 26 ++----- .../Middleware/Security/SecurityMiddlewareTest.php | 88 +++++++++++++++++++--- 2 files changed, 86 insertions(+), 28 deletions(-) (limited to 'lib/private') diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 38ce08dd09a..7eb730ac2a3 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -40,6 +40,7 @@ use OC\AppFramework\Utility\ControllerMethodReflector; 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 @@ class SecurityMiddleware extends Middleware { } /** - * 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 a631fe59a60..f51f7e9a1c6 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -95,22 +95,19 @@ class SecurityMiddlewareTest extends \Test\TestCase { $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 @@ class SecurityMiddlewareTest extends \Test\TestCase { $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__); + } } -- cgit v1.2.3