I'm trying the new logout redirect URL in admin/config/services/openid-connect/settings and I run into the following problem:
the URL appended as "redirect_logout" parameter is still in the format of a local Drupal URL, i.e. it misses the schema and domain name:

etc...etc...../openid-connect/logout?redirect_logout=/en/about

instead of

etc...etc...../openid-connect/logout?redirect_logout=https://mydrupal.com/en/about

My OIDC provider cannot obviously load "/en/about" and stops on a blank page after logout. The logout works indeed, both in Drupal and in the OIDC provider but I'm not redirected to Drupal because the URL is not correct.

Comments

pbattino created an issue. See original summary.

pbattino’s picture

It's not just that: for my OIDC provider to work (Keycloak) the parameter should be called:
redirect_uri and not redirect_logout

To sum up:what I have now:
...openid-connect/logout?redirect_logout=/en/about

what would work instead:
...openid-connect/logout?redirect_uri=http%3A//mydrupal.com/en/about

It seems that after 2.0.0-alpha6, the parameter is called post_logout_redirect_uri which in my case works as well! So the problem should be just the absolute VS. relative url. Can somebody confirm? This could be solved with
array('absolute'=>TRUE)
as options in the method Url::fromUri() , right?

pbattino’s picture

Version: 2.x-dev » 2.0.0-alpha6
pbattino’s picture

Version: 2.0.0-alpha6 » 2.x-dev
jcnventura’s picture

The post_logout_redirect_uri is not in 2.0-alpha6, but was added in #3215229: Okta integration, Logout fails to send client_id, results in error. I'll release an alpha7 soon.

Can you create, test and provide a patch to make the URL absolute?

pbattino’s picture

Exactly! That's why I changed back the tag to 2.x-dev.
I will try to create the patch (I'm a bit rusty on Drupal patches, forgive me!). I was held back by #3216991: Argument 1 passed to OpenIDConnectClientBase::retrieveUserInfo() must be of the type string, array given but I see it has been resolved so I can start testing again.

pbattino’s picture

I hope it's the correct format...

  • jcnventura committed d818d82 on 2.x
    Issue #3216938 by pbattino, jcnventura: logout redirect URL missing...
jcnventura’s picture

Status: Active » Fixed

It was, but I decided to make it absolute only when we use an end session endpoint. The code I committed now should be functionally the same as the one you provided.

pbattino’s picture

I get
Error: Call to a member function getGeneratedUrl() on string in Drupal\openid_connect\Controller\OpenIDConnectRedirectController->redirectLogout() (line 311 of modules/contrib/openid_connect/src/Controller/OpenIDConnectRedirectController.php).

Shouln't it be already a string, after redirect_logout_url->setAbsolute()->toString() ? Why getGeneratedUrl() ?

pbattino’s picture

Status: Fixed » Active

  • jcnventura committed 91321c5 on 2.x
    Issue #3216938 by jcnventura: fix "Call to a member function...
jcnventura’s picture

Status: Active » Fixed

Sorry. The fix seemed so simple that I changed the code without proper tests.

pbattino’s picture

Status: Fixed » Active

now I get this:
LogicException: The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Please ensure you are not rendering content too early. Returned object class: Drupal\Core\Routing\TrustedRedirectResponse. in Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (line 154 of core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php).
absolutely no idea of what it means...

slasher13’s picture

Status: Active » Needs review
StatusFileSize
new951 bytes

addresses #14

pbattino’s picture

isn't #15 just restoring the status of #10 ?
I get the same error and it seems the file is exactly the same as it was after https://git.drupalcode.org/project/openid_connect/commit/d818d82 or not?

slasher13’s picture

The patch wasn't committed.
This was the 1st commit: https://git.drupalcode.org/project/openid_connect/-/commit/d818d82806be1...
and 2nd was https://git.drupalcode.org/project/openid_connect/-/commit/91321c5d4165d...

Line 311: ->toString(TRUE) fixes the early rendering problem. Maybe you have the same problem in hook_openid_connect_redirect_logout_alter().

see https://www.lullabot.com/articles/early-rendering-a-lesson-in-debugging-...

pbattino’s picture

oh gosh! So sorry slasher13, I got confused in switching branches locally, yes you are totally right!
Forget my comment.

jcnventura’s picture

Status: Needs review » Fixed

I fail to understand why #15 is necessary. At this point we need only a string to set in the the end session's post_logout_redirect_uri query parameter. The existing toString() function does just that and nothing more.

I know I've messed up the module when using the end session endpoint, but I need to understand why this is a problem.

Also, this problem is popping up in two separate issues at the moment: this and #3215229: Okta integration, Logout fails to send client_id, results in error.

Since this no longer has anything to do with the objective of this issue - setting the URL to absolute - I'm setting this back to fixed. Please keep all comments in the other issue.

pbattino’s picture

Status: Fixed » Needs review

I confirm the patch works, thank you!
jcnventura #15 is necessary to avoid the early rendering problem, which is something I totally don't understand even after having read many time Matt Oliviera's post on Lullabot. :/ If I understand correctly, the problem was caused by the "TRUE" missing in ->toString(TRUE) right?

jcnventura’s picture

Status: Needs review » Fixed

Let this issue close, please. Or I'll close it myself.

pbattino’s picture

Sorry jcnventura, I probably confused the issue, I don't know why but it seems that by just commenting I moved the status back to "Needs review", is that possible?

Anyway my comment was just confirming that the patch #15 fixes the issue so if you merge it into dev the issue can be closed.

jcnventura’s picture

Yes, @slasher13 proposed this patch in the issue where I'm focusing on fixing this. Even though I still fail to understand why this is fixing it.

Status: Fixed » Closed (fixed)

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