feat(users): Add users and group management to admin delegation#46418

Merged

artonge merged 6 commits intomasterfrom

artonge/feat/user_admin_delegation

Jul 24, 2024

Contributor

artonge commented Jul 10, 2024 •

edited

Loading

Done

Fix some error handling in the front end
Create a IDelegatedSettings for users management
Show the 'Accounts' menu to delegated admins
Add the AuthorizedAdminSetting annotation to endpoints that are admin restricted
Tweak the inner permissions conditions in endpoints that are not admin restricted
Hide the 'Admins' section to delegated admins

I suspect the most critical part is to not let delegated admins escalate privileges to full admins. I tried to ensure that this is not possible. So a delegated admin cannot:

Create a user with admin rights
Delete, edit, wipe devices, disable an admin
Add a user to the admin group
Remove an admin from the admin group

But a delegated admin can:

Escalate admin settings delegation by adding himself to a group

But I might have missed a scenario.

Doc feat(admin): Users management delegation documentation#12056

artonge requested review from JuliaKirschenheuter, Pytal, nickvergessen and provokateurin as code owners

July 10, 2024 15:56

artonge marked this pull request as draft

July 10, 2024 15:56

artonge self-assigned this

artonge added enhancement 2. developing php labels

artonge added this to the Nextcloud 30 milestone

artonge removed request for JuliaKirschenheuter, Pytal, nickvergessen and provokateurin

July 10, 2024 15:57

github-advanced-security bot found potential problems

View reviewed changes

apps/provisioning_api/lib/Controller/UsersController.php Outdated

               			// Check if admin / subadmin
