fix(admin-delegation): Prevent delegation to group if delegation already #56646

printminion-co · 2025-11-24T20:56:34Z

Resolves: [Bug]: occ admin-delegation:add allows adding the same delegation multiple times #46609

Before

Multiple delegation to same group is possible

$ php occ admin-delegation:add OCA\\Settings\\Settings\\Admin\\Overview admin
 [OK] Administration of OCA\Settings\Settings\Admin\Overview delegated to admin.                                        

$ php occ admin-delegation:add OCA\\Settings\\Settings\\Admin\\Overview admin
 [OK] Administration of OCA\Settings\Settings\Admin\Overview delegated to admin.                                        

$ php occ admin-delegation:add OCA\\Settings\\Settings\\Admin\\Overview admin
 [OK] Administration of OCA\Settings\Settings\Admin\Overview delegated to admin.                                        

$ php occ admin-delegation:show 

Current delegations
===================

Section: overview
-----------------

 ------------------------- -------------------------------------- --------------------- 
  Name                      SettingId                              Delegated to groups  
 ------------------------- -------------------------------------- --------------------- 
  Security & setup checks   OCA\Settings\Settings\Admin\Overview   admin, admin, admin  
 ------------------------- -------------------------------------- ---------------------

After fix

Fixes issue with delegation via occ and web.
No multiple entries in DB.

php occ admin-delegation:add OCA\\Settings\\Settings\\Admin\\Overview admin

 [OK] Administration of OCA\Settings\Settings\Admin\Overview delegated to admin.

php occ admin-delegation:add OCA\\Settings\\Settings\\Admin\\Overview admin

 [WARNING] The OCA\Settings\Settings\Admin\Overview is already delegated to admin.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)
Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
Milestone added for target branch/version (ex: 32.x for stable32)

Copilot

Pull request overview

This PR prevents duplicate admin delegation assignments by adding validation to check if a group is already delegated to a specific settings class before creating a new assignment.

Key Changes:

Introduces duplicate detection logic in AuthorizedGroupService::create() that throws ConflictException when attempting to assign the same group to the same class twice
Updates the CLI command to handle the conflict gracefully with a warning message and appropriate exit code
Adds comprehensive unit and integration tests to verify duplicate prevention behavior

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
apps/settings/lib/Service/ConflictException.php	New exception class for signaling duplicate assignment conflicts
apps/settings/lib/Service/AuthorizedGroupService.php	Adds duplicate check before creating assignments, throwing ConflictException when duplicates are detected
apps/settings/lib/Command/AdminDelegation/Add.php	Catches ConflictException and displays warning message with exit code 4
apps/settings/tests/Service/AuthorizedGroupServiceTest.php	Unit tests verifying duplicate prevention and allowed multi-group/multi-class scenarios
apps/settings/tests/Command/AdminDelegation/AddTest.php	Unit test for CLI command's duplicate handling behavior
apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php	Integration tests validating end-to-end duplicate prevention workflow
apps/settings/composer/composer/autoload_static.php	Registers new ConflictException class in autoloader
apps/settings/composer/composer/autoload_classmap.php	Adds ConflictException to class map

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-24T21:57:41Z

apps/settings/lib/Service/AuthorizedGroupService.php

+			$existing = $this->mapper->findByGroupIdAndClass($groupId, $class);
+			if ($existing) {
+				throw new ConflictException('Group is already assigned to this class');
+			}


The condition if ($existing) is redundant. The findByGroupIdAndClass method will either return an entity or throw DoesNotExistException. If it returns, the entity will always be truthy. The exception should be thrown immediately after the method call without the conditional check.

Suggested change

$existing = $this->mapper->findByGroupIdAndClass($groupId, $class);

if ($existing) {

throw new ConflictException('Group is already assigned to this class');

}

$this->mapper->findByGroupIdAndClass($groupId, $class);

throw new ConflictException('Group is already assigned to this class');

Copilot · 2025-11-24T21:57:42Z

apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php

+		$this->service->delete($initialId);
+
+		// Verify it's deleted by trying to find it
+		$this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class);


The test expects DoesNotExistException on line 111 but catches NotFoundException on line 114. These are different exception types. The expectException should be removed since the test is manually catching and handling the exception, or the caught exception type should match the expected one.

Suggested change

$this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class);

come-nc

It would be a bit cleaner to add a unique constraint on the table and simply ignore the exception on insert if it’s a REASON_UNIQUE_CONSTRAINT_VIOLATION I think. But that would require a migration to remove existing duplicates and stuff, not sure it’s worth it.

