WordPress / two-factor

Two-Factor Authentication for WordPress.
https://wordpress.org/plugins/two-factor/
GNU General Public License v2.0
705 stars 145 forks source link

Unset session data when disabling all providers to prevent fatal #566

Closed iandunn closed 1 year ago

iandunn commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

iandunn commented 1 year ago

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 commented 1 year ago

Fixed by https://github.com/WordPress/two-factor/pull/567