Merge pull request #48538 from nextcloud/fix/get-managers-as-subadmin

fix: Return correct list of managers for a user

6 files changed, 66 insertions, 49 deletions

diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php
index eb881db45e0..603c6306227 100644
--- a/apps/provisioning_api/lib/Controller/AUserData.php
+++ b/apps/provisioning_api/lib/Controller/AUserData.php
@@ -8,7 +8,7 @@ declare(strict_types=1);
  */
 namespace OCA\Provisioning_API\Controller;
 
-use OC\Group\Manager;
+use OC\Group\Manager as GroupManager;
 use OC\User\Backend;
 use OC\User\NoUserException;
 use OC_Helper;
@@ -20,9 +20,10 @@ use OCP\AppFramework\OCS\OCSException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
 use OCP\Files\NotFoundException;
+use OCP\Group\ISubAdmin;
 use OCP\IConfig;
-use OCP\IGroupManager;
 use OCP\IRequest;
+use OCP\IUser;
 use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\L10N\IFactory;
@@ -45,35 +46,18 @@ abstract class AUserData extends OCSController {
 	public const USER_FIELD_MANAGER = 'manager';
 	public const USER_FIELD_NOTIFICATION_EMAIL = 'notify_email';
 
-	/** @var IUserManager */
-	protected $userManager;
-	/** @var IConfig */
-	protected $config;
-	/** @var Manager */
-	protected $groupManager;
-	/** @var IUserSession */
-	protected $userSession;
-	/** @var IAccountManager */
-	protected $accountManager;
-	/** @var IFactory */
-	protected $l10nFactory;
-
-	public function __construct(string $appName,
+	public function __construct(
+		string $appName,
 		IRequest $request,
-		IUserManager $userManager,
-		IConfig $config,
-		IGroupManager $groupManager,
-		IUserSession $userSession,
-		IAccountManager $accountManager,
-		IFactory $l10nFactory) {
+		protected IUserManager $userManager,
+		protected IConfig $config,
+		protected GroupManager $groupManager,
+		protected IUserSession $userSession,
+		protected IAccountManager $accountManager,
+		protected ISubAdmin $subAdminManager,
+		protected IFactory $l10nFactory,
+	) {
 		parent::__construct($appName, $request);
-
-		$this->userManager = $userManager;
-		$this->config = $config;
-		$this->groupManager = $groupManager;
-		$this->userSession = $userSession;
-		$this->accountManager = $accountManager;
-		$this->l10nFactory = $l10nFactory;
 	}
 
 	/**
@@ -136,8 +120,8 @@ abstract class AUserData extends OCSController {
 		$data['backend'] = $targetUserObject->getBackendClassName();
 		$data['subadmin'] = $this->getUserSubAdminGroupsData($targetUserObject->getUID());
 		$data[self::USER_FIELD_QUOTA] = $this->fillStorageInfo($targetUserObject->getUID());
-		$managerUids = $targetUserObject->getManagerUids();
-		$data[self::USER_FIELD_MANAGER] = empty($managerUids) ? '' : $managerUids[0];
+		$managers = $this->getManagers($targetUserObject);
+		$data[self::USER_FIELD_MANAGER] = empty($managers) ? '' : $managers[0];
 
 		try {
 			if ($includeScopes) {
@@ -207,6 +191,34 @@ abstract class AUserData extends OCSController {
 	}
 
 	/**
+	 * @return string[]
+	 */
+	protected function getManagers(IUser $user): array {
+		$currentLoggedInUser = $this->userSession->getUser();
+
+		$managerUids = $user->getManagerUids();
+		if ($this->groupManager->isAdmin($currentLoggedInUser->getUID()) || $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID())) {
+			return $managerUids;
+		}
+
+		if ($this->subAdminManager->isSubAdmin($currentLoggedInUser)) {
+			$accessibleManagerUids = array_values(array_filter(
+				$managerUids,
+				function (string $managerUid) use ($currentLoggedInUser) {
+					$manager = $this->userManager->get($managerUid);
+					if (!($manager instanceof IUser)) {
+						return false;
+					}
+					return $this->subAdminManager->isUserAccessible($currentLoggedInUser, $manager);
+				},
+			));
+			return $accessibleManagerUids;
+		}
+
+		return [];
+	}
+
+	/**
 	 * Get the groups a user is a subadmin of
 	 *
 	 * @param string $userId
diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php
index 4b05f772e8f..f0712d12261 100644
--- a/apps/provisioning_api/lib/Controller/GroupsController.php
+++ b/apps/provisioning_api/lib/Controller/GroupsController.php
@@ -21,6 +21,7 @@ use OCP\AppFramework\OCS\OCSException;
 use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
+use OCP\Group\ISubAdmin;
 use OCP\IConfig;
 use OCP\IGroup;
 use OCP\IGroupManager;
@@ -47,6 +48,7 @@ class GroupsController extends AUserData {
 		IGroupManager $groupManager,
 		IUserSession $userSession,
 		IAccountManager $accountManager,
+		ISubAdmin $subAdminManager,
 		IFactory $l10nFactory,
 		LoggerInterface $logger) {
 		parent::__construct($appName,
@@ -56,6 +58,7 @@ class GroupsController extends AUserData {
 			$groupManager,
 			$userSession,
 			$accountManager,
+			$subAdminManager,
 			$l10nFactory
 		);
 
diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php
index 5be0b6b1464..273e63c742d 100644
--- a/apps/provisioning_api/lib/Controller/UsersController.php
+++ b/apps/provisioning_api/lib/Controller/UsersController.php
@@ -31,6 +31,7 @@ use OCP\AppFramework\OCS\OCSForbiddenException;
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Group\ISubAdmin;
 use OCP\HintException;
 use OCP\IConfig;
 use OCP\IGroup;
@@ -63,6 +64,7 @@ class UsersController extends AUserData {
 		IGroupManager $groupManager,
 		IUserSession $userSession,
 		IAccountManager $accountManager,
+		ISubAdmin $subAdminManager,
 		IFactory $l10nFactory,
 		private IURLGenerator $urlGenerator,
 		private LoggerInterface $logger,
@@ -81,6 +83,7 @@ class UsersController extends AUserData {
 			$groupManager,
 			$userSession,
 			$accountManager,
+			$subAdminManager,
 			$l10nFactory
 		);
 
@@ -946,7 +949,7 @@ class UsersController extends AUserData {
 			$permittedFields[] = IAccountManager::PROPERTY_PROFILE_ENABLED;
 			$permittedFields[] = IAccountManager::PROPERTY_BIRTHDATE;
 			$permittedFields[] = IAccountManager::PROPERTY_PRONOUNS;
-		
+
 			$permittedFields[] = IAccountManager::PROPERTY_PHONE . self::SCOPE_SUFFIX;
 			$permittedFields[] = IAccountManager::PROPERTY_ADDRESS . self::SCOPE_SUFFIX;
 			$permittedFields[] = IAccountManager::PROPERTY_WEBSITE . self::SCOPE_SUFFIX;
diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php
index f9c0848fb72..5ddf5d7eff8 100644
--- a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php
+++ b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php
@@ -8,10 +8,10 @@
 namespace OCA\Provisioning_API\Tests\Controller;
 
 use OC\Group\Manager;
-use OC\SubAdmin;
 use OC\User\NoUserException;
 use OCA\Provisioning_API\Controller\GroupsController;
 use OCP\Accounts\IAccountManager;
+use OCP\Group\ISubAdmin;
 use OCP\IConfig;
 use OCP\IRequest;
 use OCP\IUser;
@@ -34,12 +34,12 @@ class GroupsControllerTest extends \Test\TestCase {
 	protected $userSession;
 	/** @var IAccountManager|\PHPUnit\Framework\MockObject\MockObject */
 	protected $accountManager;
+	/** @var ISubAdmin|\PHPUnit\Framework\MockObject\MockObject */
+	protected $subAdminManager;
 	/** @var IFactory|\PHPUnit\Framework\MockObject\MockObject */
 	protected $l10nFactory;
 	/** @var LoggerInterface|\PHPUnit\Framework\MockObject\MockObject */
 	protected $logger;
-	/** @var SubAdmin|\PHPUnit\Framework\MockObject\MockObject */
-	protected $subAdminManager;
 
 	/** @var GroupsController|\PHPUnit\Framework\MockObject\MockObject */
 	protected $api;
@@ -54,11 +54,10 @@ class GroupsControllerTest extends \Test\TestCase {
 		$this->groupManager = $this->createMock(Manager::class);
 		$this->userSession = $this->createMock(IUserSession::class);
 		$this->accountManager = $this->createMock(IAccountManager::class);
+		$this->subAdminManager = $this->createMock(ISubAdmin::class);
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->logger = $this->createMock(LoggerInterface::class);
 
-		$this->subAdminManager = $this->createMock(SubAdmin::class);
-
 		$this->groupManager
 			->method('getSubAdmin')
 			->willReturn($this->subAdminManager);
@@ -72,6 +71,7 @@ class GroupsControllerTest extends \Test\TestCase {
 				$this->groupManager,
 				$this->userSession,
 				$this->accountManager,
+				$this->subAdminManager,
 				$this->l10nFactory,
 				$this->logger
 			])
diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php
index d0d7dc72855..2ccb1196fbb 100644
--- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php
+++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php
@@ -23,6 +23,7 @@ use OCP\Accounts\IAccountPropertyCollection;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\OCS\OCSException;
 use OCP\EventDispatcher\IEventDispatcher;
+use OCP\Group\ISubAdmin;
 use OCP\IConfig;
 use OCP\IGroup;
 use OCP\IL10N;
@@ -57,6 +58,8 @@ class UsersControllerTest extends TestCase {
 	protected $api;
 	/** @var IAccountManager|MockObject */
 	protected $accountManager;
+	/** @var ISubAdmin|MockObject */
+	protected $subAdminManager;
 	/** @var IURLGenerator|MockObject */
 	protected $urlGenerator;
 	/** @var IRequest|MockObject */
@@ -86,6 +89,7 @@ class UsersControllerTest extends TestCase {
 		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->request = $this->createMock(IRequest::class);
 		$this->accountManager = $this->createMock(IAccountManager::class);
+		$this->subAdminManager = $this->createMock(ISubAdmin::class);
 		$this->urlGenerator = $this->createMock(IURLGenerator::class);
 		$this->l10nFactory = $this->createMock(IFactory::class);
 		$this->newUserMailHelper = $this->createMock(NewUserMailHelper::class);
@@ -108,6 +112,7 @@ class UsersControllerTest extends TestCase {
 				$this->groupManager,
 				$this->userSession,
 				$this->accountManager,
+				$this->subAdminManager,
 				$this->l10nFactory,
 				$this->urlGenerator,
 				$this->logger,
@@ -376,6 +381,7 @@ class UsersControllerTest extends TestCase {
 				$this->groupManager,
 				$this->userSession,
 				$this->accountManager,
+				$this->subAdminManager,
 				$this->l10nFactory,
 				$this->urlGenerator,
 				$this->logger,
@@ -931,7 +937,6 @@ class UsersControllerTest extends TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 		$loggedInUser
-			->expects($this->exactly(2))
 			->method('getUID')
 			->willReturn('admin');
 		$targetUser = $this->getMockBuilder(IUser::class)
@@ -941,16 +946,13 @@ class UsersControllerTest extends TestCase {
 			->method('getSystemEMailAddress')
 			->willReturn('demo@nextcloud.com');
 		$this->userSession
-			->expects($this->once())
 			->method('getUser')
 			->willReturn($loggedInUser);
 		$this->userManager
-			->expects($this->exactly(2))
 			->method('get')
 			->with('UID')
 			->willReturn($targetUser);
 		$this->groupManager
-			->expects($this->once())
 			->method('isAdmin')
 			->with('admin')
 			->willReturn(true);
@@ -1079,7 +1081,6 @@ class UsersControllerTest extends TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 		$loggedInUser
-			->expects($this->exactly(2))
 			->method('getUID')
 			->willReturn('subadmin');
 		$targetUser = $this->getMockBuilder(IUser::class)
@@ -1090,16 +1091,13 @@ class UsersControllerTest extends TestCase {
 			->method('getSystemEMailAddress')
 			->willReturn('demo@nextcloud.com');
 		$this->userSession
-			->expects($this->once())
 			->method('getUser')
 			->willReturn($loggedInUser);
 		$this->userManager
-			->expects($this->exactly(2))
 			->method('get')
 			->with('UID')
 			->willReturn($targetUser);
 		$this->groupManager
-			->expects($this->once())
 			->method('isAdmin')
 			->with('subadmin')
 			->willReturn(false);
@@ -1267,23 +1265,19 @@ class UsersControllerTest extends TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 		$loggedInUser
-			->expects($this->exactly(3))
 			->method('getUID')
 			->willReturn('UID');
 		$targetUser = $this->getMockBuilder(IUser::class)
 			->disableOriginalConstructor()
 			->getMock();
 		$this->userSession
-			->expects($this->once())
 			->method('getUser')
 			->willReturn($loggedInUser);
 		$this->userManager
-			->expects($this->exactly(2))
 			->method('get')
 			->with('UID')
 			->willReturn($targetUser);
 		$this->groupManager
-			->expects($this->once())
 			->method('isAdmin')
 			->with('UID')
 			->willReturn(false);
@@ -3667,6 +3661,7 @@ class UsersControllerTest extends TestCase {
 				$this->groupManager,
 				$this->userSession,
 				$this->accountManager,
+				$this->subAdminManager,
 				$this->l10nFactory,
 				$this->urlGenerator,
 				$this->logger,
@@ -3756,6 +3751,7 @@ class UsersControllerTest extends TestCase {
 				$this->groupManager,
 				$this->userSession,
 				$this->accountManager,
+				$this->subAdminManager,
 				$this->l10nFactory,
 				$this->urlGenerator,
 				$this->logger,
diff --git a/lib/private/SubAdmin.php b/lib/private/SubAdmin.php
index c025ab7b012..335e901a321 100644
--- a/lib/private/SubAdmin.php
+++ b/lib/private/SubAdmin.php
@@ -259,6 +259,9 @@ class SubAdmin extends PublicEmitter implements ISubAdmin {
 	 * @return bool
 	 */
 	public function isUserAccessible(IUser $subadmin, IUser $user): bool {
+		if ($subadmin->getUID() === $user->getUID()) {
+			return true;
+		}
 		if (!$this->isSubAdmin($subadmin)) {
 			return false;
 		}