apps/settings/lib/Command/AdminDelegation/Add.php

apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php

github-actions · 2025-12-09T02:10:58Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

artonge

The suggestion should be enough to fix the nodb test

apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php

…ady exist Signed-off-by: Misha M.-Kupriyanov <[email protected]>

printminion-co · 2025-12-11T16:52:48Z

/backport to stable31

printminion-co · 2025-12-11T16:52:59Z

/backport to stable32

backportbot · 2025-12-11T16:53:20Z

The backport to stable31 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable31
git pull origin stable31

# Create the new backport branch
git checkout -b backport/56646/stable31

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 770ad624

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/56646/stable31

Error: Failed to check for changes with origin/stable31: No changes found in backport branch

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

printminion-co requested a review from a team as a code owner November 24, 2025 20:56

printminion-co requested review from Altahrim, leftybournes, salmart-dev and yemkareems and removed request for a team November 24, 2025 20:56

printminion-co force-pushed the fix/no_double_admin_delegations branch from 8cd9c11 to 74a6b24 Compare November 24, 2025 21:05

AndyScherzinger requested review from artonge and sorbaugh November 24, 2025 21:36

AndyScherzinger added the 3. to review Waiting for reviews label Nov 24, 2025

printminion-co requested a review from Copilot November 24, 2025 21:57

Copilot AI reviewed Nov 24, 2025

View reviewed changes

come-nc reviewed Nov 25, 2025

View reviewed changes

apps/settings/lib/Command/AdminDelegation/Add.php Outdated Show resolved Hide resolved

apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php Outdated Show resolved Hide resolved

printminion-co force-pushed the fix/no_double_admin_delegations branch 3 times, most recently from d6116fc to 59d9bf4 Compare November 27, 2025 13:18

artonge approved these changes Dec 2, 2025

View reviewed changes

sorbaugh approved these changes Dec 2, 2025

View reviewed changes

artonge requested a review from come-nc December 2, 2025 09:58

printminion-co force-pushed the fix/no_double_admin_delegations branch from 59d9bf4 to 72b3a51 Compare December 2, 2025 11:16

come-nc approved these changes Dec 2, 2025

View reviewed changes

printminion-co force-pushed the fix/no_double_admin_delegations branch from 72b3a51 to 8192a06 Compare December 4, 2025 08:17

github-actions bot added the feedback-requested label Dec 9, 2025

printminion-co force-pushed the fix/no_double_admin_delegations branch from 8192a06 to cd10528 Compare December 9, 2025 08:38

artonge reviewed Dec 10, 2025

View reviewed changes

apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php Show resolved Hide resolved

fix(admin-delegation): Prevent delegation to group if delegation alre…

770ad62

…ady exist Signed-off-by: Misha M.-Kupriyanov <[email protected]>

printminion-co force-pushed the fix/no_double_admin_delegations branch from b778710 to 770ad62 Compare December 10, 2025 14:09

artonge merged commit 3efb1d8 into master Dec 11, 2025
232 of 249 checks passed

artonge deleted the fix/no_double_admin_delegations branch December 11, 2025 16:45

backportbot bot added the backport-request label Dec 11, 2025

backportbot bot mentioned this pull request Dec 11, 2025

[stable32] fix(admin-delegation): Prevent delegation to group if delegation already #57013

Open

2 tasks

backportbot bot removed the backport-request label Dec 11, 2025

printminion-co mentioned this pull request Dec 16, 2025

[stable31] fix(admin-delegation): Prevent delegation to group if delegation alre… #57119

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(admin-delegation): Prevent delegation to group if delegation already #56646

fix(admin-delegation): Prevent delegation to group if delegation already #56646

Uh oh!

printminion-co commented Nov 24, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

Copilot AI Nov 24, 2025

Uh oh!

come-nc left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 9, 2025

Uh oh!

artonge left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

printminion-co commented Dec 11, 2025

Uh oh!

printminion-co commented Dec 11, 2025

Uh oh!

backportbot bot commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

fix(admin-delegation): Prevent delegation to group if delegation already #56646

fix(admin-delegation): Prevent delegation to group if delegation already #56646

Uh oh!

Conversation

printminion-co commented Nov 24, 2025

Before

After fix

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 9, 2025

Uh oh!

artonge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

printminion-co commented Dec 11, 2025

Uh oh!

printminion-co commented Dec 11, 2025

Uh oh!

backportbot bot commented Dec 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

artonge left a comment •

edited

Loading