User role cleared after log in

This sounds like a similar issue as #3487116, which should have corrected this.

- What OpenID Client are you using?
- Can you provide the plugin's configuration before and after the upgrade (obfuscating the sensitive parts)?

Did you run the database updates after updating to alpha5?

I encountered the same problem, and re-saving the OpenID Connect settings resolved it for me.

alpha5 has the fix for this issue.

Update to alpha5, and there are configuration changes for the module settings that needs to be exported. After the export the configuration looks something like this.

_core:
  default_config_hash: NFnOZU6VCOfURCAR4-O3V7eMHTx2LNqnCmDKRSNjjns
always_save_userinfo: true
connect_existing_users: true
override_registration_settings: false
end_session_enabled: true
user_login_display: above
redirect_login: /user
redirect_logout: /
userinfo_mappings: {  }
role_mappings: {  }
 

I did run drush deploy after upgrading so I assume database update was successful.

I'd need to know more about your deployment workflow. Did you update the module on a dev environment, update the database (without using drush deploy), export the configuration, commit the configuration, push your changes to production, install the update, and then run drush deploy?

I asked about the workflow because drush deploy runs database updates and then imports your configuration. If a person did that on an environment without following a strict procedure for configuration management, then their database would be updated and then immediately overwritten with the old configuration. That would result in no changes being apparent.

In any case, if the database updates were run but your configuration remains in a broken state, then it's probably going to need to be manually repaired at this point. @gokul.jayan gave the solution. The role_mappings key needs to be reduced to an empty array, role_mappings: { }. Personally, I would just edit the config file to do it and then import the configuration.

Hi @hktang
I also encountered the same problem with alpha5. In my case, when I resaved the module settings, there were changes to the role mappings—specifically, the removal of all role mappings and their replacement with only the role_mapping key containing an empty array. I believe resaving the module settings should resolve the issue.

Yeah. That should work too.

Hi @hktang
During the Drush deploy process, configuration changes were there in the openid_connect.settings, and the new changes were overridden. Please try resaving the module settings (/admin/config/people/openid-connect/settings) and then run drush cex.

+1 We have also run into this issue where our custom Drupal roles are getting removed on user log in. We aren't using the role mapping feature built into the module and the experimental fields are blank. We do use drush deploy, and I agree with @hktang's comment #26 that it should work either way with drush deploy or drush cim. Also thank you to @hktang for diving into the issue and finding out this weird problem.

+1 Also ran into this issue where we don't have role mapping enabled, but the role_mappings config not being an empty array and overriding on login.
After changing the config manually it works again.

hi,
We were facing this issue with openid_connect. As the users was logging in the role was removed for the users. The fix provided helped us resolve the issue.

Thanks

I am not moving the issue to RTBC as the pipeline for the MR is failing. Once the pipeline passes it will be ready for RTBC.

It is also possible to run into this situation if you are using config_split to only enable certain modules on upper environments (eg. Prod) and forget to account for an update hook for those modules.

I think there are situations where a site would prefer to have the roles cleared if the groups array was empty (as is the current behavior). We should probably add a configuration option that would allow a site to choose the behavior. Then, we need to decide which option should be the default. I'm leaning to making the default to _not_ clear the roles, as that feels like the least intrusive, but am open to other opinions.

We are facing the issue after updating the roles after each re-login is cleared. (our mapping is default, email)

So also looking for a solution to that.

Patch from the MR. It works for me.

