Polish gh-1384

spring-projects · Oct 17, 2023 · 8c08d7b · 8c08d7b
1 parent 96c90dd
commit 8c08d7b
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 56 deletions.
diff --git 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/...ork/security/oauth2/server/authorization/web/OAuth2DeviceAuthorizationEndpointFilter.java b/...ork/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/...g/springframework/security/oauth2/server/authorization/web/OAuth2TokenEndpointFilter.java b/...g/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/...uth2/server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandler.java b/...uth2/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<OAuth2Error> errorHttpResponseConverter = new OAuth2ErrorHttpMessageConverter();
+	private HttpMessageConverter<OAuth2Error> 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<OAuth2Error> errorHttpResponseConverter) {
-		this.errorHttpResponseConverter = errorHttpResponseConverter;
+	public void setErrorResponseConverter(HttpMessageConverter<OAuth2Error> errorResponseConverter) {
+		Assert.notNull(errorResponseConverter, "errorResponseConverter cannot be null");
+		this.errorResponseConverter = errorResponseConverter;
 	}
+
 }
diff --git a/...server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandlerTests.java b/...server/authorization/web/authentication/OAuth2ErrorAuthenticationFailureHandlerTests.java
@@ -15,75 +15,72 @@
  */
 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;
 import org.springframework.security.core.AuthenticationException;
 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<OAuth2Error> errorHttpMessageConverter;
-
-	private HttpServletRequest request;
-
-	private HttpServletResponse response;
-
-	@BeforeEach
-	@SuppressWarnings("unchecked")
-	public void setUp() {
-		errorHttpMessageConverter = (HttpMessageConverter<OAuth2Error>) 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<OAuth2Error> 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);
 	}
 
 }