Contrib.social

Design of map_users_roles setting is problematic

Open on Drupal.org →
Created on 29 April 2021, over 3 years ago
Updated 11 January 2024, 11 months ago

Problem/Motivation

We run a large site with thousands of logged-in users, hundreds of admins, and dozens of admin-specific roles. As 'map_users_roles' is currently implemented, if a user account has any roles that are NOT selected in the samlauth module's config, the user will be unable to log in. Therefore, to allow all our users to log in, we would need to specifically enable every role we define.

Occasionally, we need to add another role, and, naturally, some users will be assigned to this role. As currently implemented, these users would then be unable to log in until we update the configuration of the samlauth module. This non-obvious cross-coupling of module configuration settings will almost certainly be forgotten and cause problems somewhere down the road.

Proposed resolution

Most importantly, there needs to be a way to ignore this config setting. For example, if the setting is blank, it can be ignored. In SamlService.php, at around line 458, where the setting value is loaded, if it's empty, just skip that whole clause.

As currently implemented, a blank setting would prevent anyone with any role from logging in. Since this is not a valid use-case, it might be fine to treat "empty" as "ignore". In addition, this is consistent with the way other filter-like settings work in Drupal (block targeting, for example).

Feature request: The current design might be termed a *NOT-blacklist* of roles. It functions like a blacklist, but only specifies which roles are not on the list. For our use-case, it would be preferable to have an explicit *whitelist* of roles. For example, at Berklee, users having a Faculty, Staff, Student, Alumni or Trustee role should be able to log in via SAML. Others should be linked but not logged in. With the current settings, we are not able to do this.

💬 Support request
Status

Fixed

Version

3.1

Component

Code

Created by

🇺🇸United States bmelvin1

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

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇺🇸United States John Franklin

    Reroll of #13 against 3.8.

  • Comment 11 months ago →
    System Message
    • roderik → committed 9301532a on 8.x-3.x 
      Issue #3211479 by roderik, bmelvin1, adamfranco, John Franklin, azinck:...
  • Status changed to Fixed 11 months ago
  • Comment 11 months ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    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.

  • Comment 11 months ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024