-              			if ($isAdminOrSubadmin) {
-              				// They have permissions over the user
+              			if ($isAdminOrSubadmin || $isDelegatedAdmin && !$this>!group|| ger->isInGroup($targetUser->getUID(), 'admin')) {

Check failure

Code scanning / Psalm

UndefinedConstant

Const group is not defined

apps/provisioning_api/lib/Controller/UsersController.php Outdated

               			// Check if admin / subadmin
-              			if ($isAdminOrSubadmin) {
-              				// They have permissions over the user
+              			if ($isAdminOrSubadmin || $isDelegatedAdmin && !$this>!group|| ger->isInGroup($targetUser->getUID(), 'admin')) {

Check failure

Code scanning / Psalm

UndefinedConstant

Const ger is not defined

apps/provisioning_api/lib/Controller/UsersController.php Outdated

-              			if ($isAdminOrSubadmin) {
-              				// They have permissions over the user
+              			if ($isAdminOrSubadmin || $isDelegatedAdmin && !$this>!group|| ger->isInGroup($targetUser->getUID(), 'admin')) {
+              			if ($isAdminOrSubadmin || $isDelegatedAdmin & !$this||grup|| g&&-!>isInGroup($targetUser->getUID(), 'admin')) {

Check failure

Code scanning / Psalm

ParseError

Syntax error, unexpected '>' on line 808

apps/provisioning_api/lib/Controller/UsersController.php Outdated

-              			if ($isAdminOrSubadmin) {
-              				// They have permissions over the user
+              			if ($isAdminOrSubadmin || $isDelegatedAdmin && !$this>!group|| ger->isInGroup($targetUser->getUID(), 'admin')) {
+              			if ($isAdminOrSubadmin || $isDelegatedAdmin & !$this||grup|| g&&-!>isInGroup($targetUser->getUID(), 'admin')) {

Check failure

Code scanning / Psalm

ParseError

Syntax error, unexpected ')' on line 808

apps/provisioning_api/lib/Settings/Admin/Groups.php Outdated

+              	 */
+              	public function getForm(): TemplateResponse {
+              		return new /** @template-extends TemplateResponse<Http::STATUS_OK, array{}> */ class($this->appName, '') extends TemplateResponse {

Check failure

Code scanning / Psalm

InvalidTemplateParam

Extended template param S expects type int, type OCA\Provisioning_API\Settings\Admin\Http::STATUS_OK given

apps/provisioning_api/lib/Controller/UsersController.php Outdated

               		// If not permitted
               		$subAdminManager = $this->groupManager->getSubAdmin();
-              		if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
+              		$isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()) || $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php Outdated

               		}
-              		if ($targetUser->getUID() === $loggedInUser->getUID() || $this->groupManager->isAdmin($loggedInUser->getUID())) {
+              		$isAdmin = $this->groupManager->isAdmin($loggedInUser->getUID()) || $this->groupManager->isDelegatedAdmin($loggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php

               		$loggedInUser = $this->userSession->getUser();
               		$subAdminManager = $this->groupManager->getSubAdmin();
-              		if (!$this->groupManager->isAdmin($loggedInUser->getUID()) && !$subAdminManager->isSubAdminOfGroup($loggedInUser, $group)) {
+              		$isAdmin = $this->groupManager->isAdmin($loggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php

               		// If they're not an admin, check they are a subadmin of the group in question
               		$subAdminManager = $this->groupManager->getSubAdmin();
-              		if (!$this->groupManager->isAdmin($loggedInUser->getUID()) && !$subAdminManager->isSubAdminOfGroup($loggedInUser, $group)) {
+              		$isAdmin = $this->groupManager->isAdmin($loggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php Outdated

               		// Check if admin / subadmin
               		$subAdminManager = $this->groupManager->getSubAdmin();
+              		$isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID()) || $this->groupManager->isDelegatedAdmin($currentLoggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

artonge force-pushed the artonge/feat/user_admin_delegation branch from 2e9f01d to c255339 Compare

July 11, 2024 10:11

github-advanced-security bot found potential problems

View reviewed changes

apps/provisioning_api/lib/Controller/UsersController.php

               					throw new OCSException($this->l10n->t('Group %1$s does not exist', [$group]), 104);
               				}
-              				if (!$isAdmin && !$subAdminManager->isSubAdminOfGroup($user, $this->groupManager->get($group))) {
+              				if (!$isAdmin && !($isDelegatedAdmin && $group !== 'admin') && !$subAdminManager->isSubAdminOfGroup($user, $this->groupManager->get($group))) {

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 2 of OC\SubAdmin::isSubAdminOfGroup cannot be null, possibly null value provided

apps/provisioning_api/lib/Controller/UsersController.php

               		// If not permitted
               		$subAdminManager = $this->groupManager->getSubAdmin();
-              		if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
+              		$isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php

               		// If not permitted
               		$subAdminManager = $this->groupManager->getSubAdmin();
-              		if (!$this->groupManager->isAdmin($currentLoggedInUser->getUID()) && !$subAdminManager->isUserAccessible($currentLoggedInUser, $targetUser)) {
+              		$isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php

               		// If they're not an admin, check they are a subadmin of the group in question
               		$subAdminManager = $this->groupManager->getSubAdmin();
-              		if (!$this->groupManager->isAdmin($loggedInUser->getUID()) && !$subAdminManager->isSubAdminOfGroup($loggedInUser, $group)) {
+              		$isAdmin = $this->groupManager->isAdmin($loggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php

               		// Check if admin / subadmin
               		$subAdminManager = $this->groupManager->getSubAdmin();
+              		$isAdmin = $this->groupManager->isAdmin($currentLoggedInUser->getUID());

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method getUID on possibly null value

artonge force-pushed the artonge/feat/user_admin_delegation branch from c255339 to 752f90c Compare

July 11, 2024 10:22

artonge marked this pull request as ready for review

July 11, 2024 10:25

artonge added 3. to review javascript and removed 2. developing labels

artonge force-pushed the artonge/feat/user_admin_delegation branch 5 times, most recently from 78d9bf1 to c710545 Compare

July 16, 2024 13:35

artonge changed the title ~~feat(users): Add users and group management to admin delegation - WIP~~ feat(users): Add users and group management to admin delegation

artonge force-pushed the artonge/feat/user_admin_delegation branch from e62a668 to 1c5d27c Compare

July 17, 2024 10:27

sorbaugh requested a review from Altahrim

July 17, 2024 12:55

artonge requested a review from Pytal

July 22, 2024 11:18

artonge force-pushed the artonge/feat/user_admin_delegation branch from 544bfd4 to a2b98a7 Compare

July 22, 2024 13:42

artonge mentioned this pull request

feat(admin): Users management delegation nextcloud/documentation#12056

Merged

artonge force-pushed the artonge/feat/user_admin_delegation branch 3 times, most recently from 28c2575 to fb37346 Compare

July 22, 2024 14:18

artonge added 2 commits

July 22, 2024 17:17


          fix(users): Improve error handling of some fields update

1af827f

Signed-off-by: Louis Chemineau <[email protected]>


          feat(users): Add support for admin delegation for users and groups ma…

dff8815

…nagement

Signed-off-by: Louis Chemineau <[email protected]>

artonge force-pushed the artonge/feat/user_admin_delegation branch 3 times, most recently from 4371d2e to 15e73b4 Compare

July 22, 2024 16:08

Pytal approved these changes

View reviewed changes

artonge added 4 commits

July 22, 2024 19:58


          test(users): Adapt tests to new admin delegation checks

26beede

Signed-off-by: Louis Chemineau <[email protected]>


          feat(users): Enable features for delegated user admins

3574d7e

Signed-off-by: Louis Chemineau <[email protected]>


          fix(users): Remove useless filtering condition

5f8c466

Signed-off-by: Louis Chemineau <[email protected]>


          chore: Compile assets

7f0f671

Signed-off-by: Louis Chemineau <[email protected]>

artonge force-pushed the artonge/feat/user_admin_delegation branch from 15e73b4 to 7f0f671 Compare

July 22, 2024 17:59

artonge requested review from ArtificialOwl, skjnldsv and yemkareems

July 23, 2024 12:19

yemkareems approved these changes

View reviewed changes

artonge merged commit 7266a9e into master

artonge deleted the artonge/feat/user_admin_delegation branch

July 24, 2024 09:15

artonge removed the pending documentation label

blizzz mentioned this pull request

30.0.0 beta 1 #46713

Merged

joshtrichards mentioned this pull request

Enable non-admins to edit users and groups #36733

Closed

Contributor

susnux commented Apr 1, 2025

/backport 1af827f to stable29

backportbot bot added the backport-request label

backportbot bot mentioned this pull request

[stable29] fix(users): Improve error handling of some fields update #51834

Merged

2 tasks

backportbot bot removed the backport-request label

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ChristophWurst ChristophWurst left review comments

Pytal Pytal approved these changes

Altahrim Awaiting requested review from Altahrim

come-nc Awaiting requested review from come-nc

icewind1991 Awaiting requested review from icewind1991

blizzz Awaiting requested review from blizzz

ArtificialOwl Awaiting requested review from ArtificialOwl

skjnldsv Awaiting requested review from skjnldsv

+1 more reviewer

yemkareems yemkareems approved these changes

Labels

3. to review enhancement feature: settings feature: users and groups javascript php