Unset session data when disabling all providers to prevent fatal

[iandunn]

Fixes #565

Unsets the 2FA session data when all providers are disabled, so that the user's current session is no longer considered a 2FA session. That avoids the fatal error, and allows them to enable a provider again.

Testing Instructions

Repeat the reproduction steps from #565.

Changelog Entry

None needed, because #529 has not been released yet.

[iandunn]

This is working in my tests for wp-admin, but not yet for 3rd party clients that use the REST API endpoints.

Maybe we should provide a new endpoint to update the session, and it'd be their responsibility to call it? It seems like an internal detail that should be handled automatically, though 🤔

[dd32]

What about changing is_current_user_session_two_factor() to be false when the user has no 2FA providers active?

Another other option would be to alter current_user_can_update_two_factor_options():
https://github.com/dd32/two-factor/blob/9032f2d825623df58542e4f9d8968bc86b95698a/class-two-factor-core.php#L1126-L1129

 		// If the current user is not a two-factor user, not having a two-factor session is okay.
-		if ( ! self::is_user_using_two_factor( $user_id ) && ! $is_two_factor_session ) {
+		if ( ! self::is_user_using_two_factor( $user_id ) ) {
 			return true;
 		}

This feels somewhat expected, I'm not sure why the additional conditional is included there

[dd32]

I've taken this in a slightly different direction via #567 but adding my thoughts inline here.

[dd32]

In the context of the two-factor plugin as it stands today, the provider being setup/reset probably doesn't warrant the session changing.

My reasoning for that is that these only control the Provider details, it doesn't affect the actual enabled providers for the user, even though they should likely be far more tightly coupled. The UI and design of two-factor has them as very distinct components, where as wporg-two-factor has them far more coupled.

[dd32]

Perhaps converting the XHR endpoints to REST API methods was poorly thought out, and instead it should've been something more generic, which ran through a centralised Two_Factor_Core handler that decided what the request did, removing the providers from the equation.

# Details about 2FA
GET /me/two-factor
{ enabled: [ two_factor_dummy ], available: { two_factor_email: [ param1, param2 ] } }

# Setup a 2FA
POST /me/two-factor
{ provider: two_factor_totp, { param1: ..., ... } }

# Reset / Deactivate
DELETE /me/two-factor
{ provider: two_factor_dummy }

[dd32]

The main intention would not to extend their 2fa grace time by simply constantly enabling and disabling an additional provider I think

[dd32]

I guess at the time I thought something like "The fact the user is no longer logged in with 2FA doesn't suddenly make their session not a 2fa session", but somewhere along the line with the logic changes I've missed that they couldn't then update the session after disabling it..

[iandunn]

What about changing is_current_user_session_two_factor() to be false when the user has no 2FA providers active?

That way my first approach, but it felt like a smell to leave the the raw session data in an incorrect state. I worried that it could lead to confusion and maybe even bugs in the future.

...alter current_user_can_update_two_factor_options() [to remove && ! $is_two_factor_session]

That seems fine to me regardless 👍🏻

[iandunn]

Fixed by #567