Update Redirect Uri

Just access your site via https and the redirect URL will change accordingly.
Also, this is just a hint for what you have to give to the provider, if your site is accessible via https , simply adjust the URL.

Please configure your site with ssl active.
The login block should only be available with ssl active.
Clear caches, if the URL doesn't change.

We could hard-code ssl, as openid connect should not be used without it anyways.

Am also facing the same issue.

If you can give some guidence on how to fix this issue, we can implement the fix.

This issue is a blocker for us - we are unable allow users to login.

And it seems all we need to do is add one letter to the redirect url to make it work.

So any help or information on how/where to fix this is appreciated.

Hi,

I also experience the same behavior as describe in the issue summary.
SSL is enabled but the redirect URL does not use https but http.

I attached a patch that:

• Add a method getRedirectUrl to the \Drupal\openid_connect\Plugin\OpenIDConnectClientBase
• Use this method instead of re-code again and again in methods buildConfigurationForm, authorize,
  retrieveTokens the redirect URL. This allow people to override the redirect URL without copy/paste then modify all the methods above just to change the redirect URL.
• Force to use https since OpenId Connect should not be used without SSL
• Use DI instead of static call for the language manager and the page cache kill switch

Oops.. forgot to change the status.. sorry

Works as expected.

@johnreytanquinco Did you try the patch or did you find an issue on your server?

Enforcing HTTPS with openid_connect makes life harder for any local development/testing. You then would need an SSL certificate for all environments using this module to authenticate its users.

While there is no doubt that SSL in production is a must, it rather should be solved on webserver level than within this module. I strongly vote against merging this patch into the module.

Yet, without having applied it and just reading the patch, it also contains some best practices fixes (e.g. using dependency injection rather than static getters for the language manager). Maybe these could be extracted to a separate issue/patch?

PS.: I also closed the related issue within our Keycloak plugin module as won't fix and gave some advices to setup SSL on the webserver.

Perhaps an alternative approach is to give the admin the option to to use http/https in the redirect.

An according setting to optionally switch SSL enforcement on/off within the UI (or settings.php/configuration export/import) would indeed be the better alternative to hard-coding the protocol.

I still find this setting is not needed at all, if the server is configured properly. But this issue here and also with our module's issue queue proves that not every user of openid_connect seems to being able to do so.

Just the final judgement remains with the maintainer(s) of openid_connect.

I still find this setting is not needed at all, if the server is configured
properly.

I think this is why a switch is the only way - because what is proper or right is too hard to determine. There are too many ways to configure this now. For me personally, ssl is configured by the container orchestration layer, which is outside of, and precedes the server altogether.

I have a similar problem in my current project, where the vendor of the identity provider asks for a predefined redirect URL path on all clients.

The patch of this issue enables just that, in backwards compatible way.

The code looks great to me, I think this is RTBC. In the attached patch, I only adjusted the following small details:

- langcode=none is a sane default that can be always applied; a client plugin is still able to override it.

- Converted the route name into a constant, so that client plugins are also able to override just that without overriding the method.

Ignore the patch attached to this comment, it combines this change here with #2888048: Allow OpenIDConnectClient plugins to provide default config for composer.json

Please use the patch in #25

@sun: I set the issue back to "needs review". Your patch may work in your environment and for your use case, yet it still has to undergo a review with other environments before it can be applied to the openid_connect module. Especially, as there are quite some changes involved in your patch.

Could you please provide some more information, why your identity provider requires a pre-defined redirect URL? All IdP's I've come across so far allowed to specify a redirect URL for the clients.

Yes, the IdP we're dealing with supports it, too, but it's a tedious and progress-delaying process. Meanwhile they changed it, but I wasn't able to continue to work on the integration with the mismatching redirect URI before. This patch helped us to make a lot of progress in the meantime.

It seems artificially limited to me that a client plugin can practically customize everything – except of the redirect URI.

The redirect URI should not be configurable by site administrators, but a custom client plugin implementation (i.e., custom code) should be able to customize it more easily. Right now you need to hack/patch openid_connect module to achieve it, which isn't good design.

The actual changes performed by this patch are very small. There are no API changes involved.

The patch contains additional dependency injection cleanups by @beram, which I didn't remove, because they are perfectly correct and fully made sense to me, too.

I did not actually change much in the patch myself, which is why my usage & polishing of @beram's patch should be interpreted as the third-party testing you're asking for; please have another look at the interdiff I provided in #25.

I think a more elegant solution that would serve everyone's needs (including my unrelated needs) would be to implement a hook here so that we can get at the outbound URL via code before the user is redirected. I have the need to addend additional parameters for a Salesforce implementation and I'm not able to other than to create a patch. Cheers.

Hi! Sorry for being long gone.

Enforcing HTTPS with openid_connect makes life harder for any local development/testing. You then would need an SSL certificate for all environments using this module to authenticate its users.

IMHO today using SSL for all environments is very easy (and it should be done) so I don't find this remark relevant.
Furthermore, forcing the scheme to be https respect the OpenID Connect specifications.

Anyway IMHO forcing https is not the important part of this patch.
The important part is what @sun explained in #28:

The redirect URI should not be configurable by site administrators, but a custom client plugin implementation (i.e., custom code) should be able to customize it more easily. Right now you need to hack/patch openid_connect module to achieve it, which isn't good design.

With the actual code: when you create your custom plugin you need to redevelop all methods because there is no method to get/set the redirect URL (and in each methods that needs the redirect URL it is duplicate code). Functions should do one thing, it is easier to develop with, to test it etc..

If forcing https is blocking this patch it's OK to remove that as long as people are able to change the redirect URL easily.

I think a more elegant solution that would serve everyone's needs (including my unrelated needs) would be to implement a hook here so that we can get at the outbound URL via code before the user is redirected. I have the need to addend additional parameters for a Salesforce implementation and I'm not able to other than to create a patch. Cheers.

IMHO adding a hook is not more elegant than using OOP and design pattern :)

I ran into this issue where we have SSL termination at load balancers (serving HTTPS), which reverse proxy web application servers (HTTP). Users only ever see HTTPS, but the application servers don't know that. The redirect URL, token and auth URLs all come out http, which our IDP doesn't like.

The simplest hack for me is to enforce HTTPS in those three places. This is against the 8.x-1.0-beta5 release:

--- /Users/leeo/Downloads/OpenIDConnectClientBase.php	2020-02-18 23:38:30.000000000 -0800
+++ /Users/leeo/Downloads/OpenIDConnectClientBase.php.patched	2020-02-18 23:38:47.000000000 -0800
@@ -109,6 +109,7 @@
       ],
       [
         'absolute' => TRUE,
+        'https'    => TRUE,
       ]
     );
     $form['redirect_url'] = [
@@ -172,6 +173,7 @@
       ],
       [
         'absolute' => TRUE,
+        'https'    => TRUE,
         'language' => $language_none,
       ]
     )->toString(TRUE);
@@ -220,6 +222,7 @@
       ],
       [
         'absolute' => TRUE,
+        'https'    => TRUE,
         'language' => $language_none,
       ]
     )->toString();

For anyone who just needs OpenID Connect to use HTTPS paths even if Drupal is for some reason defaulting to HTTP.

I'm agreeing with @sun on #25, as that patch is basically moving some code that is repeated 3x in this class to a single function that can then be easily overridden by each plugin, along with some DI cleanup.

The only thing I'm not committing at this time is the 'https' => TRUE,. Usually, if Drupal is defaulting to http, when it should be https, it means your reverse proxy configuration is broken. That ended up being the only line in #25 that was new code.

Creating a URL Alias for '/openid-connect/{client}' to your desired route might do the trick. This will not handle HTTP / HTTPS issue but you can have a custom redirect url.

Thank you @Igna Burmova for your patch!
it works fine but i think it's better to check on schema of the site to set https to true or not !

You shouldn't check for https usage yourself, Drupal's url functions already do that for you.

For those who have this problem in connection with Windows AAD (or want to see how we got around it), check out #3124961: Enforce HTTPS.

jcnventura is right, we can simply overwrite the `getRedirectUrl()` method and set `'https' => TRUE` there before calling the parent method.

Thank you @dpacassi we just need to add `'https' => TRUE` to allow https

This fix works on 3.0.0.alpha2 and enforces HTTPS on redirect URLs.