Verification interruptions can break MFA flow

[Cheddam]

Module version(s) affected

All

Description

SessionStore performs a sanity check when setting the active MFA method, throwing an exception if the given method has already been verified. This seems fairly innocuous, and generally has no impact (in fact, it doesn't even work during session hydration as the verified methods are set after the current method.)

However, there's an edge-case where it does trigger a failure: If a user hits the 'start verification' API for a method they have already verified. It's effectively impossible to reproduce this scenario under normal operating conditions, but someone managed to hit it on a production environment.

Once this failure has triggered, the user is presented with a permanent loading state in the MFA UI, and is forced to manually restart the login process.

How to reproduce

Tested in an environment with the TOTP method previously registered for a user.

You'll need to trigger an unexpected response from the verification endpoint after verification has completed - for example, simulating a gateway timeout on this line:

return $this->jsonResponse(['error' => 'Gateway timeout'], 502);

1. Log in with username/password and reach the TOTP verification UI
2. Enter a valid TOTP token, see the 'Unknown error' message triggered by the gateway timeout.
3. Try refreshing the page, receive a permanent loading screen (with a 500 response from the start verification API endpoint.)

Possible Solution

My PR simply drops the sanity check in SessionStore, because it's superfluous - the verification process is sufficiently idempotent and safeguarded to avoid any risks this logic would otherwise protect against. For example, SessionStore::addVerifiedMethod already avoids adding previously-verified methods, so it's not possible for a user to bypass multi-phase MFA (if this was even configured) by triggering multiple verifications of the same method.

I wouldn't usually advocate for removing sanity checks, but in this case it can be actively harmful to UX.

The alternative fix would be to actually catch and handle this exception, but that would be a lot more work and introduce further complexity - we'd need to signal to the UI that it should skip past the current verification method, so both the API and UI would need additional logic.

Additional Context

No response

Validations

• Check that there isn't already an issue that reports the same bug
• Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

Prs

• FIX Drop already-verified check from SessionStore #590

[GuySartorelli]

It's effectively impossible to reproduce this scenario under normal operating conditions

This comment stands out to me..... firstly how did they do it then? And secondly, this suggests to me that probably nothing needs to change?

[Cheddam]

I can't be completely sure of how the user encountered this bug, but based on the stack trace and how I reproduced it, I suspect the CDN or the user's connection flaked out in some way.

To me, this falls under the same category as the session renewal adjustments we covered last year - whilst the framework / modules can't prevent connection failures, they should do their best to sensibly recover from them.

[GuySartorelli]

Sorry I've been a bit slow to respond here. The recommended change feels like it could have unexpected side effects so I'll need to dedicate more time than I currently have to review it.