From 8c08d7b4d7dbe87582bb61dd368cae3a3ada1fc5 Mon Sep 17 00:00:00 2001 From: Joe Grandja Date: Mon, 16 Oct 2023 12:35:49 -0400 Subject: [PATCH] Polish gh-1384 --- .../ROOT/pages/protocol-endpoints.adoc | 8 +-- ...uth2DeviceAuthorizationEndpointFilter.java | 2 +- .../web/OAuth2TokenEndpointFilter.java | 2 +- ...uth2ErrorAuthenticationFailureHandler.java | 34 +++++---- ...rrorAuthenticationFailureHandlerTests.java | 69 +++++++++---------- 5 files changed, 59 insertions(+), 56 deletions(-) diff --git a/docs/modules/ROOT/pages/protocol-endpoints.adoc b/docs/modules/ROOT/pages/protocol-endpoints.adoc index ecb4b3f91..6f244c110 100644 --- a/docs/modules/ROOT/pages/protocol-endpoints.adoc +++ b/docs/modules/ROOT/pages/protocol-endpoints.adoc @@ -167,7 +167,7 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h * `*AuthenticationConverter*` -- An `OAuth2DeviceAuthorizationRequestAuthenticationConverter`. * `*AuthenticationManager*` -- An `AuthenticationManager` composed of `OAuth2DeviceAuthorizationRequestAuthenticationProvider`. * `*AuthenticationSuccessHandler*` -- An internal implementation that handles an "`authenticated`" `OAuth2DeviceAuthorizationRequestAuthenticationToken` and returns the `OAuth2DeviceAuthorizationResponse`. -* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler` instance that handles the `OAuth2Error` associated with the `OAuth2AuthenticationException` and returns the `OAuth2Error` response. +* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler`. [[oauth2-device-verification-endpoint]] == OAuth2 Device Verification Endpoint @@ -264,7 +264,7 @@ The supported https://datatracker.ietf.org/doc/html/rfc6749#section-1.3[authoriz * `*AuthenticationConverter*` -- A `DelegatingAuthenticationConverter` composed of `OAuth2AuthorizationCodeAuthenticationConverter`, `OAuth2RefreshTokenAuthenticationConverter`, `OAuth2ClientCredentialsAuthenticationConverter`, and `OAuth2DeviceCodeAuthenticationConverter`. * `*AuthenticationManager*` -- An `AuthenticationManager` composed of `OAuth2AuthorizationCodeAuthenticationProvider`, `OAuth2RefreshTokenAuthenticationProvider`, `OAuth2ClientCredentialsAuthenticationProvider`, and `OAuth2DeviceCodeAuthenticationProvider`. * `*AuthenticationSuccessHandler*` -- An internal implementation that handles an `OAuth2AccessTokenAuthenticationToken` and returns the `OAuth2AccessTokenResponse`. -* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler` instance that handles the `OAuth2Error` associated with the `OAuth2AuthenticationException` and returns the `OAuth2Error` response. +* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler`. [[oauth2-token-introspection-endpoint]] == OAuth2 Token Introspection Endpoint @@ -311,7 +311,7 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h * `*AuthenticationConverter*` -- An `OAuth2TokenIntrospectionAuthenticationConverter`. * `*AuthenticationManager*` -- An `AuthenticationManager` composed of `OAuth2TokenIntrospectionAuthenticationProvider`. * `*AuthenticationSuccessHandler*` -- An internal implementation that handles an "`authenticated`" `OAuth2TokenIntrospectionAuthenticationToken` and returns the `OAuth2TokenIntrospection` response. -* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler` instance that handles the `OAuth2Error` associated with the `OAuth2AuthenticationException` and returns the `OAuth2Error` response. +* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler`. [[oauth2-token-revocation-endpoint]] == OAuth2 Token Revocation Endpoint @@ -358,7 +358,7 @@ public SecurityFilterChain authorizationServerSecurityFilterChain(HttpSecurity h * `*AuthenticationConverter*` -- An `OAuth2TokenRevocationAuthenticationConverter`. * `*AuthenticationManager*` -- An `AuthenticationManager` composed of `OAuth2TokenRevocationAuthenticationProvider`. * `*AuthenticationSuccessHandler*` -- An internal implementation that handles an "`authenticated`" `OAuth2TokenRevocationAuthenticationToken` and returns the OAuth2 revocation response. -* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler` instance that handles the `OAuth2Error` associated with the `OAuth2AuthenticationException` and returns the `OAuth2Error` response. +* `*AuthenticationFailureHandler*` -- An `OAuth2ErrorAuthenticationFailureHandler`. [[oauth2-authorization-server-metadata-endpoint]] == OAuth2 Authorization Server Metadata Endpoint diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2DeviceAuthorizationEndpointFilter.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2DeviceAuthorizationEndpointFilter.java index 8595f0714..b6b6db2ff 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2DeviceAuthorizationEndpointFilter.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2DeviceAuthorizationEndpointFilter.java @@ -41,8 +41,8 @@ import org.springframework.security.oauth2.server.authorization.authentication.OAuth2DeviceAuthorizationRequestAuthenticationProvider; import org.springframework.security.oauth2.server.authorization.authentication.OAuth2DeviceAuthorizationRequestAuthenticationToken; import org.springframework.security.oauth2.server.authorization.context.AuthorizationServerContextHolder; -import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2ErrorAuthenticationFailureHandler; import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2DeviceAuthorizationRequestAuthenticationConverter; +import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2ErrorAuthenticationFailureHandler; import org.springframework.security.web.authentication.AuthenticationConverter; import org.springframework.security.web.authentication.AuthenticationFailureHandler; import org.springframework.security.web.authentication.AuthenticationSuccessHandler; diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilter.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilter.java index 4a1980c43..655162c32 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilter.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilter.java @@ -48,11 +48,11 @@ import org.springframework.security.oauth2.server.authorization.authentication.OAuth2ClientCredentialsAuthenticationProvider; import org.springframework.security.oauth2.server.authorization.authentication.OAuth2DeviceCodeAuthenticationProvider; import org.springframework.security.oauth2.server.authorization.authentication.OAuth2RefreshTokenAuthenticationProvider; -import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2ErrorAuthenticationFailureHandler; import org.springframework.security.oauth2.server.authorization.web.authentication.DelegatingAuthenticationConverter; import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2AuthorizationCodeAuthenticationConverter; import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2ClientCredentialsAuthenticationConverter; import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2DeviceCodeAuthenticationConverter; +import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2ErrorAuthenticationFailureHandler; import org.springframework.security.oauth2.server.authorization.web.authentication.OAuth2RefreshTokenAuthenticationConverter; import org.springframework.security.web.authentication.AuthenticationConverter; import org.springframework.security.web.authentication.AuthenticationFailureHandler; diff --git a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandler.java b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandler.java index e0ba23f1a..a5d350a65 100644 --- a/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandler.java +++ b/oauth2-authorization-server/src/main/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandler.java @@ -20,9 +20,10 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; -import org.springframework.core.log.LogMessage; + import org.springframework.http.HttpStatus; import org.springframework.http.converter.HttpMessageConverter; import org.springframework.http.server.ServletServerHttpResponse; @@ -31,42 +32,47 @@ import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.core.http.converter.OAuth2ErrorHttpMessageConverter; import org.springframework.security.web.authentication.AuthenticationFailureHandler; +import org.springframework.util.Assert; /** - * A default implementation of an {@link AuthenticationFailureHandler} used for handling an {@link OAuth2AuthenticationException} - * and returning the {@link OAuth2Error Error Response}. + * An implementation of an {@link AuthenticationFailureHandler} used for handling an {@link OAuth2AuthenticationException} + * and returning the {@link OAuth2Error OAuth 2.0 Error Response}. * * @author Dmitriy Dubson * @see AuthenticationFailureHandler - * @since 1.2.0 + * @see OAuth2ErrorHttpMessageConverter + * @since 1.2 */ public final class OAuth2ErrorAuthenticationFailureHandler implements AuthenticationFailureHandler { - private final Log logger = LogFactory.getLog(getClass()); - - private HttpMessageConverter errorHttpResponseConverter = new OAuth2ErrorHttpMessageConverter(); + private HttpMessageConverter errorResponseConverter = new OAuth2ErrorHttpMessageConverter(); @Override - public void onAuthenticationFailure(HttpServletRequest request, HttpServletResponse response, AuthenticationException authenticationException) throws IOException, ServletException { + public void onAuthenticationFailure(HttpServletRequest request, HttpServletResponse response, + AuthenticationException authenticationException) throws IOException, ServletException { ServletServerHttpResponse httpResponse = new ServletServerHttpResponse(response); httpResponse.setStatusCode(HttpStatus.BAD_REQUEST); if (authenticationException instanceof OAuth2AuthenticationException) { OAuth2Error error = ((OAuth2AuthenticationException) authenticationException).getError(); - this.errorHttpResponseConverter.write(error, null, httpResponse); + this.errorResponseConverter.write(error, null, httpResponse); } else { if (this.logger.isWarnEnabled()) { - this.logger.warn(LogMessage.format("Authentication exception must be of type 'org.springframework.security.oauth2.core.OAuth2AuthenticationException'. Provided exception was '%s'", authenticationException.getClass().getName())); + this.logger.warn(AuthenticationException.class.getSimpleName() + " must be of type " + + OAuth2AuthenticationException.class.getName() + + " but was " + authenticationException.getClass().getName()); } } } /** - * Sets OAuth error HTTP message converter to write to upon authentication failure + * Sets the {@link HttpMessageConverter} used for converting an {@link OAuth2Error} to an HTTP response. * - * @param errorHttpResponseConverter the error HTTP message converter to set + * @param errorResponseConverter the {@link HttpMessageConverter} used for converting an {@link OAuth2Error} to an HTTP response */ - public void setErrorHttpResponseConverter(HttpMessageConverter errorHttpResponseConverter) { - this.errorHttpResponseConverter = errorHttpResponseConverter; + public void setErrorResponseConverter(HttpMessageConverter errorResponseConverter) { + Assert.notNull(errorResponseConverter, "errorResponseConverter cannot be null"); + this.errorResponseConverter = errorResponseConverter; } + } diff --git a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandlerTests.java b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandlerTests.java index a56fb867c..1dbfdfba5 100644 --- a/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandlerTests.java +++ b/oauth2-authorization-server/src/test/java/org/springframework/security/oauth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandlerTests.java @@ -15,16 +15,10 @@ */ package org.springframework.security.oauth2.server.authorization.web.authentication; -import java.io.IOException; - -import jakarta.servlet.ServletException; -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; + import org.springframework.http.HttpStatus; import org.springframework.http.converter.HttpMessageConverter; -import org.springframework.http.server.ServletServerHttpResponse; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.BadCredentialsException; @@ -32,58 +26,61 @@ import org.springframework.security.oauth2.core.OAuth2AuthenticationException; import org.springframework.security.oauth2.core.OAuth2Error; import org.springframework.security.oauth2.core.OAuth2ErrorCodes; +import org.springframework.security.oauth2.core.endpoint.OAuth2ParameterNames; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.ArgumentMatchers.isNull; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; /** - * Tests for {@link OAuth2ErrorAuthenticationFailureHandler} + * Tests for {@link OAuth2ErrorAuthenticationFailureHandler}. * * @author Dmitriy Dubson */ public class OAuth2ErrorAuthenticationFailureHandlerTests { + private final OAuth2ErrorAuthenticationFailureHandler authenticationFailureHandler = new OAuth2ErrorAuthenticationFailureHandler(); - private HttpMessageConverter errorHttpMessageConverter; - - private HttpServletRequest request; - - private HttpServletResponse response; - - @BeforeEach - @SuppressWarnings("unchecked") - public void setUp() { - errorHttpMessageConverter = (HttpMessageConverter) mock(HttpMessageConverter.class); - request = new MockHttpServletRequest(); - response = new MockHttpServletResponse(); + @Test + public void setErrorResponseConverterWhenNullThenThrowIllegalArgumentException() { + // @formatter:off + assertThatThrownBy(() -> this.authenticationFailureHandler.setErrorResponseConverter(null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessage("errorResponseConverter cannot be null"); + // @formatter:on } @Test - public void onAuthenticationFailure() throws IOException, ServletException { - OAuth2Error invalidRequestError = new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST); - AuthenticationException authenticationException = new OAuth2AuthenticationException(invalidRequestError); - OAuth2ErrorAuthenticationFailureHandler handler = new OAuth2ErrorAuthenticationFailureHandler(); - handler.setErrorHttpResponseConverter(errorHttpMessageConverter); + public void onAuthenticationFailureWhenValidExceptionThenErrorResponse() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + OAuth2Error error = new OAuth2Error(OAuth2ErrorCodes.INVALID_REQUEST, "error description", "error uri"); + AuthenticationException authenticationException = new OAuth2AuthenticationException(error); - handler.onAuthenticationFailure(request, response, authenticationException); + this.authenticationFailureHandler.onAuthenticationFailure(request, response, authenticationException); - verify(errorHttpMessageConverter).write(eq(invalidRequestError), isNull(), any(ServletServerHttpResponse.class)); assertThat(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + assertThat(response.getContentAsString()).contains("invalid_request"); + assertThat(response.getContentAsString()).contains("error description"); + assertThat(response.getContentAsString()).contains("error uri"); } @Test - public void onAuthenticationFailure_ifExceptionProvidedIsNotOAuth2AuthenticationException() throws ServletException, IOException { - OAuth2ErrorAuthenticationFailureHandler handler = new OAuth2ErrorAuthenticationFailureHandler(); - handler.setErrorHttpResponseConverter(errorHttpMessageConverter); + public void onAuthenticationFailureWhenInvalidExceptionThenStatusResponse() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpServletResponse response = new MockHttpServletResponse(); + AuthenticationException authenticationException = new BadCredentialsException("Not a valid exception."); + + HttpMessageConverter errorResponseConverter = mock(HttpMessageConverter.class); + this.authenticationFailureHandler.setErrorResponseConverter(errorResponseConverter); - handler.onAuthenticationFailure(request, response, new BadCredentialsException("Not a valid exception.")); + this.authenticationFailureHandler.onAuthenticationFailure(request, response, authenticationException); - verifyNoInteractions(errorHttpMessageConverter); + verifyNoInteractions(errorResponseConverter); assertThat(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); + assertThat(response.getContentAsString()).doesNotContain(OAuth2ParameterNames.ERROR); + assertThat(response.getContentAsString()).doesNotContain(OAuth2ParameterNames.ERROR_DESCRIPTION); + assertThat(response.getContentAsString()).doesNotContain(OAuth2ParameterNames.ERROR_URI); } }