Merge pull request #46021 from nextcloud/middleware-delay-admin

perf: delay getting (sub)admin status for user in the security middleware untill we need it
nextcloud · Aug 23, 2024 · 4a3ade5 · 4a3ade5
2 parents 3e76952 + 8b60df1
commit 4a3ade5
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 11 deletions.
diff --git 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
@@ -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
@@ -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,