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

sebrhex commented 4 months ago

⚠️ This issue respects the following points: ⚠️

[X] This is a bug, not a question or a configuration/webserver/proxy issue.
[X] This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
[X] Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
[X] I agree to follow Nextcloud's Code of Conduct.

Bug description

When I use e.g. occ admin-delegation:add 'OCA\LogReader\Settings\Admin' logreaders I can invoke this several times and subsequently have several identical entries of the same group for the delegation:

Section: logging
----------------

 -------- ------------------------------ ------------------------------------ 
  Name     SettingId                      Delegated to groups                 
 -------- ------------------------------ ------------------------------------ 
  Global   OCA\LogReader\Settings\Admin   logreaders, logreaders, logreaders  
 -------- ------------------------------ ------------------------------------

There should be a check to prevent this as this is at least clogging up the view.

Steps to reproduce

Add an admin delegation using occ admin-delegation:add
Add the identical admin delegation using occ admin-delegation:add
Check the delegations in the "Administration privileges" section of the web frontend or using occ admin-delegation:show

Expected behavior

An existing delegation should not be added.

Installation method

Community Docker image

Nextcloud Server version

29

Operating system

Debian/Ubuntu

PHP engine version

PHP 8.2

Web server

Apache (supported)

Database engine version

PostgreSQL

Is this bug present after an update or on a fresh install?

Fresh Nextcloud Server install

Are you using the Nextcloud Server Encryption module?

Encryption is Disabled

What user-backends are you using?

[X] Default user-backend (database)
[ ] LDAP/ Active Directory
[X] SSO - SAML
[ ] Other

Configuration report

{
    "system": {
        "memcache.local": "\\OC\\Memcache\\APCu",
        "apps_paths": [
            {
                "path": "\/var\/www\/html\/apps",
                "url": "\/apps",
                "writable": false
            },
            {
                "path": "\/var\/www\/html\/custom_apps",
                "url": "\/custom_apps",
                "writable": true
            }
        ],
        "memcache.distributed": "\\OC\\Memcache\\Redis",
        "memcache.locking": "\\OC\\Memcache\\Redis",
        "redis": {
            "host": "***REMOVED SENSITIVE VALUE***",
            "password": "***REMOVED SENSITIVE VALUE***",
            "port": 6379
        },
        "trusted_proxies": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpmode": "smtp",
        "mail_smtphost": "***REMOVED SENSITIVE VALUE***",
        "mail_smtpport": "465",
        "mail_smtpsecure": "ssl",
        "mail_smtpauth": true,
        "mail_smtpauthtype": "LOGIN",
        "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
        "mail_from_address": "***REMOVED SENSITIVE VALUE***",
        "mail_domain": "***REMOVED SENSITIVE VALUE***",
        "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
        "upgrade.disable-web": true,
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "secret": "***REMOVED SENSITIVE VALUE***",
        "datadirectory": "***REMOVED SENSITIVE VALUE***",
        "dbtype": "pgsql",
        "version": "29.0.3.4",
        "overwrite.cli.url": "***REMOVED SENSITIVE VALUE***",
        "dbname": "***REMOVED SENSITIVE VALUE***",
        "dbhost": "***REMOVED SENSITIVE VALUE***",
        "dbport": "",
        "dbtableprefix": "oc_",
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "instanceid": "***REMOVED SENSITIVE VALUE***",
        "default_phone_region": "DE",
        "maintenance_window_start": 1,
        "allow_local_remote_servers": false,
        "debug": false,
        "trusted_domains": [
            "***REMOVED SENSITIVE VALUE***"
        ],
        "htaccess.RewriteBase": "\/",
        "skeletondirectory": "",
        "allow_user_to_change_display_name": false,
        "lost_password_link": "disabled",
        "user_oidc": {
            "use_pkce": true
        },
        "hide_login_form": true,
        "auth.webauthn.enabled": false
    }
}

List of activated Apps

