-
Notifications
You must be signed in to change notification settings - Fork 6k
OAuth2AuthorizedClientManager implementation works outside of request #7122
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
OAuth2AuthorizedClientManager implementation works outside of request #7122
Conversation
665ab67
to
b53fdbf
Compare
b53fdbf
to
e975a5f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want this to work outside of the scope of an HttpServletRequest we should not require HttpServletRequest on the classpath. I haven't looked at how we would do this yet, so some research would be necessary
The use case we're trying to realize is the ability for a client to obtain an access token outside of a For more details, please see this comment and this one:
|
e975a5f
to
37eff2b
Compare
* @see OAuth2AuthorizedClientProvider | ||
* @see OAuth2AuthorizedClientService | ||
*/ | ||
public final class OAuth2AuthorizedClientManagerService implements OAuth2AuthorizedClientManager { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you please explain your reasoning for this name? Traditionally, I'd read this as "A class that serves OAuth2AuthorizedClientManagers", kind of like how OAuth2AuthorizedClientService
serves OAuth2AuthorizedClient
s.
Maybe ServiceOAuth2AuthorizedClientManager
is better since it places the thing that it is (an OAuth2AuthorizedClientManager
) at the end and the thing it's reliant on (the OAuth2AuthorizedClientService
) at the beginning of the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talked with @jgrandja and looks like we'll rename this to AuthorizedClientServiceOAuth2AuthorizedClientManager
so that the interface it's implementing goes at the end.
@jgrandja I've approved the changes, pending the class rename. |
Merged via f7d0385 |
Fixes gh-6780
Related #6683
NOTE: This PR is rebased off #7352. Please review the most recent commit, which includes all the changes for this PR.