Fix Sign in with Google for accounts using the WordPress Two-Factor plugin#13118
Fix Sign in with Google for accounts using the WordPress Two-Factor plugin#13118shervElmi wants to merge 9 commits into
Conversation
When the Two-Factor plugin is active, turn off two-factor authentication for accounts that Sign in with Google creates, and return an error instead of linking an existing account that already uses two-factor. An account created with two-factor left on breaks the later sign-in, and linking one would remove its challenge while its password still works.
Cover the new authenticator behavior with a test double that fakes the Two-Factor plugin state, since the test environment has no Two-Factor plugin.
Confirm the login screen adds the two-factor message for the new error code, and leaves an unrecognized code untouched.
🎭 Playwright reports for e6f2957: 📦 Build files for e6f2957:
|
Modify the assertion in the login error handling test to check for the specific error message related to existing accounts using two-factor authentication. This ensures that the test accurately reflects the expected behavior when two-factor authentication is enabled.
Update the `authenticate_user` method to streamline user creation logic and enhance readability. Adjust the `user_has_two_factor` method to simplify the conditional checks for two-factor plugin activation.
This change allows users to sign in with Google without being blocked by the Two-Factor plugin's login challenge. It modifies the Authenticator class to remove the two-factor login check during authentication. Additionally, tests are updated to verify this behavior for both new and returning users.
| if ( $this->is_two_factor_plugin_active() ) { | ||
| $two_factor_challenge_priority = has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ); | ||
| if ( false !== $two_factor_challenge_priority ) { | ||
| remove_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), $two_factor_challenge_priority ); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is not very reliable because it relies on the method name in the Two_Factor_Core class, which may be renamed at any point in the future. I think what we can do is override the enabled providers for the current user.
add_filter( 'two_factor_enabled_providers_for_user', function( $enabled_providers, $user_id ) use ( $user ) {
return $user_id !== $user->ID ? $enabled_providers : array();
}, 10, 2 );We can use this filter unconditionally because if the 2FA plugin is not installed, this filter will never be executed and will just be ignored. So, is_two_factor_plugin_active and related functions are not needed.
| // Don't link an account that has two-factor authentication enabled, | ||
| // as signing in with Google would bypass the account's two-factor challenge. | ||
| if ( $this->user_has_two_factor( $user->ID ) ) { | ||
| return new WP_Error( self::ERROR_TWO_FACTOR_ENABLED ); | ||
| } |
There was a problem hiding this comment.
Can you remind me why we shouldn't link an account? I don't see why it can be bad.
There was a problem hiding this comment.
If we link the account automatically and it has two-factor turned on, the person can sign in with Google and skip the second code. Two-factor is there so the password alone isn't enough. Linking would let someone in without that second step. The user can still link Google from their own profile page. So we only block the automatic link, not the one they do themselves.
AC says linking would "remove 2FA". That was true, which changed the account and turned two-factor off. But now doesn't change anything. It only skips the check for that one Google sign-in.
There was a problem hiding this comment.
If we link the account automatically and it has two-factor turned on, the person can sign in with Google and skip the second code.
Yes, if the user signs in using Google OAuth, then they won't need to enter their 2FA code. That is what happens when we call skip_two_factor_for_user in the sign_in_user method. So, I don't see why we shouldn't link Google and WordPress accounts automatically if 2FA is enabled.
AC says linking would "remove 2FA".
I doubt this is a valid statement. When we link accounts, we just save Google user ID in the user metadata. That's all that happens here, and it can't remove 2FA for that user.
I think we need to remove this and continue automatically link user accounts.
There was a problem hiding this comment.
Oh... I think I got what you mean. So, we want to prevent users from signing in using their Google accounts if their accounts already have two-factor authentication enabled. But if they manually connect a Google account in the admin, they should be able to sign in with Google. Right?
There was a problem hiding this comment.
Here's a small difference. The block is on linking, not on signing in.
This change modifies the authentication process to bypass the Two-Factor plugin's login challenge when users sign in with Google. It ensures that the two-factor providers are cleared for the signed-in user while leaving other users' settings intact.
| protected function is_two_factor_plugin_active() { | ||
| return class_exists( 'Two_Factor_Core' ); | ||
| } |
There was a problem hiding this comment.
I don't think we need this one-line method that is used only once in another method in this class. Let's call class_exists directly in the user_has_two_factor method.
| protected function skip_two_factor_for_user( $user ) { | ||
| add_filter( | ||
| 'two_factor_enabled_providers_for_user', | ||
| function ( $enabled_providers, $user_id ) use ( $user ) { | ||
| return $user_id === $user->ID ? array() : $enabled_providers; | ||
| }, | ||
| 10, | ||
| 2 | ||
| ); | ||
| } |
There was a problem hiding this comment.
Same here, one-line method used only once doesn't make too much sense. Let's add the filter directly in the sign_in_user method.
| function ( $enabled_providers, $user_id ) use ( $user ) { | ||
| return $user_id === $user->ID ? array() : $enabled_providers; | ||
| }, |
There was a problem hiding this comment.
Let's use an arrow function to make this a bit more concise.
| protected function user_has_two_factor( int $user_id ) { | ||
| return $this->is_two_factor_plugin_active() && \Two_Factor_Core::is_user_using_two_factor( $user_id ); // @phpstan-ignore class.notFound (Two_Factor_Core comes from the optional Two-Factor plugin, and is_two_factor_plugin_active() confirms it exists.) | ||
| } |
There was a problem hiding this comment.
So, if we want to prevent users from signing in with their Google accounts if two-factor security is enabled and their accounts are not linked, then we need to check whether their accounts are linked to correctly validate whether the user can sign in or not.
| $error->add( self::MODULE_SLUG, __( 'The user is not registered on this site.', 'google-site-kit' ) ); | ||
| break; | ||
| case Authenticator::ERROR_TWO_FACTOR_ENABLED: | ||
| $error->add( self::MODULE_SLUG, __( 'An existing account using two-factor authentication was detected with that email address. To connect an account using Sign in with Google, two-factor auth must be disabled for your user account.', 'google-site-kit' ) ); |
There was a problem hiding this comment.
We need to rephrase this message to correctly inform users what they need to do.
| if ( $user ) { | ||
| // Don't link an account that has two-factor authentication enabled, |
There was a problem hiding this comment.
Let's flip this if statement to return early if the user is not found:
$user = get_user_by( 'email', $payload['email'] );
if ( ! $user ) {
return null;
}
if ( $this->user_has_two_factor( $user->ID ) ) {
return new WP_Error( self::ERROR_TWO_FACTOR_ENABLED );
}
$user_options = clone $this->user_options;
$user_options->switch_user( $user->ID );
$user_options->set( Hashed_User_ID::OPTION, $google_user_hashed_id );
return $user;| do_action( | ||
| 'googlesitekit_render_sign_in_with_google_button', | ||
| array( | ||
| 'class' => 'googlesitekit-sign-in-with-google__existing-user-connect-button', | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Here we render the sign-in button that allows users to connect existing accounts with Google accounts. This won't work in the current implementation because we will return an error saying that existing accounts have 2FA enabled.
What we need to do is to set a transient for this user that will indicate that the user wants to connect their accounts and check that transient in the find_user method to bypass user_has_two_factor check if the user has this transient.
…-factor-account-handling.
Summary
Addresses issue:
Relevant technical choices
TWO_FACTOR_ENABLED_PROVIDERS_META_KEYconstant for the_two_factor_enabled_providersmeta key instead of repeating the literal.FakeTwoFactorAuthenticatorfile instead of an inline subclass.Authenticator, soWooCommerce_Authenticatorinherits it.authenticate_user()instead of a duplicate one.user_has_two_factor()and the test double's override as single returns.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist