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.