Use Credentials object instead of 2 attributes for Basic Authentication

This commit changes the usage of two separate attributes (username and
password) into one: a single `Credentials` object.
Additionally, the attributes key under which the credentials are stored
is changed to be specific to Basic Authentication, in order to allow for
other sorts of authentication later.

Issue: SPR-15764

Author: poutsma

spring-webflux/src/main/java/org/springframework/web/reactive/function/client/ExchangeFilterFunctions.java

@@ -16,10 +16,12 @@
 
 package org.springframework.web.reactive.function.client;
 
-import java.nio.charset.Charset;
+import java.nio.charset.CharsetEncoder;
 import java.nio.charset.StandardCharsets;
 import java.util.Base64;
+import java.util.Map;
 import java.util.Optional;
+import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Predicate;
 
@@ -40,52 +42,45 @@
 public abstract class ExchangeFilterFunctions {
 
 	/**
-	 * Name of the {@link ClientRequest} attribute that contains the username, as used by
+	 * Name of the {@link ClientRequest} attribute that contains the {@link Credentials}, as used by
 	 * {@link #basicAuthentication()}
 	 */
-	public static final String USERNAME_ATTRIBUTE = ExchangeFilterFunctions.class.getName() + ".username";
-
-	/**
-	 * Name of the {@link ClientRequest} attribute that contains the password, as used by
-	 * {@link #basicAuthentication()}
-	 */
-	public static final String PASSWORD_ATTRIBUTE = ExchangeFilterFunctions.class.getName() + ".password";
+	public static final String BASIC_AUTHENTICATION_CREDENTIALS_ATTRIBUTE = ExchangeFilterFunctions.class.getName() + ".basicAuthenticationCredentials";
 
 
 	/**
 	 * Return a filter that adds an Authorization header for HTTP Basic Authentication, based on
 	 * the given username and password.
+	 * <p>Note that Basic Authentication only supports characters in the
+	 * {@link StandardCharsets#ISO_8859_1 ISO-8859-1} character set.
 	 * @param username the username to use
 	 * @param password the password to use
 	 * @return the {@link ExchangeFilterFunction} that adds the Authorization header
+	 * @throws IllegalArgumentException if either {@code username} or {@code password} contain
+	 * characters that cannot be encoded to ISO-8859-1
 	 */
 	public static ExchangeFilterFunction basicAuthentication(String username, String password) {
 		Assert.notNull(username, "'username' must not be null");
 		Assert.notNull(password, "'password' must not be null");
 
+		checkIllegalCharacters(username, password);
 		return basicAuthenticationInternal(r -> Optional.of(new Credentials(username, password)));
 	}
 
 	/**
 	 * Return a filter that adds an Authorization header for HTTP Basic Authentication, based on
-	 * the username and password provided in the
-	 * {@linkplain ClientRequest#attributes() request attributes}. If the attributes are not found,
-	 * no authorization header
+	 * the {@link Credentials} provided in the
+	 * {@linkplain ClientRequest#attributes() request attributes}. If the attribute is not found,
+	 * no authorization header is added.
+	 * <p>Note that Basic Authentication only supports characters in the
+	 * {@link StandardCharsets#ISO_8859_1 ISO-8859-1} character set.
 	 * @return the {@link ExchangeFilterFunction} that adds the Authorization header
-	 * @see #USERNAME_ATTRIBUTE
-	 * @see #PASSWORD_ATTRIBUTE
+	 * @see #BASIC_AUTHENTICATION_CREDENTIALS_ATTRIBUTE
+	 * @see Credentials#basicAuthenticationCredentials(String, String)
 	 */
 	public static ExchangeFilterFunction basicAuthentication() {
 		return basicAuthenticationInternal(
-				request -> {
-					Optional<String> username = request.attribute(USERNAME_ATTRIBUTE).map(o -> (String)o);
-					Optional<String> password = request.attribute(PASSWORD_ATTRIBUTE).map(o -> (String)o);
-					if (username.isPresent() && password.isPresent()) {
-						return Optional.of(new Credentials(username.get(), password.get()));
-					} else {
-						return Optional.empty();
-					}
-				});
+				request -> request.attribute(BASIC_AUTHENTICATION_CREDENTIALS_ATTRIBUTE).map(o -> (Credentials)o));
 	}
 
 	private static ExchangeFilterFunction basicAuthenticationInternal(
@@ -106,12 +101,28 @@ private static ExchangeFilterFunction basicAuthenticationInternal(
 	}
 
 	private static String authorization(Credentials credentials) {
-		byte[] credentialBytes = credentials.toByteArray(StandardCharsets.ISO_8859_1);
+		String credentialsString = credentials.username + ":" + credentials.password;
+		byte[] credentialBytes = credentialsString.getBytes(StandardCharsets.ISO_8859_1);
 		byte[] encodedBytes = Base64.getEncoder().encode(credentialBytes);
 		String encodedCredentials = new String(encodedBytes, StandardCharsets.ISO_8859_1);
 		return "Basic " + encodedCredentials;
 	}
 
+	/*
+	 * Basic authentication only supports ISO 8859-1, see
+	 * https://stackoverflow.com/questions/702629/utf-8-characters-mangled-in-http-basic-auth-username#703341
+	 */
+	private static void checkIllegalCharacters(String username, String password) {
+		CharsetEncoder encoder = StandardCharsets.ISO_8859_1.newEncoder();
+		if (!encoder.canEncode(username) || !encoder.canEncode(password)) {
+			throw new IllegalArgumentException(
+					"Username or password contains characters that cannot be encoded to ISO-8859-1");
+		}
+
+	}
+
+
+
 	/**
 	 * Return a filter that returns a given {@link Throwable} as response if the given
 	 * {@link HttpStatus} predicate matches.
@@ -140,20 +151,63 @@ public static ExchangeFilterFunction statusError(Predicate<HttpStatus> statusPre
 	}
 
 
-	private static final class Credentials {
+	/**
+	 * Represents a combination of username and password, as used by {@link #basicAuthentication()}.
+	 * @see #basicAuthenticationCredentials(String, String)
+	 */
+	public static final class Credentials {
 
-		private String username;
+		private final String username;
 
-		private String password;
+		private final String password;
 
+		/**
+		 * Create a new {@code Credentials} instance with the given username and password.
+		 * @param username the username
+		 * @param password the password
+		 */
 		public Credentials(String username, String password) {
+			Assert.notNull(username, "'username' must not be null");
+			Assert.notNull(password, "'password' must not be null");
+
 			this.username = username;
 			this.password = password;
 		}
 
-		public byte[] toByteArray(Charset charset) {
-			String credentials = this.username + ":" + this.password;
-			return credentials.getBytes(charset);
+		/**
+		 * Return a consumer that stores the given username and password in the
+		 * {@linkplain ClientRequest.Builder#attributes(java.util.function.Consumer) request
+		 * attributes} as a {@code Credentials} object.
+		 * @param username the username
+		 * @param password the password
+		 * @return a consumer that adds the given credentials to the attribute map
+		 * @see ClientRequest.Builder#attributes(java.util.function.Consumer)
+		 * @see #BASIC_AUTHENTICATION_CREDENTIALS_ATTRIBUTE
+		 */
+		public static Consumer<Map<String, Object>> basicAuthenticationCredentials(String username, String password) {
+			Credentials credentials = new Credentials(username, password);
+			checkIllegalCharacters(username, password);
+
+			return attributes -> attributes.put(BASIC_AUTHENTICATION_CREDENTIALS_ATTRIBUTE, credentials);
+		}
+
+		@Override
+		public boolean equals(Object o) {
+			if (this == o) {
+				return true;
+			}
+			if (o instanceof Credentials) {
+				Credentials other = (Credentials) o;
+				return this.username.equals(other.username) &&
+						this.password.equals(other.password);
+
+			}
+			return false;
+		}
+
+		@Override
+		public int hashCode() {
+			return 31 * this.username.hashCode() + this.password.hashCode();
 		}
 
 	}

spring-webflux/src/test/java/org/springframework/web/reactive/function/client/ExchangeFilterFunctionsTests.java

@@ -28,6 +28,7 @@
 import static org.junit.Assert.*;
 import static org.mockito.Mockito.*;
 import static org.springframework.http.HttpMethod.GET;
+import static org.springframework.web.reactive.function.client.ExchangeFilterFunctions.Credentials.basicAuthenticationCredentials;
 
 /**
  * @author Arjen Poutsma
@@ -84,7 +85,7 @@ public void apply() throws Exception {
 	}
 
 	@Test
-	public void basicAuthentication() throws Exception {
+	public void basicAuthenticationUsernamePassword() throws Exception {
 		ClientRequest request = ClientRequest.method(GET, URI.create("http://example.com")).build();
 		ClientResponse response = mock(ClientResponse.class);
 
@@ -100,11 +101,17 @@ public void basicAuthentication() throws Exception {
 		assertEquals(response, result);
 	}
 
+	@Test(expected = IllegalArgumentException.class)
+	public void basicAuthenticationInvalidCharacters() throws Exception {
+
+		ExchangeFilterFunctions.basicAuthentication("foo", "\ud83d\udca9");
+	}
+
 	@Test
 	public void basicAuthenticationAttributes() throws Exception {
 		ClientRequest request = ClientRequest.method(GET, URI.create("http://example.com"))
-				.attribute(ExchangeFilterFunctions.USERNAME_ATTRIBUTE, "foo")
-				.attribute(ExchangeFilterFunctions.PASSWORD_ATTRIBUTE, "bar").build();
+				.attributes(basicAuthenticationCredentials("foo", "bar"))
+				.build();
 		ClientResponse response = mock(ClientResponse.class);
 
 		ExchangeFunction exchange = r -> {