From b90851d9681fbe7e0fce3da9c0db97ed813117d1 Mon Sep 17 00:00:00 2001 From: Jonny Coddington Date: Fri, 26 Apr 2024 14:09:28 +0100 Subject: [PATCH] Improve Error Messages for PasswordEncoder Closes gh-14880 Signed-off-by: Jonny Coddington --- .../InMemoryUserDetailsManagerTests.java | 39 +++++++++++++++++++ .../password/DelegatingPasswordEncoder.java | 23 +++++++++-- .../DelegatingPasswordEncoderTests.java | 25 +++++++----- 3 files changed, 73 insertions(+), 14 deletions(-) diff --git a/core/src/test/java/org/springframework/security/provisioning/InMemoryUserDetailsManagerTests.java b/core/src/test/java/org/springframework/security/provisioning/InMemoryUserDetailsManagerTests.java index a9a1f2ea0a..9791c2daf5 100644 --- a/core/src/test/java/org/springframework/security/provisioning/InMemoryUserDetailsManagerTests.java +++ b/core/src/test/java/org/springframework/security/provisioning/InMemoryUserDetailsManagerTests.java @@ -18,10 +18,18 @@ package org.springframework.security.provisioning; import java.util.Collection; 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.CredentialsContainer; import org.springframework.security.core.GrantedAuthority; @@ -31,6 +39,7 @@ 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.core.userdetails.UsernameNotFoundException; +import org.springframework.security.crypto.factory.PasswordEncoderFactories; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; @@ -151,6 +160,36 @@ public class InMemoryUserDetailsManagerTests { assertThat(manager.loadUserByUsername(user.getUsername())).isSameAs(user); } + @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 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.")); + } + static class CustomUser implements MutableUserDetails, CredentialsContainer { private final UserDetails delegate; diff --git a/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java b/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java index 22711242f2..a7bdc66073 100644 --- a/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java +++ b/crypto/src/main/java/org/springframework/security/crypto/password/DelegatingPasswordEncoder.java @@ -19,6 +19,8 @@ package org.springframework.security.crypto.password; 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 * identifier. @@ -129,9 +131,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 +297,18 @@ public class DelegatingPasswordEncoder implements PasswordEncoder { @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(DelegatingPasswordEncoder.this.idPrefix); + int end = prefixEncodedPassword.indexOf(DelegatingPasswordEncoder.this.idSuffix, start); + if (start < 0 && end < 0) { + throw new IllegalArgumentException(NO_PASSWORD_ENCODER_PREFIX); + } + } + throw new IllegalArgumentException(String.format(MALFORMED_PASSWORD_ENCODER_PREFIX, + DelegatingPasswordEncoder.this.idPrefix, DelegatingPasswordEncoder.this.idSuffix)); } } diff --git a/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java index a3222fb41e..2ac32d8a53 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java +++ b/crypto/src/test/java/org/springframework/security/crypto/password/DelegatingPasswordEncoderTests.java @@ -43,8 +43,6 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; @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,7 +201,7 @@ public class DelegatingPasswordEncoderTests { 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); } @@ -203,7 +209,7 @@ public class DelegatingPasswordEncoderTests { 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); } @@ -211,7 +217,7 @@ public class DelegatingPasswordEncoderTests { 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); } @@ -219,7 +225,7 @@ public class DelegatingPasswordEncoderTests { 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 class DelegatingPasswordEncoderTests { 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 class DelegatingPasswordEncoderTests { 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 @@ public class DelegatingPasswordEncoderTests { assertThatIllegalArgumentException() .isThrownBy(() -> this.passwordEncoder.matches("rawPassword", "prefixEncodedPassword")) .isInstanceOf(IllegalArgumentException.class) - .withMessage(NO_PASSWORD_ENCODER); + .withMessage(NO_PASSWORD_ENCODER_PREFIX); verifyNoMoreInteractions(this.bcrypt, this.noop); - } }