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.

Comments

briand44 created an issue. See original summary.

briand44’s picture

Status: Active » Needs review
StatusFileSize
new2.8 KB

Here is a patch.

gravelpot’s picture

Hey @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:

  1. In my patch, I added a setting to allow site administrators to choose whether to evaluate manually-assigned roles separately from the SAML evaluated roles. You're making it a non-optional change, which changes a fundamental way the module works without giving the site administrators the option to continue as they did previously, not drawing a distinction between manual and SAML roles. I can imagine cases where a site administrator might want to strictly enforce role population ONLY via SAML evaluation, and for any other roles to be removed. I think that we could easily take that piece of my approach and mix it in to yours to make this preservation of manually-assigned roles an option.
  2. There is a custom hook called hook_simplesamlphp_auth_user_roles_alter() that is defined in simplesamlphp_auth.api.php, and which is invoked via drupal_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?
mforbes’s picture

Huge 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:

  1. I agree that existing sites should have to opt into the new behavior, but I also think sites installing for the first time should get the new behavior by default. Surely this kind of thing happens whenever a new toggle is added to a module and with a clever hook_update_N we can make it happen. I'll look into it more.
  2. An important consideration is whether the &$roles parameter for that hook should contain just those evaluated roles, or all roles after the evaluated ones are merged with the non-evaluated ones. The API doc's wording is:
     * Whenever a user's roles are evaluated this hook will be called, allowing
     * custom logic to be used to alter or even completely replace the roles
     * evaluated.
     *
     * @param array &$roles
     *   The roles that have been selected for the current user
     *   by the role evaluation process.
     * @param array $attributes
     *   The SimpleSAMLphp attributes for this user.
    

    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.

mforbes’s picture

Seems 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.

gravelpot’s picture

@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.

djdevin’s picture

Status: Needs review » Closed (duplicate)