Close #213: honor post-success listeners that may block authentication

[giosh94mhz]

Description

Symfony’s standard authentication includes several "post-checks" that run immediately after a successful login, most notably TwoFactorAuthenticator::onAuthenticationSuccess.

Starting with Symfony 7.x and 2fa-bundle 7.x (and possibly others), these listeners can generate a loop-redirect, as described in #213.

The Issue

The root cause is that TwoFactorToken marks a provider as completed during badge verification. If an error occurs after this stage, the entire login process is left in an inconsistent state for several reasons:

1. There is no "current provider," even if the system is still processing a provider (e.g., TOTP).
2. At this point, the stored providers (i.e., getTwoFactorProviders) are empty, so the active token in TokenStorage is inconsistent
3. If authentication fails, the current authentication token is already switched and the previous inconsistent token remains in storage.
4. Subsequent requests then generate a new "prepared providers" list, but no providers are available, triggering the failure loop.

The Solution

During badge verification, providers must not be marked as complete in the TwoFactorToken, as this happens too early in the process. This is critical to keep TwoFactorTokenInterface::getCurrentTwoFactorProvider stable throughout the request and avoid the loop issue.

Instead, we need to identify another point to mark the TwoFactorToken provider as complete—ideally, at the very end of the authentication process.

By examining the Symfony\Component\Security\Http\Authentication\AuthenticatorManager::executeAuthenticator class, I see that the AuthenticationEvents::AUTHENTICATION_SUCCESS event (where UserCheckers and other checks run) is considered part of the standard process, as it is within the try-catch block. Immediately after this event, $authenticator->onAuthenticationSuccess is called, making it the right place to mark the flow as completed.

Therefore, the solution is to mark the current provider for any TwoFactorToken that reaches TwoFactorAuthenticator::onAuthenticationSuccess as completed—not before.

Tests

I ran many tests in my application, which uses both stateful and stateless authenticators, as well as multiple post-authentication locks, and everything seems to work. However, we only have one OTP method, so the code for handling multiple OTP methods and $this->dispatchTwoFactorAuthenticationEvent(TwoFactorAuthenticationEvents::REQUIRE, $request, $token) still needs some testing.

Minor Breaking Changes

I foresee two minor breaking changes:

1. Updating users may experience changes in TwoFactorTokenInterface::getCurrentTwoFactorProvider, e.g. from null to totp (though this is likely a fix rather than an issue, IMO).
2. The TwoFactorTokenInterface::allTwoFactorProvidersAuthenticated method will likely never return true, since the previous token forgot after onSuccessfulAuthentication.

The main struggle with TwoFactorTokenInterface is that the setTwoFactorProviderComplete method actually removes a provider from the token, which can cause glitches. My patch addresses this by requiring the authenticator to estimate the "on complete" status by comparing getTwoFactorProviders and the active providers.

Regarding point 2, I may update the TwoFactorToken inside the authenticator success so that TwoFactorTokenInterface::allTwoFactorProvidersAuthenticated is updated for potential external classes still referencing the token and triggering in post-success events, like TwoFactorAuthenticationEvents::COMPLETE.

In a future release, TwoFactorTokenInterface may need a cleanup or an improvements to avoid this edge cases.

[scheb]

I'm wondering where in the process the provider is marked as completed, if it's no longer happening in AbstractCheckCodeListener.

Should this line actually be un-commented?

[giosh94mhz]

You are right to wonder, and the answer is... actually never. (I tried to explain it in the issue, hopefully I passed the language barrier 😄 )

We can't uncomment that line, that's why I explicitly added it so it's not attempted in the future. If you do, you get the same looping bug, since "post-auth" lister are executed right after the token is switched.

The correct place is always TwoFactorAuthenticator::onSuccessfulAuthentication, but at that point we no longer have the original TwoFactorToken only the SymfonyToken (or whatever token). So we have three solution:

1. doing nothing, as the token will be forgotten right away (also, the TokenStorage no longer contains it)
2. keep a local map like "SymfonyToken => TwoFactorToken", so that we can retrieve TwoFactorToken
3. add a temporary attribute on SymfonyToken like "otp_completed_token = TwoFactorToken" , and reset it after use on success event

