-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Disabled account is shown "Bad credentials" error #42793
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.
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
Comments
Won't something like this work in your login controller?
|
It doesn't work, Also there is another common use case, if the account is not active because they haven't verified their email address. |
I think this should stay "safe" by default and not reveal any infos. Does https://symfony.com/doc/current/reference/configuration/security.html#hide-user-not-found help? Update: sorry, just saw you tried that. I still think changing this might be problematic |
@fmonts not sure if this is still an issue but I had a similar problem the other day where I wanted to customize the "Bad credentials" message. What I did to change the error message was to modify my public function onAuthenticationFailure(Request $request, AuthenticationException $exception): Response
{
if ($exception instanceof BadCredentialsException)
{
$exception = throw new CustomUserMessageAuthenticationException('Invalid email or password', [], 0, $exception);
}
return parent::onAuthenticationFailure($request, $exception);
} In your situation, if I understand your issue correctly, you could do something similar but changing This way you can keep your login route short and clean #[Route("/login", name: "login", methods: ["GET", "POST"])]
public function login(AuthenticationUtils $authenticationUtils): Response
{
$error = $authenticationUtils->getLastAuthenticationError();
$lastUsername = $authenticationUtils->getLastUsername();
return $this->render('security/login.html.twig', [
'error' => $error,
'last_username' => $lastUsername
]);
} |
Hey, thanks for your report! |
Could I get an answer? If I do not hear anything I will assume this issue is resolved or abandoned. Please get back to me <3 |
Hey, I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen! |
After upgrading to Symfony 5.4, I've run into this problem as well. If there is an account that is locked or inactive for one reason or another, the user gets the 'Invalid credentials' error message. I'm not sure if I have something setup incorrectly, but it is definitely not behaving the same way as it did in previous versions. Looking at the code in AuthenticatorManager#handleAuthenticationFailure:271 it does seem like a bug or at least a feature with a misleading name: hideUserNotFoundExceptions. Replacing that conditional block with this made it work as expected and as previously:
I would expect such a feature to only hide exceptions when the username is not found, not including cases where the user is found, but is inactive or locked out in some way. This used to provide useful feedback for our clients so they would not waste time trying to reset their password. I guess some could argue it is better to obfuscate the error in all cases, but this seems a bit paranoid, and again the feature seems improperly named. I would propose to either rename this or better yet, separate this into two features: e.g. hideUserNotFoundExceptions and hideAllAuthExceptions. For now, my workaround is to disable 'hide_user_not_found' (which is not the default setting), and replacing the error message in the login controller method like this:
It looks like this bit of logic is in many classes so I'm curious to understand others' thoughts on this and if it could be addressed somehow. |
I agree, the best behavior to ensure both privacy and good user experience, should be:
|
I'm using the old 5.2 Security component, since we don't have docs for the new system yet.
I noticed that when
User::isEnabled()
returns false (for example if the user is banned or not yet approved), at login the user is shown "Bad credentials" error, because of this:A
DisabledException
is thrown...symfony/src/Symfony/Component/Security/Core/User/UserChecker.php
Lines 41 to 45 in a47cf7e
Which extends
AccountStatusException
...symfony/src/Symfony/Component/Security/Core/Exception/DisabledException.php
Line 20 in e34cd7d
Which throws a
BadCredentialsException
symfony/src/Symfony/Component/Security/Core/Authentication/Provider/UserAuthenticationProvider.php
Lines 88 to 91 in e34cd7d
This was changed in May, here: https://symfony.com/blog/cve-2021-21424-prevent-user-enumeration-in-authentication-mechanisms
But I do not think this is a proper solution, if a user is banned or pending approval, and they insert the valid credentials, they shouldn't see an "Invalid credential" errors, this will make them think they forgot the password or their account has been stolen, and try to reset the password... (without success, since after the reset the same error will be shown, so they will end up contacting the admin in despair)
In my case I did this in login controller:
Which is not working anymore.
A workaround is to set
hide_user_not_found: false
, but this will cause other unwanted effects (and as per the option name, it should regard "not found" users, not "disabled" ones)The text was updated successfully, but these errors were encountered: