New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged

artonge merged 6 commits into master from 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 Pytal, JuliaKirschenheuter, provokateurin and nickvergessen 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 nickvergessen, JuliaKirschenheuter, Pytal and provokateurin

July 10, 2024 15:57

github-advanced-security bot found potential problems

View reviewed changes

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

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 Error

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

@@ @@ -1300,7 +1318,10 @@ @@
               		// 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 Note

Cannot call method getUID on possibly null value

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

@@ @@ -1329,7 +1350,8 @@ @@
               			throw new OCSException('', OCSController::RESPOND_NOT_FOUND);
               		}
-              		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 Note

Cannot call method getUID on possibly null value

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

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

@@ @@ -1574,9 +1613,10 @@ @@
               		// 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 Note

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 Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

apps/provisioning_api/lib/Controller/UsersController.php Dismissed Show dismissed Hide dismissed

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 force-pushed the artonge/feat/user_admin_delegation branch from 2a6824f to 544bfd4 Compare

July 22, 2024 11:17

artonge requested review from blizzz and Pytal and removed request for miaulalala and tcitworld

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 <louis@chmn.me>


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

dff8815

…nagement

Signed-off-by: Louis Chemineau <louis@chmn.me>

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 <louis@chmn.me>


          feat(users): Enable features for delegated user admins

3574d7e

Signed-off-by: Louis Chemineau <louis@chmn.me>


          fix(users): Remove useless filtering condition

5f8c466

Signed-off-by: Louis Chemineau <louis@chmn.me>


          chore: Compile assets

7f0f671

Signed-off-by: Louis Chemineau <louis@chmn.me>

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

167 checks passed

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

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

yemkareems yemkareems 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

Labels

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