From ea5e27dcd89820c9939598d53628f9d0f9988ceb Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Fri, 4 Oct 2024 16:28:23 -0700 Subject: [PATCH 1/4] fix: Subadmin can access self Signed-off-by: Christopher Ng --- lib/private/SubAdmin.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/private/SubAdmin.php b/lib/private/SubAdmin.php index c025ab7b01246..335e901a321b6 100644 --- a/lib/private/SubAdmin.php +++ b/lib/private/SubAdmin.php @@ -259,6 +259,9 @@ public function isSubAdmin(IUser $user): bool { * @return bool */ public function isUserAccessible(IUser $subadmin, IUser $user): bool { + if ($subadmin->getUID() === $user->getUID()) { + return true; + } if (!$this->isSubAdmin($subadmin)) { return false; } From 026b7b8b15a0b5c40279d39569ef2432f4343f65 Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Fri, 4 Oct 2024 16:28:23 -0700 Subject: [PATCH 2/4] fix: Return correct list of managers for a user Signed-off-by: Christopher Ng --- .../lib/Controller/AUserData.php | 38 ++++++++++++++++++- .../lib/Controller/GroupsController.php | 3 ++ .../lib/Controller/UsersController.php | 5 ++- 3 files changed, 43 insertions(+), 3 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index eb881db45e03e..54ef3691caea8 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -20,9 +20,11 @@ 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; @@ -55,6 +57,8 @@ abstract class AUserData extends OCSController { protected $userSession; /** @var IAccountManager */ protected $accountManager; + /** @var ISubAdmin */ + protected $subAdminManager; /** @var IFactory */ protected $l10nFactory; @@ -65,6 +69,7 @@ public function __construct(string $appName, IGroupManager $groupManager, IUserSession $userSession, IAccountManager $accountManager, + ISubAdmin $subAdminManager, IFactory $l10nFactory) { parent::__construct($appName, $request); @@ -73,6 +78,7 @@ public function __construct(string $appName, $this->groupManager = $groupManager; $this->userSession = $userSession; $this->accountManager = $accountManager; + $this->subAdminManager = $subAdminManager; $this->l10nFactory = $l10nFactory; } @@ -136,8 +142,8 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar $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) { @@ -206,6 +212,34 @@ protected function getUserData(string $userId, bool $includeScopes = false): ?ar return $data; } + /** + * @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 * diff --git a/apps/provisioning_api/lib/Controller/GroupsController.php b/apps/provisioning_api/lib/Controller/GroupsController.php index 4b05f772e8f27..f0712d122618a 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\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 @@ public function __construct(string $appName, IGroupManager $groupManager, IUserSession $userSession, IAccountManager $accountManager, + ISubAdmin $subAdminManager, IFactory $l10nFactory, LoggerInterface $logger) { parent::__construct($appName, @@ -56,6 +58,7 @@ public function __construct(string $appName, $groupManager, $userSession, $accountManager, + $subAdminManager, $l10nFactory ); diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 5be0b6b1464b2..273e63c742dab 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\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 @@ public function __construct( IGroupManager $groupManager, IUserSession $userSession, IAccountManager $accountManager, + ISubAdmin $subAdminManager, IFactory $l10nFactory, private IURLGenerator $urlGenerator, private LoggerInterface $logger, @@ -81,6 +83,7 @@ public function __construct( $groupManager, $userSession, $accountManager, + $subAdminManager, $l10nFactory ); @@ -946,7 +949,7 @@ public function editUser(string $userId, string $key, string $value): DataRespon $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; From 1dcdc87b449a2241d3b715e7eb280e049609b723 Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Fri, 4 Oct 2024 16:28:23 -0700 Subject: [PATCH 3/4] refactor: Clean up Signed-off-by: Christopher Ng --- .../lib/Controller/AUserData.php | 44 +++++-------------- 1 file changed, 11 insertions(+), 33 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/AUserData.php b/apps/provisioning_api/lib/Controller/AUserData.php index 54ef3691caea8..603c63062273f 100644 --- a/apps/provisioning_api/lib/Controller/AUserData.php +++ b/apps/provisioning_api/lib/Controller/AUserData.php @@ -8,7 +8,7 @@ */ 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; @@ -22,7 +22,6 @@ use OCP\Files\NotFoundException; use OCP\Group\ISubAdmin; use OCP\IConfig; -use OCP\IGroupManager; use OCP\IRequest; use OCP\IUser; use OCP\IUserManager; @@ -47,39 +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 ISubAdmin */ - protected $subAdminManager; - /** @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, - ISubAdmin $subAdminManager, - 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->subAdminManager = $subAdminManager; - $this->l10nFactory = $l10nFactory; } /** From 96035d2de300e6dbdd7e99d8cc323d894d1777ec Mon Sep 17 00:00:00 2001 From: Christopher Ng Date: Tue, 8 Oct 2024 16:44:06 -0700 Subject: [PATCH 4/4] test: Fix tests Signed-off-by: Christopher Ng --- .../tests/Controller/GroupsControllerTest.php | 10 +++++----- .../tests/Controller/UsersControllerTest.php | 20 ++++++++----------- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/apps/provisioning_api/tests/Controller/GroupsControllerTest.php b/apps/provisioning_api/tests/Controller/GroupsControllerTest.php index f9c0848fb7249..5ddf5d7eff888 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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $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 d0d7dc7285534..2ccb1196fbbb8 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -23,6 +23,7 @@ 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 @@ protected function setUp(): void { $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 @@ protected function setUp(): void { $this->groupManager, $this->userSession, $this->accountManager, + $this->subAdminManager, $this->l10nFactory, $this->urlGenerator, $this->logger, @@ -376,6 +381,7 @@ public function testAddUserSuccessfulWithDisplayName(): void { $this->groupManager, $this->userSession, $this->accountManager, + $this->subAdminManager, $this->l10nFactory, $this->urlGenerator, $this->logger, @@ -931,7 +937,6 @@ public function testGetUserDataAsAdmin(): void { ->disableOriginalConstructor() ->getMock(); $loggedInUser - ->expects($this->exactly(2)) ->method('getUID') ->willReturn('admin'); $targetUser = $this->getMockBuilder(IUser::class) @@ -941,16 +946,13 @@ public function testGetUserDataAsAdmin(): void { ->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 @@ public function testGetUserDataAsSubAdminAndUserIsAccessible(): void { ->disableOriginalConstructor() ->getMock(); $loggedInUser - ->expects($this->exactly(2)) ->method('getUID') ->willReturn('subadmin'); $targetUser = $this->getMockBuilder(IUser::class) @@ -1090,16 +1091,13 @@ public function testGetUserDataAsSubAdminAndUserIsAccessible(): void { ->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 @@ public function testGetUserDataAsSubAdminSelfLookup(): void { ->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 @@ public function testGetCurrentUserLoggedIn(): void { $this->groupManager, $this->userSession, $this->accountManager, + $this->subAdminManager, $this->l10nFactory, $this->urlGenerator, $this->logger, @@ -3756,6 +3751,7 @@ public function testGetUser(): void { $this->groupManager, $this->userSession, $this->accountManager, + $this->subAdminManager, $this->l10nFactory, $this->urlGenerator, $this->logger,