Reevaluate roles every time the user logs in does not remove roles

Subscribe, indeed the roles are still available for the user while not in the attribute anymore. Will there be a solution soon?

Thanks
Nielsvoo

Ok,

i think option A is much better, why keep bindings on a user role while the source disabled them? Can you add the patch?

Thanks in advance

We tested the patch in #5 and it works very well. The roles are properly reevaluate and no regression appears. Thanks rjg for the patch.

Hope it will be merged soon.

I guess the issue exposed here was going to be implemented on the module, otherwise I don't understand the following sentence on the module's configuration form:

Reevaluate roles every time the user logs in
NOTE: This means users could lose any roles that have been assigned manually in Drupal.

We have also tested patch #5 and makes the reevaluate functionality complete (by adding and/or removing roles)

Any plan to have it included in a newer version of the module soon?

Many thanks.

Eduardo

This is a quite interesting issue, as our current logic is based on roles being only added and not removed, because there are also existing local accounts with additional roles. But as pointed out in #8, this behavior seems to be a bug and has a misleading form description.

I would really like to see a change-record and message via hook_update_n(), when this issue is going to be merged into the module. Otherwise there will be some site logics silently get broken by the update.

I have created the change record: https://www.drupal.org/node/2939077

Can someone review it?

Added a rerolled patch that adds an empty update hook to provide messaging.

You may consider the change record at https://www.drupal.org/node/2939077 to be RBTC by me. This modified patch should be reviewed and RBTC'd.

Rerolled the patch applying the Codesniffer fixes. I do not have time at the moment to troubleshoot the other test failures.

Any progress on this issue? @szeidler. I thought about your comment "This is a quite interesting issue, as our current logic is based on roles being only added and not removed, because there are also existing local accounts with additional roles"

What about combining reevaluate with exceptions of some roles, you could show a list of all roles that doesn't reevaluate at log in.

thanks

What about combining reevaluate with exceptions of some roles, you could show a list of all roles that doesn't reevaluate at log in.

This could be a good option to distinguish between Drupal specific local roles and roles, that make absolutely sense to be controlled from the simplesaml endpoint.

Thanks for your change record @timcosgrove. I sounds fine for me.

Here is a re-roll against latest, that hopefully addresses the failing tests. I'm just fixing the tests, I'm not sure there is appropriate addition here for testing the new code...

I'm also not sure how the last few comments relate to this patch and if there are changes needed. I just wanted to provide a passing patch ideally.

Hmm, I'm at a bit of a loss. My patch is running for me via the simpletest UI with no issues.

I think I see the typo for the test... as I mentioned, this is still running locally for me, so hopefully this will pass the tests now.

Ok, sweet... that typo was the problem. Not sure what to do with this one now. Going to put it back to "Needs review" - but it seems @rjgwiz found another problem here, so this may make more sense as "Needs work"? I think if we add the changes from #22, we'll probably also have to add testing for those changes as well...?

+++ b/src/Service/SimplesamlphpDrupalAuth.php
@@ -261,21 +261,41 @@ class SimplesamlphpDrupalAuth {
   public function roleMatchAdd(UserInterface $account) {
+    // Load user entity so we have access to the full user interface.
+    /** @var Drupal\user\Entity\User $user */
+    $user = $this->entityTypeManager->getStorage('user')->load($account->id());

Is this needed? Param $account appears to be an instance of `UserInterface` already - note further down previously it was calling `::addRole` so I don't think it was an `AccountProxyInterface`

Good point @trwill. Removed the bonus call to load the user object from the user object we already have.

Not sure what's going on with these files, but I'm going to hide the old patches and only display this latest.

This patch works for me.

I conducted the following test

1) Added config to add role_x to user_y user based on an attribute
2) Logged in as user_y via simplesamlphp_auth
3) Verify the new role is added to user_y
4) Remove config to map role_x to user_y
5) Log out as user_y
6) Log back in as user_y
7) Find role_x removed from user

If there is an option for the site admin to select which roles to be "deleted" or "excluded from deletion", it would help a lot. The option can be in the form of prefix, or in a comma-delimited list.

The scenario is that the IdP may not have roles specifically created for each of the Drupal sites. They only have generic roles such as staff. It is very likely that someone logged in via the IdP would be manually given a role in Drupal (administrator, or admin of any content types). If deletion is applied to every role evaluation, that will render their manually assigned "admin" roles be deleted too.

@adamschan - I think that's a good suggestion. You should probably open a new feature request issue to support it.

The new patch looks good and works for me as well - bumping to RTBC \m/

+++ b/simplesamlphp_auth.install
@@ -97,3 +97,12 @@ function simplesamlphp_auth_update_8001() {
+function simplesamlphp_auth_update_8003() {

Is providing an update hook to point to a CR a pattern used in core or elsewhere? I think we should avoid adding update hooks in patches when not 100% necessary as it could greatly complicated the normal update operations of the module

I added that part of the patch, and bonus is right, there are scenarios in which using patches ahead of merge into the project can be problematic. This issue has sat for many months, so happy to remove the update hook.

Thanks, @timcosgrove. To qualify my comments in #37 a bit; when relying on patches that include update hooks, site admins also assume the burden of maintaining an upgrade path. If a site admin was to apply #32 and a change was introduced into the mainline branch that included a competing simplesamlphp_auth_update_8003 , that change would never run. The patch in #38 (or similar w/o an update hook) seems like a good option for users applying this patch until #32 is considered and accepted into the project. I am still curious if there is another best practice is for these types of in-update notifications rather than hook_update_n.

Edit: hook_post_update_NAME (CR) might be a much better option. Noting in contrib, Webform uses drupal_set_message banners to announce important changes on admin pages.

Tests are failing on both #32 and #38.

Failures are on the main 8.x-3.x branch as well so not related to these patches.

+++ b/src/Service/SimplesamlphpDrupalAuth.php
@@ -260,22 +260,39 @@ class SimplesamlphpDrupalAuth {
-      $account->save();
     }
+    // Save the user.
+    $user->save();

I realize this is an existing issue, but wondering if we should optimize it while touching this. This always saves the user, and it's only minimally worse than before when not matching any roles didn't save.

But it would be pretty easy to add a boolean flag that is set when a change actually happens and then only save if necessary.

Also agreed with not having an update function for this.

Rerolling patch #38 against current HEAD.

#32 introduced an unnecessary method argument name change from $account to $user; undid this change while leaving the intention of the change (not loading the user object redundantly).

I also addressed Berdir's concerns about only saving the user object when needed (#42).

Reroll, so no interdiff.

Hi,

just added a test on matching roles variable to remove all roles if the user is not any more in AD Groups

1. +++ b/src/Service/SimplesamlphpDrupalAuth.php
   @@ -295,7 +295,26 @@ class SimplesamlphpDrupalAuth {
      public function roleMatchAdd(UserInterface $account) {
        // Get matching roles based on retrieved SimpleSAMLphp attributes.
        $matching_roles = $this->getMatchingRoles();
   +    // Get user's current roles, excluding locked roles (e.g. Authenticated).
   

   The method is called roleMatchAdd(), neither the method name nor the description say anything about removing roles, so that's a bit weird. Not sure what to do about that. At last update the description?

   that said, the UI explicitly mentions that this might remove role, so it makes sense to actually do so.

2. +++ b/src/Service/SimplesamlphpDrupalAuth.php
   @@ -295,7 +295,26 @@ class SimplesamlphpDrupalAuth {
   +    // Remove non-locked roles not mapped to the user via SAML.
   +    foreach ($current_roles as $role_id) {
   +      if (empty($matching_roles) || !in_array($role_id, $matching_roles)) {
   +        if ($this->config->get('debug')) {
   +          $this->logger->debug('Removing role %role from user %name', [
   

   I think you can simplify that a bit to foreach (array_diff($current_roles, $matching_roles) as $role_id).

   that will loop over all roles in $current_roles that aren't in $matching_roles and you can skip the inner condition.

3. +++ b/tests/src/Unit/Service/SimplesamlphpDrupalAuthTest.php
   @@ -173,11 +173,25 @@ class SimplesamlphpDrupalAuthTest extends UnitTestCase {
        // Get a Mock User object to test the external login method.
        // Expect the role "student" to be added to the user entity.
        $this->entityAccount->expects($this->once())
          ->method('addRole')
          ->with($this->equalTo('student'));
   +    $this->entityAccount->expects($this->any())
   +      ->method('getRoles')
   +      ->will($this->returnValue(['student']));
        $this->entityAccount->expects($this->once())
          ->method('save');
    
   

   Shouldn't this also have an assertion on removeRole()? I don't think this actually fails yet without the new code?

Changed a patch according to Berdir's review:
1) Renamed a roleMatchAdd method to roleMatchSync, changed method description.
2) array_diff() is used now. Additionally, the cycle with addRole changed to behave the same. Previously there was no check if role is already assigned before adding it to user object.
3) Changed unit test.

#46 worked for me and looks good. Tested for syncing of single and multiple roles.
Moving to RTBC.

+++ b/src/Service/SimplesamlphpDrupalAuth.php
@@ -287,25 +287,43 @@ class SimplesamlphpDrupalAuth {
+   * Synchronizes (adds/removes) user account roles.
    *
    * @param \Drupal\user\UserInterface $account
-   *   The Drupal user to add roles to.
+   *   The Drupal user to sync roles for.
    */
-  public function roleMatchAdd(UserInterface $account) {
+  public function roleMatchSync(UserInterface $account) {

Hm, that's not quite what I meant with my comment on the method name. Technically this is an API change, but I assume that it's pretty unlikely that someone is using that directly or even overwrote it.

So, lets hope we don't break someones site with this ;)

Hm, that's not quite what I meant with my comment on the method name. Technically this is an API change, but I assume that it's pretty unlikely that someone is using that directly or even overwrote it.

So, lets hope we don't break someones site with this ;)

I have a function that hooks into the simplesamlphp_auth_user_roles_alter so we only sync on roles we select

$allowed_roles = array('role1', 'role2', 'role3', 'etc');
foreach ($allowed_roles as $rid) {
$account->removeRole($rid);
}

This stopped working with the update, but I can change $current_roles to $allowed_roles in the roleMatchSync function to achieve the same thing. It would be great to have a configuration to select which roles we want to revaluate on login. (I know this feature has been requested so I won't put it in again)

If there is a way to add this via a patch that would also be great, I'm just unsure how to do that.

Thanks!

Well, that was obviously going to happen: https://twitter.com/bojan_zivanovic/status/1209477975533588480

I'm not sure I quite understand though. If you don't like the behavior, you can always disable the setting and do whatever you want yourself? there's also the other, more generic mapping hook, I'm using that in a project to map some but not other roles. If that's not enough then feel free to create a new issue.

I'm trying to avoid adding too many complex settings, but try to allow full flexibility through hooks and so on.