WebClient support should get new access token when expired and client_credentials #6127

warrenbailey · 2018-11-22T21:59:29Z

Updating the Servlet and Server OAuth2 authorized exchange filter functions so that they get a new access token when the current access token has expired and client_credentials flow is used.

Fixes #5893

lfarmer · 2018-11-23T08:47:23Z

.../client/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunction.java

@@ -245,13 +249,26 @@ public void setAccessTokenExpiresSkew(Duration accessTokenExpiresSkew) {
 	}

 	private Mono<OAuth2AuthorizedClient> refreshIfNecessary(ClientRequest request, ExchangeFunction next, OAuth2AuthorizedClient authorizedClient) {
-		if (shouldRefresh(authorizedClient)) {
+		ClientRegistration clientRegistration = authorizedClient.getClientRegistration();
+		if (AuthorizationGrantType.CLIENT_CREDENTIALS.equals(clientRegistration.getAuthorizationGrantType()) && hasExpired(authorizedClient)) {


I wonder if a helper method would be better here? Something like

isClientCredentialsGrantType(authorizedClient)

Yep can do that

jgrandja

Thanks for the PR @warrenbailey! This looks good and we should be able to merge after you apply some of the minor updates. See my comments inline.

jgrandja · 2018-12-13T14:34:47Z

...client/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunction.java

+		return hasExpired(authorizedClient);
+	}
+
+	private boolean hasExpired(OAuth2AuthorizedClient authorizedClient) {


Let's rename the method to hasTokenExpired

jgrandja · 2018-12-13T14:39:29Z

.../client/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunction.java

+		this(authorizedClientRepository, new OAuth2AuthorizedClientResolver(clientRegistrationRepository, authorizedClientRepository));
+	}
+
+	protected ServerOAuth2AuthorizedClientExchangeFilterFunction(ServerOAuth2AuthorizedClientRepository authorizedClientRepository, OAuth2AuthorizedClientResolver authorizedClientResolver) {


Let's remove protected so it's package-private instead.

jgrandja · 2018-12-13T14:43:18Z

.../client/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunction.java

+		return AuthorizationGrantType.CLIENT_CREDENTIALS.equals(clientRegistration.getAuthorizationGrantType());
+	}
+
+	private Mono<OAuth2AuthorizedClient> getNewAuthorizedClient(ClientRegistration clientRegistration, OAuth2AuthorizedClientResolver.Request request) {


Let's rename the method to authorizeWithClientCredentials

jgrandja · 2018-12-13T14:44:11Z

.../client/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunction.java

+		return hasExpired(authorizedClient);
+	}
+
+	private boolean hasExpired(OAuth2AuthorizedClient authorizedClient) {


Let's rename the method to hasTokenExpired

jgrandja · 2018-12-13T14:57:54Z

...t/web/reactive/function/client/ServletOAuth2AuthorizedClientExchangeFilterFunctionTests.java

@@ -423,6 +424,80 @@ public void filterWhenRefreshRequiredThenRefresh() {
 		assertThat(getBody(request1)).isEmpty();
 	}

+	@Test
+	public void filterWhenClientCredentialsNotExpiresDoesNotGetNewToken() {


Can we rename the method to filterWhenClientCredentialsTokenNotExpiredThenUseCurrentToken

jgrandja · 2018-12-13T15:18:14Z

...nt/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+	}
+
+	@Test
+	public void filterWhenClientCredentialsHasNotExpiredDoNotGetNewToken() {


Can we rename the method to filterWhenClientCredentialsTokenNotExpiredThenUseCurrentToken

jgrandja · 2018-12-13T15:19:22Z

...nt/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunctionTests.java

@@ -144,6 +149,88 @@ public void filterWhenExistingAuthorizationThenSingleAuthorizationHeader() {
 		assertThat(headers.get(HttpHeaders.AUTHORIZATION)).containsOnly("Bearer " + this.accessToken.getTokenValue());
 	}

+	@Test
+	public void filterWhenClientCredentialsExpiresGetNewToken() {


Can we rename the method to filterWhenClientCredentialsTokenExpiredThenGetNewToken

jgrandja · 2018-12-13T15:21:39Z

...nt/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+
+		this.function = new ServerOAuth2AuthorizedClientExchangeFilterFunction(this.authorizedClientRepository, this.oAuth2AuthorizedClientResolver);
+
+		OAuth2AccessToken newOauth2AccessToken = new OAuth2AccessToken(OAuth2AccessToken.TokenType.BEARER,


Please rename to newAccessToken

jgrandja · 2018-12-13T15:23:56Z

...nt/web/reactive/function/client/ServerOAuth2AuthorizedClientExchangeFilterFunctionTests.java

+		when(this.oAuth2AuthorizedClientResolver.clientCredentials(any(), any(), any())).thenReturn(Mono.just(newAuthorizedClient));
+		when(this.oAuth2AuthorizedClientResolver.createDefaultedRequest(any(), any(), any())).thenReturn(Mono.just(r));
+
+		when(this.authorizedClientRepository.saveAuthorizedClient(any(), any(), any())).thenReturn(Mono.empty());


Is this needed?

Yes, otherwise I end up with a null pointer in the authorizedWithClientCredentials method line 273

jgrandja · 2018-12-13T15:41:07Z

...work/security/oauth2/client/web/reactive/function/client/OAuth2AuthorizedClientResolver.java

@@ -133,7 +133,7 @@ public void setClientCredentialsTokenResponseClient(
 			});
 }

-	private Mono<? extends OAuth2AuthorizedClient> clientCredentials(
+	public Mono<OAuth2AuthorizedClient> clientCredentials(


I don't think we should expose a public API here. What you can do instead of:

when(this.oAuth2AuthorizedClientResolver.clientCredentials(any(), any(), any())).thenReturn(Mono.just(newAuthorizedClient));

try this

Mock ReactiveOAuth2AccessTokenResponseClient<OAuth2ClientCredentialsGrantRequest> and set it via OAuth2AuthorizedClientResolver.setClientCredentialsTokenResponseClient() and you'll also have to spy(oAuth2AuthorizedClientResolver)

Makes sense?

Ok I'll have a think about how it could be reworked.

Though I made it public as I need access in the authorizeWithClientCredentials method in ServerOAuthorizedCientExchangeFilterFunction not just for tests.

on second review, instead of what I suggested previously for the test, let's just change the method signature to:

Mono<OAuth2AuthorizedClient> clientCredentials (with package-private access)

Sounds good, i'll do that now

warrenbailey · 2018-12-14T11:00:23Z

Thanks for the feedback @jgrandja I'll make those changes

jgrandja · 2018-12-17T20:53:17Z

Thanks for the updates @warrenbailey! There are a couple of comments I provided.

After those are resolved please squash the commits to 1 and format the commit message to include "Fixes gh-5893".

…ials token. Once client credentials access token has expired retrieve a new token from the OAuth2 authorization server. These tokens can't be refreshed because they do not have a refresh token associated with. This is standard behaviour for Oauth 2 client credentails

jgrandja · 2019-01-07T19:04:11Z

Thank you @warrenbailey. This is now in master! I also added a polish commit for this feedback comment.

lfarmer reviewed Nov 23, 2018

View reviewed changes

warrenbailey force-pushed the gh-5893-new-client-credentials-token branch from bb73377 to 756837d Compare December 11, 2018 09:23

rwinch assigned jgrandja Dec 12, 2018

jgrandja requested changes Dec 13, 2018

View reviewed changes

jgrandja changed the title ~~When using a Client Credentials Flow retrieve a new token if the current token expires~~ client_credentials client should get new token when expired Dec 13, 2018

jgrandja changed the title ~~client_credentials client should get new token when expired~~ WebClient support should get new access token when expired and client_credentials Dec 13, 2018

jgrandja added type: enhancement A general enhancement Reactive in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Dec 13, 2018

jgrandja added this to the 5.2.0.M1 milestone Dec 13, 2018

warrenbailey force-pushed the gh-5893-new-client-credentials-token branch from 27d1a2b to 321bb75 Compare December 22, 2018 10:26

jgrandja approved these changes Jan 7, 2019

View reviewed changes

jgrandja added a commit that referenced this pull request Jan 7, 2019

Polish gh-6127

993e11d

jgrandja closed this Jan 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WebClient support should get new access token when expired and client_credentials #6127

WebClient support should get new access token when expired and client_credentials #6127

warrenbailey commented Nov 22, 2018

lfarmer Nov 23, 2018 •

edited

Loading

warrenbailey Nov 23, 2018

warrenbailey Dec 11, 2018

jgrandja left a comment

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 13, 2018

warrenbailey Dec 14, 2018

jgrandja Dec 17, 2018 •

edited

Loading

warrenbailey Dec 22, 2018

warrenbailey commented Dec 14, 2018

jgrandja commented Dec 17, 2018

jgrandja commented Jan 7, 2019


		this.function = new ServerOAuth2AuthorizedClientExchangeFilterFunction(this.authorizedClientRepository, this.oAuth2AuthorizedClientResolver);

		OAuth2AccessToken newOauth2AccessToken = new OAuth2AccessToken(OAuth2AccessToken.TokenType.BEARER,

WebClient support should get new access token when expired and client_credentials #6127

WebClient support should get new access token when expired and client_credentials #6127

Conversation

warrenbailey commented Nov 22, 2018

lfarmer Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgrandja Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warrenbailey commented Dec 14, 2018

jgrandja commented Dec 17, 2018

jgrandja commented Jan 7, 2019

lfarmer Nov 23, 2018 •

edited

Loading

jgrandja Dec 17, 2018 •

edited

Loading