How to merge with local craft user based on unique id from saml response

[nettum]

Hi! This is a question more than a bug / feature request. It's kind of related to issue #66.

Here is the workflow I'm trying to get to work with the plugin: 1) I want to ask for as little as possible from the idp on login. In my case I'm only asking for one attribute (a unique id for the users from this service). 2) An administrator creates a user manually in craft CP before the users login. Administrators adds the correct permissions etc. manually as this will vary from user to user. The username is manually set to the same as the unique id we will get from idp after user login (the id is known by the administrators of the site). 3) When a user logs in via the idp, login the user if (and only if!) the user is already in the craft database and have this unique id stored on it. This is because I only want to allow certain preselected users from the idp to login to our service.

So, to make this work I tried the following options:

1) Add saml-sp.php with the following settings:

   'mergeLocalUsers' => true,
   'createUser' => false,
   'autoCreateGroups' => false,
   'syncGroups' => false,

2) Add mapping to the idp in craft admin (e.g. username field):

I'm getting the following error: User save failed: {"email":["E-mail is required."]}.

If I change the mapping above to "email" instead of "username" I get the following error: User save failed: {"email":["E-mail \"unique@user\" has already been taken."]}

I've also tried to map email manually to the user object in the \flipbox\saml\sp\services\Login::EVENT_BEFORE_RESPONSE_TO_USER but then I'm getting the following error: User save failed: {"username":["Username \"unique@user\" has already been taken."]}

Am I misunderstanding how mergeLocalUsers works?

The unique id I get from the idp does not need to be mapped to the username, it could be mapped to a custom field on the user (e.g. idpUserid) but I'm not getting this to work either.

[nettum]

Or maybe a FR after looking into the code. Could the workflow above be possible if we could configure to map username to another attribute than NameID? https://github.com/flipboxfactory/saml-sp/blob/899c77a490848e4b2ebebb52c3551eb8764c8232/src/services/login/User.php#L56-L61

[dsmrt]

👋

Actually, I think you've pointed out a couple bugs and a feature request. Some of these settings are from the early days and need to be reviewed.

1. As for mergeLocalUsers, we put that in place in the early development and never utilized it like we thought we would. It was meant to protect from overwriting existing users if you didn't want that, but there's no way to tell (we haven't built a way through the plugin, at least) when a user was created by Craft or by the plugin. At this point, I don't see any reason for changing that setting from your side. Basically, leave it as the default/true. We will probably deprecate and remove it sooner than later. Please remove mergeLocalUsers from the saml-sp.php so if we do remove it in the future, it's not going to give you any problems.

2. It looks like you are configuring the settings correctly by setting createUser=false, since you only want preselected users to have access. Although, I see an issue with that setting as well. Looks like the user will still be created but it'll error before they login. I need to fix this so the error is thrown before user creation (obviously).

3. As you recommended (https://github.com/flipboxfactory/saml-sp/issues/73#issuecomment-669271491), I think it'd be nice to have an override to look-up and setter for the username that differs from NameID.

I can get these packaged up in a release with hopes of getting something out this week.

[nettum]

Thank you for the clarification! I'll remove the mergeLocaUsers setting. As for 2. and 3. that would be amazing to get in place this week if possible as we are working on a tight schedule.

Thanks for the quick response!

[dsmrt]

I got all of those up in 2.3.0

1. As for mergeLocalUsers, The setting has been deprecated but the functionality of that was removed. So if you still have that in your config, it's not going to break anything but it's not going to do anything either.

2. As for createUser, I moved that to the correct location.

3. For time sake, I add\flipbox\saml\sp\models\Settings::$nameIdAttributeOverride as a config. You can add nameIdAttributeOverride to the saml-sp.php, and it'll be used in place of the NameID.

Let me know if you have any issues with any of this!

[nettum]

Thank you, will test this today!

[nettum]

Tested with the following settings now:

   'nameIdAttributeOverride' => 'uniqueIdFromIDP',
   'createUser' => false,
   'autoCreateGroups' => false,
   'syncGroups' => false,

User that is not created in Craft CP yet, thus should not have access to anything: Exception – yii\base\UserException System doesn't have permission to create a new user.

User that is added and have the correct unique id mapped as username: User was logged in as expected.

Seems to work smoothly. Thanks again!

[nettum]

Or I think I may have found a bug. If I log the user out and then log in the same user again with the same idp I get the following integrity constraint violation:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '3-12474' for key 'saml_sp_provider_identity_providerId_userId_unq_idx'
The SQL being executed was: INSERT INTO `saml_sp_provider_identity` (`providerId`, `nameId`, `userId`, `enabled`, `sessionId`, `lastLoginDate`, `dateCreated`, `dateUpdated`, `uid`) VALUES (3, '<nameId>', 12474, 1, '<sessionId>', '2020-08-06 08:12:23', '2020-08-06 08:12:23', '2020-08-06 08:12:23', '<uid>')
Error Info: Array
(
    [0] => 23000
    [1] => 1062
    [2] => Duplicate entry '3-12474' for key 'saml_sp_provider_identity_providerId_userId_unq_idx'
)

Is this something that also needs code update after e.g. the nameIdAttributeOverride change? I'll test some more to be sure it isn't anything wrong with my settings.

[dsmrt]

I'll add nameId as part of that constraint.

[nettum]

Thanks! Give me a nudge if I should test :+1:

[dsmrt]

Ok ... I think I've got that patched up. Since this is a db thing, please backup the db (at least the saml_sp_provider_identity table) and let me know how it goes.

Thank you!

[nettum]

Thanks! Seems to work nicely now!

[dsmrt]

Awesome! Closing this issue but reach out if you find anything else!