Problem: when I save a provisioned user in Drupal, the authmap table is updated with the wrong authname and the user can no longer log in.

In config, under USER INFO AND SYNCING i have:
users name: eduPersonPrincipalName
unique identifier: eduPersonPrincipalName
mail: mail

Register users (provisioning) is ON, everything else is turned off, except that uid 1 can login in locally.

When a user logs in for the first time an account is created. In the users table, 'name' is set to eduPersonPrincipalName, however 'mail' is empty.

Ok, so I set 'mail' myself just to get past this point. Flush caches etc and I'm back in business.

If I look at 'authmap' everything is fine. 'authname' is eduPersonPrincipalName as it should be.

Now, if I edit a user, I see that 'Enable this user to leverage SAML authentication' is checked. I then proceed to add a role and save. (Reevaluate roles every time the user logs in is turned OFF).

After save, the 'authname' in 'authmap' is changed from eduPersonPrincipalName to the mail address. If I go back to the user, the checkbox 'Enable this user to leverage SAML authentication' is OFF. The result is that the user can't log in anymore. It will try to re-register the user but stops here: "User [blabla@example.com] could not be registered because that username already exists and is not SAML enabled."

(blabla@example.com is the eduPersonPrincipalName).

I guess it can't log in because the 'authname' is changed, and it can't register the user anew because 1) the username exists and 2) the username is not SAML enabled. I don't think "not SAML enabled" is something to look at right now, the problem I think is the authmap. If this is solved, perhaps the SAML enabled-part will get solved too, so this issue focuses on the authmap.

There are several things going on here, but I think the problem is this: simplesamlphp_auth_unique_id can never be anything else than mail. When I save a user in Drupal, authname is set to the mail address. The config option "unique identifier: eduPersonPrincipalName" is ignored/overridden.

Looking at simplesamlphp_auth.module I see this function:

function _simplesamlphp_auth_get_authname() {
  global $_simplesamlphp_auth_saml_attributes;

  $authname = '';

  // Check if valid local session exists.
  if (isset($_simplesamlphp_auth_saml_attributes)) {
    _simplesaml_auth_debug(t('_simplesamlphp_auth_get_authname: Valid local SAML session exists'));
    if (isset($_simplesamlphp_auth_saml_attributes[variable_get('simplesamlphp_auth_unique_id', 'eduPersonPrincipalName')])) {
      $authname = $_simplesamlphp_auth_saml_attributes[variable_get('simplesamlphp_auth_unique_id', 'eduPersonPrincipalName')][0];
    }
    else {
      throw new Exception(t('Error in simplesamlphp_auth.module: no valid unique id attribute set.'));
    }
  }

  return $authname;
}

If simplesamlphp_auth_unique_id is not set, it defaults to eduPersonPrincipalName, which is what I want, but Drupal insists on using mail as simplesamlphp_auth_unique_id.

I don't have any code that interferes with the authmap myself.

I don't see how this module can work for anyone unless simplesamlphp_auth_unique_id set as mail, because Drupal will always change the authmap to mail. Do anyone have a working site with the 3.x-version? Can you edit users and still have them log in afterwards? I find this issue to be critical. I really hope it's just me that have misunderstood something, please enlighten me.

CommentFileSizeAuthor
#10 2357879-use_name_as_authmap-simplesamlphp_auth.patch461 bytessnufkin

Comments

odegard’s picture

odegard commented

I think I've found the bug, if it is a bug. Please can someone share their thoughts with me on this.

The symptom is that the authname in authmap is set to the mail instead of eduPersonPrincipalName when saving the user in Drupal.

There is this function here:

/**
 * Submit callback to enable SAML authentication for a given user.
 */
function simplesaml_auth_user_profile_form_submit(&$form, $form_state) {
  $values = $form_state['values'];

  if (isset($values['saml'])) {
    // Enter this username into the authmap table.
    if ($values['saml']) {
      db_merge('authmap')
        ->key(array(
          'uid' => $values['uid'],
          'module' => 'simplesamlphp_auth',
        ))
        ->fields(array(
          'authname' => $values['mail'],
        ))
        ->execute();
    }
    // Remove this username from the authmap table.
    else {
      db_delete('authmap')
        ->condition('uid', $values['uid'])
        ->execute();
    }
  }
}

It's this line in particular:

          'authname' => $values['mail'],

that assumes that the authname is mail. If I change this to 

          'authname' => $values['name'],

everything works as expected! The user can now login because the authname doesn't change but is set to name... which is eduPersonPrincipalName.

I believe this is a bug. The code assumes 'mail', but it should rather be 'name', right?

BPm’s picture

BPm commented

Confirm using $values['name'] works.

msound’s picture

msound commented

I did not change $values['mail'] to $values['name']. Instead in the config form, I set the "Which attribute from simpleSAMLphp should be used as unique identifier for the user" to "email".

odegard’s picture

odegard commented

@msound, yes that will work. The problem is when you don't want to use 'mail' as unique identifier.

msound’s picture

msound commented

@odegard: Now that https://www.drupal.org/node/2387463 is fixed, can you retest and confirm if "mail" field for a newly auto-created user is now having the correct value. I understand this fixes only the first half of your problem described in your issue.

snufkin’s picture

snufkin commented

Correct me if i'm wrong, but it doesn't matter what we are using for authname (username or email) as long it is unique and consistent.

When the users profile is saved as "use saml", it is adding an authmap entry using the users email address to the authmap table. When we are checking if the user can log in via SAML, we are performing a lookup against the $account->init property, which is the email address. This check happens in simplesaml_auth.module:173 and 272. If we were to change the authname to be the username, these checks would then fail.

The init property in the login form submission is set here: https://api.drupal.org/api/drupal/modules%21user%21user.module/function/...

I'm inclined to say that this issue is a result of #2387463: hook_user_insert exiting prematurely and not a bug on its own.

odegard’s picture

odegard commented

@snufkin, you're not wrong, but my usecase is different from yours and perhaps everyone elses. I can't use email as unique identifier but rather eduPersonPrincipalName, but I sort of understand why the code is what it is, because it is always *assumed* that email is unique.

To tell you the whole story. We've made a product for the higher education sector in Norway (and soon everywhere else). In Norway there is a central agency, Uninett, which facilitates identity management through what is called Feide. All higher education institutions in Norway use Feide for single sign-on to pretty much everything, including our product.

According to their documentation mail can not be considered unique:
https://www.feide.no/attribute/mail
so we've used the recommended eduPersonPrincipalName instead:
https://www.feide.no/attribute/edupersonprincipalname

I have no experience with SAML anywhere else, but I guess that mail not being unique is special for norway? (I don't understand *why* it's not unique... but I don't ask questions, I just implement. :))

snufkin’s picture

snufkin commented

I see what you mean. It is a pity that we have to populate Drupal email addresses for SSO users as they may not have any, I've ran into that problem myself as well.

I've checked now the 3.x branch, and on top of your issue, there is an inconsistency as well. On simplesaml_auth.module:267 the module is checking for authnames, but is using the name property, not the email, which means that the checkbox for SAML auth will always remain unchecked. First I want to check with grasmash who created this bit of the code to see what the rationale was behind this approach.

Your usecase seems perfectly logical to me as well, and actually, if you check, the 2.x series did use the username as the authname.

snufkin’s picture

snufkin commented

Just a summary: the core of this problem is which drupal user property, the email or the username do we use to generate the authmap. The IdP can send us various properties, which can be mapped to either property, except for the validation limitation of the email addresses.

I've talked to grasmash, the decision was based on a particular client requirement, but that requirement can be met by assigning the IdP email value to the username and using the username.

tl;dr: lets use the username as the authmap property everywhere as it allows every string, not just email addresses

snufkin’s picture

snufkin commented
Status: Active » Needs work
StatusFileSize
new 2357879-use_name_as_authmap-simplesamlphp_auth.patch461 bytes

For now I have just rolled the above mentioned change into a patch. I'm concerned about the usage of ->init in the module as that is still the initially registered email address. I'll start testing for it.

Any help in this issue would be most welcome, I'm sure there are edge cases I wont be able to test for.

snufkin’s picture

snufkin commented
Status: Needs work » Needs review

  • snufkin committed 7a0b209 on 7.x-3.x 
    Issue #2357879 by snufkin: Problems altering user after provisioning,...
snufkin’s picture

snufkin commented
Status: Needs review » Fixed

I've pushed this change to the dev branch, because it is crucial to get this out there and tested before the stable release.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented
Status: Closed (fixed) » Needs work

I've been testing this on the latest code of 7.x-3.x branch, because the same occurs in 8.x-3.x, and the original problem still persists.

How to reproduce:

1/ Make sure "simplesamlphp_auth_user_name" setting & "simplesamlphp_auth_unique_id" map to a different attribute (which is a valid use case).
e.g.:
- "user_name" is set to "displayName" SAML attribute (for our example user, this could be "John Doe"
- unique_id is set to "eduPersonPrincipalName" SAML attribute (for our example user, this could be "john_doe_unique".

2/ Log in using SimpleSAMLphp for a user that doesn't exist yet in Drupal, so a new Drupal user will be provisioned
- data stored in the {users} table:
--> "name": first, "name" is set to "john_doe_unique" by user_external_login_register() - afterwards it is changed to "John Doe" by simplesamlphp_auth_user_insert() via _simplesamlphp_auth_get_default_name().
--> "init": "john_doe_unique" is stored by user_external_login_register()
- data stored in {authmap} table for this user:
--> "authmap": "john_doe_unique" is stored by user_external_login_register().

At this point the user can login and logout as expected, given that user_external_load() finds the correct Drupal user based on the given authname.

3/ As an admin user, edit that new Drupal user profile.
- The checkbox "Enable this user to leverage SAML authentication" is unchecked, where it should be checked, because of user_get_authmaps($form['#user']->name), since the user "name" (mapped by simplesamlphp_auth_user_name) is different from the entry in the authmap table (mapped by simplesamlphp_auth_unique_id)

4/ Upon saving, the following occurs:
--> If user is saved without the checkbox enabled, the authmap entry gets deleted.
--> If user is saved with the checkbox enabled, the authmap entry gets updated.

In both cases, the Drupal user still exist, but the authmap entry is invalid. If the user tries to login again, the module thinks it's a new user. This results in an error because the "name" value for the new user cannot be saved, since there is already a Drupal user with this name.

Solution:
* Make sure Drupal {users}.name is always mapped to the SimpleSAMLphp unique_id attribute. This would imply that the "simplesamlphp_auth_user_name" setting should be removed, and we should warn users that altering Drupal usernames will potentially result in problems.
* Use "init" field to store & check the unique_id / authname. This might still result in problems when a pre-existing Drupal user tries to login via SimpleSAMLphp, should be investigated.

svendecabooter’s picture

svendecabooter
he/him
Primary language Dutch
Location Gent
commented

See also #2561611: Existing user is identified by matching uniqueIdField with users.name but should be users.init