I started a new branch and merge request from the patch of MR 133 (I couldn't figure out how to rename the branch 🤷‍♂️). See MR #157 for the updated code.

The new MR includes the aforementioned configuration option that would allow sites reliant on the current default behavior to continue working. I set the default value to be off. Any sites that want this behavior to continue will need to manually enable that configuration option. I don't see a method to enable that for some sites and not others. I'll need to over communicate that breaking change in the release notes after this merges.

Thanks for the review @hktang! I resolved all the threads in the MR. Can you review again?

Am I right, that this behavior to remove all roles if no "groups" is set came in with #3492759: Mapped user roles are not always revoked?

See here: https://git.drupalcode.org/project/openid_connect/-/merge_requests/130/d...

You are correct @hktang, the way it is configured now, any site that wants the roles cleared will have to update the configuration and enable the new checkbox. I don't think there is a reliable way to write an update hook to turn it on for some sites and off for others.

The other option is to default the new configuration to be enabled and force existing sites to opt-in to disabling the role map clearing. This would be more consistent with existing behavior and would requires sites like yours to disable the new configuration option.

Either way, some sites will have to make a configuration change.

I like defaulting the option to off and forcing new sites to enable the feature as I think it will cause less confusion in the future. Unfortunately, this is an inconvenience for existing sites. I'm open to other suggestions/thoughts.

@guedressel, I believe the functionality always existed prior to the change you called out. Well... unless `groups` was never defined in $userinfo 🤔. Maybe the conditional should be added back in?

If you're going to add this as a config option for the module without advanced notice (like announcing and waiting a release cycle), I agree it will be least disruptive for existing users to have the default setting match current behavior.

If you notify users between releases that the change is coming in an upcoming release, ensuring it's visible on the module description page, then changing the default behavior from what it is now should reduce the risk that any users are caught off guard by the change and then it becomes the standard behavior.

We use this module in a setup where we rely on the OIDC group to Drupal role mapping. But there it is okay to have not group defined (empty) and also to have Drupal roles manually assigned to user. With the change in #3492759: Mapped user roles are not always revoked all manually assigned roles get lost on login of an user.
Hence we are currently working with a patch to roll-back the changes from #3492759: Mapped user roles are not always revoked and waiting for this issue to give a way forward without patching.

+1 to default setting match current behavior. If we want to change the default in the future, what about including a message in watchdog or status-page that there now is a new setting that controls the module role behavior, that should be verified? I provide admins ample time to review and test the implementation. The module can through the issue queue get feedback on this new behavior before changing the default in the future. Though it should perhaps be done before rolling the first beta of 3.x.

- I refactored the logic based on the latest feedback and I reverted the default `force_reset` option to be enabled by default, which should be more in line with the existing behavior.
- I added back the original isset($context['userinfo']['groups'])` check to get back to the original role behavior before #3492759.
- I replaced the unit testing with better functional testing of the group logic.

I believe all of the functionality is back in place with the new configuration option. Would love some testing/feedback.

What is left in here? Is it only manual testing?

Can someone verify that the issue is reproduceable and fixed by the changes?

Yes, I believe the only thing left here is manual testing to confirm the original role mapping is retained after updating.

alpha8-compatible patch

I haven't manually tested, but I reviewed pretty closely about three weeks ago, especially with regard to the test coverage. I think the coverage is sound. It's kind of a bummer that some fast unit tests are being replaced by some slow functional tests, but I think the closer-to-real-life issues scenarios are hard to do with unit tests. The only missing coverage I noticed and commented on has been addressed.

I am adding a "Proposed resolution" section to the issue summary.

Comment #36 suggested closing this issue and opening a new one for the new setting. I think it would have been simpler to follow that suggestion (converting the issue to a support request).

I have reviewed the non-test code, and it looks good. @danflanagan8 approved the test code (Comment #57). If someone has done manual testing, then we can consider this issue RTBC.

The individual configuration options are not documented in the README, but they are (with screenshots) on https://www.drupal.org/docs/8/modules/openid-connect/configuration. I added #3582142: Update documentation for the 3.x branch.

I set up a local test site. Using ddev share and following the instructions on https://www.drupal.org/docs/contributed-modules/openid-connect/client-co..., I configured Google as an OIDC provider.

I tested both the original report on this issue and the effect of the new force_reset_role_mappings setting. One thing I missed until today is the last part of the description for the "User role mapping" setting:

Note: The module will not update user roles with no mapped external groups. If all mappings to one of the roles are removed, users will keep that role until it is removed in the Drupal user administration.

I am updating the issue summary to take that into account.

The new setting has the expected effect. Along with Comments #57 and #58, I can now set the status to RTBC.

Thanks everyone! This has been added to the merge train 🚆