Skip to content

Validate account status in OneTimeTokenAuthenticationProvider #17656

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+68 −1
Open

Validate account status in OneTimeTokenAuthenticationProvider #17656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 55 additions & 1 deletion ...a/org/springframework/security/authentication/ott/OneTimeTokenAuthenticationProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,22 @@

package org.springframework.security.authentication.ott;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

import org.springframework.context.MessageSource;
import org.springframework.context.MessageSourceAware;
import org.springframework.context.support.MessageSourceAccessor;
import org.springframework.security.authentication.AccountExpiredException;
import org.springframework.security.authentication.AuthenticationProvider;
import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.authentication.DisabledException;
import org.springframework.security.authentication.LockedException;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.core.SpringSecurityMessageSource;
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.core.userdetails.UserDetailsChecker;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
import org.springframework.util.Assert;
Expand All @@ -31,14 +42,21 @@
* {@link UserDetailsService} to fetch user authorities.
*
* @author Marcus da Coregio
* @author Andrey Litvitski
* @since 6.4
*/
public final class OneTimeTokenAuthenticationProvider implements AuthenticationProvider {
public final class OneTimeTokenAuthenticationProvider implements AuthenticationProvider, MessageSourceAware {

private final OneTimeTokenService oneTimeTokenService;

private final UserDetailsService userDetailsService;

private final Log logger = LogFactory.getLog(getClass());

private UserDetailsChecker authenticationChecks = new DefaultAuthenticationChecks();

private MessageSourceAccessor messages = SpringSecurityMessageSource.getAccessor();

public OneTimeTokenAuthenticationProvider(OneTimeTokenService oneTimeTokenService,
UserDetailsService userDetailsService) {
Assert.notNull(oneTimeTokenService, "oneTimeTokenService cannot be null");
Expand All @@ -56,6 +74,7 @@ public Authentication authenticate(Authentication authentication) throws Authent
}
try {
Copy link
Contributor

@ronodhirSoumik ronodhirSoumik Aug 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the catch can be common something considering your added exceptions - like AuthenticationException
Then you can send specific exception message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

UserDetails user = this.userDetailsService.loadUserByUsername(consumed.getUsername());
this.authenticationChecks.check(user);
OneTimeTokenAuthenticationToken authenticated = OneTimeTokenAuthenticationToken.authenticated(user,
user.getAuthorities());
authenticated.setDetails(otpAuthenticationToken.getDetails());
Expand All @@ -71,4 +90,39 @@ public boolean supports(Class<?> authentication) {
return OneTimeTokenAuthenticationToken.class.isAssignableFrom(authentication);
}

@Override
public void setMessageSource(MessageSource messageSource) {
this.messages = new MessageSourceAccessor(messageSource);
}

public void setAuthenticationChecks(UserDetailsChecker authenticationChecks) {
this.authenticationChecks = authenticationChecks;
}

private class DefaultAuthenticationChecks implements UserDetailsChecker {

@Override
public void check(UserDetails user) {
if (!user.isAccountNonLocked()) {
OneTimeTokenAuthenticationProvider.this.logger
.debug("Failed to authenticate since user account is locked");
throw new LockedException(OneTimeTokenAuthenticationProvider.this.messages
.getMessage("AbstractUserDetailsAuthenticationProvider.locked", "User account is locked"));
Comment on lines +109 to +110
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (!user.isEnabled()) {
OneTimeTokenAuthenticationProvider.this.logger
.debug("Failed to authenticate since user account is disabled");
throw new DisabledException(OneTimeTokenAuthenticationProvider.this.messages
.getMessage("AbstractUserDetailsAuthenticationProvider.disabled", "User is disabled"));
}
if (!user.isAccountNonExpired()) {
OneTimeTokenAuthenticationProvider.this.logger
.debug("Failed to authenticate since user account has expired");
throw new AccountExpiredException(OneTimeTokenAuthenticationProvider.this.messages
.getMessage("AbstractUserDetailsAuthenticationProvider.expired", "User account has expired"));
}
}

}

}
13 changes: 13 additions & 0 deletions .../springframework/security/authentication/ott/OneTimeTokenAuthenticationProviderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.mockito.junit.jupiter.MockitoExtension;

import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.core.userdetails.User;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
Expand All @@ -42,6 +43,7 @@
* Tests for {@link OneTimeTokenAuthenticationProvider}.
*
* @author Max Batischev
* @author Andrey Litvitski
*/
@ExtendWith(MockitoExtension.class)
public class OneTimeTokenAuthenticationProviderTests {
Expand Down Expand Up @@ -79,6 +81,17 @@ void authenticateWhenAuthenticationTokenIsPresentThenAuthenticates() {
assertThat(CollectionUtils.isEmpty(user.getAuthorities())).isTrue();
}

@Test
void authenticateWhenAuthenticationTokenIsPresentThenFails() {
given(this.oneTimeTokenService.consume(any()))
.willReturn(new DefaultOneTimeToken(TOKEN, USERNAME, Instant.now().plusSeconds(120)));
given(this.userDetailsService.loadUserByUsername(anyString()))
.willReturn(new User(USERNAME, PASSWORD, false, false, false, false, List.of()));
OneTimeTokenAuthenticationToken token = new OneTimeTokenAuthenticationToken(TOKEN);

assertThatExceptionOfType(AuthenticationException.class).isThrownBy(() -> this.provider.authenticate(token));
}

@Test
void authenticateWhenOneTimeTokenIsNotFoundThenFails() {
given(this.oneTimeTokenService.consume(any())).willReturn(null);
Expand Down