Implement ".well-known" automatic provider discovery

This is a different spec, and is optional as far as OpenID Connect is concerned. Changing to feature request.

I'm +1 on implementing this, but we'd need to make sure that the oauth2-server-php library and the oauth2_server module support it first (since they are usually used together with this module).

Hi all,

Just wanted to let you know that I have released a sandbox project that can hopefully be part of the OpenID Connect module in future releases. It includes the Discovery workflow as well as the logout (end session, refresh token) workflow for openid connect.

You can find the sandbox project here: https://www.drupal.org/sandbox/antrecu/2475233
Please visit the project page and the demo sites for more information and to provide feedback.

Thank you!

I have reviewed the sandbox module. I would really like to see this included in the OpenID module as standard.

How can I help?

Here is a lighter solution.
Hope it helps someone.

I've tested and reviewed the patch in #4. I think it's a sensible approach and the patch works as advertised. I'm attaching a re-roll that adds some minor docblock formatting/comment fixes.

It would be great to get a patch for 8.x branch.

We are looking at using the generic provider, with this feature added, instead of the google provider to deal with the Google Plus shutdown starting in January. It would be nice if we could enable discovery for the google provider, as well, so we wouldn't have to migrate the authmap data. We aren't using profile pictures - the only thing the google provider offers apart from hard-coded endpoints (as far as I can tell) - so that isn't a problem for us, but might be for others. Does anyone have any thoughts on this?

Thanks!

This is first pass on a 8.x port of the patch in #5. I'm going to change the version on this issue since I believe the convention is to get 8.x patches committed first, then backport.

Cheers -

Just tried out the 7.x version of this patch from #5, and have some feedback to offer:

• There needs to be more error handling, for cases in which the wrong URL has been provided for the "Issuer URL". Currently, if you get it wrong, the first (and only) error you receive is:
  

  Notice: Undefined index: authorization in OpenIDConnectClientBase->authorize() (line 116 of /modules/openid_connect/includes/OpenIDConnectClientBase.class.php).

• getEndpoints() should not allow the structure of the auto-discover document to drive the detection of the endpoints. Here's why:
  • What if the document has 3,000 key value pairs? It will iterate over all 3,000 just to get the 3 values it needs (0.1% of the document). That's not efficient.
  • This module's implementation of OpenID connect requires three endpoints to function ("authorization", "token", and "userinfo"). What if the ".well-known" document is faulty and one or more of these endpoints is missing? By letting the document drive the process of acquiring the endpoints, the module will never catch that some endpoints are missing (until you get the PHP notice as I've mentioned).
• It would be more efficient and provide a better user experience to attempt to fetch and validate the ".well-known" endpoint upon saving the settings form. At that point, it would be possible to attempt to extract all three endpoint values and persist them in the same settings that this module previously was using before this patch. That way the rest of the code path doesn't actually need to specially-handle the ".well-known" use case, and immediate feedback can be provided to admins about an incorrect URL and/or malformed document.

Setting to Needs Work b/c my feedback appears to also apply to the 8.x version, since it's a port of the 7.x verson.

I'm so glad the code I shared became useful! I'm taking down the testing sites but if theres any help needed on this matter, I'll be around. Thanks everyone for the hard work!

Shouldn't we verify the configuration on save and actually store the value acquired from the .well-known to normal configuration values and regularly (cron) update these values. That way we don't spam the .well-known address and if the configuration is temporary missing or flawed we still have to old working configuration values.

Here is an 8.x patch based on feedback from @GuyPaddock and @tim_dj. I worked on it awhile back but then I got busy. No doubt it needs work but hopefully it gets us most of the way there.

Edit: Don't use this patch. See #12

Here's a better patch but it still needs work.

Major changes since patch in #6 (interdiff provided):

- Provides a new OpenIDConnectAutoDiscover service that makes a request to the issuer URL and returns the endpoint values.
- Modifies OpenIDConnectGenericClient to validate issuer URL and update _endpoint values when configuration form is submitted.
- Implements hook_cron to initiate auto-discovery of endpoints on a weekly basis.

+++ b/config/install/openid_connect.settings.generic.yml
@@ -2,6 +2,8 @@ enabled: false
 settings:
   client_id:
   client_secret:
+  autodiscover_endpoints:
+  issuer_url:

We need an update hook to handle these config changes.

Edit: Also add these new keys to config\schema\openid_connect.schema.yml.

Here is another patch based on my comments in #13. A review would be appreciated. :)

@othermachines: WIll test it but I think you should make the tests pass as well.

Patch no longer applies

Re-rolled #14 and created MR 5 from it. I only did minor changes to docblocks.

I'm also attaching:

• Patch from MR.
• Screencast showing how auto-discovery works; that site is on Drupal 8.9.

I didn't test:

• Config export of settings.
• Cron functionality.

Needs a re-roll again now that I've changed how the module works.

Thanks for the patches everyone. I've committed a re-rolled version still without the cron update. Mostly because this must be left to the plugin itself how to handle. A normal IDP shouldn't be switching their endpoints regularly, so I think most plugins can choose to keep the endpoint URLs hard-coded.

I'll create a new issue for the cron update.

Now that this is fixed in 8.x, shouldn't we backport to 7.x-1.x? Attaching a re-rolled version of the patch in #5. I'm not sure if I need to open a brand new issue. Please advise. :)