fix(admin role): fix old and wrong way to determine whether user is a…

…dmin - fixes Settings knowing who is an admin of non-local group backend groups - obsoletes and removes a little old, deprecated code - double checks proper parameter type on Group\Manager::isAdmin - also fixes legacy OC_User code to check whether user is an admin Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
nextcloud · Jan 11, 2024 · ed89474 · ed89474
1 parent cfd1cc5
commit ed89474
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 25 deletions.
diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php
@@ -123,10 +123,6 @@ public function register(IRegistrationContext $context): void {
 		/**
 		 * Core class wrappers
 		 */
-		/** FIXME: Remove once OC_User is non-static and mockable */
-		$context->registerService('isAdmin', function () {
-			return \OC_User::isAdminUser(\OC_User::getUser());
-		});
 		/** FIXME: Remove once OC_SubAdmin is non-static and mockable */
 		$context->registerService('isSubAdmin', function () {
 			$userObject = \OC::$server->getUserSession()->getUser();

diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php
@@ -83,8 +83,6 @@ class UsersController extends Controller {
 	private $userSession;
 	/** @var IConfig */
 	private $config;
-	/** @var bool */
-	private $isAdmin;
 	/** @var IL10N */
 	private $l10n;
 	/** @var IMailer */
@@ -114,7 +112,6 @@ public function __construct(
 		IGroupManager $groupManager,
 		IUserSession $userSession,
 		IConfig $config,
-		bool $isAdmin,
 		IL10N $l10n,
 		IMailer $mailer,
 		IFactory $l10nFactory,
@@ -131,7 +128,6 @@ public function __construct(
 		$this->groupManager = $groupManager;
 		$this->userSession = $userSession;
 		$this->config = $config;
-		$this->isAdmin = $isAdmin;
 		$this->l10n = $l10n;
 		$this->mailer = $mailer;
 		$this->l10nFactory = $l10nFactory;
@@ -168,6 +164,7 @@ public function usersListByGroup(): TemplateResponse {
 	public function usersList(): TemplateResponse {
 		$user = $this->userSession->getUser();
 		$uid = $user->getUID();
+		$isAdmin = $this->groupManager->isAdmin($uid);
 
 		\OC::$server->getNavigationManager()->setActiveEntry('core_users');
 
@@ -192,7 +189,7 @@ public function usersList(): TemplateResponse {
 		/* GROUPS */
 		$groupsInfo = new \OC\Group\MetaData(
 			$uid,
-			$this->isAdmin,
+			$isAdmin,
 			$this->groupManager,
 			$this->userSession
 		);
@@ -210,7 +207,7 @@ public function usersList(): TemplateResponse {
 		$userCount = 0;
 
 		if (!$isLDAPUsed) {
-			if ($this->isAdmin) {
+			if ($isAdmin) {
 				$disabledUsers = $this->userManager->countDisabledUsers();
 				$userCount = array_reduce($this->userManager->countUsers(), function ($v, $w) {
 					return $v + (int)$w;
@@ -265,7 +262,7 @@ public function usersList(): TemplateResponse {
 		// groups
 		$serverData['groups'] = array_merge_recursive($adminGroup, [$disabledUsersGroup], $groups);
 		// Various data
-		$serverData['isAdmin'] = $this->isAdmin;
+		$serverData['isAdmin'] = $isAdmin;
 		$serverData['sortGroups'] = $sortGroupsBy;
 		$serverData['quotaPreset'] = $quotaPreset;
 		$serverData['allowUnlimitedQuota'] = $allowUnlimitedQuota;

diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php
@@ -137,6 +137,10 @@ protected function setUp(): void {
 	 * @return UsersController | \PHPUnit\Framework\MockObject\MockObject
 	 */
 	protected function getController($isAdmin = false, $mockedMethods = []) {
+		$this->groupManager->expects($this->any())
+			->method('isAdmin')
+			->willReturn($isAdmin);
+
 		if (empty($mockedMethods)) {
 			return new UsersController(
 				'settings',
@@ -145,7 +149,6 @@ protected function getController($isAdmin = false, $mockedMethods = []) {
 				$this->groupManager,
 				$this->userSession,
 				$this->config,
-				$isAdmin,
 				$this->l,
 				$this->mailer,
 				$this->l10nFactory,
@@ -167,7 +170,6 @@ protected function getController($isAdmin = false, $mockedMethods = []) {
 						$this->groupManager,
 						$this->userSession,
 						$this->config,
-						$isAdmin,
 						$this->l,
 						$this->mailer,
 						$this->l10nFactory,

diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php
@@ -52,6 +52,7 @@
 use OCP\IGroupManager;
 use OCP\IUser;
 use Psr\Log\LoggerInterface;
+use function is_string;
 
 /**
  * Class Manager
@@ -356,7 +357,7 @@ public function getUserIdGroups(string $uid): array {
 	 */
 	public function isAdmin($userId) {
 		foreach ($this->backends as $backend) {
-			if ($backend->implementsActions(Backend::IS_ADMIN) && $backend->isAdmin($userId)) {
+			if (is_string($userId) && $backend->implementsActions(Backend::IS_ADMIN) && $backend->isAdmin($userId)) {
 				return true;
 			}
 		}

diff --git a/lib/private/legacy/OC_User.php b/lib/private/legacy/OC_User.php
@@ -38,7 +38,10 @@
 
 use OC\User\LoginException;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\IGroupManager;
+use OCP\IUser;
 use OCP\IUserManager;
+use OCP\Server;
 use OCP\User\Events\BeforeUserLoggedInEvent;
 use OCP\User\Events\UserLoggedInEvent;
 use Psr\Log\LoggerInterface;
@@ -93,7 +96,7 @@ public static function useBackend($backend = 'database') {
 				case 'database':
 				case 'mysql':
 				case 'sqlite':
-					\OCP\Server::get(LoggerInterface::class)->debug('Adding user backend ' . $backend . '.', ['app' => 'core']);
+					Server::get(LoggerInterface::class)->debug('Adding user backend ' . $backend . '.', ['app' => 'core']);
 					self::$_usedBackends[$backend] = new \OC\User\Database();
 					\OC::$server->getUserManager()->registerBackend(self::$_usedBackends[$backend]);
 					break;
@@ -102,7 +105,7 @@ public static function useBackend($backend = 'database') {
 					\OC::$server->getUserManager()->registerBackend(self::$_usedBackends[$backend]);
 					break;
 				default:
-					\OCP\Server::get(LoggerInterface::class)->debug('Adding default user backend ' . $backend . '.', ['app' => 'core']);
+					Server::get(LoggerInterface::class)->debug('Adding default user backend ' . $backend . '.', ['app' => 'core']);
 					$className = 'OC_USER_' . strtoupper($backend);
 					self::$_usedBackends[$backend] = new $className();
 					\OC::$server->getUserManager()->registerBackend(self::$_usedBackends[$backend]);
@@ -147,10 +150,10 @@ public static function setupBackends() {
 					self::useBackend($backend);
 					self::$_setupedBackends[] = $i;
 				} else {
-					\OCP\Server::get(LoggerInterface::class)->debug('User backend ' . $class . ' already initialized.', ['app' => 'core']);
+					Server::get(LoggerInterface::class)->debug('User backend ' . $class . ' already initialized.', ['app' => 'core']);
 				}
 			} else {
-				\OCP\Server::get(LoggerInterface::class)->error('User backend ' . $class . ' not found.', ['app' => 'core']);
+				Server::get(LoggerInterface::class)->error('User backend ' . $class . ' not found.', ['app' => 'core']);
 			}
 		}
 	}
@@ -303,7 +306,7 @@ public static function getLogoutUrl(\OCP\IURLGenerator $urlGenerator) {
 		}
 
 		$user = \OC::$server->getUserSession()->getUser();
-		if ($user instanceof \OCP\IUser) {
+		if ($user instanceof IUser) {
 			$backend = $user->getBackend();
 			if ($backend instanceof \OCP\User\Backend\ICustomLogout) {
 				return $backend->getLogoutUrl();
@@ -323,12 +326,9 @@ public static function getLogoutUrl(\OCP\IURLGenerator $urlGenerator) {
 	 * @return bool
 	 */
 	public static function isAdminUser($uid) {
-		$group = \OC::$server->getGroupManager()->get('admin');
-		$user = \OC::$server->getUserManager()->get($uid);
-		if ($group && $user && $group->inGroup($user) && self::$incognitoMode === false) {
-			return true;
-		}
-		return false;
+		$user = Server::get(IUserManager::class)->get($uid);
+		$isAdmin = $user && Server::get(IGroupManager::class)->isAdmin($user->getUID());
+		return $isAdmin && self::$incognitoMode === false;
 	}