Spring Security Add UserDetails Object to Exceptions thrown by AccountStatusUserDetailsChecker

Expected Behavior When handling exceptions like LockedException in an AuthenticationFailureHandler, have the UserDetails object that triggered the exception available for context.

Current Behavior In an AuthenticationFailureHandler, there is all context of the UserDetails removed from the (now generic) exception.

Context I would like to return a cause for a user lock as a response in case of an authentication failure. I can lock a user in my UserDetailsService, but there is no way to give a cause. I get the generic LockedException in the AuthenticationFailureHandler.

Workaround: Override the default provided DaoAuthenticationProvider with a customized bean of this type that has a custom UserDetailsChecker instead of the AccountStatusUserDetailsChecker, that based on context data from the UserDetails object available in the UserDetailsChecker throw a custom exception that extends AuthenticationException and contains the UserDetails object or relevant parts of it. I then get this exception (instead of the LockedException) into the AuthenticationFailureHandler and can handle it appropriately. While this is working, this seems to be overly complicated, when i am only missing a context information on the LockedException.

If this idea seems to be fine, i am happy to try a PR for this. Thank you very much.

Comment From: jzheaux

Hi, @dl1ely, thanks for the suggestion and for the offer to contribute!

The concern with adding UserDetails to LockedException is that the exception is used in other places in Spring Security that don't use a UserDetails. While it's possible to add a setter, it seems odd since it would only be used for that scenario.

I think that you can make the UserDetails available to your failure handler by doing:

@Bean 
AuthenticationProvider authenticationProvider() {
    DaoAuthenticationProvider provider = new DaoAuthenticationProvider();
    AccountStatusUserDetailsChecker delegate = new AccountStatusUserDetailsChecker();
    provider.setPostAuthenticationChecks(details -> {
        try {
            delegate.check(details);
        } catch (AuthenticationException e) {
            throw new MyAuthenticationException(details, e);
        }
    });
    return provider;
}

While this could be further simplified with additional support, it already seems simple.

Since you were saying it's overly complicated, it's possible I'm missing something, though. Is your use case more complex than that?

Comment From: dl1ely

Hi Josh,

thanks for getting back to me in such a detailed manner. My requirement was to block a user for 10 minutes after his 3rd failed login attempt in a row, and in the 401 error response return the remaining block time for displaying in a SPA application. After the 4th failed attempts the block should be 20 minutes, then 30, and so on.

In the UserDetailsService i checked an existing block when loading the user from the DB by username, and if so, i would set the value of remaining blocked minutes in a custom UserDetails Object. In the AuthenticationCheck in the AuthenticationProvider the LockedException would be thrown, and in a custom FailureHandler i would create the JSON error response with the remaining block time for that user. But i cannot because the LockedException has no information how long the lock will still persist. I also tried throwing a custom exception as you proposed (but i did the whole thing in a more awkward way, i did not use a delegate but copy-and-pasted the code from the AccountStatusDetailsChecker, d'oh!), but i also need the AuthenticationFailureBadCredentialsEvent to be published because this gets used to calculate the new block time. In order to make my custom Exception also fire the event i needed to modify the DefaultAuthenticationEventPublisher. At that point i felt like i am going down a rabbit hole here, and touching too many different parts for such a small task.

In the end i needed to go another way, anyway, because i hit a dead end because when i am loading the user after his 2nd failed login attemts in my custem UserDetailsService, i have no idea whether this 3rd attempt will fail or succeed. So i have no means to populate the custom UserDetails object with the needed information. And if it is his 3rd failed attempt then, i get the BadCredentialsException, not the LockedException, because i did not lock him yet, still i need to return a 10 minute block time in my JSON error response.

I resorted to put my failed logins context information in the RequestContext (populated from the UserDetailsService), and use this information in the FailureHandler to create my JSON error response regardless if it is a BadCredentialsException (after 3rd failed attempts) or a LockedException (after 4th and all future failed attempts).

I somehow expected it to be easier to get the necessary information to the FailureHandler, but as the feature requested above would still not help me regarding the requirement i was working on (as i now realized), i am fine with this not being considered.

I am still wondering if there is some easier way to implement a dynamic time based locking of users (with feedback in the 401 error response about the lock) based on their failed login attempts. Do i miss something obvious?

Closing this issue.

Comment From: jzheaux

Sorry for my delay in responding to your question.

This seems like a great question for StackOverflow where you'll get feedback more easily from the entire community. In fact, I think there's a StackOverflow answer that could be a good start for simplifying your approach. For clarity, I've just posted a sample to my GitHub that roughly follows Rob's recommendation listed there.