If simplesamlphp_auth is configured to "Reevaluate roles every time the user logs in" any manually assigned roles will be removed. It would be nice if any roles granted by simplesamlphp_auth were stored separately so that when roles are reevaluated at login only the roles that were assigned by simplesamlphp_auth would be evaluated.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | simplesamlphp_auth-store-saml-roles-separate-2707269-2.patch | 2.8 KB | briand44 |
Comments
Comment #2
briand44 commentedHere is a patch.
Comment #3
gravelpot commentedHey @briand44, I'm checking in over from https://www.drupal.org/node/2117259, where you pointed out to me that you already had this patch in play after I posted mine that does basically the same thing.
I've tested your patch against my IdP, and it seems to work great. Overall, I think your approach has some definite advantages over mine, but I had two observations/questions:
hook_simplesamlphp_auth_user_roles_alter()that is defined insimplesamlphp_auth.api.php, and which is invoked viadrupal_alter()at the end of the_simplesamlphp_auth_rolepopulation()function. In your implementation, there will be further manipulation of the user's roles *after* this hook has been invoked. Should we worry about that?Comment #4
mforbes commentedHuge thanks to both of you. I've been meaning to get something like this going since I posted in the other issue. I can suggest my opinion on those two points:
There, the phrases "the roles evaluated" and "evaluation process" get ambiguous no matter what we do. If we decided to make that &$roles parameter be post-array-merge, this hook allows manipulation of the merged set (manual and evaluated). If we don't (so &$roles is pre-merge) then this hook offers no ability for an invoker to revoke manually maintained roles based on complex assessment of SAML attributes (but no existing invocations can either, due to the current wipe out of manual roles, only grant, which will still work no matter which way we go). That actually sounds like a good thing, mandating that manually maintained roles are always manually maintained, and shouldn't be automated here.
Comment #5
mforbes commentedSeems like we would add a toggle whose default state is FALSE, then in hook_install() we can set it to TRUE, and not have a hook_update_N() touch it whatsoever. That way it's FALSE for all upgrading/patching users, and TRUE for fresh installations of any release following the commit of whatever we come up with.
Comment #6
gravelpot commented@mforbes, that's what I did in my patch on https://www.drupal.org/node/2117259, and I'm planning on implementing that same thing on this approach as soon as I get the time.
Comment #7
djdevinContinued in #2117259: Selective role syncing