Feature Request: Auto-assign default user group

[benjaminkohl]

We have a site with a user group that is designated as the default group in the Craft users settings. We just realized a day after launching that there are hundreds of accounts that have been created and none of them have been assigned to the default user group so the users' permissions are all messed up on the site.

These users originate from Okta and the user group in question is not something they designate there.

Can you please update the plugin to honor Craft's default user group when one isn't supplied and a new user is created?

[dsmrt]

The only time \craft\services\Users::assignUserToDefaultGroup is called is within a public registration, where "Allow public registration?" is checked. It doesn't make sense to utilize this default group when it's specific to public registration because that's probably not going to be enabled if you are using this plugin.

I think it'd be a nice feature to have a default group or a list of groups within the plugin settings to easily assign all users to. I'll move forward with that.

If you need to get this working right away, you can use an event like below:

        Event::on(
            \flipbox\saml\sp\services\Login::class,
            \flipbox\saml\sp\services\Login::EVENT_AFTER_RESPONSE_TO_USER,
            function (\flipbox\saml\sp\events\UserLogin $event) {

                /** @var \craft\elements\User $user */
                $user = $event->user;

                $groups = [];

                #change to the applicable handle
                $group = \Craft::$app->getUserGroups()->getGroupByHandle('default');
                $groups[] = $group->id;

                \Craft::$app->getUsers()->assignUserToGroups($user->id, $groups);
            }
        );

[dsmrt]

Just a heads up on the above code snippet. The Craft user service method assignUserToGroups deletes any existing groups and adds the ones you've passed. Make sure to add the existing groups to the array if you don't want to lose them.

[dsmrt]

After the realization the aforementioned deletion of groups, I found the following is more thorough. This will account for existing groups and make sure all assigned groups are set back on the user. You want to assign the groups back to the user in case another event/operation needs it.

Event::on(
            \flipbox\saml\sp\services\Login::class,
            \flipbox\saml\sp\services\Login::EVENT_AFTER_RESPONSE_TO_USER,
            function (\flipbox\saml\sp\events\UserLogin $event) {

                /** @var \craft\elements\User $user */
                $user = $event->user;

                /**
                 * get existing groups
                 */
                $groups = [];
                foreach ($user->getGroups() as $group) {
                    $groups[$group->id] = $group;
                }

                /**
                 * get the default group by handle
                 */
                $group = \Craft::$app->getUserGroups()->getGroupByHandle('default');

                /**
                 * add it to the group array
                 */
                $groups[$group->id] = $group;

                /**
                 * get an array of ids
                 */
                $groupIds = array_map(
                    function ($group) {
                        return $group->id;
                    },
                    $groups
                );

                /**
                 * Assign them to the user
                 */
                if (\Craft::$app->getUsers()->assignUserToGroups($user->id, $groupIds)) {
                    /**
                     * Set the groups back on the user just in case it's being used after this.
                     *
                     * This may seem strange because the they do this in the `assignUserToGroups`
                     * method but the user they set the groups to isn't *this* user object,
                     * so this is needed.
                     */
                    $user->setGroups($groups);
                }
            }
        );

[dsmrt]

Added this feature. You can assign all users that login with SAML to a group by adding the user group id in the defaultGroupAssignments config key. Edit or add a config/saml-sp.php config file and add the key like so:

<?php

return [
    'defaultGroupAssignments' => [
        # User Group Id
        4,
    ],
];

[benjaminkohl]

Thank you for implementing this feature so quickly. Our client will be very pleased.

[dsmrt]

Thank you for the feedback! Glad it’s working out and let me know if you find anything else.

[benjaminkohl]

It looks like this new setting is being applied every time a user is logged in rather than just when they are created. A number of our existing users that are assigned to various administrative groups are losing their group assignments whenever they access the site and our client keeps having to reassign them each time.

[benjaminkohl]

Or could it be the syncGroups plugin setting being true when there are no groups coming over from the IDP?

[dsmrt]

Looking into this now.

[dsmrt]

I think you might be onto something with the syncGroups. \flipbox\saml\sp\services\login\UserGroups::assignDefaultGroups DOES account for existing groups (so that should be good) but there might be a bug within \flipbox\saml\sp\services\login\UserGroups::syncByAssertion deleting existing groups. That craft method, \craft\services\Users::assignUserToGroups works differently than I would expect. Since you aren't using the syncGroups functionality, it might be worth switching that to false or making sure that a groups attribute isn't being sent over. If a groups attribute is being sent over and you don't need it you may want to just turn it off in OKTA as well.

Still doing some investigation.

[benjaminkohl]

Ok, thank you. I disabled the syncGroups setting and I am awaiting feedback from the client.

[dsmrt]

Ok. Seems like turning off the syncGroups should have worked. There is a bug with \flipbox\saml\sp\services\login\UserGroups::syncByAssertion not accounting for \craft\services\Users::assignUserToGroups deleting groups, which I think is unintuitive and bizarre behavior (but that is beside the point).

I have a pull request in progress now: https://github.com/flipboxfactory/saml-sp/pull/22

[dsmrt]

@benjaminkohl I got that fix merged into 1.0.6 if you want to give that an update.

composer update flipboxfactory/saml-sp

It seems like there are groups were being passed from OKTA (correct me if I'm wrong) so changing syncGroups=false would have done the trick. With that said I patched UserGroups::syncByAssertion so that existing user groups wouldn't be deleted.

In response to an earlier question, UserGroups::syncByAssertion and UserGroups::assignDefaultGroups are run on every user sync/login, not just creation. If this is not desired behavior, you may want to go the event route with the business logic you need (as shown above in a previous comment).

Let me know how all of this works out. I'll leave this issue open for now.

[benjaminkohl]

I'll look into getting an event handler in place that only assigns the user groups to users that are non-admin and don't have a user group. Thanks.