From 6e1ed9fd87bb502a1ab31ee53aed82eb875bd9eb Mon Sep 17 00:00:00 2001 From: Pavel Efros Date: Thu, 25 May 2023 15:09:56 +0200 Subject: [PATCH 1/2] Fix NPE on access token in OAuth2AuthorizationCodeAuthenticationProvider Closes gh-1233 --- ...thorizationCodeAuthenticationProvider.java | 9 ++++--- ...zationCodeAuthenticationProviderTests.java | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java index 83602d75..6535136f 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java @@ -36,6 +36,7 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; import org.springframework.security.core.session.SessionInformation; import org.springframework.security.core.session.SessionRegistry; +import org.springframework.security.oauth2.core.AbstractOAuth2Token; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClaimAccessor; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; @@ -154,12 +155,12 @@ public final class OAuth2AuthorizationCodeAuthenticationProvider implements Auth if (!authorizationCode.isActive()) { if (authorizationCode.isInvalidated()) { - OAuth2Token token = authorization.getRefreshToken() != null ? - authorization.getRefreshToken().getToken() : - authorization.getAccessToken().getToken(); + OAuth2Authorization.Token token = authorization.getRefreshToken() != null ? + authorization.getRefreshToken() : + authorization.getAccessToken(); if (token != null) { // Invalidate the access (and refresh) token as the client is attempting to use the authorization code more than once - authorization = OAuth2AuthenticationProviderUtils.invalidate(authorization, token); + authorization = OAuth2AuthenticationProviderUtils.invalidate(authorization, token.getToken()); this.authorizationService.save(authorization); if (this.logger.isWarnEnabled()) { this.logger.warn(LogMessage.format("Invalidated authorization token(s) previously issued to registered client '%s'", registeredClient.getId())); diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java index 2378a559..f5075772 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java @@ -283,6 +283,33 @@ public class OAuth2AuthorizationCodeAuthenticationProviderTests { assertThat(updatedAuthorization.getRefreshToken().isInvalidated()).isTrue(); } + // gh PR 1233 + @Test + public void authenticateWhenInvalidatedCodeAndNullRefreshAndAccessTokensThenThrowOAuth2AuthenticationException() { + RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build(); + OAuth2AuthorizationCode authorizationCode = new OAuth2AuthorizationCode( + AUTHORIZATION_CODE, Instant.now(), Instant.now().plusSeconds(120)); + OAuth2Authorization authorization = TestOAuth2Authorizations.authorization(registeredClient, authorizationCode) + .token(authorizationCode, (metadata) -> metadata.put(OAuth2Authorization.Token.INVALIDATED_METADATA_NAME, true)) + .build(); + + when(this.authorizationService.findByToken(eq(AUTHORIZATION_CODE), eq(AUTHORIZATION_CODE_TOKEN_TYPE))) + .thenReturn(authorization); + + OAuth2ClientAuthenticationToken clientPrincipal = new OAuth2ClientAuthenticationToken( + registeredClient, ClientAuthenticationMethod.CLIENT_SECRET_BASIC, registeredClient.getClientSecret()); + OAuth2AuthorizationRequest authorizationRequest = authorization.getAttribute( + OAuth2AuthorizationRequest.class.getName()); + OAuth2AuthorizationCodeAuthenticationToken authentication = + new OAuth2AuthorizationCodeAuthenticationToken(AUTHORIZATION_CODE, clientPrincipal, authorizationRequest.getRedirectUri(), null); + + assertThatThrownBy(() -> this.authenticationProvider.authenticate(authentication)) + .isInstanceOf(OAuth2AuthenticationException.class) + .extracting(ex -> ((OAuth2AuthenticationException) ex).getError()) + .extracting("errorCode") + .isEqualTo(OAuth2ErrorCodes.INVALID_GRANT); + } + // gh-290 @Test public void authenticateWhenExpiredCodeThenThrowOAuth2AuthenticationException() { From bdc0b4de5fa32a08ea00affa1ecff1d74ad93e7e Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Thu, 15 Jun 2023 15:19:36 -0400 Subject: [PATCH 2/2] Polish gh-1233 --- .../OAuth2AuthorizationCodeAuthenticationProvider.java | 3 +-- ...Auth2AuthorizationCodeAuthenticationProviderTests.java | 8 +++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java index 6535136f..ab84a3bd 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProvider.java @@ -36,7 +36,6 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.AuthenticationException; import org.springframework.security.core.session.SessionInformation; import org.springframework.security.core.session.SessionRegistry; -import org.springframework.security.oauth2.core.AbstractOAuth2Token; import org.springframework.security.oauth2.core.AuthorizationGrantType; import org.springframework.security.oauth2.core.ClaimAccessor; import org.springframework.security.oauth2.core.ClientAuthenticationMethod; @@ -155,7 +154,7 @@ public final class OAuth2AuthorizationCodeAuthenticationProvider implements Auth if (!authorizationCode.isActive()) { if (authorizationCode.isInvalidated()) { - OAuth2Authorization.Token token = authorization.getRefreshToken() != null ? + OAuth2Authorization.Token token = authorization.getRefreshToken() != null ? authorization.getRefreshToken() : authorization.getAccessToken(); if (token != null) { diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java index f5075772..802282ad 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/authentication/OAuth2AuthorizationCodeAuthenticationProviderTests.java @@ -85,6 +85,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -283,16 +284,15 @@ public class OAuth2AuthorizationCodeAuthenticationProviderTests { assertThat(updatedAuthorization.getRefreshToken().isInvalidated()).isTrue(); } - // gh PR 1233 + // gh-1233 @Test - public void authenticateWhenInvalidatedCodeAndNullRefreshAndAccessTokensThenThrowOAuth2AuthenticationException() { + public void authenticateWhenInvalidatedCodeAndAccessTokenNullThenThrowOAuth2AuthenticationException() { RegisteredClient registeredClient = TestRegisteredClients.registeredClient().build(); OAuth2AuthorizationCode authorizationCode = new OAuth2AuthorizationCode( AUTHORIZATION_CODE, Instant.now(), Instant.now().plusSeconds(120)); OAuth2Authorization authorization = TestOAuth2Authorizations.authorization(registeredClient, authorizationCode) .token(authorizationCode, (metadata) -> metadata.put(OAuth2Authorization.Token.INVALIDATED_METADATA_NAME, true)) .build(); - when(this.authorizationService.findByToken(eq(AUTHORIZATION_CODE), eq(AUTHORIZATION_CODE_TOKEN_TYPE))) .thenReturn(authorization); @@ -308,6 +308,8 @@ public class OAuth2AuthorizationCodeAuthenticationProviderTests { .extracting(ex -> ((OAuth2AuthenticationException) ex).getError()) .extracting("errorCode") .isEqualTo(OAuth2ErrorCodes.INVALID_GRANT); + + verify(this.authorizationService, never()).save(any()); } // gh-290