Don't overwrite an already two factor secret unless force = true

[danmatthews]

Add a force parameter that only overwrites existing two factor secret keys and recovery codes in the database if present when calling the enable two factor authentication action.

Why

If the secret has already been set, the QR code may have already been shown to the end user, and this endpoint is erroneously called again - through a page refresh or some JS issue, the secret is overridden, rendering the code the end user just scanned useless without informing them that anything has changed.

Mitigation

When calling the /user/two-factor-authentication endpoint, the code now checks to see if the two_factor_secret is not null and assumes that it shouldn't overwrite it to prevent situations like this.

To force it to overwrite, you can pass the force parameter - which will fill out the two factor columns with new values as it did previously.

The note here is to focus on intent - a piece of code should only overwrite this through the intent of the user (maybe clicking a button that refreshes the QR code before scanning it) or through some app-level action like resetting a user's twoFA status.

[github-actions]

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

[driesvints]

Can you please tell us more about your use case?

[danmatthews]

@driesvints so we've just been bitten by this.

We're using Inertia and Fortify, and a condition on our 2FA setup screen meant that the axios call to enable two factor auth was sent again AFTER two factor had been confirmed by the user.

This meant that the code they entered on the confirmation screen worked, but after they had been logged in, their 2fa secret changed, and upon returning, they couldn't log in.

While this was a bug on our end, I think we should enforce that the secret should not be changed or filled out if it is not null - meaning the user is either in the middle of confirming, or has already confirmed, in both cases, the person has likely already scanned the QR code, and this shouldn't be changing without specific intent.

This PR:

• Makes the default behaviour to NOT overwrite.
• Allows the user to pass force=true in the POST request to force overwriting with intent.

Feedback welcome!

Will fix tests this evening as i'm currently still at work and getting back to the users who were affected by this.

Thanks.

[taylorotwell]

Can you elaborate why this was called twice in your code?

[danmatthews]

@taylorotwell sure - upon clicking a button to open a modal to enable two factor auth in your account, the onMount() would send a request to the 'enable' endpoint and upon receiving a 200 response, would use Inertia's partial reloads to load the QR code SVG.

Because we have URL based persistent modals, refreshing the page after scanning the QR code would call the endpoint again, resulting in the secret being overridden, but the user would have already scanned it.

It took us a LONG time to track this behaviour down as it was such an edge case as a result of user behaviour - but it meant that people would be locked out.

We were surprised to find that multiple calls to the enable endpoint didn't check to see if it had already been 'activated' and overwrote the secret. and that's what resulted in this PR - I personally think the enable endpoint should be non destructive unless 'forced', or there should be a separate 'refresh' endpoint.

Actually, 'refresh' might be a better parameter / way of putting it than 'force'.