Improve Error Messages for PasswordEncoder
Closes gh-14880 Signed-off-by: Jonny Coddington <bottlerocketjonny@protonmail.com>
This commit is contained in:
Jonny Coddington
Josh Cummings
parent
2c9c309d7f
commit
b90851d968
|
|
@ -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<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."));
|
||||
}
|
||||
|
||||
static class CustomUser implements MutableUserDetails, CredentialsContainer {
|
||||
|
||||
private final UserDetails delegate;
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue