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.

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

JwtDecoders and ReactiveJwtDecoders with customizable RestTemplate and WebClient #8690

Conversation

bedla
Copy link
Contributor

@bedla bedla commented Jun 14, 2020

Hi,
I have created PR that makes JwtDecoders and ReactiveJwtDecoders customizable with passed RestTemplate or WebClient. Changes are around all related classes that use this classes. This PR is related to my second PR #8624 that has to be changed in size.
I am open any changes you propose.
Thx,
Ivos

@bedla bedla force-pushed the oauth-jwtdecoders-rest-customizable branch from 610517d to a50017c Compare June 14, 2020 18:15
@jzheaux jzheaux added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 15, 2020
@jgrandja
Copy link
Contributor

jgrandja commented Jun 15, 2020

@bedla There are still too many changes in this PR. As mentioned in this comment, we need to align on strategy for configuring the http clients so I'm expecting that to be demonstrated with one use case only:

I would recommend that the PR demonstrate how a user would provide a customized RestTemplate and configure it via DefaultClientCredentialsTokenResponseClient.setRestOperations().

After we align on approach, then the rest can be implemented. I'm going to close #8624 and keep this one open for now.

@jgrandja jgrandja added the type: enhancement A general enhancement label Jun 15, 2020
@bedla
Copy link
Contributor Author

bedla commented Jun 20, 2020

@jgrandja From my understanding of code base there are two seperate areas where RestTemplate/WebClient are used - first is code around JwtDecoder/ReactiveJwtDecoder and second is around DefaultClientCredentialsTokenResponseClient type of classes. I have created this second PR to solve first seperately (and lower number class changes).
Are you OK with this seperation? If so, I will prepare use case for customization of JwtDecoder. Do you prefere it to be in form of unit test or sample application or something else?

@jgrandja
Copy link
Contributor

@bedla I've submitted #8732, which addresses HTTP client configuration for OAuth 2.0 Client. The team is currently discussing on what approach we should take for HTTP client configuration. After we decide on approach for OAuth 2.0 Client, we can implement a similar pattern in the JOSE stack. For now, please hold off on any updates.

Having said that, I did manage to do a quick review and I will say there are quite a few setWebClient() and setRestOperations() in the PR which are redundant. I also noticed some changes in existing API's that would break existing applications. I'm going to hold off on the detailed review until we decide on approach for #8732.

@jgrandja
Copy link
Contributor

@bedla Please see this comment for more context.

@jgrandja
Copy link
Contributor

@bedla Thank you for your time on this PR. However, we won't be able to accept these changes.

There has been a lot of investigation work on figuring out how to allow an application to provide a custom RestOperations or WebClient. Unfortunately, we ran into a few challenges. Please see gh-8882 for the details as well as JwtDecoders.

@jgrandja jgrandja closed this Jul 28, 2020
@jgrandja jgrandja added status: declined A suggestion or change that we don't feel we should currently apply and removed type: enhancement A general enhancement labels Jul 28, 2020
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: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants