Contrib.social

Saving a user overrides the authname

Open on Drupal.org →
Created on 11 January 2023, almost 2 years ago
Updated 6 October 2023, about 1 year ago

Problem/Motivation

When editing and saving a user, the hook simplesamlphp_auth_user_form_submit() calls $externalauth->linkExistingAccount() in order to ensure that an entry in the authmap table exists, and passes the Drupal user's username as $authname by default.

This makes sense in most cases, but after #2910506: Usernames (authnames) do not update in authmap table → changed the behavior of the linkExistingAccount() method in External Authentication v2.0.3, you might lose sync between SAML and Drupal users if you don't use the default settings.

Steps to reproduce

  • Configure "uid" for unique_identifier as and "displayName" username for .
  • Log in with with SAML User "John Doe"; a Drupal user "John Doe" will get created; the corresponding authname is the UID "john_doe".
  • Edit and save that user in Drupal, e.g. because you want to assign some roles.
  • The authname is now "John Doe", and further login attempts will result in a second Drupal user created, since no user with authname "john_doe" is found.

Proposed resolution

Not sure yet.

The described behavior is being caused by the following code in simplesamlphp_auth_user_form_submit():

    // Link an authmap entry to this account, if not yet existing.
    // By default, we use the username as authname.
    // This can be altered if needed. See simplesamlphp_auth.api.php for
    // details.
    $authname = $account->getAccountName();
    \Drupal::modulehandler()->alter('simplesamlphp_auth_account_authname', $authname, $account);
    $externalauth->linkExistingAccount($authname, 'simplesamlphp_auth', $account);

A possible workaround is to provide an alter hook:

/**
 * Implements hook_simplesamlphp_auth_account_authname_alter().
 */
function mymodule_simplesamlphp_auth_account_authname_alter(&$authname, UserInterface $account): void {
  $authmap = \Drupal::service('externalauth.authmap');
  // Check whether an entry for the account already exists in the authmap table.
  // If so, change the authname to the value already stored there
  $current_authname = $authmap->get($account->id(), 'simplesamlphp_auth');
  if ($current_authname) {
    $authname = $current_authname;
  }
}

But of course, this is not the real solution.
The above logic in simplesamlphp_auth_user_form_submit() works as expected when you keep the default value "eduPersonPrincipalName" for both user_name and unique_id. Maybe we should call $externalauth->linkExistingAccount() only if these settings have the same value, indicating that the Drupal user's username and the authmap's authname should be equal?

Remaining tasks

  1. Agree on a solution
  2. Write patch
  3. Commit.
🐛 Bug report
Status

Fixed

Version

3.0

Component

Code

Created by

🇩🇪Germany mrshowerman Munich

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • @jrearick opened merge request.
  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States jrearick Iowa

    We're running into the same issue. Just to get things moving, I created a MR and attached a patch file. It's hard to test without a proper environment, so any help testing would be appreciated. I assume we'll want to add tests for this, but I suppose that can wait until we know this is the solution to go with.

  • Comment over 1 year ago →
    🇨🇦Canada gocaps

    We use a custom Unique Identifier field and so we were running into this issue, where the authname would update in the authmap table each time the user was edited. On next login attempts, Drupal would create a new user prefixed with "simplesamlphp_auth_" and log an error:

    Error on synchronizing name attribute for uid 12345: an account with the username johndoe and uid 123 already exists.

    Seems related to https://www.drupal.org/project/simplesamlphp_auth/issues/3347716 💬 Module is auto-creating users prefixed with "simplesamlphp_auth_" Active

    Patch #4 is so far working.

  • Comment about 1 year ago →
    🇮🇹Italy Giuseppe87

    I'm facing the same bug updating simplesamlphp_auth on a existing project from 3.2 to 4.x-dev, and #4 works.

  • Comment about 1 year ago →
    🇺🇸United States caesius

    @Giuseppe87 could you confirm that you ran into this issue by updating from 3.2 to 4.x-dev without also updating the externalauth module? The original ticket description seems to indicate that this issue would come up after updating externalauth (since it introduces new behavior) regardless of the version of the simplesamlphp_auth module.

  • Comment about 1 year ago →
    🇮🇹Italy Giuseppe87

    @caesius you're right, I mixed the two modules responsibilities.

    I've done the tests you asked:

    • simplesamlphp_auth 3.2/3.3 with externalauth 1.4: no bug
    • simplesamlphp_auth 4.x-dev with externalauth 1.4: no bug
    • simplesamlphp_auth 4.x-dev with externalauth 2.0.3: bug happens

    Therefore as you stated isn't something strictly due to 4.x-dev upgrade.

    However, I'd point out that this issue could be implicitly linked to the 4.x.x release, for dependency reasons.

    If 4.x.x is gonna the branch with D10 support, being 2.x.x the only branch with D10 support for externalauth, when upgrading core and thus contribute modules, everyone using simplesamlphp_auth will face this issue.

  • Status changed to RTBC about 1 year ago
  • Comment about 1 year ago →
    🇮🇹Italy Giuseppe87
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    21 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.1.x + Environment: PHP 8.1 & MySQL 5.7
    last update about 1 year ago
    Composer require failure
  • Comment about 1 year ago →
    🇺🇸United States caesius

    Re-ran the 3.x tests and it seems good now. 4.x failed due to the same Composer plugin error that 3.x originally ran into, but it's obviously not related to this patch.

  • Comment about 1 year ago →
    System Message
  • Comment about 1 year ago →
    System Message
  • Status changed to Fixed about 1 year ago
  • Comment about 1 year ago →
    🇨🇭Switzerland berdir Switzerland

    Committed, thanks.

  • Comment about 1 year ago →
    🇨🇦Canada bbombachini London, ON

    I know the issue has been resolved, hence closed. But is there any suggestions about how to fix the users that have already been affected?

  • Comment about 1 year ago →
    🇮🇹Italy Giuseppe87

    I don't know if there's a better way, but from the debug I did to identify this bug switching to 4.x.x, I suppose the following method method should work - please be aware you have to test it, I haven't actually tried it.

    You have to update the table `authmap` for the the user whose external `authname` have been overriden with the local\Drupal one.

    In my case, the external authname is a string like "00556409645g0050SJQww2" while the drupal one is an email, so I could query the table to list all the elements with an email as authname, and then manually update them.

    You obviously need some way to find the external authnames for the broken records - e.g. a backup db.

  • Comment about 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024