Skip to content
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.

Sign up for GitHub

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

Jump to bottom

enable customization of headers in AbstractWebClientReactiveOAuth2AccessTokenResponseClient #10131

Closed
wants to merge 1 commit into from
Closed

enable customization of headers in AbstractWebClientReactiveOAuth2AccessTokenResponseClient #10131

wants to merge 1 commit into from

Conversation

vboulaye
Copy link
Contributor

Hi,

This is an attempt at fixing #10130

I tried adding an headers consumer into the AbstractWebClientReactiveOAuth2AccessTokenResponseClient to customize the headers after they are set by populateTokenRequestHeaders(grantRequest, headers)
Is this the proper way to do it ?

@pivotal-cla
Copy link

@vboulaye Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 20, 2021
@sjohnr
Copy link
Member

sjohnr commented Jul 20, 2021

Hi @vboulaye! See this comment for a starting point. Let me know if you have any more questions.

@sjohnr sjohnr self-assigned this Jul 20, 2021
@sjohnr sjohnr added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 20, 2021
@pivotal-cla
Copy link

@vboulaye Thank you for signing the Contributor License Agreement!

@vboulaye
Copy link
Contributor Author

Hi, I added the header converter + some unit tests for each of the AbstractWebClientReactiveOAuth2AccessTokenResponseClient variants.
let me know if something is missing.

sjohnr
sjohnr requested changes Jul 27, 2021
View reviewed changes
Copy link
Member

@sjohnr sjohnr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @vboulaye, my apologies for the delay. Thanks for the updates. I have a few more suggestions below.

Update

Also, please review contributing guidelines, specifically make sure to run checks and address style issues, squash your commits, and ensure your commit message directly addresses the improvement from the original issue. Finally, add "Closes gh-10130" on a separate line in the message.

...urity/oauth2/client/endpoint/WebClientReactiveAuthorizationCodeTokenResponseClientTests.java Outdated Show resolved Hide resolved
...urity/oauth2/client/endpoint/WebClientReactiveAuthorizationCodeTokenResponseClientTests.java Outdated Show resolved Hide resolved
...ecurity/oauth2/client/endpoint/AbstractWebClientReactiveOAuth2AccessTokenResponseClient.java Outdated Show resolved Hide resolved
...urity/oauth2/client/endpoint/WebClientReactiveAuthorizationCodeTokenResponseClientTests.java Show resolved Hide resolved
...urity/oauth2/client/endpoint/WebClientReactiveAuthorizationCodeTokenResponseClientTests.java Outdated Show resolved Hide resolved
@vboulaye vboulaye force-pushed the gh-10130-headers-customization branch from e1ad878 to 9703229 Compare July 28, 2021 21:34
@vboulaye
enable customization of headers in AbstractWebClientReactiveOAuth2Acc…
47cb4eb 
…essTokenResponseClient

adds the possibility to customize the headers of the access token request, similarly to what is done in the AbstractOAuth2AuthorizationGrantRequestEntityConverter

Closes spring-projectsgh-10130
@vboulaye vboulaye force-pushed the gh-10130-headers-customization branch from 9703229 to 47cb4eb Compare July 28, 2021 21:41
@vboulaye
Copy link
Contributor Author

vboulaye commented Aug 5, 2021

Hi,
I think I handled all the review comments.
Except if you want me to change the visibility of the getHeadersConverter() this should be ready to be merged.

sjohnr added a commit that referenced this pull request Aug 9, 2021
@sjohnr
Polish gh-10131
acca3db
@sjohnr
Copy link
Member

sjohnr commented Aug 9, 2021

Thanks @vboulaye! This is now in main.

@sjohnr sjohnr closed this Aug 9, 2021
@sjohnr sjohnr added this to the 5.6.0-M2 milestone Aug 9, 2021
akohli96 pushed a commit to akohli96/spring-security that referenced this pull request Aug 25, 2021
@sjohnr
Polish spring-projectsgh-10131
3a3a63a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjohnr sjohnr sjohnr requested changes

Assignees

@sjohnr sjohnr

Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Milestone
5.6.0-M2
Development

Successfully merging this pull request may close these issues.
4 participants
@vboulaye @pivotal-cla @sjohnr @spring-projects-issues