Introduce Reactive OAuth2AuthorizedClient Manager/Provider #7116
Conversation
jgrandja
added
in: oauth2
jgrandja
force-pushed
the
gh-6811-webclient-ext-reuse-reactive
branch
2 times, most recently
from
3d0fd4d to
1e7e4bd
Compare
jgrandja
changed the title
|
@rwinch @jzheaux I just merged #6845 and rebased this PR off master. As an FYI, all the review feedback from #6845 has also been applied to this PR. I believe this is close to merging. It would be ideal if we can merge this for M4 as well. Feel free to add polish in order to get this merged while I am away. Thank you. |
jgrandja
force-pushed
the
gh-6811-webclient-ext-reuse-reactive
branch
from
1e7e4bd to
d5e4325
Compare
| OAuth2Error oauth2Error = new OAuth2Error(INVALID_TOKEN_RESPONSE_ERROR_CODE, | ||
| "An error occurred while attempting to retrieve the OAuth 2.0 Access Token Response: " + | ||
| "HTTP Status Code " + response.rawStatusCode(), null); | ||
| throw new OAuth2AuthorizationException(oauth2Error); |
There was a problem hiding this comment.
possible resource leak
from org.springframework.web.reactive.function.client.WebClient.RequestHeadersSpec#exchange javadoc
NOTE: You must always use one of the body or entity methods of the response to ensure resources are released. See ClientResponse for more details.
There was a problem hiding this comment.
@robotmrv I'm not sure I'm following? The ClientResponse.body method is used for a successful response...
response.body(oauth2AccessTokenResponse())
There was a problem hiding this comment.
The ClientResponse.body method is used for a successful response...
exactly
it is requirement to consume body in any case
there are several issues related to this problem (e.g. spring-projects/spring-framework#22005 (comment) )
see last paragraph from reference guide https://docs.spring.io/spring/docs/current/spring-framework-reference/web-reactive.html#webflux-client-exchange
So my proposal is just to use in case of error
return clientResponse.bodyToMono(Void.class)
.then(Mono.error(new OAuth2AuthorizationException(oauth2Error)));There was a problem hiding this comment.
Thanks for catching this @robotmrv. I'll update similar to your proposal.
...mework/security/oauth2/client/endpoint/WebClientReactiveRefreshTokenTokenResponseClient.java
Show resolved
Hide resolved
...ramework/security/oauth2/client/AuthorizationCodeReactiveOAuth2AuthorizedClientProvider.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private boolean hasTokenExpired(AbstractOAuth2Token token) { | ||
| return token.getExpiresAt().isBefore(Instant.now().minus(this.clockSkew)); |
There was a problem hiding this comment.
We should probably allow injecting a Clock vs using Instant.now. This comment should be applied globally throughout this PR. Additionally, we should do the same on the imperative side
There was a problem hiding this comment.
This enhancement is tracked in this ticket #7114
...th2/client/web/reactive/result/method/annotation/OAuth2AuthorizedClientArgumentResolver.java
Outdated
Show resolved
Hide resolved
| .body(tokenRequestBody(refreshTokenGrantRequest)) | ||
| .exchange() | ||
| .flatMap(response -> { | ||
| if (!response.statusCode().is2xxSuccessful()) { |
There was a problem hiding this comment.
It is possible to catch IllegalArgumentException for non-standard status code
Not sure about real probability of the problem but
there are several related issues
spring-cloud/spring-cloud-sleuth#1382
spring-projects/spring-framework#23367
spring-projects/spring-framework#23366
There was a problem hiding this comment.
Thanks @robotmrv You are right we need to ensure we don't cause a leak by calling statusCode() when the status is unknown. Instead we should do something like:
HttpStatus status = HttpStatus.resolve(response.rawStatusCode());
if (status == null || !status.is2xxSuccessful()) {There was a problem hiding this comment.
@jgrandja I see that this case was not covered in your final commit.
Just check that You did not miss it.
This PR addresses the reactive implementation of #6811.