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.
CommentFileSizeAuthor
#4 3332611-4.diff845 bytesjrearick

Issue fork simplesamlphp_auth-3332611

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mrshowerman created an issue. See original summary.

jrearick made their first commit to this issue’s fork.

jrearick opened merge request !20

jrearick’s picture

jrearick
he/him
Primary language English
Location Iowa
commented
Status: Active » Needs review
StatusFileSize
new 3332611-4.diff845 bytes

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.

gocaps’s picture

gocaps commented

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

Patch #4 is so far working.

giuseppe87’s picture

giuseppe87 commented

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

caesius’s picture

caesius commented

@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.

Also, the latest 3.x version is 3.3, so check to make sure that the issue doesn't come up when going from 3.2 -> 3.3 instead. The 4.x version should be unlikely to cause any issues that don't also occur in 3.3.

One last thing -- since multiple people have indicated #4 works, perhaps this should be marked RTBC by someone who has confirmed that it does work?

giuseppe87’s picture

giuseppe87 commented

@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.

giuseppe87’s picture

giuseppe87 commented
Status: Needs review » Reviewed & tested by the community
caesius’s picture

caesius commented

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.

  • Berdir committed aba9a9e4 on 4.x authored by jrearick 
    Issue #3332611 by jrearick: Saving a user overrides the authname

  • Berdir committed feead597 on 8.x-3.x authored by jrearick 
    Issue #3332611 by jrearick: Saving a user overrides the authname
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks.

bbombachini’s picture

bbombachini
Location London, ON
commented

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?

giuseppe87’s picture

giuseppe87 commented

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, so maybe I'm missing something in externalauth module flow.

In the end, you have to update the database 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 if I needed 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.

A direct database query should be good enough, but to be 100% sure all the externalauth module flow is followed, you could write a batch function which use externalauth/src/ExternalAuth.php::linkExistingAccount() to update the $users with this problem.

Status: Fixed » Closed (fixed)

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