Include Compromised Password Information in `UserDetails`

[marcusdacoregio]

Currently, there is no easy way to flag a UserDetails object that has its password compromised. There is no first-class property in Spring Security to identify that.

This will entail:

• Update UserDetails with a default method that shows if the password is compromised
• Update User and it's builder to have the property
• Update DaoAuthenticationProvider to ensure to set the property

See this sample https://github.com/spring-projects/spring-security-samples/tree/main/servlet/spring-boot/java/authentication/username-password/compromised-password-checker

[jgrandja]

@rwinch After doing some investigation, I don't think adding the new property UserDetails.isPasswordCompromised() will work.

The property UserDetails.passwordCompromised would need to be set in a mutable instance of UserDetails, for example, User.UserBuilder.passwordCompromised(true). However, since DaoAuthenticationProvider.createSuccessAuthentication():

spring-security/core/src/main/java/org/springframework/security/authentication/dao/DaoAuthenticationProvider.java

Line 131 in 567933d

&& this.compromisedPasswordChecker.check(presentedPassword).isCompromised();

receives a UserDetails instance, there is no way for us to set the property given the instance is likely immutable and more importantly we don't know the type since it could be a custom UserDetails type. We could make this work if the type is User (and it's not extended as a custom type) and simply make a copy of it and set User.UserBuilder.passwordCompromised(true) but it would only work for this scenario. Personally, I don't think this is the right solution since it would not work for all cases.

Given we need access to a mutable UserDetails instance in order to set passwordCompromised(true), it's logical to think that the CompromisedPasswordChecker.check() should be moved to UserDetailsService.loadUserByUsername(). This would entail updating the current implementations JdbcUserDetailsManager, InMemoryUserDetailsManager and LdapUserDetailsManager. However, custom implementations of UserDetailsService would be responsible for updating the new property, but we can provide some helper (inherited) method that does the password check. However, the issue with this implementation strategy is if the stored password is hashed then it won't work since we need the cleartext password in order to perform the password check. So again, this is not a solution either IMO.

Based on my analysis, I'm not seeing how adding a new property UserDetails.isPasswordCompromised() will work in the described scenarios. Given that 6.4.0-RC1 is just over a week away, I don't think it's a good idea to rush this enhancement as we should give this some further thought and push it to the next release cycle.

What are your thoughts?

[rwinch]

Thanks @jgrandja I agree with your assessment to think on how best to address this.