fix(settings): Inject subadmin manager and adapt tests, resolve a FIXME comment

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

apps/settings/lib/AppInfo/Application.php

@@ -87,9 +87,7 @@ use OCP\Defaults;
 use OCP\Group\Events\GroupDeletedEvent;
 use OCP\Group\Events\UserAddedEvent;
 use OCP\Group\Events\UserRemovedEvent;
-use OCP\IGroupManager;
 use OCP\IServerContainer;
-use OCP\IUserSession;
 use OCP\Settings\Events\DeclarativeSettingsGetValueEvent;
 use OCP\Settings\Events\DeclarativeSettingsSetValueEvent;
 use OCP\Settings\IManager;
@@ -133,15 +131,6 @@ class Application extends App implements IBootstrap {
 		/**
 		 * Core class wrappers
 		 */
-		/** FIXME: Remove once OC_SubAdmin is non-static and mockable */
-		$context->registerService('isSubAdmin', function () {
-			$userObject = \OCP\Server::get(IUserSession::class)->getUser();
-			$isSubAdmin = false;
-			if ($userObject !== null) {
-				$isSubAdmin = \OCP\Server::get(IGroupManager::class)->getSubAdmin()->isSubAdmin($userObject);
-			}
-			return $isSubAdmin;
-		});
 		$context->registerService(IProvider::class, function (IAppContainer $appContainer) {
 			/** @var IServerContainer $serverContainer */
 			$serverContainer = $appContainer->query(IServerContainer::class);

apps/settings/lib/Middleware/SubadminMiddleware.php

@@ -1,9 +1,13 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2019-2024 Nextcloud GmbH and Nextcloud contributors/**
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
+
 namespace OCA\Settings\Middleware;
 
 use OC\AppFramework\Http;
@@ -12,27 +16,29 @@ use OC\AppFramework\Utility\ControllerMethodReflector;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Middleware;
+use OCP\Group\ISubAdmin;
 use OCP\IL10N;
+use OCP\IUserSession;
 
 /**
  * Verifies whether an user has at least subadmin rights.
  * To bypass use the `@NoSubAdminRequired` annotation
  */
 class SubadminMiddleware extends Middleware {
-	/** @var ControllerMethodReflector */
-	protected $reflector;
-
-	/**
-	 * @param ControllerMethodReflector $reflector
-	 * @param bool $isSubAdmin
-	 * @param IL10N $l10n
-	 */
 	public function __construct(
-		ControllerMethodReflector $reflector,
-		protected $isSubAdmin,
+		protected ControllerMethodReflector $reflector,
+		protected IUserSession $userSession,
+		protected ISubAdmin $subAdminManager,
 		private IL10N $l10n,
 	) {
-		$this->reflector = $reflector;
+	}
+
+	private function isSubAdmin(): bool {
+		$userObject = $this->userSession->getUser();
+		if ($userObject === null) {
+			return false;
+		}
+		return $this->subAdminManager->isSubAdmin($userObject);
 	}
 
 	/**
@@ -43,7 +49,7 @@ class SubadminMiddleware extends Middleware {
 	 */
 	public function beforeController($controller, $methodName) {
 		if (!$this->reflector->hasAnnotation('NoSubAdminRequired') && !$this->reflector->hasAnnotation('AuthorizedAdminSetting')) {
-			if (!$this->isSubAdmin) {
+			if (!$this->isSubAdmin()) {
 				throw new NotAdminException($this->l10n->t('Logged in account must be a subadmin'));
 			}
 		}

apps/settings/tests/Middleware/SubadminMiddlewareTest.php

@@ -1,9 +1,13 @@
 <?php
+
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2019-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2014 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-or-later
  */
+
 namespace OCA\Settings\Tests\Middleware;
 
 use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException;
@@ -11,7 +15,11 @@ use OC\AppFramework\Utility\ControllerMethodReflector;
 use OCA\Settings\Middleware\SubadminMiddleware;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\Group\ISubAdmin;
 use OCP\IL10N;
+use OCP\IUser;
+use OCP\IUserSession;
+use PHPUnit\Framework\MockObject\MockObject;
 
 /**
  * Verifies whether an user has at least subadmin rights.
@@ -20,31 +28,40 @@ use OCP\IL10N;
  * @package Tests\Settings\Middleware
  */
 class SubadminMiddlewareTest extends \Test\TestCase {
-	/** @var SubadminMiddleware */
-	private $subadminMiddlewareAsSubAdmin;
-	/** @var SubadminMiddleware */
-	private $subadminMiddleware;
-	/** @var ControllerMethodReflector */
-	private $reflector;
-	/** @var Controller */
-	private $controller;
-	/** @var IL10N */
-	private $l10n;
+	private SubadminMiddleware $subadminMiddleware;
+
+	private IUserSession&MockObject $userSession;
+	private ISubAdmin&MockObject $subAdminManager;
+	private ControllerMethodReflector&MockObject $reflector;
+	private Controller&MockObject $controller;
+	private IL10N&MockObject $l10n;
 
 	protected function setUp(): void {
 		parent::setUp();
 		$this->reflector = $this->getMockBuilder(ControllerMethodReflector::class)
 			->disableOriginalConstructor()->getMock();
+		$this->userSession = $this->createMock(IUserSession::class);
+		$this->subAdminManager = $this->createMock(ISubAdmin::class);
+		$this->l10n = $this->createMock(IL10N::class);
+
+		$this->subadminMiddleware = new SubadminMiddleware(
+			$this->reflector,
+			$this->userSession,
+			$this->subAdminManager,
+			$this->l10n,
+		);
+
 		$this->controller = $this->getMockBuilder(Controller::class)
 			->disableOriginalConstructor()->getMock();
-		$this->l10n = $this->createMock(IL10N::class);
 
-		$this->subadminMiddlewareAsSubAdmin = new SubadminMiddleware($this->reflector, true, $this->l10n);
-		$this->subadminMiddleware = new SubadminMiddleware($this->reflector, false, $this->l10n);
+		$this->userSession
+			->expects(self::any())
+			->method('getUser')
+			->willReturn($this->createMock(IUser::class));
 	}
 
 
-	public function testBeforeControllerAsUserWithExemption(): void {
+	public function testBeforeControllerAsUserWithoutAnnotation(): void {
 		$this->expectException(NotAdminException::class);
 
 		$this->reflector
@@ -54,20 +71,31 @@ class SubadminMiddlewareTest extends \Test\TestCase {
 				['NoSubAdminRequired'],
 				['AuthorizedAdminSetting'],
 			)->willReturn(false);
+
+		$this->subAdminManager
+			->expects(self::once())
+			->method('isSubAdmin')
+			->willReturn(false);
+
 		$this->subadminMiddleware->beforeController($this->controller, 'foo');
 	}
 
 
-	public function testBeforeControllerAsUserWithoutExemption(): void {
+	public function testBeforeControllerWithAnnotation(): void {
 		$this->reflector
 			->expects($this->once())
 			->method('hasAnnotation')
 			->with('NoSubAdminRequired')
 			->willReturn(true);
+
+		$this->subAdminManager
+			->expects(self::never())
+			->method('isSubAdmin');
+
 		$this->subadminMiddleware->beforeController($this->controller, 'foo');
 	}
 
-	public function testBeforeControllerAsSubAdminWithoutExemption(): void {
+	public function testBeforeControllerAsSubAdminWithoutAnnotation(): void {
 		$this->reflector
 			->expects($this->exactly(2))
 			->method('hasAnnotation')
@@ -75,16 +103,13 @@ class SubadminMiddlewareTest extends \Test\TestCase {
 				['NoSubAdminRequired'],
 				['AuthorizedAdminSetting'],
 			)->willReturn(false);
-		$this->subadminMiddlewareAsSubAdmin->beforeController($this->controller, 'foo');
-	}
 
-	public function testBeforeControllerAsSubAdminWithExemption(): void {
-		$this->reflector
-			->expects($this->once())
-			->method('hasAnnotation')
-			->with('NoSubAdminRequired')
+		$this->subAdminManager
+			->expects(self::once())
+			->method('isSubAdmin')
 			->willReturn(true);
-		$this->subadminMiddlewareAsSubAdmin->beforeController($this->controller, 'foo');
+
+		$this->subadminMiddleware->beforeController($this->controller, 'foo');
 	}
 
 	public function testAfterNotAdminException(): void {