Proposition for group management

[quenenni]

Hello,

Here is a proposal to add group management.

I was able to make it works, but I couldn't add the option in the configuration form.
I guess it's because I have to recompile the app with compose and other stuffs, but as I did this on a test cloud on a production server, I did'nt want to install all the require dependances for this.

I simply added the option manually in the Db and all is good.

Could you please, when you have a bit of time, give a look at this and tell me if it looks right.

I know we'll need to wait before having this in the official code (if accepted of course), but I really need to have it working asap to put it in production.
We used the other Oidc plugin but had several security problems and decided to stop using it.
But the other plugin is managing groups and in order to switch, I need the group management also with this one.

I tested adding the user to a (new / existing) group, removing from a group, removing the group if group is empty after removing the user, and all is working well on my setup.

I could add an option to delete or not empty groups (once you removed the user from that group) if it's wanted.

1. file lib/Service/ProviderService.php

43.    public const SETTING_MAPPING_QUOTA = 'mappingQuota';
Added ->    public const SETTING_MAPPING_GROUPS = 'mappingGroups';

2. lib/Command/UpsertProvider.php

65.             ->addOption('mapping-quota', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the quota')
Added ->    ->addOption('mapping-groups', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the user groups (must be an array)')

98.                     'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota',
replaced by ->   'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota', 'mapping-groups',

167.             $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_MAPPING_QUOTA, $mapping);
               }
Added ->        if ($mapping = $input->getOption('mapping-groups')) {
Added ->            $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_MAPPING_GROUPS, $mapping);
        }

3. lib/Controller/LoginController.php

31. use OC\Authentication\Token\IProvider;
Added -> use OC\Group\Manager as GroupManager;

64. use OCP\Session\Exceptions\SessionNotAvailableException;
Added -> use OCP\GroupInterface;
Added -> use OCP\IGroupManager;

114.     private $discoveryService;
Added ->    (blank line)
Added ->    /** @var GroupManager */
Added ->     protected $groupManager;

153.             ILogger $logger,   (<- added a ,)
Added ->     IGroupManager $groupManager

175.         $this->request = $request;
Added ->        $this->groupManager = $groupManager;

237.           $quotaAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_QUOTA, 'quota');
Added ->   $groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');

248.                  $quotaAttribute => null,
Added ->           $groupsAttribute => null,

254.                  $quotaAttribute => null,
Added ->           $groupsAttribute => null,

486.                        // get name/email/quota information from the token itself
Replaced by ->       // get name/email/quota/groups information from the token itself

493.                $quota = $idTokenPayload->{$quotaAttribute} ?? null;
Added ->        $groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
Added ->        $groups = $idTokenPayload->{$groupsAttribute} ?? null;

536.        $user->setQuota($event->getValue());
              }

Added all the lines below ->
        // Update groups (create, delete the group or add/remove the user to/from the group)
        // First, get the list of all groups this user is in, before any manipulation
        $userGroups=$this->groupManager->getUserGroupIds($user);

        // Treat each group the user is in
        foreach ($groups as $group) {
            $event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_GROUPS, $idTokenPayload, $group);
            $this->eventDispatcher->dispatchTyped($event);
            $this->logger->debug("Group '$group' mapping event dispatched");
            if ($event->hasValue()) {
                if ($this->groupManager->groupExists($event->getValue())) {
                    $this->logger->debug('Group '.$event->getValue().' already exists');
                    if (! $this->groupManager->isInGroup($user->getUID(), $event->getValue())) {
                        $this->logger->debug('Adding user '.$user->getUID().' to group '.$event->getValue());
                        $groupItem = $this->groupManager->get($event->getValue());
                        $groupItem->addUser($user);
                    }
                } else {
                    $this->logger->debug('Creating group '.$event->getValue());
                    $this->groupManager->createGroup($event->getValue());

                    $this->logger->debug('Adding user '.$user->getUID().' to group '.$event->getValue());
                    $groupItem = $this->groupManager->get($event->getValue());
                    $groupItem->addUser($user);
                }
            }
            // Remove from the userGroups list the one we just managed
            $userGroups = array_merge(array_diff($userGroups, array($event->getValue())));
        }

        // At this point, userGroups list contains only this user groups that he's not in anymore.
        // If the group is empty, we can delete it (maybe add an option in the configuration webui for this feature)
        foreach ($userGroups as $oldGroup) {
            $groupItem = $this->groupManager->get($oldGroup);
            $listUserGroup = $groupItem->getUsers();
            if (count($listUserGroup) == 1 && isset($listUserGroup[$user->getUID()])) {
                $this->logger->debug("Deleting group $oldGroup because it's empty");
                $groupItem->delete();
            } else {
                $this->logger->debug("Removing user ".$user->getUID()." from group $oldGroup");
                $groupItem->removeUser($user);
            }
        }

