Design of map_users_roles setting is problematic

As currently implemented, these users would then be unable to log in until we update the configuration of the samlauth module.

These users would be disallowed from being linked - i.e. when the SAML user first logs in and no 'link' (from the unique ID passed in that login) to an existing user exists yet... then if a matching but as-yet-unlinked user with an not-explicitly-allowed role exists, access is denied.

This turns into:

1) If in fact 'regular' (already having logged in before / having been linked) users are also denied login after getting a new role... then we have either a bug or configuration issue on our hands which we should explore further.

2) How many 'new' not-linked (precreated, never logged in through SAML / not created during SAML login) Drupal users do you have? Do you also regularly assign those users new roles and then require them to still be available for login/linking?

Reason for the current implementation (and why we won't default to allowing all users): this first-time linking is a potential security hazard. If anyone on the IdP side knows which not-yet-linked Drupal users exist and can alter the right properties (e.g. name or email address) to match the 'matching rules', they can take over such a Drupal user.

I may need to refine wording in the config screen about the fact that "matching + linking" happens only on first login, not on every login (because the 'link' from the unique ID to the user is saved for subsequent logins).

I'm struggling to keep things concise but not write whole paragraphs there.

(I'm listening... but going to push back for another message and ask something else.)

a list of roles that individually allow linking -> brittle: accounts with admin roles would not be prevented from linking if they got one of those roles assigned.

a list of sensitive roles that individually prevent linking -> default setting is 'insecure'. (This really comes down to the question "is that ok?")

The potential bad actor doesn't have to be an IdP admin. On some IdPs, people can change the value of 'whatever property is being linked by'. We had a security issue reported by someone whose IdP allowed individual IdP users to change their e-mail addresses. They argued fairly strongly that the whole linking facility is inherently insecure and should possibly be deleted altogether.

Now, this was heightened by another bug that has been fixed in 8.x-3.1... and I'm not saying I agree with that statement, or think this is an issue for the average IdP setup out there... but I'm now in a position that requires choosing between inconveniencing some module users, and giving some module users options that allow them to inadvertently set up insecure configurations.

And my thinking is evolving from "oh, linking existing users is a neat option, let's implement it" to "I should probably work to discourage people from doing it if possible". Closed/secure/conservative configurations are better, if possible.

So here's my questions:

You import users before first login. Can you make it so that you also import the links / authmap entries? Then you can disable the linking options altogether. This depends on what field you're using for the Unique ID and if you have it available at the time of import.

And do you think this module should provide documentation/example code that helps admins steer into that direction?

I can do the documentation (and have added some wording to the README recently) but I'm not sure about code examples. I don't know how you prefer doing imports - but if you are a fan of using Migrate over direct SQL: the externalauth module contains a migrate destination for the authmap entries.

Wow! How could anyone live with a system where users can change their own unique identifier to a non-unique value? (...)

I don't see anything in this setup necessarily pointing to non-uniqueness. But this feels like an argument I have no part in.

I understand the concern about module users who might not change the configuration and therefore may not configure proper security for their use-case. If you disable linking by default, doesn't that essentially eliminate that issue?

That's a valid point.

(...)The 'administrator' role should be pre-selected on a fresh module install. Users can add other roles depending on their security needs. To my eye, this is the most straightforward and useful way to present the configuration. However, you could also offer the option of a whitelist (i.e. choose all roles that *don't* prevent linking), as you currently have. Maybe some users will prefer that option?

Yeah... I'm not sure. And in the security issue thread, there wasn't a big audience for the 'design' around this option.

We may have to go for an option of how the role setting behaves, indeed... if for no other reason than that the current way has ended up in a stable release already. (Then we may decide to retire one of the methods by v4, if noone cares for it).

But... given you indicated this isn't something that is a problem now**, I'm tempted to leave this sitting in the queue for a while, to see if anyone wants to comment about having the same issue, and about the solution. Or if someone wants to pick up the work.

** unlike e.g. the unfortunate eternal redirect I introduced in 3.0-RC1.

Great!

If your team prefers using -dev versions / overriding services, over using patched modules... The latest -dev now has the corresponding code plus a little more, moved into SamlService::linkExistingAccount(). Which will keep existing at least until v4.x. https://git.drupalcode.org/project/samlauth/-/commit/ed805af67ab11a19d10...

You'd need to copy some boilerplate check code into the overridden linkExistingAccount(), but not much.

@bmelvin1:

I was the user that raised the issue to roderik.

Wow! How could anyone live with a system where users can change their own unique identifier to a non-unique value? That's inconceivable. Basic rule: Unique identifiers must be unique. I would push back hard on these people to correct their systems, rather than inconvenience users of the module.

If linking were somehow only allowed based on guaranteed unique IDs that are not alterable by users, then I'd agree with you. But this module makes it easy to configure arbitrary attributes as the basis for linking, and thus makes it easy to inadvertently configure a security issue. In our specific case the IdP's email property was not guaranteed unique, and was editable by various administrators on the IdP system. Our IdP system was also a AMS used for a wide variety of things in the organization, and it wasn't obvious to us, as integrators, that the email address was so easily changeable. In our case, the problem we experienced was less a case of a bad actor than it was well-meaning admins changing and moving accounts around and, through email collisions, inadvertently granting privileged access to people who should not have had it. This was exacerbated by buggy behavior of this module (since resolved) that goes beyond what I think it prudent to discuss here. So it's fair to say that we should never have configured it that way, but I think it's also good to keep in mind that it's hard to know all the implications of every configuration decision, and Roderik's additional failsafes and cautionary text I think go a long way towards making the module more secure.

The core of my argument is this: the purpose of the authmap table is to map drupal users to SAML users. We should make it easier to pre-populate those explicit mappings rather than have a second-class, more edge-case-ridden, mapping solution based upon a variety of other configuration. Roderik has kindly already moved things in that direction with his introduction of the Views integration and ability to clear out authmap entries.

All that said, I agree with making it possible to override the behavior when you know what you're doing, so I'm happy to see the direction this discussion is going.

We recently upgraded samlauth and ran into this issue. In our case we have a trusted IdP and a dozen sites with MANY roles which manage access to menus via https://www.drupal.org/project/menu_admin_per_menu . Requiring an extra config change to SamlAuth every time a new menu+role is added is a frustrating situation and prone to mistakes that block new editors from logging in the first time. I would much rather be able to disable this check completely or have a block-list which prevented accounts with several more-sensitive Drupal roles (e.g. "site adminstrator") from auto-mapping on login.

Here's a patch that implements bmelvin1's suggestion to ignore map_users_roles if none are selected. This may not be the eventual desired direction, but this is a work-around I'm comfortable with in our environment until a better solution is developed.

I stumbled across a nice example of the University of Iowa working around this challenge by using the configuration override api to allow all roles dynamically.
https://github.com/uiowa/uiowa_auth/blob/main/src/ConfigOverride/Samlaut...

I haven't tested it myself, as I didn't need to go quite that far, but I thought it may help someone else.

I can confirm that the example of using a dynamic configuration override as mentioned in #14 seems to work as well as the patch in #13 (which no longer applies in 3.8). In my configuration override I also added a form hook to avoid confusion by any site admin as to why the settings might not do anything anymore.

mymodule/mymodule.module:

use Drupal\Core\Form\FormStateInterface;

/**
 * Implements hook_form_FORM_ID_alter().
 */
function mymodule_form_samlauth_configure_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  // Disable "Roles allowed for linking" as we are allowing all.
  $form['user_info']['linking']['map_users_roles']["#title"] .= t(" (Disabled by mymodule)");
  $form['user_info']['linking']['map_users_roles']["#description"] = t("Normally SamlAuth blocks linking of accounts that are given roles not explicitly selected here. The mymodule module overrides this to allow all roles to be mapped as we trust our IdP to not supply bogus email addresses and usernames for users.");
  $form['user_info']['linking']['map_users_roles']["#disabled"] = TRUE;
}

Reroll of #13 against 3.8.

Thank you all for your input.

Linking is now still disabled by default, but the restriction can be undone by configuring a special value ["anonymous"] (must be a single-element array value) as the 'map_users_roles' setting.

So everyone who is running with a patch from this issue, can drop the patch after they update to the next version (when released) and change their 'map_users_roles' setting. In the UI, this is equivalent to checking the "Allow all Drupal users to be linked" box.

I needed to let this stew "for a bit" (which can easily become years) and

• There have been arguments for making this deny-list of role into a sort of allow-list, but I'm not convinced of the added security benefit (vs. the original security issue that added this option) and no argument came from a practical situation. So I've ignored that for now. If anyone finds this issue / wants this, they can reopen it.
• The option to explicitly bypass the whole role checking (which is what the patch also adds) is fine, though... as long as we don't have that on by default / on new installations.
• I realized I don't need a new configuration value to implement this. In practice, the array value for 'map_users_roles' never contains the values "anonymous" and "authenticated". So instead of 'empty array' (as the contributed patch does), I can use the special value ["anonymous"] for this.

So that's done now, with some UI tweaking to make things clearer.

I hope / think I haven't missed anything else important in the conversation.