Fix access token expiry check with clock skew

Fixes spring-projectsgh-7511
rweisleder · Oct 23, 2019 · 1c53a78 · 1c53a78
1 parent 62c7de0
commit 1c53a78
Show file tree

Hide file tree

Showing 12 changed files with 161 additions and 31 deletions.
diff --git a/...ringframework/security/oauth2/client/ClientCredentialsOAuth2AuthorizedClientProvider.java b/...ringframework/security/oauth2/client/ClientCredentialsOAuth2AuthorizedClientProvider.java
@@ -86,7 +86,7 @@ public OAuth2AuthorizedClient authorize(OAuth2AuthorizationContext context) {
 	}
 
 	private boolean hasTokenExpired(AbstractOAuth2Token token) {
-		return token.getExpiresAt().isBefore(Instant.now(this.clock).minus(this.clockSkew));
+		return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
 	}
 
 	/**

diff --git a/...ework/security/oauth2/client/ClientCredentialsReactiveOAuth2AuthorizedClientProvider.java b/...ework/security/oauth2/client/ClientCredentialsReactiveOAuth2AuthorizedClientProvider.java
@@ -82,7 +82,7 @@ public Mono<OAuth2AuthorizedClient> authorize(OAuth2AuthorizationContext context
 	}
 
 	private boolean hasTokenExpired(AbstractOAuth2Token token) {
-		return token.getExpiresAt().isBefore(Instant.now(this.clock).minus(this.clockSkew));
+		return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
 	}
 
 	/**

diff --git a/...va/org/springframework/security/oauth2/client/PasswordOAuth2AuthorizedClientProvider.java b/...va/org/springframework/security/oauth2/client/PasswordOAuth2AuthorizedClientProvider.java
@@ -104,7 +104,7 @@ public OAuth2AuthorizedClient authorize(OAuth2AuthorizationContext context) {
 	}
 
 	private boolean hasTokenExpired(AbstractOAuth2Token token) {
-		return token.getExpiresAt().isBefore(Instant.now(this.clock).minus(this.clockSkew));
+		return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
 	}
 
 	/**

diff --git a/...pringframework/security/oauth2/client/PasswordReactiveOAuth2AuthorizedClientProvider.java b/...pringframework/security/oauth2/client/PasswordReactiveOAuth2AuthorizedClientProvider.java
@@ -102,7 +102,7 @@ public Mono<OAuth2AuthorizedClient> authorize(OAuth2AuthorizationContext context
 	}
 
 	private boolean hasTokenExpired(AbstractOAuth2Token token) {
-		return token.getExpiresAt().isBefore(Instant.now(this.clock).minus(this.clockSkew));
+		return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
 	}
 
 	/**

diff --git a/...rg/springframework/security/oauth2/client/RefreshTokenOAuth2AuthorizedClientProvider.java b/...rg/springframework/security/oauth2/client/RefreshTokenOAuth2AuthorizedClientProvider.java
@@ -94,7 +94,7 @@ public OAuth2AuthorizedClient authorize(OAuth2AuthorizationContext context) {
 	}
 
 	private boolean hasTokenExpired(AbstractOAuth2Token token) {
-		return token.getExpiresAt().isBefore(Instant.now(this.clock).minus(this.clockSkew));
+		return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
 	}
 
 	/**

diff --git a/...gframework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizedClientProvider.java b/...gframework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizedClientProvider.java
@@ -93,7 +93,7 @@ public Mono<OAuth2AuthorizedClient> authorize(OAuth2AuthorizationContext context
 	}
 
 	private boolean hasTokenExpired(AbstractOAuth2Token token) {
-		return token.getExpiresAt().isBefore(Instant.now(this.clock).minus(this.clockSkew));
+		return this.clock.instant().isAfter(token.getExpiresAt().minus(this.clockSkew));
 	}
 
 	/**

diff --git a/...ramework/security/oauth2/client/ClientCredentialsOAuth2AuthorizedClientProviderTests.java b/...ramework/security/oauth2/client/ClientCredentialsOAuth2AuthorizedClientProviderTests.java
@@ -154,21 +154,32 @@ public void authorizeWhenClientCredentialsAndTokenNotExpiredThenNotReauthorize()
 		assertThat(this.authorizedClientProvider.authorize(authorizationContext)).isNull();
 	}
 
+	// gh-7511
 	@Test
-	public void authorizeWhenClientCredentialsAndTokenNotExpiredByClockSkewThenNotReauthorize() {
-		ClientCredentialsOAuth2AuthorizedClientProvider authorizedClientProvider =
-				new ClientCredentialsOAuth2AuthorizedClientProvider();
-		authorizedClientProvider.setClockSkew(Duration.ofHours(24));
-		Instant issuedAt = Instant.now().minus(Duration.ofDays(1));
-		OAuth2AccessToken expiredToken = new OAuth2AccessToken(OAuth2AccessToken.TokenType.BEARER, "token",
-				issuedAt, issuedAt.plus(Duration.ofHours(1)));
+	public void authorizeWhenClientCredentialsAndTokenNotExpiredButClockSkewForcesExpiryThenReauthorize() {
+		Instant now = Instant.now();
+		Instant issuedAt = now.minus(Duration.ofMinutes(60));
+		Instant expiresAt = now.minus(Duration.ofMinutes(1));
+		OAuth2AccessToken expiresInOneMinAccessToken = new OAuth2AccessToken(
+				OAuth2AccessToken.TokenType.BEARER, "access-token-1234", issuedAt, expiresAt);
 		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(
-				this.clientRegistration, this.principal.getName(), expiredToken);
+				this.clientRegistration, this.principal.getName(), expiresInOneMinAccessToken);
+
+		// Shorten the lifespan of the access token by 90 seconds, which will ultimately force it to expire on the client
+		this.authorizedClientProvider.setClockSkew(Duration.ofSeconds(90));
+
+		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse().build();
+		when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(accessTokenResponse);
 
 		OAuth2AuthorizationContext authorizationContext =
 				OAuth2AuthorizationContext.withAuthorizedClient(authorizedClient)
 						.principal(this.principal)
 						.build();
-		assertThat(authorizedClientProvider.authorize(authorizationContext)).isNull();
+
+		OAuth2AuthorizedClient reauthorizedClient = this.authorizedClientProvider.authorize(authorizationContext);
+
+		assertThat(reauthorizedClient.getClientRegistration()).isSameAs(this.clientRegistration);
+		assertThat(reauthorizedClient.getPrincipalName()).isEqualTo(this.principal.getName());
+		assertThat(reauthorizedClient.getAccessToken()).isEqualTo(accessTokenResponse.getAccessToken());
 	}
 }
diff --git a/.../security/oauth2/client/ClientCredentialsReactiveOAuth2AuthorizedClientProviderTests.java b/.../security/oauth2/client/ClientCredentialsReactiveOAuth2AuthorizedClientProviderTests.java
@@ -154,4 +154,33 @@ public void authorizeWhenClientCredentialsAndTokenNotExpiredThenNotReauthorize()
 						.build();
 		assertThat(this.authorizedClientProvider.authorize(authorizationContext).block()).isNull();
 	}
+
+	// gh-7511
+	@Test
+	public void authorizeWhenClientCredentialsAndTokenNotExpiredButClockSkewForcesExpiryThenReauthorize() {
+		Instant now = Instant.now();
+		Instant issuedAt = now.minus(Duration.ofMinutes(60));
+		Instant expiresAt = now.minus(Duration.ofMinutes(1));
+		OAuth2AccessToken expiresInOneMinAccessToken = new OAuth2AccessToken(
+				OAuth2AccessToken.TokenType.BEARER, "access-token-1234", issuedAt, expiresAt);
+		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(
+				this.clientRegistration, this.principal.getName(), expiresInOneMinAccessToken);
+
+		// Shorten the lifespan of the access token by 90 seconds, which will ultimately force it to expire on the client
+		this.authorizedClientProvider.setClockSkew(Duration.ofSeconds(90));
+
+		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse().build();
+		when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse));
+
+		OAuth2AuthorizationContext authorizationContext =
+				OAuth2AuthorizationContext.withAuthorizedClient(authorizedClient)
+						.principal(this.principal)
+						.build();
+
+		OAuth2AuthorizedClient reauthorizedClient = this.authorizedClientProvider.authorize(authorizationContext).block();
+
+		assertThat(reauthorizedClient.getClientRegistration()).isSameAs(this.clientRegistration);
+		assertThat(reauthorizedClient.getPrincipalName()).isEqualTo(this.principal.getName());
+		assertThat(reauthorizedClient.getAccessToken()).isEqualTo(accessTokenResponse.getAccessToken());
+	}
 }
diff --git a/...g/springframework/security/oauth2/client/PasswordOAuth2AuthorizedClientProviderTests.java b/...g/springframework/security/oauth2/client/PasswordOAuth2AuthorizedClientProviderTests.java
@@ -188,24 +188,34 @@ public void authorizeWhenPasswordAndAuthorizedWithRefreshTokenAndTokenExpiredThe
 		assertThat(this.authorizedClientProvider.authorize(authorizationContext)).isNull();
 	}
 
+	// gh-7511
 	@Test
-	public void authorizeWhenPasswordAndAuthorizedWithoutRefreshTokenAndTokenNotExpiredByClockSkewThenNotReauthorize() {
-		PasswordOAuth2AuthorizedClientProvider authorizedClientProvider =
-				new PasswordOAuth2AuthorizedClientProvider();
-		authorizedClientProvider.setClockSkew(Duration.ofHours(24));
-		Instant issuedAt = Instant.now().minus(Duration.ofDays(1));
-		Instant expiresAt = issuedAt.plus(Duration.ofMinutes(60));
-		OAuth2AccessToken accessToken = new OAuth2AccessToken(
-				OAuth2AccessToken.TokenType.BEARER, "access-token-expired", issuedAt, expiresAt);
+	public void authorizeWhenPasswordAndAuthorizedAndTokenNotExpiredButClockSkewForcesExpiryThenReauthorize() {
+		Instant now = Instant.now();
+		Instant issuedAt = now.minus(Duration.ofMinutes(60));
+		Instant expiresAt = now.minus(Duration.ofMinutes(1));
+		OAuth2AccessToken expiresInOneMinAccessToken = new OAuth2AccessToken(
+				OAuth2AccessToken.TokenType.BEARER, "access-token-1234", issuedAt, expiresAt);
 		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(
-				this.clientRegistration, this.principal.getName(), accessToken);	// without refresh token
+				this.clientRegistration, this.principal.getName(), expiresInOneMinAccessToken);		// without refresh token
+
+		// Shorten the lifespan of the access token by 90 seconds, which will ultimately force it to expire on the client
+		this.authorizedClientProvider.setClockSkew(Duration.ofSeconds(90));
+
+		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse().build();
+		when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(accessTokenResponse);
 
 		OAuth2AuthorizationContext authorizationContext =
 				OAuth2AuthorizationContext.withAuthorizedClient(authorizedClient)
 						.attribute(OAuth2AuthorizationContext.USERNAME_ATTRIBUTE_NAME, "username")
 						.attribute(OAuth2AuthorizationContext.PASSWORD_ATTRIBUTE_NAME, "password")
 						.principal(this.principal)
 						.build();
-		assertThat(authorizedClientProvider.authorize(authorizationContext)).isNull();
+
+		OAuth2AuthorizedClient reauthorizedClient = this.authorizedClientProvider.authorize(authorizationContext);
+
+		assertThat(reauthorizedClient.getClientRegistration()).isSameAs(this.clientRegistration);
+		assertThat(reauthorizedClient.getPrincipalName()).isEqualTo(this.principal.getName());
+		assertThat(reauthorizedClient.getAccessToken()).isEqualTo(accessTokenResponse.getAccessToken());
 	}
 }
diff --git a/...framework/security/oauth2/client/PasswordReactiveOAuth2AuthorizedClientProviderTests.java b/...framework/security/oauth2/client/PasswordReactiveOAuth2AuthorizedClientProviderTests.java
@@ -188,4 +188,35 @@ public void authorizeWhenPasswordAndAuthorizedWithRefreshTokenAndTokenExpiredThe
 						.build();
 		assertThat(this.authorizedClientProvider.authorize(authorizationContext).block()).isNull();
 	}
+
+	// gh-7511
+	@Test
+	public void authorizeWhenPasswordAndAuthorizedAndTokenNotExpiredButClockSkewForcesExpiryThenReauthorize() {
+		Instant now = Instant.now();
+		Instant issuedAt = now.minus(Duration.ofMinutes(60));
+		Instant expiresAt = now.minus(Duration.ofMinutes(1));
+		OAuth2AccessToken expiresInOneMinAccessToken = new OAuth2AccessToken(
+				OAuth2AccessToken.TokenType.BEARER, "access-token-1234", issuedAt, expiresAt);
+		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(
+				this.clientRegistration, this.principal.getName(), expiresInOneMinAccessToken);		// without refresh token
+
+		// Shorten the lifespan of the access token by 90 seconds, which will ultimately force it to expire on the client
+		this.authorizedClientProvider.setClockSkew(Duration.ofSeconds(90));
+
+		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse().build();
+		when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse));
+
+		OAuth2AuthorizationContext authorizationContext =
+				OAuth2AuthorizationContext.withAuthorizedClient(authorizedClient)
+						.attribute(OAuth2AuthorizationContext.USERNAME_ATTRIBUTE_NAME, "username")
+						.attribute(OAuth2AuthorizationContext.PASSWORD_ATTRIBUTE_NAME, "password")
+						.principal(this.principal)
+						.build();
+
+		OAuth2AuthorizedClient reauthorizedClient = this.authorizedClientProvider.authorize(authorizationContext).block();
+
+		assertThat(reauthorizedClient.getClientRegistration()).isSameAs(this.clientRegistration);
+		assertThat(reauthorizedClient.getPrincipalName()).isEqualTo(this.principal.getName());
+		assertThat(reauthorizedClient.getAccessToken()).isEqualTo(accessTokenResponse.getAccessToken());
+	}
 }
diff --git a/...ringframework/security/oauth2/client/RefreshTokenOAuth2AuthorizedClientProviderTests.java b/...ringframework/security/oauth2/client/RefreshTokenOAuth2AuthorizedClientProviderTests.java
@@ -134,6 +134,38 @@ public void authorizeWhenAuthorizedAndAccessTokenNotExpiredThenNotReauthorize()
 		assertThat(this.authorizedClientProvider.authorize(authorizationContext)).isNull();
 	}
 
+	// gh-7511
+	@Test
+	public void authorizeWhenAuthorizedAndAccessTokenNotExpiredButClockSkewForcesExpiryThenReauthorize() {
+		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse()
+				.refreshToken("new-refresh-token")
+				.build();
+		when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(accessTokenResponse);
+
+		Instant now = Instant.now();
+		Instant issuedAt = now.minus(Duration.ofMinutes(60));
+		Instant expiresAt = now.minus(Duration.ofMinutes(1));
+		OAuth2AccessToken expiresInOneMinAccessToken = new OAuth2AccessToken(
+				OAuth2AccessToken.TokenType.BEARER, "access-token-1234", issuedAt, expiresAt);
+		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(this.clientRegistration, this.principal.getName(),
+				expiresInOneMinAccessToken, this.authorizedClient.getRefreshToken());
+
+		// Shorten the lifespan of the access token by 90 seconds, which will ultimately force it to expire on the client
+		this.authorizedClientProvider.setClockSkew(Duration.ofSeconds(90));
+
+		OAuth2AuthorizationContext authorizationContext =
+				OAuth2AuthorizationContext.withAuthorizedClient(authorizedClient)
+						.principal(this.principal)
+						.build();
+
+		OAuth2AuthorizedClient reauthorizedClient = this.authorizedClientProvider.authorize(authorizationContext);
+
+		assertThat(reauthorizedClient.getClientRegistration()).isSameAs(this.clientRegistration);
+		assertThat(reauthorizedClient.getPrincipalName()).isEqualTo(this.principal.getName());
+		assertThat(reauthorizedClient.getAccessToken()).isEqualTo(accessTokenResponse.getAccessToken());
+		assertThat(reauthorizedClient.getRefreshToken()).isEqualTo(accessTokenResponse.getRefreshToken());
+	}
+
 	@Test
 	public void authorizeWhenAuthorizedAndAccessTokenExpiredThenReauthorize() {
 		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse()

diff --git a/...ework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizedClientProviderTests.java b/...ework/security/oauth2/client/RefreshTokenReactiveOAuth2AuthorizedClientProviderTests.java
@@ -135,19 +135,36 @@ public void authorizeWhenAuthorizedAndAccessTokenNotExpiredThenNotReauthorize()
 		assertThat(this.authorizedClientProvider.authorize(authorizationContext).block()).isNull();
 	}
 
+	// gh-7511
 	@Test
-	public void authorizeWhenAuthorizedAndAccessTokenNotExpiredByClockSkewThenNotReauthorize() {
-		RefreshTokenReactiveOAuth2AuthorizedClientProvider authorizedClientProvider
-				= new RefreshTokenReactiveOAuth2AuthorizedClientProvider();
-		authorizedClientProvider.setClockSkew(Duration.ofHours(24));
+	public void authorizeWhenAuthorizedAndAccessTokenNotExpiredButClockSkewForcesExpiryThenReauthorize() {
+		OAuth2AccessTokenResponse accessTokenResponse = TestOAuth2AccessTokenResponses.accessTokenResponse()
+				.refreshToken("new-refresh-token")
+				.build();
+		when(this.accessTokenResponseClient.getTokenResponse(any())).thenReturn(Mono.just(accessTokenResponse));
+
+		Instant now = Instant.now();
+		Instant issuedAt = now.minus(Duration.ofMinutes(60));
+		Instant expiresAt = now.minus(Duration.ofMinutes(1));
+		OAuth2AccessToken expiresInOneMinAccessToken = new OAuth2AccessToken(
+				OAuth2AccessToken.TokenType.BEARER, "access-token-1234", issuedAt, expiresAt);
 		OAuth2AuthorizedClient authorizedClient = new OAuth2AuthorizedClient(this.clientRegistration, this.principal.getName(),
-				this.authorizedClient.getAccessToken(), this.authorizedClient.getRefreshToken());
+				expiresInOneMinAccessToken, this.authorizedClient.getRefreshToken());
+
+		// Shorten the lifespan of the access token by 90 seconds, which will ultimately force it to expire on the client
+		this.authorizedClientProvider.setClockSkew(Duration.ofSeconds(90));
 
 		OAuth2AuthorizationContext authorizationContext =
 				OAuth2AuthorizationContext.withAuthorizedClient(authorizedClient)
 						.principal(this.principal)
 						.build();
-		assertThat(authorizedClientProvider.authorize(authorizationContext).block()).isNull();
+
+		OAuth2AuthorizedClient reauthorizedClient = this.authorizedClientProvider.authorize(authorizationContext).block();
+
+		assertThat(reauthorizedClient.getClientRegistration()).isSameAs(this.clientRegistration);
+		assertThat(reauthorizedClient.getPrincipalName()).isEqualTo(this.principal.getName());
+		assertThat(reauthorizedClient.getAccessToken()).isEqualTo(accessTokenResponse.getAccessToken());
+		assertThat(reauthorizedClient.getRefreshToken()).isEqualTo(accessTokenResponse.getRefreshToken());
 	}
 
 	@Test