From 8b60df1600ce978335c569731362b0b69fa0bcaa Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 20 Jun 2024 18:44:52 +0200 Subject: [PATCH] perf: delay getting (sub)admin status for user in the security middleware untill we need it Signed-off-by: Robin Appelman --- .../DependencyInjection/DIContainer.php | 5 +-- .../Security/SecurityMiddleware.php | 35 +++++++++++++++---- .../Security/SecurityMiddlewareTest.php | 17 +++++++-- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/lib/private/AppFramework/DependencyInjection/DIContainer.php b/lib/private/AppFramework/DependencyInjection/DIContainer.php index 208031b942fea..b11dec934263a 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; @@ -228,8 +229,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 b8de09072ce88..889872902448b 100644 --- a/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php +++ b/lib/private/AppFramework/Middleware/Security/SecurityMiddleware.php @@ -36,6 +36,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; @@ -53,6 +55,9 @@ * check fails */ class SecurityMiddleware extends Middleware { + private ?bool $isAdminUser = null; + private ?bool $isSubAdmin = null; + public function __construct( private IRequest $request, private ControllerMethodReflector $reflector, @@ -61,8 +66,8 @@ public function __construct( private LoggerInterface $logger, private string $appName, private bool $isLoggedIn, - private bool $isAdminUser, - private bool $isSubAdmin, + private IGroupManager $groupManager, + private ISubAdmin $subAdminManager, private IAppManager $appManager, private IL10N $l10n, private AuthorizedGroupMapper $groupAuthorizationMapper, @@ -71,6 +76,22 @@ public function __construct( ) { } + 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; + } + /** * This runs all the security checks before a method call. The * security checks are determined by inspecting the controller method @@ -114,10 +135,10 @@ public function beforeController($controller, $methodName) { } if (!$authorized && $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) { @@ -139,14 +160,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 fb457579fac36..b7af69842375d 100644 --- a/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php +++ b/tests/lib/AppFramework/Middleware/Security/SecurityMiddlewareTest.php @@ -24,13 +24,16 @@ 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\ISession; use OCP\IURLGenerator; +use OCP\IUser; use OCP\IUserSession; use OCP\Security\Ip\IRemoteAddress; use Psr\Log\LoggerInterface; @@ -71,6 +74,9 @@ protected function setUp(): void { $this->authorizedGroupMapper = $this->createMock(AuthorizedGroupMapper::class); $this->userSession = $this->createMock(Session::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', @@ -94,6 +100,13 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA $remoteIpAddress = $this->createMock(IRemoteAddress::class); $remoteIpAddress->method('allowsAdminActions')->willReturn(true); + $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, @@ -102,8 +115,8 @@ private function getMiddleware(bool $isLoggedIn, bool $isAdminUser, bool $isSubA $this->logger, 'files', $isLoggedIn, - $isAdminUser, - $isSubAdmin, + $groupManager, + $subAdminManager, $this->appManager, $this->l10n, $this->authorizedGroupMapper,