diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 4add17396b0d0..1508084ed312c 100644 --- a/lib/private/AppFramework/DependencyInjection/DIContainer.php +++ b/lib/private/AppFramework/DependencyInjection/DIContainer.php @@ -36,6 +36,7 @@ use OCP\Group\ISubAdmin; use OCP\IConfig; use OCP\IDBConnection; +use OCP\IGroupManager; use OCP\IInitialStateService; use OCP\IL10N; use OCP\ILogger; @@ -226,8 +227,8 @@ public function __construct(string $appName, array $urlParams = [], ?ServerConta $server->get(LoggerInterface::class), $c->get('AppName'), $server->getUserSession()->isLoggedIn(), - $this->getUserId() !== null && $server->getGroupManager()->isAdmin($this->getUserId()), - $server->getUserSession()->getUser() !== null && $server->query(ISubAdmin::class)->isSubAdmin($server->getUserSession()->getUser()), + $c->get(IGroupManager::class), + $c->get(ISubAdmin::class), $server->getAppManager(), $server->getL10N('lib'), $c->get(AuthorizedGroupMapper::class), diff --git a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php index a38ad610fc609..9b3b2e9b8384a 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -31,6 +31,8 @@ use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Middleware; use OCP\AppFramework\OCSController; +use OCP\Group\ISubAdmin; +use OCP\IGroupManager; use OCP\IL10N; use OCP\INavigationManager; use OCP\IRequest; @@ -47,60 +49,40 @@ * check fails */ class SecurityMiddleware extends Middleware { - /** @var INavigationManager */ - private $navigationManager; - /** @var IRequest */ - private $request; - /** @var ControllerMethodReflector */ - private $reflector; - /** @var string */ - private $appName; - /** @var IURLGenerator */ - private $urlGenerator; - /** @var LoggerInterface */ - private $logger; - /** @var bool */ - private $isLoggedIn; - /** @var bool */ - private $isAdminUser; - /** @var bool */ - private $isSubAdmin; - /** @var IAppManager */ - private $appManager; - /** @var IL10N */ - private $l10n; - /** @var AuthorizedGroupMapper */ - private $groupAuthorizationMapper; - /** @var IUserSession */ - private $userSession; + private ?bool $isAdminUser = null; + private ?bool $isSubAdmin = null; - public function __construct(IRequest $request, - ControllerMethodReflector $reflector, - INavigationManager $navigationManager, - IURLGenerator $urlGenerator, - LoggerInterface $logger, - string $appName, - bool $isLoggedIn, - bool $isAdminUser, - bool $isSubAdmin, - IAppManager $appManager, - IL10N $l10n, - AuthorizedGroupMapper $mapper, - IUserSession $userSession + public function __construct( + private IRequest $request, + private ControllerMethodReflector $reflector, + private INavigationManager $navigationManager, + private IURLGenerator $urlGenerator, + private LoggerInterface $logger, + private string $appName, + private bool $isLoggedIn, + private IGroupManager $groupManager, + private ISubAdmin $subAdminManager, + private IAppManager $appManager, + private IL10N $l10n, + private AuthorizedGroupMapper $groupAuthorizationMapper, + private IUserSession $userSession, ) { - $this->navigationManager = $navigationManager; - $this->request = $request; - $this->reflector = $reflector; - $this->appName = $appName; - $this->urlGenerator = $urlGenerator; - $this->logger = $logger; - $this->isLoggedIn = $isLoggedIn; - $this->isAdminUser = $isAdminUser; - $this->isSubAdmin = $isSubAdmin; - $this->appManager = $appManager; - $this->l10n = $l10n; - $this->groupAuthorizationMapper = $mapper; - $this->userSession = $userSession; + } + + private function isAdminUser(): bool { + if ($this->isAdminUser === null) { + $user = $this->userSession->getUser(); + $this->isAdminUser = $user && $this->groupManager->isAdmin($user->getUID()); + } + return $this->isAdminUser; + } + + private function isSubAdmin(): bool { + if ($this->isSubAdmin === null) { + $user = $this->userSession->getUser(); + $this->isSubAdmin = $user && $this->subAdminManager->isSubAdmin($user); + } + return $this->isSubAdmin; } /** @@ -133,10 +115,10 @@ public function beforeController($controller, $methodName) { } $authorized = false; if ($this->hasAnnotationOrAttribute($reflectionMethod, 'AuthorizedAdminSetting', AuthorizedAdminSetting::class)) { - $authorized = $this->isAdminUser; + $authorized = $this->isAdminUser(); if (!$authorized && $this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class)) { - $authorized = $this->isSubAdmin; + $authorized = $this->isSubAdmin(); } if (!$authorized) { @@ -155,14 +137,14 @@ public function beforeController($controller, $methodName) { } } if ($this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) - && !$this->isSubAdmin - && !$this->isAdminUser + && !$this->isSubAdmin() + && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin or sub admin')); } if (!$this->hasAnnotationOrAttribute($reflectionMethod, 'SubAdminRequired', SubAdminRequired::class) && !$this->hasAnnotationOrAttribute($reflectionMethod, 'NoAdminRequired', NoAdminRequired::class) - && !$this->isAdminUser + && !$this->isAdminUser() && !$authorized) { throw new NotAdminException($this->l10n->t('Logged in account must be an admin')); } diff --git a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php index 1d7753a347777..3a2d032882e59 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -22,12 +22,15 @@ use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\RedirectResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\Group\ISubAdmin; use OCP\IConfig; +use OCP\IGroupManager; use OCP\IL10N; use OCP\INavigationManager; use OCP\IRequest; use OCP\IRequestId; use OCP\IURLGenerator; +use OCP\IUser; use OCP\IUserSession; use Psr\Log\LoggerInterface; use Test\AppFramework\Middleware\Security\Mock\NormalController; @@ -67,6 +70,9 @@ protected function setUp(): void { $this->authorizedGroupMapper = $this->createMock(AuthorizedGroupMapper::class); $this->userSession = $this->createMock(IUserSession::class); + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('test'); + $this->userSession->method('getUser')->willReturn($user); $this->request = $this->createMock(IRequest::class); $this->controller = new SecurityMiddlewareController( 'test', @@ -88,6 +94,13 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA ->method('isEnabledForUser') ->willReturn($isAppEnabledForUser); + $groupManager = $this->createMock(IGroupManager::class); + $groupManager->method('isAdmin') + ->willReturn($isAdminUser); + $subAdminManager = $this->createMock(ISubAdmin::class); + $subAdminManager->method('isSubAdmin') + ->willReturn($isSubAdmin); + return new SecurityMiddleware( $this->request, $this->reader, @@ -96,8 +109,8 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA $this->logger, 'files', $isLoggedIn, - $isAdminUser, - $isSubAdmin, + $groupManager, + $subAdminManager, $this->appManager, $this->l10n, $this->authorizedGroupMapper,