The value_map field, labeled Value conversions, states that "If this is left empty, the values from the IdP will be interpreted as machine names for Drupal roles."
However, if it is left empty, role mapping is skipped, with the logged message: "value_map is not configured; skipping role mapping."
Either implement the described feature OR remove it from description
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | samlauth_user_roles-value_map_roles-bug-3506668-2.patch | 1.04 KB | isak@websystem.se |
Show commandsStart 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:
changes
Comments
Comment #2
isak@websystem.se commentedMade a small patch that, if the config for value_map turns out empty, it'll fall back to roles machine names as the field description suggests.
Comment #3
dhruv.mittal commentedComment #5
dhruv.mittal commentedAs patch has been deprecated and MR is easier to review so I raised MR.
Please Review
Comment #7
roderik@isak@websystem.se thank you for the patch. (I was almost going to complicate the whole foreach with more if/then's , but just populating $value_map is much cleaner.)
However, I did not dare to touch this code without adding some tests, to make sure nothing breaks. This is finally done now, and I cleaned up a bunch of code that was IMHO now-unnecessary or hard to read.
In the process, I discovered that the module wasn't logging a warning if a role was sent by the IdP which cannot be mapped to a Drupal role. I believe I was intending that / this should be logged. (Though my intention from 5 years ago was... hard to read.) So I added that warning now, which is a bit of a behavior change... I hope noone's logs will get spammed too much, but I believe people will want to know.
Comment #9
roderikComment #11
carstenhager commentedIt would be great if this warning could be made optional or configurable.
In our setup, the IdP sends a large number of roles, but only a small subset (3 roles) is actually relevant in Drupal. This is intentional and not a misconfiguration.
With the current behavior, every unmapped / non-existing role coming from the IdP would generate a warning on each login. At our scale (100,000+ users), this would flood the logs very quickly without providing actionable information.
Making this warning optional (e.g. via configuration, log level, or a toggle to suppress warnings for unmapped roles) would help support both:
Comment #12
roderikThanks. I sometimes need to be reminded not to introduce behavior changes, for large sites' configurations.
The configuration option is now introduced. Logging warnings is true for new installs, but false for existing installs. 8.x-1.13 released.
Comment #15
roderikWhoops. I hastily cut another release, just after the drupal.org outage / because I was afraid I might not have time. But now, tests fail on v8.x-3.13.
FWIW: the code is fine, the test is wrong. Fixed in next commit.
Comment #17
roderikAnd a final(?) comment: I was intending for the logging to be enabled on new installations, but that wasn't done until now. Will be done in 8.x-3.15.