Skip to content

Conversation

@printminion-co
Copy link

@printminion-co printminion-co commented Nov 24, 2025

Summary

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.                                      
  • Resolves: #

Summary

TODO

  • ...

Checklist

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug where admin delegation could be assigned multiple times to the same group for the same setting class, creating duplicate database entries. The fix adds duplicate detection by checking for existing assignments before creation and throwing a ConflictException when duplicates are attempted.

Key Changes:

  • Introduced ConflictException for duplicate assignment detection
  • Modified AuthorizedGroupService::create() to check for existing assignments before inserting
  • Updated the admin-delegation:add command to handle conflicts gracefully with exit code 4
  • Added comprehensive test coverage for the duplicate prevention logic

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 handling duplicate assignment conflicts
apps/settings/lib/Service/AuthorizedGroupService.php Added duplicate checking logic before creating new authorized group assignments
apps/settings/lib/Command/AdminDelegation/Add.php Added exception handling for conflicts with user-friendly warning message
apps/settings/tests/Service/AuthorizedGroupServiceTest.php Added unit tests for duplicate prevention and updated existing tests to mock duplicate checks
apps/settings/tests/Command/AdminDelegation/AddTest.php Added tests for conflict handling in the CLI command
apps/settings/tests/Integration/DuplicateAssignmentIntegrationTest.php New integration test suite verifying end-to-end duplicate prevention
apps/settings/composer/composer/autoload_classmap.php Updated autoloader mapping for new ConflictException class
apps/settings/composer/composer/autoload_static.php Updated static autoloader for new ConflictException class

$this->expectException(\OCP\AppFramework\Db\DoesNotExistException::class);
try {
$this->service->find($initialId);
} catch (\OCA\Settings\Service\NotFoundException $e) {
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test expects DoesNotExistException but catches NotFoundException. This mismatch will cause the test to fail if DoesNotExistException is thrown. Either remove the expectException call since you're using try-catch, or catch the expected exception type.

Suggested change
} catch (\OCA\Settings\Service\NotFoundException $e) {
} catch (\OCP\AppFramework\Db\DoesNotExistException $e) {

Copilot uses AI. Check for mistakes.
@printminion-co printminion-co force-pushed the feat/no_double_admin_delegations branch from d03437c to 820fe24 Compare November 24, 2025 20:28
@printminion-co printminion-co force-pushed the feat/no_double_admin_delegations branch from 820fe24 to 26d9304 Compare November 24, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants