Skip to content

Remove HttpClientConfig in favor of ResourceRetriever Bean #4478

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
rwinch opened this issue Jul 28, 2017 · 5 comments
Closed

Remove HttpClientConfig in favor of ResourceRetriever Bean #4478

rwinch opened this issue Jul 28, 2017 · 5 comments
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose)

Comments

@rwinch
Copy link
Member

rwinch commented Jul 28, 2017

Summary

Rather than having HttpClientConfig replace it with allowing the user to provide a ResourceRetriever Bean which would override the default. Users wishing to change the timeout could then provide a DefaultResourceRetriever Bean with a different timeout.

This removes configuration specific code and provides more flexibility by allowing the ResourceRetriever that is used to be replaced with a different implementation.

Related #4477

@rwinch rwinch added this to the 5.0.0.M4 milestone Jul 28, 2017
@jgrandja jgrandja added the in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) label Jul 28, 2017
@jgrandja jgrandja mentioned this issue Aug 15, 2017
28 tasks
@jgrandja
Copy link
Contributor

ResourceRetriever is used by RemoteJWKSet in NimbusJwtDecoderJwkSupport when fetching the JWK Set Uri. However, ResourceRetriever is not used for the TokenRequest in NimbusAuthorizationCodeTokenExchanger nor is it used when fetching the UserInfoRequest in NimbusOAuth2UserService.

This is the reason I decided to create HttpClientConfig so it can be used consistently across these 3 use cases and future use cases for outbound requests. Does this make sense or do you have another suggestion?

@rwinch
Copy link
Member Author

rwinch commented Sep 7, 2017

I don't like having configuration directly in our service layer. Configuration belongs in the configuration. Perhaps we need to introduce an abstraction for retrieving resources / HTTPRequest that can unify the settings. That implementation can delegate to the original methods with the proper settings

@jgrandja
Copy link
Contributor

jgrandja commented Sep 7, 2017

Ok let me see what I can come up with.

jgrandja added a commit to jgrandja/spring-security that referenced this issue Sep 13, 2017
@jgrandja
Copy link
Contributor

@rwinch I removed HttpClientConfig and set the default read and connection timeouts to 30 secs for all 3 client calls in NimbusAuthorizationCodeTokenExchanger, NimbusOAuth2UserService and NimbusJwtDecoderJwkSupport.

Let's revisit the client abstraction for retrieving resources after we go GA.

I'll leave this issue open.

@rwinch
Copy link
Member Author

rwinch commented Nov 16, 2017

Closing since HttpClientConfig has been removed and this is now invalid

@rwinch rwinch closed this as completed Nov 16, 2017
thomasdarimont pushed a commit to thomasdarimont/spring-security that referenced this issue Apr 25, 2018
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)
Projects
None yet
Development

No branches or pull requests

2 participants