🇺🇸United States @pfrilling

Thanks @attheshow. That is expected as the route requires a csrf token. Browsing directly to that route won't have the token, hence the 403. If you use the logout link provided by a menu and/or the login block, it should work. The followup issue ✨ Add _csrf_confirm_form_route option for to the user/logout route Active will get that direct link remedied with a confirmation form.

The rebase has been completed. Marking this needs review.

Thanks again for the fix. This has been merged to 3.x.

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

I'm marking this needs work to fix the update hook as it conflicts with the update hook introduced in 🐛 Warning: Undefined array key "iss_allowed_domains" Active .

Thanks everyone! This has been merged to 3.x.

Thanks all for the work on this! I reworked the patch from #7 into a new merge request. I took the moment to expand test coverage too. If someone has a moment to review, I'd appreciate it.

pfrilling → made their first commit to this issue’s fork.

Thanks @dcam! I ended up using your branch and adding the test from MR43 into it. I broke your update hook tests into their own file. The rebase was making my head hurt 😉. I'd appreciate another review if anyone has time.

It's been over a year since the last update here. I'm going to mark this postponed for now. Is anyone able to confirm they are still using the proposed patches?

Having had no response and/or other reports of this issue, I'm marking this issue as outdated.

Marking this as outdated. All current pipelines are passing, so I don't think this relevant anymore.

Thanks for the report. Closing per the latest comment.

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 feedback everyone. I understand the concerns of being forced into using an alpha. I can assure you the 3.x branch is definitely maintained.

I think next steps are to come up with a realistic list of existing bugs that need to be completed before going beta/stable. From there, we can start working toward that milestone. One of my main goals is to get test coverage up on almost everything.

I definitely want to get to the 4.x branch and @jcnventura's suggestions of using the PHP Leagues Oauth implementations, but we need a stable 3.x first.

Thanks for the help! This has been merged.

I updated the cache logic to utilize the renderer and some of it's built in methods. I did unset the `$form['#cache']` and ran the functional tests and didn't get any errors returned. I think this looks good.

pfrilling → made their first commit to this issue’s fork.

Confirming that changing the "User info endpoint configuration" option from Azure AD Graph API (v1.6) to Microsoft Graph API (v1.0) fixed the login issue in my use case.

We started seeing this error yesterday too. Our configuration is currently using the Azure AD Graph API (v1.6). I did a quick search online about that and it appears that API is being retired by Microsoft. Google’s AI Overview after searching that API returned this:

The Azure AD Graph API (v1.6) is being retired, with its functionality replaced by Microsoft Graph. New applications are already unable to use it, and all applications will lose access on June 30, 2025, unless they have configured extended access. Microsoft recommends migrating to Microsoft Graph.

Microsoft’s blog article about it is here: https://techcommunity.microsoft.com/blog/microsoft-entra-blog/action-req...

I'm going to see if the suggestions from @webflo in #11 work for our use case and will report back.

Code has been merged and the followup issue has been created to add the confirmation form here: https://www.drupal.org/project/openid_connect/issues/3518252 ✨ Add _csrf_confirm_form_route option for to the user/logout route Active

pfrilling → created an issue.

I'm marking this as RTBC from @jibus's review in #5.

@jibus, Yes, I do think we need to add the confirmation form to match core's workflow, but I was planning on doing that work in a separate issue.

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.

@srdtwc, if you apply the patch from MR #144 (https://git.drupalcode.org/project/openid_connect/-/merge_requests/144), does the error go away?

Reading through the spec for redirect uri https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest, I'm not sure we should enforce https.

REQUIRED. Redirection URI to which the response will be sent. This URI MUST exactly match one of the Redirection URI values for the Client pre-registered at the OpenID Provider, with the matching performed as described in Section 6.2.1 of [RFC3986] (Simple String Comparison). When using this flow, the Redirection URI SHOULD use the https scheme; however, it MAY use the http scheme, provided that the Client Type is confidential, as defined in Section 2.1 of OAuth 2.0, and provided the OP allows the use of http Redirection URIs in this case. Also, if the Client is a native application, it MAY use the http scheme with localhost or the IP loopback literals 127.0.0.1 or [::1] as the hostname. The Redirection URI MAY use an alternate scheme, such as one that is intended to identify a callback into a native application.

I think the better approach would be to add a new option that allows an administrator to enforce https if they choose.

CSRF protection and tests confirming have been added to the logout route.

I think we create a follow up issue to introduce the _csrf_confirm_form_route that mimics the OpenIDConnectRedirectController::redirectLogout. That seemed like a bigger refactor and likely warrants a separate issue.

Thanks for the report. Working on this today.

I closed MR #86 in favor of MR #143. I added an update test to confirm the changes. If someone wants to review and mark this RTBC, I'll get it merged.

pfrilling → made their first commit to this issue’s fork.

Looks good to me!

Thanks everyone!

I am still getting the failure locally, but it happens less frequently now. One out of every ten or fifteen times now. I think that request time needs to be set to a constant instead of relying on the modifier, which puts us at the mercy of the container's time.

Maybe I was over thinking it. If we're reliant on time later in the tests, we should probably mock the initial request time. This seems to be giving me consistent passing tests locally.

To be honest, I'm not 100% sure... I started stepping through the InstallerController::begin() method and saw this line:

$updated_time = $this->installState->getFirstUpdatedTime();

Which is returning the `__timestamp` value from the key/value store:

return $this->keyValue->get('__timestamp');

The only place I found the `__timestamp` value being set was in InstallState::setState(). In this method, it is being set if not exists.

$this->keyValue->setIfNotExists('__timestamp', $this->time->getRequestTime());

So, my theory was that the `__timestamp` value was not getting cleared/reset during the test's call to ::setFakeTimeByOffset(), and causing the flaky behavior.

Could it be that $this->keyValue->setIfNotExists should actually be $this->keyValue->set()?

pfrilling → changed the visibility of the branch 3.x to hidden.

I created a new MR with the changes from MR #108 and added functional testing. I think this looks good if someone wants to review and RTBC.

pfrilling → changed the visibility of the branch 3.x to active.

pfrilling → changed the visibility of the branch 3.x to hidden.

Thanks for reporting back @profi248. If we can get another confirmation and someone to mark this RTBC, I'll go ahead and get it merged and a new tag pushed.

I was able to consistently reproduce this issue locally about every third or fourth test run with

ddev phpunit --filter=testInstallUnlockMessage

When I deleted the `__timestamp` key/value, then it would consistently pass. I believe it's because of the ::getFirstUpdatedTime() method call in the ::begin method. Of course, I could be completely wrong.

pfrilling → made their first commit to this issue’s fork.

Thanks @jhartman1 and @r.aubin! I reviewed the documentation and all looks to be accurate.

Removed the specific composer command. Confirming this is all looks to be accurate.

Thanks @jhartman1! I would love to see some documentation pages added. I went ahead and verified your account. You should be able to create those pages. Mark this issue as needs review when complete and I'll review.

Cross posting here, but there is a potential fix for the non-existent service error posted in this comment: https://www.drupal.org/project/openid_connect/issues/3462532#comment-159... 📌 Create and utilise autowiring aliases for OpenID Connect Active . Please apply the patch from MR #144 to Alpha6 and report back on whether it starts working for you.

@anna d. Be sure the add the `file` module to the $modules array as well. See https://git.drupalcode.org/project/openid_connect/-/blob/3.x/tests/src/F... for a full list of required modules. Those should be the only three requirements in your tests.

Looking through the code, there was a call to $container->removeDefinition('openid_connect.openid_connect'); in a ServiceProvider class that was added to the 2.x branch with this issue: https://www.drupal.org/node/3206034 → . This looks to have been included when external auth was added as a dependency.

I think that makes sense now why your Kernel tests started failing with alpha6, if the service wasn't available, then the openid_connect.openid_connect service was being removed. Since the new autowiring fix was setup as an alias, it was trying to find a service that didn't exist.

I believe the service provider class should be safe to remove since the external auth module was installed in both `openid_connect_update_8203` and `openid_connect_update_8198`.

Please test the changes in MR #144 in your situations and report back.

Thanks @danflanagan8. Question, is the issue only in your Kernel tests? Does the site actually work with the alpha6 upgrade?

pfrilling → made their first commit to this issue’s fork.

Thanks @dcam, that's been my experience too with a few client projects I updated to alpha6 this week. One project is using the generic plugin, the other is using the windows_aad extension module.

@anna d, you mentioned custom kernel tests failing. Are you extending the `@openid_connect.openid_connect` service anywhere and/or injecting it in a custom class?

@joseph.olstad, I know you're using the Keycloak module. I'm unfamiliar with that code... is the openid_connect service being used in any non-standard ways?

If @joseph.olstad's fix doesn't work. Can you also try the following drush commands after updating:

drush updatedb --no-cache-clear
drush cache:rebuild

I added a hook to alter the views data and convert the double reference filter plugins to use the entity_reference and taxonomy_index_tid plugins. This seems to be working in my limited testing.

pfrilling → made their first commit to this issue’s fork.