Enabled:
  - activity: 2.21.1
  - cloud_federation_api: 1.12.0
  - dav: 1.30.1
  - drawio: 3.0.2
  - federatedfilesharing: 1.19.0
  - files: 2.1.0
  - files_external: 1.21.0
  - files_trashbin: 1.19.0
  - files_versions: 1.22.0
  - groupfolders: 17.0.1
  - logreader: 2.14.0
  - lookup_server_connector: 1.17.0
  - notes: 4.10.0
  - oauth2: 1.17.0
  - onlyoffice: 9.3.0
  - provisioning_api: 1.19.0
  - quota_warning: 1.19.0
  - serverinfo: 1.19.0
  - settings: 1.12.0
  - suspicious_login: 7.0.0
  - text: 3.10.1
  - theming: 2.4.0
  - twofactor_backupcodes: 1.18.0
  - updatenotification: 1.19.1
  - user_oidc: 5.0.3
  - viewer: 2.3.0
  - workflowengine: 2.11.0
Disabled:
  - admin_audit: 1.19.0
  - bruteforcesettings: 2.9.0
  - circles: 29.0.0-dev (installed 29.0.0-dev)
  - comments: 1.19.0 (installed 1.19.0)
  - contactsinteraction: 1.10.0 (installed 1.10.0)
  - dashboard: 7.9.0 (installed 7.9.0)
  - encryption: 2.17.0
  - federation: 1.19.0 (installed 1.19.0)
  - files_downloadlimit: 2.0.0 (installed 2.0.0)
  - files_pdfviewer: 2.10.0 (installed 2.10.0)
  - files_reminders: 1.2.0 (installed 1.2.0)
  - files_sharing: 1.21.0 (installed 1.21.0)
  - firstrunwizard: 2.18.0 (installed 2.18.0)
  - nextcloud_announcements: 1.18.0 (installed 1.18.0)
  - notifications: 2.17.0 (installed 2.17.0)
  - password_policy: 1.19.0 (installed 1.19.0)
  - photos: 2.5.0 (installed 2.5.0)
  - privacy: 1.13.0 (installed 1.13.0)
  - recommendations: 2.1.0 (installed 2.1.0)
  - related_resources: 1.4.0 (installed 1.4.0)
  - sharebymail: 1.19.0 (installed 1.19.0)
  - support: 1.12.0 (installed 1.12.0)
  - survey_client: 1.17.0 (installed 1.17.0)
  - systemtags: 1.19.0 (installed 1.19.0)
  - twofactor_admin: 4.5.0 (installed 4.5.0)
  - twofactor_totp: 11.0.0-dev (installed 11.0.0-dev)
  - user_ldap: 1.20.0
  - user_status: 1.9.0 (installed 1.9.0)
  - weather_status: 1.9.0 (installed 1.9.0)

Nextcloud Signing status

No errors have been found.

Nextcloud Logs

No response

Additional info

No response

joshtrichards commented 4 months ago

Please see if the following patch addresses it for you:

diff --git a/apps/settings/lib/Command/AdminDelegation/Add.php b/apps/settings/lib/Command/AdminDelegation/Add.php
index 4ec6510f205..73c995b4762 100644
--- a/apps/settings/lib/Command/AdminDelegation/Add.php
+++ b/apps/settings/lib/Command/AdminDelegation/Add.php
@@ -40,16 +40,24 @@ class Add extends Base {
                $io = new SymfonyStyle($input, $output);
                $settingClass = $input->getArgument('settingClass');
                if (!in_array(IDelegatedSettings::class, (array) class_implements($settingClass), true)) {
-                       $io->error('The specified class isn’t a valid delegated setting.');
+                       $io->error('The specified class is not a valid delegated setting.');
                        return 2;
                }

                $groupId = $input->getArgument('groupId');
                if (!$this->groupManager->groupExists($groupId)) {
-                       $io->error('The specified group didn’t exist.');
+                       $io->error('The specified group does not exist.');
                        return 3;
                }

+               $groups = $this->authorizedGroupService->findExistingGroupsForClass($settingClass);
+               foreach ($groups as $group) {
+                       if ($group->getGroupId() === $groupId) {
+                               $io->error('The specified group has already been delegated to.');
+                               return 4;
+                       }
+               }
+
                $this->authorizedGroupService->create($groupId, $settingClass);

                $io->success('Administration of '.$settingClass.' delegated to '.$groupId.'.');

sebrhex commented 4 months ago

Thank you very much for the fast reply, I can confirm this works for me! :smile:

nextcloud / server