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.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | openid_connect-3216938-7-logout-redirect-url-domain-name-15.patch | 951 bytes | slasher13 |
| #7 | openid_connect-3216938-7-logout-redirect-url-domain-name.patch | 1018 bytes | pbattino |
Comments
Comment #2
pbattino commentedIt'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/aboutwhat would work instead:
...openid-connect/logout?redirect_uri=http%3A//mydrupal.com/en/aboutIt 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?
Comment #3
pbattino commentedComment #4
pbattino commentedComment #5
jcnventuraThe 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?
Comment #6
pbattino commentedExactly! 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.
Comment #7
pbattino commentedI hope it's the correct format...
Comment #9
jcnventuraIt 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.
Comment #10
pbattino commentedI 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()? WhygetGeneratedUrl()?Comment #11
pbattino commentedComment #13
jcnventuraSorry. The fix seemed so simple that I changed the code without proper tests.
Comment #14
pbattino commentednow 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...
Comment #15
slasher13addresses #14
Comment #16
pbattino commentedisn'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?
Comment #17
slasher13The 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-...
Comment #18
pbattino commentedoh gosh! So sorry slasher13, I got confused in switching branches locally, yes you are totally right!
Forget my comment.
Comment #19
jcnventuraI 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.
Comment #20
pbattino commentedI 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?Comment #21
jcnventuraLet this issue close, please. Or I'll close it myself.
Comment #22
pbattino commentedSorry 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.
Comment #23
jcnventuraYes, @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.