Improved error message for PasswordEncoder #14968
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Hi, @bottlerocketjonny! Yes, we already received another PR for the same and I think it is a nice improvement. That said, if you'd like, please rebase once that is merged and push any other changes you'd like to see to this PR, and we can take a look together. |
No worries! Yeah sure thing I shall do that 👍🏻 Just for future reference, if I wanted to work on an issue should I wait to be assigned or just go for it? I left a comment on this one but wasn't sure if that was the way to go. Cheers 😀 |
|
@bottlerocketjonny, it's certainly better when I'm on top of folks' comments in tickets so that there isn't confusion. :) Sorry that you and the other contributor ended up working on the same feature. It's rare for two people to contribute near the same time, so we don't have a stated policy. Mostly, it's a best effort on my part to notice the comment, assign the issue to that person, and in the absence of that, attend to the first submitted PR. In your case, I'm happy that there were good ideas in both PRs, so I'd still encourage you to rebase. |
Signed-off-by: Jonny Coddington <[email protected]>
bottlerocketjonny
force-pushed
the
improved-error-message-password-encoder
branch
from
e161962
to
7e29776
Compare
There was a problem hiding this comment.
Thanks for the updates, @bottlerocketjonny! I've left some feedback inline.
Also, will you please update your commit message to include the ticket #14880 like so:
Your Title
Issue gh-14880
That will link this work to that ticket in GitHub, making it easier to track in the future.
| @@ -245,12 +245,12 @@ private String extractId(String prefixEncodedPassword) { | |||
| return null; | |||
| } | |||
| int start = prefixEncodedPassword.indexOf(this.idPrefix); | |||
| if (start != 0) { | |||
| if (start == -1 && !prefixEncodedPassword.contains(this.idSuffix)) { | |||
There was a problem hiding this comment.
I'd like to be a bit more conservative with changing this behavior, given that it is called by more than one flow in this class. What if the matches implementation itself did the needed checking? Something like this:
String id = extractId(prefixEncodedPassword);
if (has a value) {
throw no mapping exception
}
start = indexof idPrefix
end = indexof idSuffix, start
if (start < 0 && end < 0) {
throw no encoder prefix
}
throw malformed encoder prefixIn exchange for a small amount of duplication, we don't change the expectations of other flows calling extractId.
| @@ -286,7 +286,16 @@ public String encode(CharSequence rawPassword) { | |||
| @Override | |||
| public boolean matches(CharSequence rawPassword, String prefixEncodedPassword) { | |||
| String id = extractId(prefixEncodedPassword); | |||
| throw new IllegalArgumentException("There is no PasswordEncoder mapped for the id \"" + id + "\""); | |||
| if (id == null) { | |||
| throw new IllegalArgumentException("Password encoding information cannot be null. " + | |||
There was a problem hiding this comment.
I think another bit of advice is needed here. Something like:
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`.
| @@ -18,15 +18,24 @@ | |||
|
|
|||
| import java.util.Properties; | |||
|
|
|||
| import java.util.stream.Stream; | |||
There was a problem hiding this comment.
Please update the copyright here to 2002-2024
| @@ -245,12 +245,12 @@ private String extractId(String prefixEncodedPassword) { | |||
| return null; | |||
| } | |||
| int start = prefixEncodedPassword.indexOf(this.idPrefix); | |||
| if (start != 0) { | |||
There was a problem hiding this comment.
Please update the copyright here to 2002-2024
| @@ -193,31 +193,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\""); | |||
There was a problem hiding this comment.
Please update the copyright here to 2002-2024
jzheaux
added
in: crypto
I fear I might have been too slow to submit this! However, this closes gh-14880
Happy to make any changes.