Improved error messages for PasswordEncoder

Issue gh-14880

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,34 @@ 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", "Given that there is no default password encoder configured, each "
+						+ "password must have a password encoding prefix. Please either prefix this password with '{noop}' or set a default password encoder in `DelegatingPasswordEncoder`."),
+				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

@@ -18,6 +18,7 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import org.springframework.util.StringUtils;
 
 /**
  * A password encoder that delegates to another PasswordEncoder based upon a prefixed
@@ -129,9 +130,14 @@ public class DelegatingPasswordEncoder implements PasswordEncoder {
 
 	private static final String DEFAULT_ID_SUFFIX = "}";
 
-	public static final String NO_PASSWORD_ENCODER_MAPPED = "There is no PasswordEncoder mapped for the id \"%s\"";
+	private static final String NO_PASSWORD_ENCODER_MAPPED = "There is no password encoder mapped for the id '%s'. "
+			+ "Check your configuration to ensure it matches one of the registered encoders.";
 
-	public static final String NO_PASSWORD_ENCODER_PREFIX = "You have entered a password with no PasswordEncoder. If that is your intent, it should be prefixed with `{noop}`.";
+	private static final String NO_PASSWORD_ENCODER_PREFIX = "Given that there is no default password encoder configured, each password must have a password encoding prefix. "
+			+ "Please either prefix this password with '{noop}' or set a default password encoder in `DelegatingPasswordEncoder`.";
+
+	private static final String MALFORMED_PASSWORD_ENCODER_PREFIX = "The name of the password encoder is improperly "
+			+ "formatted or incomplete. The format should be '%sENCODER%spassword'.";
 
 	private final String idPrefix;
 
@@ -290,10 +296,17 @@ public String encode(CharSequence rawPassword) {
 		@Override
 		public boolean matches(CharSequence rawPassword, String prefixEncodedPassword) {
 			String id = extractId(prefixEncodedPassword);
-			if (id != null && !id.isEmpty()) {
+			if (StringUtils.hasText(id)) {
 				throw new IllegalArgumentException(String.format(NO_PASSWORD_ENCODER_MAPPED, id));
 			}
-			throw new IllegalArgumentException(NO_PASSWORD_ENCODER_PREFIX);
+			if (StringUtils.hasText(prefixEncodedPassword)) {
+				int start = prefixEncodedPassword.indexOf(idPrefix);
+				int end = prefixEncodedPassword.indexOf(idSuffix, start);
+				if (start < 0 && end < 0) {
+					throw new IllegalArgumentException(NO_PASSWORD_ENCODER_PREFIX);
+				}
+			}
+			throw new IllegalArgumentException(String.format(MALFORMED_PASSWORD_ENCODER_PREFIX, idPrefix, idSuffix));
 		}
 
 	}

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

@@ -43,8 +43,6 @@
 @ExtendWith(MockitoExtension.class)
 public class DelegatingPasswordEncoderTests {
 
-	public static final String NO_PASSWORD_ENCODER = "You have entered a password with no PasswordEncoder. If that is your intent, it should be prefixed with `{noop}`.";
-
 	@Mock
 	private PasswordEncoder bcrypt;
 
@@ -70,6 +68,14 @@ public class DelegatingPasswordEncoderTests {
 
 	private DelegatingPasswordEncoder onlySuffixPasswordEncoder;
 
+	private static final String NO_PASSWORD_ENCODER_MAPPED = "There is no password encoder mapped for the id 'unmapped'. "
+			+ "Check your configuration to ensure it matches one of the registered encoders.";
+
+	private static final String NO_PASSWORD_ENCODER_PREFIX = "Given that there is no default password encoder configured, "
+			+ "each password must have a password encoding prefix. Please either prefix this password with '{noop}' or set a default password encoder in `DelegatingPasswordEncoder`.";
+
+	private static final String MALFORMED_PASSWORD_ENCODER_PREFIX = "The name of the password encoder is improperly formatted or incomplete. The format should be '{ENCODER}password'.";
+
 	@BeforeEach
 	public void setup() {
 		this.delegates = new HashMap<>();
@@ -195,31 +201,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(NO_PASSWORD_ENCODER_MAPPED);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
 	@Test
 	public void matchesWhenNoClosingPrefixStringThenIllegalArgumentException() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "{bcrypt" + this.rawPassword))
-			.withMessage(NO_PASSWORD_ENCODER);
+			.withMessage(MALFORMED_PASSWORD_ENCODER_PREFIX);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
 	@Test
 	public void matchesWhenNoStartingPrefixStringThenFalse() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "bcrypt}" + this.rawPassword))
-			.withMessage(NO_PASSWORD_ENCODER);
+			.withMessage(MALFORMED_PASSWORD_ENCODER_PREFIX);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
 	@Test
 	public void matchesWhenNoIdStringThenFalse() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "{}" + this.rawPassword))
-			.withMessage(NO_PASSWORD_ENCODER);
+			.withMessage(MALFORMED_PASSWORD_ENCODER_PREFIX);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
@@ -228,7 +234,7 @@ public void matchesWhenPrefixInMiddleThenFalse() {
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches(this.rawPassword, "invalid" + this.bcryptEncodedPassword))
 			.isInstanceOf(IllegalArgumentException.class)
-			.withMessage(NO_PASSWORD_ENCODER);
+			.withMessage(MALFORMED_PASSWORD_ENCODER_PREFIX);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
@@ -238,7 +244,7 @@ public void matchesWhenIdIsNullThenFalse() {
 		DelegatingPasswordEncoder passwordEncoder = new DelegatingPasswordEncoder(this.bcryptId, this.delegates);
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> passwordEncoder.matches(this.rawPassword, this.rawPassword))
-			.withMessage(NO_PASSWORD_ENCODER);
+			.withMessage(NO_PASSWORD_ENCODER_PREFIX);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
 	}
 
@@ -296,9 +302,8 @@ void matchesShouldThrowIllegalArgumentExceptionWhenNoPasswordEncoderIsMappedForT
 		assertThatIllegalArgumentException()
 			.isThrownBy(() -> this.passwordEncoder.matches("rawPassword", "prefixEncodedPassword"))
 			.isInstanceOf(IllegalArgumentException.class)
-			.withMessage(NO_PASSWORD_ENCODER);
+			.withMessage(NO_PASSWORD_ENCODER_PREFIX);
 		verifyNoMoreInteractions(this.bcrypt, this.noop);
-
 	}
 
 }