Contrib.social

Linking a local user account to a saml account may be incorrectly blocked

Open on Drupal.org →
Created on 22 June 2023, over 1 year ago

Problem/Motivation

The Samlauth module allows SAML users to be mapped to local users by one or more of:

  • Enable matching on name
  • Enable matching on email

When logging in, Drupal\samlauth\SamlService->doLogin is called. This will do the following if no account is passed in:

  1. Look up the saml user login
  2. Look up any local users with the same login as the saml login
  3. If a local user is found, see if linking by name is allowed
  4. If linking by name is configured, allow login to proceed
  5. If linking by name is not configured throw an exception with user message "A local user account with your login name already exists, and we are disallowed from linking it"
  6. If no account was found by login, perform the same checks as above but by email

The problem is that if you are linking by email and not by name, a legitimate saml user who has both a matching name and email to a local user will not be able to log in. They'll be found by the login lookup, and because login linking is off, they'll be denied access. However, they really should be allowed access because their email does match.

Steps to reproduce

Create a local user:
login: test
email: test@example.com

Configure samlauth module with:
Enable matching on name - off
Enable matching on email - on

Attempt to log in as test user via saml - the link exception will be thrown

Proposed resolution

If a user account is found by name, and linking by name is off, do not immediately throw an exception. Rather, check if linking by email is enabled, the saml user has an email and the email matches the local user email. Only if all of those test fail throw the exception.

🐛 Bug report
Status

Active

Version

3.0

Component

Code

Created by

🇬🇧United Kingdom vittalaithal

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

Merge Requests

Comments & Activities

  • Issue created by @vittalaithal
  • Merge request !11Issue #3368600: Linking a local user account to a saml account may be incorrectly blocked → (Merged) created by Unnamed author
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.5 + Environment: PHP 7.4 & MySQL 5.7
    last update over 1 year ago
    3 pass
  • Comment over 1 year ago →
    🇳🇱Netherlands e.ruiter

    I had the same problem and the merge request !11 fixed it, thanks!

  • Comment 6 months ago →
    🇬🇧United Kingdom davej

    Same issue here with 8.x-3.9 . Merge request 11 fixed it here too, thank you!

  • Comment 5 months ago →
    🇺🇸United States liberatr Portland, OR

    Attempting to reroll against 3.10

    Still have yet to test the functionality.

  • First commit to issue fork.
  • Pipeline finished with Success
    about 2 months ago
    Total: 150s
    #359473
  • Pipeline finished with Success
    about 2 months ago
    #359496
  • Comment about 2 months ago →
    🇺🇸United States heatherwoz Seattle

    I attempted to rebase the MR and resolve the conflict, but I'm not sure if I did it correctly. Here is a patch with the latest changes.

  • Comment about 2 months ago →
    🇺🇸United States heatherwoz Seattle
  • First commit to issue fork.
  • Comment about 1 month ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU
    • The result was good, but you somehow rebased (copied) the ~39 newer commits from the 8.x-3.x branch onto the existing change, before merging.
    • In this case, a single merge commit is better (when working on a branch with others).
    • You could also better to rebase the single change commit onto the newest 8.x-3.x... if you are allowed to, and feel confident enough to force push (overwriting the original commit). I do that often IF I'm working alone on a branch.

    In this case, I rebased and force-pushed into the branch (to have cleaner history)... because I was going to merge it and close the issue anyway.

    Anyway, that's detail :-)

    ---

    Thank you for the fix. This is a clear bug, but I had ignored the issue for the previous release. Among others, because I felt like this really needs tests, to make sure that it doesn't break anything else unexpectedly. (The code in the fix looks fine, but... the existence of the bug already proves that things are getting too complicated to not have tests. I guess I should have set the issue to Needs Work in the meantime; it took longer than I thought.)

    That's done now. Tests for the user creation/linking logic are included, which makes me feel a lot safer for future rewrites/fixes. I also discovered another bug, which I'll fix in a related issue, and added some tiny code tweaks and things to the README while thinking about tested behavior.

  • Pipeline finished with Skipped
    about 1 month ago
    #379158
  • Comment about 1 month ago →
    System Message
  • Comment about 1 month ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    I still get caught by the fact that the pre-formatted commit message cannot be edited, when merging from the drupal.org issue. Otherwise I'd have changed it slightly.

    Anyway, fixed and credited.

  • Comment 17 days ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024