[Security] isAccountLocked() should be moved from checkPostAuth to checkPreAuth #8510
Description
If an account gets locked (say to a user retrying too many times), the auth will still try the credentials and return the "Bad credentials" message.
Instead once the account is locked, it should never try the authentication credentials again.
The reason for this is that it allows an attacker to guess the password of a locked account by brute force based on the response error message.
The code is in UserChecker:
/**
* {@inheritdoc}
*/
public function checkPreAuth(UserInterface $user)
{
if (!$user instanceof AdvancedUserInterface) {
return;
}
if (!$user->isCredentialsNonExpired()) {
$ex = new CredentialsExpiredException('User credentials have expired.');
$ex->setUser($user);
throw $ex;
}
}
/**
* {@inheritdoc}
*/
public function checkPostAuth(UserInterface $user)
{
if (!$user instanceof AdvancedUserInterface) {
return;
}
if (!$user->isAccountNonLocked()) {
$ex = new LockedException('User account is locked.');
$ex->setUser($user);
throw $ex;
}Which I suggest to change to be this:
/**
* {@inheritdoc}
*/
public function checkPreAuth(UserInterface $user)
{
if (!$user instanceof AdvancedUserInterface) {
return;
}
if (!$user->isCredentialsNonExpired()) {
$ex = new CredentialsExpiredException('User credentials have expired.');
$ex->setUser($user);
throw $ex;
}
if (!$user->isAccountNonLocked()) {
$ex = new LockedException('User account is locked.');
$ex->setUser($user);
throw $ex;
}
}
/**
* {@inheritdoc}
*/
public function checkPostAuth(UserInterface $user)
{
if (!$user instanceof AdvancedUserInterface) {
return;
}