I've (not) done solution 1 for the sake of simplicity, but we may implement solution 3 (my favorite) or solution 2 (more fair with SymfonyToken) if you prefer.

-- edit fixed typo

[scheb]

I understand the problem and I also understand what you're trying to do.

I believe you implemented and tested this solution under the assumption that there is only one provider that needs to pass. But if you have multi_factor: true activated on the firewall, all available providers need to pass, before the MFA phase is completed. That's why it's necessary to flag providers as completed, so that MFA can proceed with the next provider, until all have passed. If you don't do that, you'll end up in an endless loop with multi_factor: true enabled. You can actually try that out by running the test application in the app folder. You'll see that you're stuck in an endless loop, asking to enter the code for the email provider over and over again.

Related, this statement is actually not true:

The correct place is always TwoFactorAuthenticator::onSuccessfulAuthentication, but at that point we no longer have the original TwoFactorToken only the SymfonyToken (or whatever token).

You can indeed have a TwoFactorToken in this method. That happens when multi_factor: true is configured and the there are still providers left. See TwoFactorAuthenticator.php:139. So maybe the solution is to flag the provider as completed in this method?

---

Side note on the test application in app: This is also where integration tests are located. Your changes are breaking some integration tests. May be related to the problem that I've just described with multi_factor: true. In any case, I highly recommend to run the integration tests to validate to check for any regressions.

[giosh94mhz]

Sorry for the late reply, I prefer to answer with some code 😄

I've found how to run Symfony phpunit tests inside the project, didn't noticed the double configuration and so I missed the test. The failures were caused by a missing line in my patch I submitted, in my local code I had already set a provider as completed right before the "REQUIRE" event, thus fixing the whole multi-factor authentication. You were right, in the code I submitted an infinite loop will trigger.

Regarding the "on complete" status of the TwoFactorToken iteself, I've implemented in a separate commit the solution I described as "2", that is using a temporary attribute to remember the token and fixing it right before the event. Not very proud of this commit, should you prefer other solutions let me know.

[giosh94mhz]

...and fixed all the tests again. Sorry for the noise.

[scheb]

Overall, looks good, thanks for the effort!

Please remove commented out code fragments for the final, clean implementation.

[scheb]

I'd rather go with approach count($token->getTwoFactorProviders()) <= 1 to express "this is the last provider".

Also, I would comment "The current provider is the last provider. It will be completed in onAuthenticationSuccess." to make the intention clear for future reference.

[scheb]

I agree, this part isn't pretty. Feels like a "hack".

I'd say we don't need this necessarily. When the 2fa token is completed, the TwoFactorToken is replaced by the authenticated token. So changing the "complete" state for the last provider is not really necessary, since we're ditching the TwoFactorToken anyways. I checked, if we're removing this part, 2fa is still working fine, also in the multi-factor case. The important part is to call setTwoFactorProviderComplete in line 145.

So I'd say we're going with your "option 1" for when the 2fa process is finally completed:

doing nothing, as the token will be forgotten right away (also, the TokenStorage no longer contains it)

This will introduce a this small behavioral change, that you have previously described:

Updating users may experience changes in TwoFactorTokenInterface::getCurrentTwoFactorProvider, e.g. from null to totp (though this is likely a fix rather than an issue, IMO).

I'd be okay with that. Also agree, that this may rather be a "fix".

[giosh94mhz]

Please remove commented out code fragments for the final, clean implementation.

I removed most of the comments and updated the one you pointed out

count($token->getTwoFactorProviders()) <= 1

Had to check 1 === count($token->getTwoFactorProviders()) as some other test failed otherwise

So I'd say we're going with your "option 1" for when the 2fa process is finally completed: [...]

Once I implemented the "token trick" I was no longer sure it was a good idea of doing nothing until the next "major". It's a minor BC, but I don't like to break other people code.

keep a local map like "SymfonyToken => TwoFactorToken", so that we can retrieve TwoFactorToken

I've done the above solution (option 2), that is fully encapsulated in TwoFactorAuthenticator.

IMO, we may flag method allTwoFactorProvidersAuthenticated as @deprecated so that in the next major you may remove it, and revert my 2nd commit with the trick.

What do you think?