SEC-537: Allow customized account status checking in AbstractUserDetailsAuthenticationProvider.authenticate method

[spring-projects-issues]

Gerr Magnus Mes(Migrated from SEC-537) said:

Methods isAccountNonLocked(), isEnabled(), isAccountNonExpired() are invoked before checking credentials (before additionalAuthenticationChecks). So I can get any user status (locked, disabled, etc) even if I don’t now user credentials.

[spring-projects-issues]

Luke Taylor said:

This isn’t the case. It’s up to you what failure message you show to the user when a login fails – there’s nothing to stop you saying “login failed” regardless of the cause. It’s also not uncommon for applications to inform a user that their account has been locked after a fixed number of login attempts. The desired behaviour will depend on exactly how you interpret the different status flags (which is not strictly defined by the framework). The exceptions also drive the generation of events by the AuthenticationManager, so they are not necessarily there for user consumption at all. A sysadmin may want to be informed anytime someone is trying to use a locked or disabled account, for example, not just when they use the correct password.

It can also be argued that checking the password before the account locked status and showing a “locked” message only when a correct password is given would also allow brute-force password checking even after the account has been locked.

So I disagree that the behaviour is “incorrect”. I think it might be useful to be able to customize things though. Perhaps we should have methods

preAuthenticationChecks(UserDetails user)

postAuthentication(UserDetails user)

which could be overridden to alter when the differed flags are checked.

[spring-projects-issues]

Ben Alex said:

Luke, the description you provided in the second paragraph is the precise reason why the flags are checked before the password (ie avoidance of brute-force). I am not of the view it is necessary to provide protected methods to customize this behavior, as doing so seems likely to lead to less security and complicate our abstract class for little potential benefit. If you’re in agreement, please close the issue as “won’t fix”.

[spring-projects-issues]

Luke Taylor said:

We’ve decided to allow customization here by way of two pluggable strategies, as described in SEC-536. This will cater for situations where the status flags are interpreted differently (e.g. accounts are suspended temporarily for administratve reasons) or where an organization insists on a particular policy regardless.

[spring-projects-issues]

Luke Taylor said:

AbstractUserdetailsAutheticationProvider now as two UserDetailsChecker strategies:

preAuthenticationChecks

postAuthenticationChecks

which can be injected. The default implementations follow the original behaviour.