Merging user groups seems broken.

[J-Protz]

I am using this plugin in a craft 3 instance (we're working on the 4 upgrade) where on the front end, users are signing in via gigya, and for the control panel, we are only allowing users to sign in via okta.

For some reason, I cannot seem to get user groups to behave as expected. I referenced these following links found through GH issues.

• https://saml-sp.flipboxfactory.com/configure/groups.html#user-group-saml-sp-configs
• https://github.com/flipboxfactory/saml-sp/blob/master/CHANGELOG.md#added

And I have the following:

saml-sp.php
<?php

return [
    '*' => [
        'requireResponseToBeSigned' => false,
        'requireAssertionToBeSigned' => false,
        'defaultGroupAssignments'=> [], // if this isn't set it adds every okta user to 'customers' group, I have no idea why
        'mergeExistingGroups' => true,//doesnt work
    ]
];

config/project/users/users.yaml
allowPublicRegistration: true
defaultGroup: '' // no default group.
photoSubpath: null
photoVolumeUid: b9b184e1-fc6e-4c4e-860c-3722da3095dd
requireEmailVerification: false
suspendByDefault: false
validateOnPublicRegistration: false

I originally had defaultGroupAssignments set to the customers group ID, because we were only using this plugin for front end. This behaves as expected.

However upon adding the back end SSO, I removed this and moved the logic into the following:

        Event::on(
            \flipbox\saml\sp\services\login\UserGroups::class,
            \flipbox\saml\sp\services\login\UserGroups::EVENT_BEFORE_USER_GROUP_ASSIGN,
            function (\flipbox\saml\sp\events\UserGroupAssign $event) {
                $customerGroup = Craft::$app->userGroups->getGroupByHandle('customers');
                $agentsGroup = Craft::$app->userGroups->getGroupByHandle('agents');
                $event->groupToBeAssigned = [$customerGroup];//default for customers since that is most vital
                $userGroups = $event->user->getGroups();
                try {
                    $origin = Craft::$app->getRequest()->headers->toArray()['origin'][0];
                    if (stripos($origin, 'okta') !== false) {
                          $event->groupToBeAssigned = [$agentsGroup];
                    }
                } catch (\Throwable $e) {
                    Craft::error('Error assigning user group: ' . $e->getMessage(), 'User group assignment');
                }
            }
        );

This almost works as expected, however every time a user logs in via okta, their groups get over-ridden by the groups set here. So if an admin makes an agent also have 1-3 more groups, the next time they log in these groups are removed. This is where I did some digging and found the merge groups config line, however this did nothing for me.

In order to finally get it working as intended, I had to adjust the code to this:

        Event::on(
            \flipbox\saml\sp\services\login\UserGroups::class,
            \flipbox\saml\sp\services\login\UserGroups::EVENT_BEFORE_USER_GROUP_ASSIGN,
            function (\flipbox\saml\sp\events\UserGroupAssign $event) {
                $customerGroup = Craft::$app->userGroups->getGroupByHandle('customers');
                $agentsGroup = Craft::$app->userGroups->getGroupByHandle('agents');
                $event->groupToBeAssigned = [$customerGroup];//default for customers since that is most vital
                $userGroups = $event->user->getGroups();
                try {
                    $origin = Craft::$app->getRequest()->headers->toArray()['origin'][0];
                    if (stripos($origin, 'okta') !== false) {
                        if (count($userGroups) > 0) {
                            $event->groupToBeAssigned = $userGroups;
                        } else {
                            $event->groupToBeAssigned = [$agentsGroup];
                        }
                    }
                } catch (\Throwable $e) {
                    Craft::error('Error assigning user group: ' . $e->getMessage(), 'User group assignment');
                }
            }
        );

I'm not sure if maybe this was fixed in a later iteration, or we have a buggy situation edge case, or what it is. I did not see anything referencing fixing a group assignment bug when searching the issues.

Craft Version: Craft Pro 3.9.5
PHP version : 7.4.33
Plugin Version: 2.7.5

[dsmrt]

First off, You need to have one of these set to true. Figure out which one you can use but this is a BIG security issue if you don't have one set:

'requireResponseToBeSigned' => false,
'requireAssertionToBeSigned' => false,

Just want to make sure to call this out right away. Looking thru the rest of the description now.

[dsmrt]

Going line by line as best as possible here:

defaultGroupAssignments'=> [], // if this isn't set it adds every okta user to 'customers' group, I have no idea why

You may want to check the project config?

'mergeExistingGroups' => true, //doesnt work

In your event code you are showing, you are overwriting this property here:

$event->groupToBeAssigned = [$customerGroup]; //default for customers ...

When mergeExistingGroups is set, groupsToBeAssigned should have all of the user's existing groups there.

See how it's being used here:

saml-sp/src/services/login/UserGroups.php

Lines 190 to 229 in c1991f6

	protected function syncGroups(
	UserElement $user,
	Response $response,
	array $groups,
	Settings $settings
	) {
	$event = new UserGroupAssign();
	$event->user = $user;
	$event->response = $response;
	$event->existingGroups = $user->getGroups();
	$event->groupsFoundInAssertions = $groups;
	$event->groupToBeAssigned = array_merge(
	$settings->mergeExistingGroups ? $user->getGroups() : [],
	$groups
	);
	$this->trigger(
	static::EVENT_BEFORE_USER_GROUP_ASSIGN,
	$event
	);
	if(\Craft::$app->getUsers()->assignUserToGroups(
	$user->id,
	// pass the list of unique ids
	array_unique(
	array_map(
	function ($group) {
	return (int)$group->id;
	},
	$event->groupToBeAssigned
	)
	)
	)) {
	$user->setGroups(
	$event->groupToBeAssigned
	);
	}
	return;
	}

Note the Craft api for setGroups and assignUserToGroups overwrite ALL group assignments when these are called. So you have to pass all desired groups on a user. Which feeds into ...

So if an admin makes an agent also have 1-3 more groups, the next time they log in these groups are removed.

They'll probably only have the customer group right? My technique has been: get existing user groups and merge with the new group I'm setting.

I think if you just copied the second user group example in the docs, here, it work by swapping out the myAdminGroup for your customers group handle.

I hope this answers your questions. Let me know if you need more clarification.

[J-Protz]

First off, You need to have one of these set to true. Figure out which one you can use but this is a BIG security issue if you don't have one set:

'requireResponseToBeSigned' => false,
'requireAssertionToBeSigned' => false,

Just want to make sure to call this out right away. Looking thru the rest of the description now.

This is troubling. I did not set up this config file, a previous dev did. I'll need to test them set to true to see why this was done.

As for the rest i will need to dig in a bit and get back to you. Thank you for the advice

[dsmrt]

For sure. Feel free to reach out directly for more info. damien at flipbox digital

I'll update the docs to be more explicit on the importance of this setting.