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