Existing user is identified by matching uniqueIdField with users.name but should be users.init [#2561611]

If the automatic provisioning of user accounts via SSO is turned off, the user is matched by comparing
user.name and SAML attribute configured under "user's unique Identifier" (e.g. eduPersonPrincipalName) even though the user's name is configured to use a different field (e.g. displayName or cn).

This is incorrect because the unique identifier of a user is in the users.init field in Drupal. Also, it means that a user must always have his or her unique id as a user name (e.g.: kjell789faculty@feide.no) and cannot change the user name afterwards without getting a new account or being blocked.

This is also inconsistent with the way a user is created when provisioning users automatically:
user's display name => users.name
unique id => users.init
users name => users.name
users email => users.mail
This applies to all versions (6, and 7) I have checked.

Comments

Comment #1

2 September 2015 at 09:30

LiceBaseAdmin created an issue. See original summary.

Comment #2

LiceBaseAdmin CreditAttribution: LiceBaseAdmin commented 2 September 2015 at 09:56

In addition, displayName (unlike eduPersonPrincipalName) is not enforced to be a (side-wide) unique SAML attribute, while in drupal the username is enforced to be unique; this can lead to a likely collision where a new user with a displayName identical to a user already registered can take over an existing account. This would imply that displayName or cn may never be used to provision user accounts (despite the help text) and that always eduPersonPrincipalName must be used, which is inconvenient.

I propose to add a configuration option to choose either the email field or the init field for account mapping, and to never use the user name field. In addition, the provided user name needs to be checked for existence in the database and altered in case it exists already.

Comment #3

LiceBaseAdmin CreditAttribution: LiceBaseAdmin commented 2 September 2015 at 09:58

Version:	6.x-2.7	» 7.x-3.x-dev
Priority:	Normal	» Major
Issue tags:		+saml authentication

Comment #4

snufkin CreditAttribution: snufkin commented 3 September 2015 at 14:54

The problem is that in Drupal the init property is not independent, as far I understand. On registration it picks up the email address (https://api.drupal.org/api/drupal/modules%21user%21user.module/function/...).

We then have to decide to use either the name, or the email Drupal properties to use as unique identifiers. If we used email, we restrict the users to a certain pattern from the SAML side, which there were objections to (see [##2357879]). So basically the only option for us is to use the name property, and accept the compromise that yes, the users name in Drupal will have to be the unique identifier provided by SAML.

Do you see a way where we could capture user.init separately?

Comment #5

svendecabooter

Dutch

Gent

CreditAttribution: svendecabooter for Acquia commented 16 January 2016 at 15:44

This is related to the issue I posted here: https://www.drupal.org/node/2357879#comment-10755018
Didn't find this one before reopening that issue, sorry.

I think using the "init" field is still a valid solution.
* When a user gets registered through provisioning via SAML, his unique_id will be used for the authmap entry, and stored in {users}.init, where it will remain untouched, unless altered by another Drupal (contrib) module or by manual changes to the database.
user_register_submit() is only called by the Drupal registration form, which is not the case when the user gets registered through user_external_login_register().
* When a user gets registered through Drupal, he gets a unique username & the init value contains his initial email address. He has no authmap entry.
When this user uses federated login afterwards, there is a check (already in codebase) to see if the SAML authname maps with a Drupal username. If that's the case, the Drupal user gets an authmap entry.

So I think the best solution would be:
* Make sure the authmap table always contains the SAML unique_id.
* Never use {users}.name or {users}.init for mapping checks, except to find pre-registered users if configuration option is checked.
* Refactor "Enable this user to leverage SAML authentication" functionality, to check for an existing {authmap} entry first, and if none exists, only then add the Drupal username to the authmap table. Though there will probably still be a problem if you disable SAML authentication first, because it might be enabled again through the code that maps the SAML authname with Drupal username and adds an authmap entry. The enable/disable switch should probably be a separate user field to fix this.

This needs more testing & thinking through. I'll try to fix this in 8.x-3.x, and try to provide a backport for 7.x-3.x