Role assignment from attributes does not work when provisioning accounts

As there is a new version for simplesamlphp_auth, I've created a reroll for this patch. The code is exactly the same, it just moved some lines.

The function "moduleHandler->getImplementations()" is deprecated in D10, so in order to keep it working on D10 i've adjusted the code. With these adjustments it should be compatible with D9 and D10.

Confirming that with the latest patch I can login as a new user and get the right roles assigned to the user with Drupal 10.

I forgot to replace a part of the example I used, here is the updated version.

I rerolled the patch since the previous one could not be applied any longer.

I needed to make some changes to the rerolled patch after we discovered some changed behavior on our site. We had been using the patch in #11 🐛 Role assignment from attributes does not work when provisioning accounts Needs review , and it appears the reroll changed how it worked slightly. The way our SSO setup is configured, you get returned to Drupal right after registering, and should then be logged in. What we started seeing after applying the reroll in #21, was that the user was not instantly logged in. It seems the new patch started doing
return $account;
Where the #11 patch was keeping what the module was already doing.

$this->synchronizeUserAttributes($account, TRUE);
return $this->externalauth->userLoginFinalize($account, $authname, 'simplesamlphp_auth');

This new patch puts it back to the previous code, so that the user's attributes get synced and they are logged in at the end.

I think this patch needs work as it places invocation of roleMatchSync() inside the condition for new account, so role sync will only be evaluated for newly provisioned accounts and not for subsequent logins.

Could not understand what i did wrong, checked issue queue. Found this and applied patch in #22
Now it works as intended

Thank you!

dylan donkersgoed → made their first commit to this issue’s fork.

I encountered an issue with this patch preventing hook_simplesamlphp_auth_existing_user from running. This was happening because the else that triggered it was moved inside of this if which checked for an existing user:

    if (!$account) {
      $existing_user = $this->entityTypeManager->getStorage('user')->loadByProperties(['name' => $authname]);
      $existing_user = $existing_user ? reset($existing_user) : FALSE;
      if ($existing_user) {

which would not generally be true when you're implementing a custom hook_simplesamlphp_auth_existing_user() to look up the user by an attribute other than their name. I moved the else outside of this check like the original module and adjusted the linkUser method accordingly.

Patch is attached. I also opened an MR with the patch and my new change.

While the patch seems to be working for me now, I think there may still be some room for improvement. I don't know the full rationale for the drastic changes between patch 8 and patch 11, but I feel as though the patch may be doing more than it needs to to fix the issue, which may have other side-effects.

I discovered another side effect issue that seems to have been introduced in the comment #21 reroll. When I first read the previous comment, it looked like it was about the same issue, but there is another case that prevents "Automatically enable SAML authentication for existing users upon successful login" from working as intended. Essentially from patch 21 onward, if the condition is met that an existing user is found, and 'autoenablesaml' is turned on, a message is logged saying that the two accounts will be linked(if debugging is enabled). The condition exits after this, and no action is actually taken to link the accounts. After going back and comparing different versions of the patch, I determined that these two lines had been removed from the aforementioned conditional.

          $this->externalauth->linkExistingAccount($authname, 'simplesamlphp_auth', $existing_user);
          $account = $existing_user;

I'm attaching a patch that adds this functionality back, and is otherwise the same as patch #27.