diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index 38ce08dd09..7eb730ac2a 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 a631fe59a6..f51f7e9a1c 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__); + } }