-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Guard] Failed authentication attempt when authenticated, de-authenticates #25992
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
To repeat this:
The cause is this code: symfony/src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php Lines 98 to 103 in f74bda5
This was put in very purposely by me. But actually, I was just following the lead from symfony/src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php Lines 178 to 180 in f74bda5
In reality, I can't think of any reason why the token should be set to null when you fail authentication. If you're still authenticated via some other mechanism (e.g. a session cookie), then you should still be authenticated. The original I think we should fix this. But, it's a behavior change: technically, someone could for some crazy reason, be relying on the fact that a failed login attempt would cause the user to be logged out. Do others agree? Would changing this behavior be a BC break? If not, the fix is SUPER easy. Otherwise, we'll need to add some flag for the new behavior, and trigger a deprecation warning if the flag is not set. |
My guess is that in a lot of cases, the token would've been NULL (never set in the exception), and thus never triggered this, but that's the best I could come up with. |
Doesn't this happen when elevating by requiring |
Hmm. Well, your session would be hijacked in that case, it would be the remember me cookie that was hijacked. But, fair statement. Though, I'm not convinced that you would want to be logged out anyways. Without the password, I still couldn't access any |
I'm not sure whether there's any golden rule to this, but I would myself implement this as it is done now, assuming that the most recent authentication is always leading. If you can't confirm your identity now - don't trust the previous confirmation either. Perhaps we should have a bit of research on how big common sites solve this? |
@curry684 but your identity is confirmed, hence the Also note that in my previous setup with Simple* authenticators, this worked just fine |
Yes I know that the behavior is inconsistent, the question is now whether the Simple* authenticators are 'wrong' or the Guard. I see it like this: when I go the airport I have to show my passport multiple times. The first time I use it to authenticate myself I am given a boarding pass (a 'token') which implicitly confirms my identity. When boarding the plane my passport is checked again, and if it is rejected my boarding pass becomes untrusted automatically. Authentication is, imho, only ever as valid as your most recent attempt. |
I disagree with that statement. I've already confirmed that the user is authenticated. In this scenario, a typo in the password would throw away the previous authentication, rather than leaving it at the current level. The previous authentication was correct, only the new one is incorrect. Your scenario also seems incorrect. You first identify with a passport, but if your boarding pass is rejected, that doesn't automatically invalidate your passport. |
Correct, and intentional on my part.
It's the other way around - if my passport is rejected, it invalidates my boarding pass. That makes sense as it casts doubts on how I got the boarding pass in the first place. I don't think the two of us should be arguing here, I see your point and I'm not even really opposed to it. I would just do it like it's done currently myself as I'm in the strictness-above-all camp where security is concerned. I think it's up to others to decide "the Symfony way", and based on that we should either fix this one or the Simple authenticators. |
Except that if your passport was already checked and confirmed, it should yield the same result next check, otherwise the first check can't be trusted. My first check can be trusted, thus I expect my authenticator to work like this. Security and usability often conflict. The better the security, the less user friendly a system becomes. However, in this scenario it doesn't increase or decrease security, it only hinders usability. |
What you are experiencing seems more like an unfortunate side effect of code that is in itself suitably strict. The scenario you're describing isn't "your average web site with a customer login" anymore 😉 |
Let's take a look at a real example from big websites: If you are trying to perform a checkout at Amazon, you will have to reauthenticate. If you fail to do so (maybe because of a typo), you won't be logged out immediately, but you can try to login a again. In my opinion that's the expected behaviour. Just because you made a typo when typing your password shouldn't mean that the whole session should be destroyed. |
…rd (iltar) This PR was merged into the 2.8 branch. Discussion ---------- [Security] Fixed being logged out on failed attempt in guard | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25992 | License | MIT | Doc PR | ~ This fixes the issue described in the ticket. After this fix, guard will no longer "forget" your authentication when your next attempt fails. Commits ------- 4fc0ecb Fixed being logged out on failed attempt in guard
My scenario:
/hash/{hash}/
The issue I have now that I'm using guard instead of Simple*, is that every failed authentication attempt, triggers a de-authentication. That means that instead of staying logged in and showing the error message that the password was wrong, the user is logged out and has to fill in both password and username.
This is really simple to reproduce, just try to login with invalid credentials when logged in, and it will de-authenticate you.
For this particular case, it can be solved by removing the following lines: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Guard/GuardAuthenticatorHandler.php#L100-L103
I think this is a bug, as it's really weird behavior to de-authenticate a user this way. I can provide a PR to fix this in 2.8, otherwise it has to be fixed with a deprecation in 4.1 (which would take too long for my issue though).
2317443
The text was updated successfully, but these errors were encountered: