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.

CommentFileSizeAuthor
#7 3368600-7.patch2.56 KBheatherwoz
#5 3368600-5.patch2.52 KBliberatr

Issue fork samlauth-3368600

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:

  • 8.x-3.x Comparechanges, plain diff MR !11
  • 3368600-linking-a-local Comparecompare
  • 3.x Comparecompare
About issue forks

Comments

vittalaithal created an issue. See original summary.

vittalaithal opened merge request !11

ipa 🍺’s picture

ipa 🍺 commented

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

davej’s picture

davej commented

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

liberatr’s picture

liberatr
he/him
Primary language English
Location Portland, OR
commented
StatusFileSize
new 3368600-5.patch2.52 KB

Attempting to reroll against 3.10

Still have yet to test the functionality.

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

heatherwoz’s picture

heatherwoz commented
StatusFileSize
new 3368600-7.patch2.56 KB

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. Patch in #5 applied but caused errors for me.

heatherwoz’s picture

heatherwoz commented
Status: Active » Needs review

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

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Yes, patch #5 caused problems: a bracket was removed, so the whole class is invalid / causes errors.

@heatherwoz

  • 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 #3496176: SAML login must not be linked to an already-linked user, and added some tiny code tweaks and things to the README while thinking about tested behavior.

  • roderik committed c9e86d86 on 8.x-3.x authored by vittalaithal 
    Issue #3368600 by heatherwoz, roderik, vittalaithal: Linking a local...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Needs review » Fixed

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.

Status: Fixed » Closed (fixed)

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