Improved error message for PasswordEncoder

Signed-off-by: Jonny Coddington <[email protected]>

Author: bottlerocketjonny

core/src/test/java/org/springframework/security/provisioning/InMemoryUserDetailsManagerTests.java

@@ -18,15 +18,24 @@
 
 import java.util.Properties;
 
+import java.util.stream.Stream;
 import org.junit.jupiter.api.Test;
 
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.springframework.security.authentication.AuthenticationManager;
+import org.springframework.security.authentication.ProviderManager;
 import org.springframework.security.authentication.TestAuthentication;
+import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
+import org.springframework.security.authentication.dao.DaoAuthenticationProvider;
 import org.springframework.security.core.Authentication;
 import org.springframework.security.core.context.SecurityContextHolderStrategy;
 import org.springframework.security.core.context.SecurityContextImpl;
 import org.springframework.security.core.userdetails.PasswordEncodedUser;
 import org.springframework.security.core.userdetails.User;
 import org.springframework.security.core.userdetails.UserDetails;
+import org.springframework.security.crypto.factory.PasswordEncoderFactories;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
@@ -97,4 +106,33 @@ public void changePasswordWhenCustomSecurityContextHolderStrategyThenUses() {
 		verify(strategy).getContext();
 	}
 
+	@ParameterizedTest
+	@MethodSource("authenticationErrorCases")
+	void authenticateWhenInvalidMissingOrMalformedIdThenException(String username, String password, String expectedMessage) {
+		UserDetails user = User.builder()
+				.username(username)
+				.password(password)
+				.roles("USER")
+				.build();
+		InMemoryUserDetailsManager userManager = new InMemoryUserDetailsManager(user);
+
+		DaoAuthenticationProvider authenticationProvider = new DaoAuthenticationProvider();
+		authenticationProvider.setUserDetailsService(userManager);
+		authenticationProvider.setPasswordEncoder(PasswordEncoderFactories.createDelegatingPasswordEncoder());
+
+		AuthenticationManager authManager = new ProviderManager(authenticationProvider);
+
+		assertThatIllegalArgumentException()
+				.isThrownBy(() -> authManager.authenticate(new UsernamePasswordAuthenticationToken(username, "password")))
+				.withMessage(expectedMessage);
+	}
+
+	private static Stream<Arguments> authenticationErrorCases() {
+		return Stream.of(
+				Arguments.of("user", "password", "Password encoding information cannot be null. If no encoding is intended, ensure it is prefixed with '{noop}'."),
+				Arguments.of("user", "bycrpt}password", "The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'."),
+				Arguments.of("user", "{bycrptpassword", "The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'."),
+				Arguments.of("user", "{ren&stimpy}password", "There is no password encoder mapped for the id 'ren&stimpy'. Check your configuration to ensure it matches one of the registered encoders.")
+		);
+	}
 }

crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java

@@ -245,12 +245,12 @@ private String extractId(String prefixEncodedPassword) {
 			return null;
 		}
 		int start = prefixEncodedPassword.indexOf(this.idPrefix);
-		if (start != 0) {
+		if (start == -1 && !prefixEncodedPassword.contains(this.idSuffix)) {
 			return null;
 		}
-		int end = prefixEncodedPassword.indexOf(this.idSuffix, start);
-		if (end < 0) {
-			return null;
+		int end = prefixEncodedPassword.indexOf(this.idSuffix, start + this.idPrefix.length());
+		if (start != 0 || end == -1) {
+			return "";
 		}
 		return prefixEncodedPassword.substring(start + this.idPrefix.length(), end);
 	}
@@ -286,7 +286,16 @@ public String encode(CharSequence rawPassword) {
 		@Override
 		public boolean matches(CharSequence rawPassword, String prefixEncodedPassword) {
 			String id = extractId(prefixEncodedPassword);
-			throw new IllegalArgumentException("There is no PasswordEncoder mapped for the id \"" + id + "\"");
+			if (id == null) {
+				throw new IllegalArgumentException("Password encoding information cannot be null. " +
+						"If no encoding is intended, ensure it is prefixed with '{noop}'.");
+			} else if (id.isEmpty()) {
+				throw new IllegalArgumentException(String.format("The name of the password encoder is improperly "
+						+ "formatted or incomplete. The format should be '%sENCODER%spassword'.", idPrefix, idSuffix));
+			} else {
+				throw new IllegalArgumentException(String.format("There is no password encoder mapped for the id '%s'. " +
+						"Check your configuration to ensure it matches one of the registered encoders.", id));
+			}
 		}
 
 	}

crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java

@@ -193,31 +193,31 @@ public void matchesWhenNoopThenDelegatesToNoop() {
 	public void matchesWhenUnMappedThenIllegalArgumentException() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "{unmapped}" + this.rawPassword))
-			.withMessage("There is no PasswordEncoder mapped for the id \"unmapped\"");
+			.withMessage("There is no password encoder mapped for the id 'unmapped'. Check your configuration to ensure it matches one of the registered encoders.");
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
 	@Test
 	public void matchesWhenNoClosingPrefixStringThenIllegalArgumentException() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "{bcrypt" + this.rawPassword))
-			.withMessage("There is no PasswordEncoder mapped for the id \"null\"");
+			.withMessage("The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'.");
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
 	@Test
 	public void matchesWhenNoStartingPrefixStringThenFalse() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "bcrypt}" + this.rawPassword))
-			.withMessage("There is no PasswordEncoder mapped for the id \"null\"");
+			.withMessage("The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'.");
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
 	@Test
 	public void matchesWhenNoIdStringThenFalse() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "{}" + this.rawPassword))
-			.withMessage("There is no PasswordEncoder mapped for the id \"\"");
+			.withMessage("The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'.");
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
@@ -226,7 +226,7 @@ public void matchesWhenPrefixInMiddleThenFalse() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "invalid" + this.bcryptEncodedPassword))
 			.isInstanceOf(IllegalArgumentException.class)
-			.withMessage("There is no PasswordEncoder mapped for the id \"null\"");
+			.withMessage("The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'.");
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
@@ -236,7 +236,7 @@ public void matchesWhenIdIsNullThenFalse() {
 		DelegatingPasswordEncoder passwordEncoder = new DelegatingPasswordEncoder(this.bcryptId, this.delegates);
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> passwordEncoder.matches(this.rawPassword, this.rawPassword))
-			.withMessage("There is no PasswordEncoder mapped for the id \"null\"");
+			.withMessage("Password encoding information cannot be null. If no encoding is intended, ensure it is prefixed with '{noop}'.");
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}