-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Add Password Advice Support #17118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
jzheaux
wants to merge
10
commits into
spring-projects:main
Choose a base branch
from
jzheaux:check-password-advice
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Add Password Advice Support #17118
+1,452
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
babcae7
to
fb7b323
Compare
There are now two interfaces, ChangeExistingPasswordAdvisor and ChangeUpdatingPasswordAdvisor. I have a sense that more information may be wanted down the road for ChangeUpdatingPasswordAdvisor; so this would allow them to evolve independently.
- This allow the request matcher to be in the filter, preventing eager access to the session
cae543f
to
a04035d
Compare
- These aren't super helpful in the end. Applications can inspect the type and get a richer set of information.
In all likelihood, the only thing needed at the database level is the action that an admin requires be taken for a password.
- Improved class names - Introduced defaults to guide usage
rwinch
requested changes
Aug 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all of your work on this! I've provided some feedback below. Much of it is related to the sample, which is why I've posted it below rather than inline:
- Try and reduce (ideally eliminate) the need for
HttpServletRequest
andHttpServletResponse
for arguments to Spring Controllers (e.g.HomeController
in the sample). This might be something required for more advanced use cases, but I'd like to try to allow users to follow best practices of not using the request/response in the controllers. - Can you demo how to clean up the error message when changing a password? Right now the error message seems like it would be difficult for a user to understand what is going on?
- What does an application with the minimum amount of effort for a user look like (as many defaults as possible)?
- I wonder if we can remove
PasswordAdvice
from theUserDetails
. Perhaps managing this on aPasswordAdvisor
implementation? - Does
PasswordAdvisor
need aUserDetails
or could it use a more narrow scope (e.g. username/password)? I ask because there are likely passwords/secrets that are checked outside of the scope of a User (e.g. a consumer secret). I don't know that we need to solve for all of these use cases now, but somewhat thinking outloud. Note: If we have a rich object as an argument to PasswordAdvisor, we could consider adding an optionalUserDetails
later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds configuration options to
.passwordManagement
to allow for additional password management flows:You can activate by publishing a
UserDetailsPasswordManager
bean and using the PasswordManagement DSL:Since these flows require a
UserDetailsPasswordManager
bean, and because.passwordManagement
is a pre-existing DSL, they remain inactive until that bean is provided.Some things to try:
ChangePasswordAdvisor
- the PR contains several sample implementations of this interface. They can be composed inDelegatingChangePasswordAdvisor
to form a custom set of password requirements. By default, two advisors are active; the compromised password advisor and the password advice advisor checking for any existing adviceChangePasswordAdvice.Action
- the existing advisors can be configured to have a different action, for example changing the failure action toAction.MUST_CHANGE
instead ofAction.SHOULD_CHANGE
UserDetailsPasswordManager
contains any advice tied to a user. By default,.passwordManagement
only checks at login time and when a password changes. However, you can write aChangePasswordAdviceRepository
implementation that checks theUserDetailsPasswordManager
on each request so that the user is advised mid-session if changes are needed.