Validate account status in OneTimeTokenAuthenticationProvider #17656
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
therepanic
force-pushed
the
gh-17655
branch
2 times, most recently
from
c7c8f08 to
971a23c
Compare
| throw new LockedException(OneTimeTokenAuthenticationProvider.this.messages | ||
| .getMessage("AbstractUserDetailsAuthenticationProvider.locked", "User account is locked")); |
There was a problem hiding this comment.
To be honest, I agree with you that this is a slippery slope. On the other hand, it will work.
Is there any other way to do this? Yes, we could hypothetically copy the message text personally for OneTimeTokenAuthenticationProvider, but I believe that this is not flexible at all.
Are there any suggestions?
There was a problem hiding this comment.
I think we should just throw messages directly.
In AbstractUserDetailsAuthenticationProvider for BadCredentialsException, we do
this.messages.getMessage(“AbstractUserDetailsAuthenticationProvider.badCredentials”, “Bad credentials”)In turn, in OneTimeTokenAuthenticationProvider, we write this directly. Therefore, I think it would be best to simply display error messages without using MessageSourceAccessor.
The main problem is that OneTimeTokenAuthenticationProvider does not extend from AbstractUserDetailsAuthenticationProvider, which has a preauthentication check for user details. However, we do not need to extend from it because it does not fit the context of the class. In this regard, I decided to add my own checker to this commit, which performs a preauthentication check before authorizing the account, similar to how it is done in AbstractUserDetailsAuthenticationProvider. I also added a test to OneTimeTokenAuthenticationProviderTests that identifies this problem. Closes: spring-projectsgh-17655 Signed-off-by: Andrey Litvitski <[email protected]> 1 Signed-off-by: Andrey Litvitski <[email protected]>
| @@ -56,6 +74,7 @@ public Authentication authenticate(Authentication authentication) throws Authent | |||
| } | |||
| try { | |||
There was a problem hiding this comment.
the catch can be common something considering your added exceptions - like AuthenticationException
Then you can send specific exception message
There was a problem hiding this comment.
Thank you in advance for your comment.
Am I correct in assuming that you are suggesting something like this:
try {...
} catch (AuthenticationException e) {
throw new BadCredentialsException(e.getMessage());
}In general, I personally think that this is not a good idea, as we would like the user to know what type of exception is being thrown. Since in check() we throw exceptions that are primarily inherited from AccountStatusException. We should also understand that BadCredentialsException is not related to AccountStatusException, which I think could confuse the user.
You can also see how this is implemented in AbstractUserDetailsAuthenticationProvider.
The main problem is that
OneTimeTokenAuthenticationProviderdoes not extend fromAbstractUserDetailsAuthenticationProvider, which has a preauthentication check for user details. However, we do not need to extend from it because it does not fit the context of the class. In this regard, I decided to add my own checker to this commit, which performs a preauthentication check before authorizing the account, similar to how it is done inAbstractUserDetailsAuthenticationProvider. I also added a test toOneTimeTokenAuthenticationProviderTeststhat identifies this problem.Closes: #17655