Better handling of roles mapping (empty != all empty)

I don't understand this report.. The code that adds or removes user groups based not on the groups claim, but on the configured role mapping.. If the role mapping is empty, it should do nothing. See the code in https://git.drupalcode.org/project/openid_connect/-/blob/2.x/src/OpenIDC...

Think of the role mapping as being the on/off switch you are requesting, but instead of having an explicit switch, it is simplified by the presence or not of a mapping. If there's a mapping, the feature is on, if there is no mapping the feature is off.

Of course, if the role mapping is not empty (i.e. there are groups being mapped to roles), and the groups claim is set, then the module will apply the expected configuration.

Can you please clarify how is it that the site removed all groups from the user role? There might be a bug on how the mapping logic is working that wasn't handled correctly in #3224128: Revoking group access does not reflect on applied roles → .

If the role mapping is empty, it should do nothing.

This is what I expected to happen, but unfortunately it seems that if you fill in at least one role, you can't go back. That is, resetting that fieldset with all empty fields, will not remove the mapping but rather map all roles to an empty string, meaning no role will be assigned and all roles will be removed.

In short, it seems that there is no way to "empty" the configuration of role_mappings. Even if you empty all fields in https://www.efas.local/en/admin/config/people/openid-connect/settings#ed... , the configuration will be:

role_mappings:
  editor: {  }
  role1: {  }
  role2: {  }
  role3: {  }
  role4: {  }
  role5: {  }

etc..
To cross-check that this was the problem, I edited the configuration removing altogether the "role_mappings" key and this restored the expected behaviour, i.e. no mapping at all / no removal of already assigned roles.

So perhaps we should just better handle the case of when all mapping fields are empty, regardless of whether they were never used or used but then emptied.
(I stumbled upon this edge-case because errors after fresh install when no mapping is specified: 🐛 Trying to access array offset on value of type null in Drupal\openid_connect\Form\OpenIDConnectSettingsForm->buildForm() Fixed )

Yes, removing all maps should be the same as never having mapped anything. This should be fixed.

What about changing this part of submitForm() in openid_connect/src/Form/OpenIDConnectSettingsForm.php?

  public function submitForm(array &$form, FormStateInterface $form_state) {
    parent::submitForm($form, $form_state);

    $role_mappings = [];
    foreach ($form_state->getValue('role_mappings') as $role => $mapping) {
      $values = array_values(array_filter(str_getcsv($mapping, ' ')));
      if (!empty($values)) {
        $role_mappings[$role] = $values;
      }
    }

It seems to work for me, I can submit a patch in a couple of hours.

Can somebody please review?

Bumping. Can we merge this?

Hello Folks!!
I have reviewed & applied the patch on the module And I confirm the #6 patch has been applied successfully.
As you can see in my attached screenshot.
And Thank You.

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