Here is another way to show the differences if you prefer:

diff --suppress-common-lines -r apps/user_oidc/lib/Command/UpsertProvider.php user_oidc-1.2.1/lib/Command/UpsertProvider.php
66d65
<             ->addOption('mapping-groups', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the user groups (must be an array)')
98c97
<                               'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota', 'mapping-groups',
---
>                               'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota',
168,170d166
<               }
<               if ($mapping = $input->getOption('mapping-groups')) {
<                       $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_MAPPING_GROUPS, $mapping);

diff --suppress-common-lines -r apps/user_oidc/lib/Controller/LoginController.php user_oidc-1.2.1/lib/Controller/LoginController.php
32d31
< use OC\Group\Manager as GroupManager;
65,66d63
< use OCP\GroupInterface;
< use OCP\IGroupManager;
115,117d111
<
<       /** @var GroupManager */
<       protected $groupManager;
153,154c147
<               ILogger $logger,
<               IGroupManager $groupManager
---
>               ILogger $logger
176d168
<               $this->groupManager = $groupManager;
238d229
<               $groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
249d239
<                               $groupsAttribute => null,
255d244
<                               $groupsAttribute => null,
486c475
<               // get name/email/quota/groups information from the token itself
---
>               // get name/email/quota information from the token itself
493,494d481
<               $groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
<               $groups = $idTokenPayload->{$groupsAttribute} ?? null;
537,580d523
<               }
<
<               // Update groups (create, delete the group or add/remove the user to/from the group)
<               // First, get the list of all groups this user is in, before any manipulation
<               $userGroups=$this->groupManager->getUserGroupIds($user);
<
<               // Treat each group the user is in
<               foreach ($groups as $group) {
<                       $event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_GROUPS, $idTokenPayload, $group);
<                       $this->eventDispatcher->dispatchTyped($event);
<                       $this->logger->debug("Group '$group' mapping event dispatched");
<                       if ($event->hasValue()) {
<                               if ($this->groupManager->groupExists($event->getValue())) {
<                                       $this->logger->debug('Group '.$event->getValue().' already exists');
<                                       if (! $this->groupManager->isInGroup($user->getUID(), $event->getValue())) {
<                                               $this->logger->debug('Adding user '.$user->getUID().' to group '.$event->getValue());
<                                               $groupItem = $this->groupManager->get($event->getValue());
<                                               $groupItem->addUser($user);
<                                       }   
<                               } else {
<                                       $this->logger->debug('Creating group '.$event->getValue());
<                                       $this->groupManager->createGroup($event->getValue());
<
<                                       $this->logger->debug('Adding user '.$user->getUID().' to group '.$event->getValue());
<                                       $groupItem = $this->groupManager->get($event->getValue());
<                                       $groupItem->addUser($user);
<                               }
<                       }
<                       // Remove from the userGroups list the one we just managed
<                       $userGroups = array_merge(array_diff($userGroups, array($event->getValue())));
<               }
<
<               // At this point, userGroups list contains only this user groups that he's not in anymore.
<               // If the group is empty, we can delete it (maybe add an option in the configuration webui for this feature)
<               foreach ($userGroups as $oldGroup) {
<                       $groupItem = $this->groupManager->get($oldGroup);
<                       $listUserGroup = $groupItem->getUsers();
<                       if (count($listUserGroup) == 1 && isset($listUserGroup[$user->getUID()])) {
<                               $this->logger->debug("Deleting group $oldGroup because it's empty");
<                               $groupItem->delete();
<                       } else {
<                               $this->logger->debug("Removing user ".$user->getUID()." from group $oldGroup");
<                               $groupItem->removeUser($user);
<                       }

diff --suppress-common-lines -r apps/user_oidc/lib/Service/ProviderService.php user_oidc-1.2.1/lib/Service/ProviderService.php
44d43
<       public const SETTING_MAPPING_GROUPS = 'mappingGroups';

[quenenni]

As I needed to have the admin group managed by another attribute from the claim, I added 2 options.

So we have a total of 3 new options:

• Groups -> name of the token attribute that include the groups (admin group no taken into account by default)
• (check) GroupAdmin -> flag to tell the Groups option to manage the admin group as well
• IsAdmin -> name of the token attribute to manage the admin group (boolean)

If IsAdmin and GroupAdmin are not set, admin group will not be managed (if Groups value include admin).
If GroupAdmin is flagged, IsAdmin is ignored.

I feel it gives a good number of ways to manage the groups and the admin group specifically.

diff --suppress-common-lines -r apps/user_oidc/lib/Command/UpsertProvider.php user_oidc-1.2.1/lib/Command/UpsertProvider.php
66,68d65
<             ->addOption('mapping-groups', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the user groups (must be an array)')
<             ->addOption('check-groupadmin', null, InputOption::VALUE_OPTIONAL, 'Flag if admin group is managed via user groups (or use isAdmin option to map a specific field)')
<             ->addOption('mapping-isadmin', null, InputOption::VALUE_OPTIONAL, 'Attribute mapping of the admin group (GroupAdmin option must be unchecked)')
100c97
<                               'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota', 'mapping-groups', 'check-groupadmin', 'mapping-isadmin',
---
>                               'mapping-uid', 'mapping-display-name', 'mapping-email', 'mapping-quota',
170,178d166
<               }
<               if ($mapping = $input->getOption('mapping-groups')) {
<                       $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_MAPPING_GROUPS, $mapping);
<               }
<               if (($checkGroupAdmin = $input->getOption('check-groupadmin')) !== null) {
<                       $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_CHECK_GROUPADMIN, (string)$checkGroupAdmin === '0' ? '0' : '1');
<               }
<               if ($mapping = $input->getOption('mapping-isadmin')) {
<                       $this->providerService->setSetting($provider->getId(), ProviderService::SETTING_MAPPING_ISADMIN, $mapping);
diff --suppress-common-lines -r apps/user_oidc/lib/Controller/LoginController.php user_oidc-1.2.1/lib/Controller/LoginController.php
32d31
< use OC\Group\Manager as GroupManager;
65,66d63
< use OCP\GroupInterface;
< use OCP\IGroupManager;
115,117d111
<
<       /** @var GroupManager */
<       protected $groupManager;
153,154c147
<               ILogger $logger,
<               IGroupManager $groupManager
---
>               ILogger $logger
176d168
<               $this->groupManager = $groupManager;
238,239d229
<               $groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
<               $isAdmin = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_ISADMIN, 'isadmin');
250,251d239
<                               $groupsAttribute => null,
<                               $isadminAttribute => null,
257,258d244
<                               $groupsAttribute => null,
<                               $isadminAttribute => null,
489c475
<               // get name/email/quota/groups/isadmin information from the token itself
---
>               // get name/email/quota information from the token itself
496,499d481
<               $groupsAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_GROUPS, 'groups');
<               $groups = $idTokenPayload->{$groupsAttribute} ?? null;
<               $isadminAttribute = $this->providerService->getSetting($providerId, ProviderService::SETTING_MAPPING_ISADMIN, 'isadmin');
<               $isadmin = $idTokenPayload->{$isadminAttribute} ?? null;
542,609d523
<               }
<
<               // Update groups (create, delete the group or add/remove the user to/from the group)
<               // First, get the list of all groups this user is in, before any manipulation
<               $userGroups=$this->groupManager->getUserGroupIds($user);
<               $checkGroupAdmin = $this->providerService->getSetting($providerId, ProviderService::SETTING_CHECK_GROUPADMIN, '0') === '1';
<
<               // Treat each group the user is in
<               foreach ($groups as $group) {
<                       // Special admin group is managed by its specific attribute
<                       if ($group == 'admin' && !$checkGroupAdmin) continue;
<
<                       $event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_GROUPS, $idTokenPayload, $group);
<                       $this->eventDispatcher->dispatchTyped($event);
<                       $this->logger->debug("Group '$group' mapping event dispatched");
<                       if ($event->hasValue()) {
<                               if ($this->groupManager->groupExists($event->getValue())) {
<                                       $this->logger->debug('Group '.$event->getValue().' already exists');
<                                       if (! $this->groupManager->isInGroup($user->getUID(), $event->getValue())) {
<                                               $this->logger->debug('Adding user '.$user->getUID().' to group '.$event->getValue());
<                                               $groupItem = $this->groupManager->get($event->getValue());
<                                               $groupItem->addUser($user);
<                                       }   
<                               } else {
<                                       $this->logger->debug('Creating group '.$event->getValue());
<                                       $this->groupManager->createGroup($event->getValue());
<
<                                       $this->logger->debug('Adding user '.$user->getUID().' to group '.$event->getValue());
<                                       $groupItem = $this->groupManager->get($event->getValue());
<                                       $groupItem->addUser($user);
<                               }
<                       }
<                       // Remove from the userGroups list the one we just managed
<                       $userGroups = array_merge(array_diff($userGroups, array($event->getValue())));
<               }
<
<               // At this point, userGroups list contains only this user groups that he's not in anymore.
<               // If the group is empty, we can delete it (maybe add an option in the configuration webui for this feature)
<               foreach ($userGroups as $oldGroup) {
<                       $groupItem = $this->groupManager->get($oldGroup);
<                       $listUserGroup = $groupItem->getUsers();
<                       if ($oldGroup != 'admin' && count($listUserGroup) == 1 && isset($listUserGroup[$user->getUID()])) {
<                               $this->logger->debug("Deleting group $oldGroup because it's empty");
<                               $groupItem->delete();
<                       } else {
<                               $this->logger->debug("Removing user ".$user->getUID()." from group $oldGroup");
<                               $groupItem->removeUser($user);
<                       }
<               }
<
<               // Update admin group
<               if (!$checkGroupAdmin) {
<                       $event = new AttributeMappedEvent(ProviderService::SETTING_MAPPING_GROUPS, $idTokenPayload, strval($isadmin));
<                       $this->eventDispatcher->dispatchTyped($event);
<                       $this->logger->debug("isAdmin mapping event dispatched");
<                       // (boolean)json_decode(strtolower($string)) handles lots of possible variants
<                       // 'true'  => true  
<                       // 'True'  => true  
<                       // '1'     => true  
<                       // 'false' => false 
<                       // 'False' => false 
<                       // '0'     => false 
<                       // 'foo'   => false 
<                       // ''      => false 
<                       $isadminbln = (boolean)json_decode(strtolower($event->getValue()));
<                       if ($isadminbln) {  
<                               $this->groupManager->get("admin")->addUser($user);
<                       }
diff --suppress-common-lines -r apps/user_oidc/lib/Service/ProviderService.php user_oidc-1.2.1/lib/Service/ProviderService.php
44,46d43
<       public const SETTING_MAPPING_GROUPS = 'mappingGroups';
<       public const SETTING_CHECK_GROUPADMIN = 'checkGroupAdmin';
<       public const SETTING_MAPPING_ISADMIN = 'mappingIsAdmin';
53,54c50
<               self::SETTING_SEND_ID_TOKEN_HINT,
<               self::SETTING_CHECK_GROUPADMIN
---
>               self::SETTING_SEND_ID_TOKEN_HINT
137,139d132
<                       self::SETTING_MAPPING_GROUPS,
<                       self::SETTING_CHECK_GROUPADMIN,
<                       self::SETTING_MAPPING_ISADMIN,

If I git clone the app, do my modifications, what is the git command to make a proposal?
Or I can attach the 3 modified file here if you prefer.

[so-rose]

+1 for group management. However, there's already an active and open PR that adds this: #502 .

PR Workflow

@quenenni What you're looking for is the patch command, which makes it easy to apply diffs. If you want to try submitting your solution, you're going to want to fork the repo, then make a Pull Request. Here's the git commands:

git clone https://github.com/nextcloud/user_oidc.git  ## Download your forked repo
# Run the patch command.
git commit -a  ## Commit your changes
git push

Now you can suggest a PR in this repository, and compare it with your fork.

[quenenni]

@so-rose

Thanks for the info.

Unfortunately, I didn't notice it before starting my modifications.

I'm going to wait for the #502 PR, it seems more robust and generic.
I'll manage if that version doesn't offer the possibility to have a boolean value in a field to decide if the user is part of the admin group or not.

Also, thanks for the tips for the PR.
Much appreciated.

I guess we can close this ticket.