There are several modules that provide the functionality to specify where the user is redirected to post login via a "destination" query parameter. i.e. yoursite.com/user/login?destination=/node/5. In the particular case that I am currently implementing I am using the anonymous_login module to redirect all anonymous traffic to the login page where they must login using the openid_connect buttons.
Currently the openid_connect does not support the destination query parameter when used on the login page. I will provide a patch that moves the decision to redirect to the /user page if the path is /user/login to the redirect controller and also check for a destination query parameter. I've noticed there are a few other issues regarding potential methods for specifying/altering the redirection, so this patch also changes the structure of the destination array saved in the session variable to work with the standard Drupal URI options array. This will make adding in a dedicated destination alter hook or other potential variations use a standard and Drupal familiar data model.
| Comment | File | Size | Author |
|---|---|---|---|
| #38 | interdiff.txt | 5.03 KB | jcnventura |
| #37 | openid_connect-support_destination_query_param-3022086-37.patch | 12.64 KB | jcnventura |
Comments
Comment #2
acrazyanimal commentedComment #3
acrazyanimal commentedI wasn't able to properly test a destination query parameter with its own query parameters before, but now that I have I noticed an issue and have resolved it in this latest patch.
Comment #4
acrazyanimal commentedErg! Messed up the patch. This one is the one!
Comment #5
ccjjmartin commentedThis issue was trying to address two concerns.
1) Redirect to /user page. This was addressed and committed here: https://www.drupal.org/project/openid_connect/issues/2905229
2) The other issue was redirecting when a destination parameter is set. This one was a PITA to solve, here are some notes below:
This has all sorts of issues that are outside the scope of this module but to give a quick background what I found was that the Symfony Request class has a method called getQueryString(). This method is worthless for modifying query parameters as it takes the original request's query string. There is a overrideGlobals() method too but this seems to be over reaching (we don't really want to modify all PHP globals).
I would say that because it doesn't allow decent handling of query parameters the Drupal community eventually landed on creating a service that interacts with a single query parameter "destination" directly through the
redirect.destinationservice: https://api.drupal.org/api/drupal/core%21core.services.yml/service/redir...The service still has the underlying Symfony problem though and I didn't want to call overrideGlobals() just to make getQueryString() work so I landed on
http_build_query();This issue helped me by directing me to the right function to alter. There is another similar issue which I am going to close that didn't work for me because the current request at the time the authenticate method is run has already lost the destination query parameter.
https://www.drupal.org/project/openid_connect/issues/2988428
The code in my patch addresses the issue of redirecting a user when the destination query parameter is set, removes the query parameter, and sets the query. My patch is based on the 1.0-beta5 release, it may need to be rerolled for the latest dev branch.
Comment #6
ccjjmartin commentedI created a drupal stackexchange question and answer to dive into this a bit deeper if anyone is interested: https://drupal.stackexchange.com/questions/290865/symfonys-getquerystrin...
Comment #7
shamsher_alam commented#5 Patch is failed and not working.
Comment #8
shamsher_alam commented#2 updated patch.
Comment #9
shamsher_alam commentedUpdated new patch.
Comment #10
shamsher_alam commentedUpdated patch with controller changes.
Comment #11
shamsher_alam commentedAdding changes from ccjjmartin patch.
Comment #12
shamsher_alam commentedComment #13
spadxiii commentedPatch in #12 did not apply, so I re-rolled against latest dev.
Comment #14
shamsher_alam commentedHi,
I have tried with given patch and it is also getting failed so created new one.
Comment #15
shamsher_alam commentedComment #16
shamsher_alam commentedSingle patch and it works with latest release of openid module. Thanks @SpadXIII
Comment #17
spadxiii commentedThe last patch doesn't seem to apply either, so re-rolled that and also updated the code to use 'path' and 'options' keys instead of index 0 and 1 for destination.
Comment #18
ccjjmartin commented@shamsher_alam Can you describe why you made the changes you did? I had more code than I uploaded here originally but was able to cut it out and keep it small and simple. It is likely my original patch didn't apply because I was using the version of this module that was installed on the client site it is running on right now.
Comment #19
shamsher_alam commentedHi CCJJMartin,
I need a destination parameter redirect for the beta5 version of open_id. That's why i have done these changes.
Thanks
Comment #20
acrazyanimal commentedHi Everyone, now that I'm preparing for an update to Drupal 9 I've come back to ensure this issue continues to move forward. Hi fives to everyone adding patches to this issue. I've reviewed all of them and spent time considering the approaches taken.
@ccjjmartin I like how you were able to condense some of the code down. An issue I found with your approach was that it was no longer checking if the destination would redirect to /user/login. Another issue is that if the original request had query parameters AND a destination parameter it would redirect to the destination path, but add the parameters from the original request, which would not be desirable. If there is a destination then we want to go to it and just forget about any additional query params from the original request. However, if there are query params or fragments as part of the destination query parameter uri then we would want to keep those. Make sense?
@Shamsher_Alam I appreciated you re-rolling the original patch to keep the destination query param re-routing, but either your ide or intentional it was hard to read through the clutter in your patches due to the code standard fixes you were including. I recommend that if you want to clean up some coding standards you submit an issue to fix those and keep this one clean and relevant to this issue.
@SpadXIII I like that you tried to bring it all back together. The code just keeps moving I guess and re-rolls have been needed. Thanks.
Okay, so having reviewed all patches here I was able to take the best of each, reduce the code further, and I believe solve all the destination redirection issues. Its actually fairly simple. The function
\Drupal::destination()->get()that @ccjjmartin suggested in their patch actually gives us the uri (including query and fragments) from the current request's destination query param if one exists, otherwise it returns the current path and query (also simply '/' if destination is external). See the function here. So all that is really needed is to set the 'destination' in the session to this function's return value after checking to make sure its not '/user/login'. That's pretty much it!! All the query management stuff can be removed completely since its now just included in what we save to the session as the destination uri.Check out this patch and please review and test.
Cheers.
Comment #22
acrazyanimal commentedOh! Unit tests ... right! Here is an updated patch with the unit tests updated. Also, changed the services injected into
Drupal\openid_connect\OpenIDConnectSessionto only include the 'redirect.destination' services since that is all that is needed anymore.Comment #23
acrazyanimal commentedOoops..
Comment #24
jcnventuraInterdiffs, please!
Comment #25
acrazyanimal commented@jcnventura Sure here are the differences between 20-22 and 22-23. In the latter I wasn't set up locally for unit testing and I had left semicolons where they should not have been. So a small fix there.
Comment #26
generalredneckSo is this issue still relevant? I see that there is handling for the destination query parameter over in src/Form/OpenIDConnectLoginForm.php:107.
Comment #27
alegacy99 commentedThis patch didn't work for me whilst using the Anonymous Login module or Redirect 403 to User Login. This is a big drawback to this module if we can't have this working.
Comment #28
jcnventuraNeeds a re-roll for version 2.x of the module, as 1.x is no longer getting new features.
Comment #29
jcnventuraComment #30
slasher13re-roll
Comment #31
jcnventuraThis looks pretty good. Can I ask a favour @slasher13?
Instead of hard-coding a redirect to 'user', can we make that the default of a new configuration setting? So that site builders can choose where they'd like to redirect users that use the module after they login? I can imagine some would be OK with
user, others would prefer<front>...Comment #32
slasher13addresses #31
Comment #34
jeroentComment #35
jcnventuraNeeds a few things more. The config factory must be dependency injected into the session service.. And I'd rather rename the new setting to 'redirect_login' instead of 'replace_login_redirect'. I plan to introduce a 'redirect_logout' setting soon.
No interdiff because a) the previous patch is failing; and b) the interdiff is almost larger than the patch with the changes above (also c) I'm the maintainer, and I don't need to see my own interdiffs).
Comment #37
jcnventuraComment #38
jcnventuraHad forgotten to actually use the injected config factory. And the tests required some change.
Comment #40
jcnventuraComment #42
mducharme commentedI've confirmed the 1.x patch #23 with Anonymous Login. I guess this won't get committed since it's 1.x branch which is no longer supported but for those still on that branch, it's working for me.