Thanks everyone for the feature! I created a followup issue to address the configuration of which routes should redirect here: https://www.drupal.org/project/openid_connect/issues/3507138 ✨ Add setting to enable/disable user login routes from redirecting Active

pfrilling → created an issue.

Remove unrelated linked issue.

Removing unrelated issue.

Can you confirm if the `openid_connect_rebuild_container_3462532` post update function ran? That should've rebuilt the container after the service definitions changed.

pfrilling → changed the visibility of the branch 3011413-autologin-when-one to hidden.

Thanks for the report. I rebased the 3.x branch. Moving this back to ready for review.

pfrilling → changed the visibility of the branch 8.x-2.x to active.

pfrilling → changed the visibility of the branch 8.x-2.x to hidden.

pfrilling → changed the visibility of the branch 8.x-2.x to hidden.

Marking this as RTBC as the code was already reviewed in a patch on s.d.o.

pfrilling → created an issue.

Thanks for the contribution! I adjusted the language a bit to help clarify the functionality.

pfrilling → made their first commit to this issue’s fork.

Thanks for the cherry pick and the reminder to actually commit the changes. Note to self, the merge train option doesn't work if the pipeline is failing :facepalm: Now, this is officially fixed.

This has been merged. Thanks everyone!

Thanks for the validation @mstrelan. I think this all looks good.

This has been merged to the 1.x branch now. I'm okay with the tests failing there for now. It's the same code in the 3.x branch, which is confirmed with test.

Needs to be ported to the 1.x branch yet.

I think that approach makes sense. I added functional testing to confirm the forms are loading correctly, which uncovered a caching issue.

Marking this RTBC for now.

pfrilling → made their first commit to this issue’s fork.

The code looks fine to me. I left a comment on the MR about handling the exception. What do you think?

I fixed the broken test and added a post_update call to ensure cache's are cleared so the new service names are registered.

pfrilling → made their first commit to this issue’s fork.

All pipelines green! Started the merge train.

Looks good to me. I merged the 3.x branch to confirm the pipeline completes.

pfrilling → made their first commit to this issue’s fork.

Merged the PR. Thanks for getting those tests enabled!

I fixed the deprecations that were flagged for the next minor. I think this looks good.

pfrilling → made their first commit to this issue’s fork.

All tests are now green!!

Thanks @phenaproxima! That did the trick!

I went ahead and fixed the phpstan warnings too. I believe this is now ready for review (I believe the Nightwatch failures are expected).

I updated the drush test to be like the examples you provided @chrisfromredfin. The new test is passing, however, I seem to have broken phpcs/phpstan checks after my rebase. Any thoughts, or do I need to revert and try again?

The php-mock library was just used to mock the global `function dt()` from Drush. The Core documentation I found for mocking these types of functions is here: https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/unit-tes... → , but I honestly always feel like I'm doing something wrong when mocking the global functions that way.

My personal preferences aside, I can rewrite the tests to remove that extra package if you feel strongly about it.

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)?

Looks good. I believe they were all addressed.

A Drupal user account that gets provisioned through any OpenID client should be considered a federated member. Thus, the Identity Provider (IDP) which provisioned the user should be considered the canonical source for the member's data. Any updates to the member's data should be done on that platform.

In order to allow for changes to account data within Drupal, I believe a federated member should disconnect their IDP and then follow a password reset process.

I am in support of disabling the member's email/password fields in these situations. We could probably take it a step farther and provide an action that disconnects them from their IDP and then initiates a password reset to the email stored in the Drupal user account. Then, they could change their password and reconnect to an IDP if they chose.

As for updating an IDP with updates in Drupal: I do think it would be possible to bubble account changes from Drupal back up to a trusted IDP through an API request, but I believe that is a pretty specific requirement and should live in a projects custom code. There are too many variables at play with too many different API requests for it to live here.

From the reproduction steps that @joelfp I would consider this working as designed. Browsing between a browser session and incognito should never work because of the requirement of the session.

- One thing to check is that a member has the `openid connect set own password` if you want to reset a password.
- What is the IDP that you are using?

I think we need more information and solid reproduction steps to consider if this is a bug or not.

I was able to write a functional test to confirm this fix is working. During the automated testing, I discovered that the schema for empty scopes is incorrectly storing a string instead of an empty array. I updated the submit configuration form to properly save an array to configuration, but left the other fixes in place for backwards compatibility.

I'm going to create a followup issue for the other provided clients to get those saving the configuration correctly.

If someone has time to review my changes, that'd be great!

pfrilling → made their first commit to this issue’s fork.

Thanks @dcam! I made the method name change you suggested.

Since Drupal 9 is no longer supported and the form changes were already made that broke the settings form in D9, I went ahead and dropped support for that version.