Skip to content

Add baseScheme, baseHost, basePort and basePath to the post_logout_redirect_uri #11229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

MPriess
Copy link
Contributor

@MPriess MPriess commented May 13, 2022

For redirects it's possible to pass a redirect URI template (gh-6900) which support baseScheme, baseHost, basePort and basePath.

The current implementation of OidcClientInitiatedLogoutSuccessHandler only support the baseUrl. To make the API and usage more consistent I suggest to provide this options too for the post_logout_redirect_uri.

I renamed one existing testcase and added two additional testcases for the new parameters.

References:
gh-6900

@pivotal-cla
Copy link

@MPriess Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@MPriess Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 13, 2022
@MPriess MPriess changed the title Added baseScheme, baseHost, basePort and basePath uri template support Added baseScheme, baseHost, basePort and basePath to the post_logout_redirect_uri May 13, 2022
@jzheaux
Copy link
Contributor

jzheaux commented May 13, 2022

To make the API and usage more consistent

Thanks for the suggestion, @MPriess. I'd prefer to keep the API as simple as possible. Do you have a need for this support? If not, I think we leave it until someone asks for it and close the PR.

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2022
@jzheaux jzheaux self-assigned this May 13, 2022
@jzheaux jzheaux added type: enhancement A general enhancement in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels May 13, 2022
@MPriess
Copy link
Contributor Author

MPriess commented May 13, 2022

Do you have a need for this support?

I run my application behind a reverse proxy and have some trouble with the basePath. That was the inital reason for the change.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 13, 2022
@jzheaux
Copy link
Contributor

jzheaux commented May 17, 2022

@MPriess, thanks for the extra detail. And sorry for my delayed response.

Am I understanding correctly that the reason you would like to use {basePath} is so that you can specify the host yourself? e.g. setPostLogoutRedirectUri("https://reverseproxyhost/{basePath}")?

Since you are using a reverse proxy, I'm wondering if you've already tried using ForwardedHeaderFilter. I believe this would allow you to use {baseUrl} since it maps X-Forwarded-xyz headers to their non-proxy counterparts.

@jzheaux jzheaux added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels May 17, 2022
@MPriess
Copy link
Contributor Author

MPriess commented May 25, 2022

@jzheaux

I want to exclude the context path for all URLs which are generated by Spring Security.

e.g. setPostLogoutRedirectUri("https://{baseHost}{basePort}/signout-callback-oidc")
e.g. .loginPageUri("https://{baseHost}{basePort}/oauth2/authorization/myCustomer") (TO BE IMPLEMENTED)

Why?

I tried it with NGINX and the ForwardedHeaderFilter and it works, but lead to an other problem in my case:

      proxy_set_header Host $host;
      proxy_set_header X-Forwarded-Host $host;
      proxy_set_header X-Forwarded-Server $host;
      proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
      proxy_set_header X-Forwarded-Proto $scheme;
      proxy_set_header X-Forwarded-Prefix /;

If I'm using the ForwardedHeaderFilter (X-Forwarded-Prefix) the context path is gone. This is a problem, because an other servlet provided by a 3rd party software vendor need the context path.

Bildschirmfoto 2022-05-25 um 15 11 29

If you say this feature is helpful for other customers I can continue to work on the feature. Not sure if this is an edge case which affect only me.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 25, 2022
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @MPriess! I've left some feedback inline. Also would you please format the commit message to meet the contribution guidelines?

Specifically, it would be helpful to have it be something like this:

Add placeholders to post_logout_redirect_uri

Now supports baseScheme, baseHost, basePort, and basePath in addition
to extant baseUrl and registrationId support.

Issue gh-6900

Having a short title and linking to the related issue makes future maintenance easier.

@jzheaux jzheaux changed the title Added baseScheme, baseHost, basePort and basePath to the post_logout_redirect_uri Add baseScheme, baseHost, basePort and basePath to the post_logout_redirect_uri May 25, 2022
@jzheaux jzheaux removed the status: feedback-provided Feedback has been provided label May 25, 2022
@jzheaux jzheaux added this to the 5.8.0-M1 milestone May 25, 2022
@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label May 27, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Jun 9, 2022

Hi, @MPriess. Are you able to make the requested changes and fix the build errors?

@MPriess
Copy link
Contributor Author

MPriess commented Jun 10, 2022

Hi, @MPriess. Are you able to make the requested changes and fix the build errors?

Yes, sure!

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 10, 2022
Now supports baseScheme, baseHost, basePort, and basePath in addition
to extant baseUrl.

Issue spring-projectsgh-11229
@MPriess
Copy link
Contributor Author

MPriess commented Jun 14, 2022

@jzheaux Done

@jzheaux jzheaux closed this in cb0ab49 Jun 16, 2022
jzheaux added a commit that referenced this pull request Jun 16, 2022
Now also supports baseScheme, baseHost, basePort, and basePath

Issue gh-11229
@jzheaux
Copy link
Contributor

jzheaux commented Jun 16, 2022

Thanks, @MPriess, this is now merged into main. I also added a polish commit to add the same behavior to the reactive equivalent class.

jzheaux pushed a commit that referenced this pull request Jun 16, 2022
Now supports baseScheme, baseHost, basePort, and basePath in addition
to extant baseUrl.

Closes gh-11229
jzheaux added a commit that referenced this pull request Jun 16, 2022
Now also supports baseScheme, baseHost, basePort, and basePath

Issue gh-11229
@github-actions github-actions bot added the status: backported An issue that has been backported to maintenance branches label Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: backported An issue that has been backported to maintenance branches status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants