Hi @bananer, would you be able to provide more information on your configuration and how someone could look into this? Perhaps a fork of https://github.com/symfony/symfony-standard ?

Interesting, I would not have thought that would be the order things happen with authentication, but it is and seems to be by design. This happens in the UserChecker in checkPreAuth():

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/User/UserChecker.php#L29

And checkPreAuth() is called is called in the UserAuthenticationProvider prior to the authentication happening (As the name of the function would state I suppose...hah):

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php#L85

Telling whether the account is locked, disabled, or expired probably should only occur if authentication is verified (from a security perspective).

Actually, it seems that those checks used to belong to the checkPostAuth method. @fabpot changed this behavior in #9902.

I'm currently facing the same problem, so I assume this Security enhancement has not been looked at in the past year. Would it be possible to schedule this in a future update? It would be fantastic.

@popovicidinu Can you tell us more about the "problem" you are facing?

I have a User class that implements the AdvancedUserInterface. My User has a boolean field (isEnabled) via which I check if the user account is enabled. My authentication is done by using an email/password combination.

If I try to login with a "disabled" account, no matter what password I use, the authentication system will return "This account is disabled.". From what I've read in the previous comments, it seems that the check for "isEnabled" is done prior to verifying credentials.

Whilst this is not a problem itself, it does open up the system to a small privacy leak. For example, if someone would know the email addresses of the users, they could try and see if they're accounts are disabled or not.

@popovicidinu That was exactly my thinking as well. I think this makes it easier to tell what accounts an attacker should focus on. I think by default it should return a "Bad Credentials" message unless the credentials used were valid and the account is disabled, then it should actually display that the account is disabled.

This is somewhat configurable by setting the hide_user_not_found message for the form login that replaces all of the specific messages with with a "Bad Credentials" message. However, this is an all or nothing approach as it also hides the "User account is disabled" message even for the user that entered the correct password, which is more helpful to know than "Bad Credentials" in that case.

My two cents anyway.

@ChadSikorra good two cents!

One great thing about symfony is that you can customize things, so you can implement a custom user checker as per http://symfony.com/doc/current/security/user_checkers.html

From there you can put back the logic that was change as part of #9902 or customize it in any other way to make it suit your needs.

#8510 was a valid security concern: if an account has been locked because someone is trying to brute force the password, you shouldn't allow them to keep trying different passwords. The fix in #9902 seems too far reaching however, and the only justification is that another project does it that way. There is no understanding of why spring does it that way or how it came to be different in Symfony.

It's the expired check that is causing a problem for me. In my login controller I have the following:

$authenticationUtils = $this->get('security.authentication_utils');
$error = $authenticationUtils->getLastAuthenticationError();
if ($error instanceof AccountExpiredException) {
    // display subscription renewal form
}
else {
    // render login form and any error messages
}

The intent is that after identifying themselves, if a user's account has expired they should be immediately asked to renew it. However, because AccountExpiredException is shown preAuth, anyone can bring up an expired user's renewal form just by knowing their username. This renewal form might leak personal information not intended to be shown to unauthenticated users, but in my case it's just a button sending you to PayPal. I therefore had a user contact me today who had mistakenly entered someone else's expired username and renewed the other user's subscription instead of their own!

Unless anyone has a good reason why you'd need to prevent an expired (or disabled) account from checking their password I think the expired (and disabled) checks should be moved back to postAuth.

Note we are in the process of deprecating the AdvancedUserInterface #23508

The best solution is to create your own UserChecker and stop using the "default" implementation.

Hi,
@sstok I was checking the official doc of symfony.
https://symfony.com/doc/current/security/user_checkers.html
it's seems like we can not customize user checker class with symfony2.7 .
Any Idea?
thanks

@persiasama The easiest move would be to update to Symfony 2.8. Otherwise, you will have to dig into what the SecurityBundle is doing and replace some internal services so that your custom user checker is used.

I'm using SF 4 and this problem still exists. isAccountNonLocked() method should be checked before authentication and should stay in checkPreAuth but rest of methods (isEnabled() and isAccountNonExpired()) maybe should be moved to the checkPostAuth?

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

I haven't tested it on the recent version of symfony, but just looking at the code https://github.com/symfony/symfony/blob/9f5d178/src/Symfony/Component/Security/Core/User/UserChecker.php#L47 it sure seems like it's still the same.

Fixed in 2a581d2.

@chalasr Can you explain how this fixes the issue? It's still checked in "checkPreAuth", but now even from the Controller we don't know if it's because of wrong password or account disabled. How is it intended to be used in the Controller or wherever?

We'd still like to tell the user that the account is disabled but only IF he entered the correct password...

@LorisZ Indeed, only the risky part of the issue has been fixed i.e:

This means anybody can find out whether another user's account is enabled or not. Seems like a (minor) privacy leak to me.

For the remaining (checking account status before or after authentication happens), if one needs a more fine-grained error handling, one can create a custom user checker to perform the checks one needs.
Defining a custom user checker is supported since Symfony 2.8 (see https://symfony.com/doc/current/security/user_checkers.html) and is strongly recommended since 5.3 where the default user checker has been renamed to InMemoryUserChecker in order to highlight that it should only be used for testing purposes (see #40443).

Also, you can opt-out from obfuscating the real error message on invalid account status by setting the security.hide_user_not_found to false, but beware that doing so means that you are on your own and potentially exposing your app to user enumeration based attacks.

Since 5.2, an other way to control the error message displayed to the end user is to throw a CustomUserMessageAuthenticationException or CustomUserMessageAccountStatusException.

As said above, the account status checks have been moved to checkPreAuth for valid reasons (which you can find in #9902 and the related tickets).
We won't move them back for the very same reasons, and I rather suggest to write a custom user checker adapted to your User implementation and your business requirements.

Changing the behavior on this area is really tricky, I hope everyone can understand that issues like this one cannot be easily fixed since those are design issues, and that the best thing we can do is to improve the situation on newer versions, that is what we are trying to do.

@chalasr I haven't tested it properly yet, so forgive me if I missed something, but it feels like this change only made it so that Disabled/Locked Exception are always silent no matter if it was thrown before or after authentication (given that hide_user_not_found flag is set to true).

This way we can't seem to use custom user checked to only trigger disabled/locked notification after validating password (except using CustomUserMessage).

Please. correct if I am wrong.