From 26b2705e02192d83bc96c83c5bd3880935e10c57 Mon Sep 17 00:00:00 2001 From: Sherv Elmi Date: Wed, 15 Jul 2026 14:38:37 +0300 Subject: [PATCH 1/9] Add two-factor plugin handling to Sign in with Google. 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. --- includes/Modules/Sign_In_With_Google.php | 4 + .../Sign_In_With_Google/Authenticator.php | 79 ++++++++++++++++++- 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/includes/Modules/Sign_In_With_Google.php b/includes/Modules/Sign_In_With_Google.php index 23d8fc9b3e0..eebc537e273 100644 --- a/includes/Modules/Sign_In_With_Google.php +++ b/includes/Modules/Sign_In_With_Google.php @@ -331,6 +331,7 @@ public function handle_comments_form() { * Adds custom errors if Google auth flow failed. * * @since 1.140.0 + * @since n.e.x.t Added the two-factor authentication error message. * * @param WP_Error $error WP_Error instance. * @return WP_Error $error WP_Error instance. @@ -349,6 +350,9 @@ public function handle_login_errors( $error ) { case Authenticator::ERROR_SIGNIN_FAILED: $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' ) ); + break; default: break; } diff --git a/includes/Modules/Sign_In_With_Google/Authenticator.php b/includes/Modules/Sign_In_With_Google/Authenticator.php index 6b9b430bf35..cb59531d4a8 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -36,6 +36,13 @@ class Authenticator implements Authenticator_Interface { const ERROR_INVALID_REQUEST = 'googlesitekit_auth_invalid_request'; const ERROR_SIGNIN_FAILED = 'googlesitekit_auth_failed'; + /** + * Error code used when the email-matched user has two-factor authentication enabled. + * + * @since n.e.x.t + */ + const ERROR_TWO_FACTOR_ENABLED = 'googlesitekit_auth_two_factor_enabled'; + /** * User meta key marking users created via Sign in with Google. * @@ -43,6 +50,13 @@ class Authenticator implements Authenticator_Interface { */ const CREATED_BY_META_KEY = 'googlesitekitpersistent_created_by'; + /** + * User meta key holding the two-factor providers the Two-Factor plugin has enabled for a user. + * + * @since n.e.x.t + */ + const TWO_FACTOR_ENABLED_PROVIDERS_META_KEY = '_two_factor_enabled_providers'; + /** * Nonce action used by the existing-user link flow. * @@ -96,6 +110,9 @@ public function authenticate_user( Input $input ) { $payload = $this->profile_reader->get_profile_data( $credential ); if ( ! is_wp_error( $payload ) ) { $user = $this->find_user( $payload ); + if ( is_wp_error( $user ) ) { + return $this->get_error_redirect_url( $user->get_error_code() ); + } if ( ! $user instanceof WP_User ) { // We haven't found the user using their Google user id and email. Thus we need to create // a new user. But if the registration is closed, we need to return an error to identify @@ -213,9 +230,10 @@ protected function sign_in_user( $user ) { * Finds an existing user using the Google user ID and email. * * @since 1.145.0 + * @since n.e.x.t Returns a WP_Error when the email-matched user has two-factor authentication enabled. * * @param array $payload Google auth payload. - * @return WP_User|null User object if found, null otherwise. + * @return WP_User|WP_Error|null User object if found, WP_Error if the email-matched user has two-factor authentication enabled, null otherwise. */ protected function find_user( $payload ) { // Check if there are any existing WordPress users connected to this Google account. @@ -238,6 +256,12 @@ protected function find_user( $payload ) { // Find an existing user that matches the email and link to their Google account by store their user ID in user meta. $user = get_user_by( 'email', $payload['email'] ); if ( $user ) { + // 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 ); + } + $user_options = clone $this->user_options; $user_options->switch_user( $user->ID ); $user_options->set( Hashed_User_ID::OPTION, $google_user_hashed_id ); @@ -252,6 +276,7 @@ protected function find_user( $payload ) { * Create a new user using the Google auth payload. * * @since 1.145.0 + * @since n.e.x.t Disables two-factor authentication for the new user when the Two-Factor plugin is active. * * @param array $payload Google auth payload. * @return WP_User|WP_Error User object if found or created, WP_Error otherwise. @@ -285,6 +310,8 @@ protected function create_user( $payload ) { $user_options->set( Hashed_User_ID::OPTION, $google_user_hashed_id ); $user_options->set( self::CREATED_BY_META_KEY, Sign_In_With_Google::MODULE_SLUG ); + $this->disable_two_factor_for_new_user( $user_id ); + // Add the user to the current site if it is a multisite. if ( is_multisite() ) { add_user_to_blog( get_current_blog_id(), $user_id, $default_role ); @@ -296,6 +323,56 @@ protected function create_user( $payload ) { return get_user_by( 'id', $user_id ); } + /** + * Checks whether the Two-Factor plugin is active. + * + * Detects the plugin by its Two_Factor_Core class, so any way of loading + * it (a plugin, an mu-plugin, or a Composer package) counts as active. + * + * @since n.e.x.t + * + * @return bool True if the Two-Factor plugin is active, false otherwise. + */ + protected function is_two_factor_plugin_active(): bool { + return class_exists( 'Two_Factor_Core' ); + } + + /** + * Checks whether the given user has two-factor authentication enabled. + * + * @since n.e.x.t + * + * @param int $user_id User ID. + * @return bool True if the Two-Factor plugin is active and the user has two-factor authentication enabled, false otherwise. + */ + protected function user_has_two_factor( int $user_id ): bool { + if ( ! $this->is_two_factor_plugin_active() ) { + return false; + } + + return \Two_Factor_Core::is_user_using_two_factor( $user_id ); // @phpstan-ignore class.notFound (Two_Factor_Core comes from the optional Two-Factor plugin; is_two_factor_plugin_active() confirms it exists.) + } + + /** + * Disables two-factor authentication for a newly created user. + * + * Accounts created by Sign in with Google rely on the two-factor + * authentication of the user's Google Account, so the WordPress-level + * challenge stays off to keep the Sign in with Google flow working. + * + * @since n.e.x.t + * + * @param int $user_id User ID. + */ + protected function disable_two_factor_for_new_user( int $user_id ): void { + if ( ! $this->is_two_factor_plugin_active() ) { + return; + } + + // An empty provider list turns off two-factor authentication in the Two-Factor plugin. + update_user_meta( $user_id, self::TWO_FACTOR_ENABLED_PROVIDERS_META_KEY, array() ); + } + /** * Gets the hashed Google user ID from the provided payload. * From 10af462009da96f19e272cac8a6b8feb27712097 Mon Sep 17 00:00:00 2001 From: Sherv Elmi Date: Wed, 15 Jul 2026 14:38:47 +0300 Subject: [PATCH 2/9] Add authenticator tests for two-factor plugin handling. 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. --- .../FakeTwoFactorAuthenticator.php | 53 +++++++++ .../Sign_In_With_Google/AuthenticatorTest.php | 109 +++++++++++++++++- 2 files changed, 159 insertions(+), 3 deletions(-) create mode 100644 tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php diff --git a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php new file mode 100644 index 00000000000..610c9aec8d7 --- /dev/null +++ b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php @@ -0,0 +1,53 @@ +is_two_factor_plugin_active; + } + + /** + * @inheritDoc + */ + protected function user_has_two_factor( int $user_id ): bool { + if ( ! is_array( $this->user_ids_with_two_factor ) ) { + return parent::user_has_two_factor( $user_id ); + } + + return in_array( $user_id, $this->user_ids_with_two_factor, true ); + } +} diff --git a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php index 1316f1cbe7b..d124db7d3b2 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php @@ -75,17 +75,28 @@ public function tear_down() { $_POST = $this->post_data; } - private function do_authenticate_user( $profile_reader_data = array() ) { - $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); + private function create_mock_profile_reader( $profile_reader_data ) { $mock_profile_reader = $this->getMockBuilder( Profile_Reader_Interface::class ) ->setMethods( array( 'get_profile_data' ) ) ->getMock(); $mock_profile_reader->method( 'get_profile_data' )->willReturn( $profile_reader_data ); - $authenticator = new Authenticator( $user_options, $mock_profile_reader ); + + return $mock_profile_reader; + } + + private function do_authenticate_user( $profile_reader_data = array() ) { + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); + $authenticator = new Authenticator( $user_options, $this->create_mock_profile_reader( $profile_reader_data ) ); return $authenticator->authenticate_user( new MutableInput() ); } + private function create_two_factor_authenticator( $profile_reader_data ) { + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); + + return new FakeTwoFactorAuthenticator( $user_options, $this->create_mock_profile_reader( $profile_reader_data ) ); + } + public function test_authenticate_user_fails_when_profile_reader_returns_error() { $expected = add_query_arg( 'error', Authenticator::ERROR_INVALID_REQUEST, wp_login_url() ); $actual = $this->do_authenticate_user( new WP_Error( 'test_error' ) ); @@ -172,6 +183,98 @@ public function test_authenticate_user_creates_new_user_when_registration_is_all ); } + public function test_authenticate_user__errors_when_email_matched_user_has_two_factor() { + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + + $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); + $authenticator->is_two_factor_plugin_active = true; + $authenticator->user_ids_with_two_factor = array( $user->ID ); + + $expected = add_query_arg( 'error', 'googlesitekit_auth_two_factor_enabled', wp_login_url() ); + $actual = $authenticator->authenticate_user( new MutableInput() ); + + $this->assertEquals( $expected, $actual, 'Should redirect to login with the two-factor error when the email-matched user has two-factor authentication enabled.' ); + + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $this->assertFalse( + metadata_exists( 'user', $user->ID, $user_options->get_meta_key( Hashed_User_ID::OPTION ) ), + 'The Google account should not be linked to the user.' + ); + $this->assertEquals( 0, get_current_user_id(), 'The user should not be signed in.' ); + } + + public function test_authenticate_user__links_email_matched_user_without_two_factor() { + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + + $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); + $authenticator->is_two_factor_plugin_active = true; + $authenticator->user_ids_with_two_factor = array(); + + $expected = admin_url( '/profile.php' ); + $actual = $authenticator->authenticate_user( new MutableInput() ); + + $this->assertEquals( $expected, $actual, 'Should redirect to profile when the email-matched user has no two-factor authentication.' ); + $this->assertEquals( $user->ID, get_current_user_id(), 'Authenticated user ID should match the email-matched user.' ); + + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $this->assertEquals( + md5( self::$existing_user_payload['sub'] ), + $user_options->get( Hashed_User_ID::OPTION ), + 'The Google account should be linked to the user.' + ); + } + + public function test_authenticate_user__links_email_matched_user_when_two_factor_plugin_is_inactive() { + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + update_user_meta( $user->ID, '_two_factor_enabled_providers', array( 'Two_Factor_Email' ) ); + + // The plugin-active flag stays false, and no two-factor users are + // faked, so the real user_has_two_factor() runs. + $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); + + $expected = admin_url( '/profile.php' ); + $actual = $authenticator->authenticate_user( new MutableInput() ); + + $this->assertEquals( $expected, $actual, 'Should redirect to profile when the Two-Factor plugin is inactive.' ); + $this->assertEquals( $user->ID, get_current_user_id(), 'Authenticated user ID should match the email-matched user.' ); + } + + public function test_authenticate_user__disables_two_factor_for_new_user() { + add_filter( 'option_users_can_register', '__return_true' ); + + $authenticator = $this->create_two_factor_authenticator( self::$new_user_payload ); + $authenticator->is_two_factor_plugin_active = true; + + $authenticator->authenticate_user( new MutableInput() ); + + $user_id = get_current_user_id(); + $this->assertNotEmpty( $user_id, 'A new user should be created and signed in.' ); + $this->assertTrue( + metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), + 'The two-factor providers meta should be written for the new user.' + ); + $this->assertEquals( + array(), + get_user_meta( $user_id, '_two_factor_enabled_providers', true ), + 'The new user should have two-factor authentication disabled.' + ); + } + + public function test_authenticate_user__does_not_write_two_factor_meta_when_plugin_is_inactive() { + add_filter( 'option_users_can_register', '__return_true' ); + + $authenticator = $this->create_two_factor_authenticator( self::$new_user_payload ); + + $authenticator->authenticate_user( new MutableInput() ); + + $user_id = get_current_user_id(); + $this->assertNotEmpty( $user_id, 'A new user should be created and signed in.' ); + $this->assertFalse( + metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), + 'No two-factor providers meta should be written when the Two-Factor plugin is inactive.' + ); + } + /** * @group ms-required */ From 3c14e8780c06ece67bd84082ee46d5fdd21799b9 Mon Sep 17 00:00:00 2001 From: Sherv Elmi Date: Wed, 15 Jul 2026 14:38:54 +0300 Subject: [PATCH 3/9] Add a login error test for the two-factor message. Confirm the login screen adds the two-factor message for the new error code, and leaves an unrecognized code untouched. --- .../Modules/Sign_In_With_GoogleTest.php | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php b/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php index 810fff77b23..c0c4ff163ca 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php @@ -23,6 +23,7 @@ use Google\Site_Kit\Tests\Exception\RedirectException; use Google\Site_Kit\Tests\MutableInput; use Google\Site_Kit\Tests\TestCase; +use WP_Error; use WP_User; use WPDieException; @@ -671,6 +672,26 @@ public function test_handle_auth_callback_should_redirect_for_post_method() { } } + public function test_handle_login_errors__adds_two_factor_error_message() { + $_GET['error'] = Authenticator::ERROR_TWO_FACTOR_ENABLED; + + $error = $this->module->handle_login_errors( new WP_Error() ); + + $this->assertStringContainsString( + 'two-factor authentication', + $error->get_error_message( Sign_In_With_Google::MODULE_SLUG ), + 'The two-factor error message should be added for the two-factor error code.' + ); + } + + public function test_handle_login_errors__ignores_unrecognized_error_code() { + $_GET['error'] = 'unrecognized_error_code'; + + $error = $this->module->handle_login_errors( new WP_Error() ); + + $this->assertFalse( $error->has_errors(), 'An unrecognized error code should leave the error object untouched.' ); + } + protected function create_disconnect_nonce( $user_id ) { return wp_create_nonce( Sign_In_With_Google::ACTION_DISCONNECT . '-' . $user_id ); } From 54b3634d1a35c4856583d3dbb71f9998adb13862 Mon Sep 17 00:00:00 2001 From: Sherv Elmi Date: Wed, 15 Jul 2026 21:54:49 +0300 Subject: [PATCH 4/9] Update two-factor authentication error message in tests. 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. --- tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php b/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php index c0c4ff163ca..0fac66a0a33 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php @@ -677,8 +677,8 @@ public function test_handle_login_errors__adds_two_factor_error_message() { $error = $this->module->handle_login_errors( new WP_Error() ); - $this->assertStringContainsString( - 'two-factor authentication', + $this->assertEquals( + '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.', $error->get_error_message( Sign_In_With_Google::MODULE_SLUG ), 'The two-factor error message should be added for the two-factor error code.' ); From 43018c737c52cd43bc0e25690682f24f61af701c Mon Sep 17 00:00:00 2001 From: Sherv Elmi Date: Fri, 17 Jul 2026 20:41:19 +0300 Subject: [PATCH 5/9] Refactor authentication methods for improved clarity. 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. --- .../Sign_In_With_Google/Authenticator.php | 17 +++++------------ .../FakeTwoFactorAuthenticator.php | 12 +++++------- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/includes/Modules/Sign_In_With_Google/Authenticator.php b/includes/Modules/Sign_In_With_Google/Authenticator.php index cb59531d4a8..aa4d55eb0f4 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -103,25 +103,22 @@ public function __construct( User_Options $user_options, Profile_Reader_Interfac * @param Input $input Input instance. * @return string Redirect URL. */ - public function authenticate_user( Input $input ) { + public function authenticate_user( Input $input ): string { $credential = $input->filter( INPUT_POST, 'credential' ); $user = null; $payload = $this->profile_reader->get_profile_data( $credential ); if ( ! is_wp_error( $payload ) ) { $user = $this->find_user( $payload ); - if ( is_wp_error( $user ) ) { - return $this->get_error_redirect_url( $user->get_error_code() ); - } - if ( ! $user instanceof WP_User ) { + if ( null === $user ) { // We haven't found the user using their Google user id and email. Thus we need to create // a new user. But if the registration is closed, we need to return an error to identify // that the sign in process failed. if ( ! $this->is_registration_open() ) { return $this->get_error_redirect_url( self::ERROR_SIGNIN_FAILED ); - } else { - $user = $this->create_user( $payload ); } + + $user = $this->create_user( $payload ); } } @@ -346,11 +343,7 @@ protected function is_two_factor_plugin_active(): bool { * @return bool True if the Two-Factor plugin is active and the user has two-factor authentication enabled, false otherwise. */ protected function user_has_two_factor( int $user_id ): bool { - if ( ! $this->is_two_factor_plugin_active() ) { - return false; - } - - return \Two_Factor_Core::is_user_using_two_factor( $user_id ); // @phpstan-ignore class.notFound (Two_Factor_Core comes from the optional Two-Factor plugin; is_two_factor_plugin_active() confirms it exists.) + 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.) } /** diff --git a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php index 610c9aec8d7..75b28f919a8 100644 --- a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php +++ b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php @@ -23,7 +23,7 @@ class FakeTwoFactorAuthenticator extends Authenticator { * * @var bool */ - public $is_two_factor_plugin_active = false; + public bool $is_two_factor_plugin_active = false; /** * IDs of users treated as having two-factor authentication enabled, or @@ -31,7 +31,7 @@ class FakeTwoFactorAuthenticator extends Authenticator { * * @var int[]|null */ - public $user_ids_with_two_factor; + public ?array $user_ids_with_two_factor = null; /** * @inheritDoc @@ -44,10 +44,8 @@ protected function is_two_factor_plugin_active(): bool { * @inheritDoc */ protected function user_has_two_factor( int $user_id ): bool { - if ( ! is_array( $this->user_ids_with_two_factor ) ) { - return parent::user_has_two_factor( $user_id ); - } - - return in_array( $user_id, $this->user_ids_with_two_factor, true ); + return is_array( $this->user_ids_with_two_factor ) + ? in_array( $user_id, $this->user_ids_with_two_factor, true ) + : parent::user_has_two_factor( $user_id ); } } From bbecdf7f6fef34a68cf917ded017662ce2ff79a7 Mon Sep 17 00:00:00 2001 From: Sherv Date: Tue, 21 Jul 2026 14:37:15 +0300 Subject: [PATCH 6/9] Remove two-factor authentication challenges for Google sign-in. 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. --- .../Sign_In_With_Google/Authenticator.php | 60 ++++++------------- .../FakeTwoFactorAuthenticator.php | 4 +- .../Sign_In_With_Google/AuthenticatorTest.php | 37 +++++++----- 3 files changed, 42 insertions(+), 59 deletions(-) diff --git a/includes/Modules/Sign_In_With_Google/Authenticator.php b/includes/Modules/Sign_In_With_Google/Authenticator.php index aa4d55eb0f4..79fa2b80625 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -33,14 +33,8 @@ class Authenticator implements Authenticator_Interface { /** * Error codes. */ - const ERROR_INVALID_REQUEST = 'googlesitekit_auth_invalid_request'; - const ERROR_SIGNIN_FAILED = 'googlesitekit_auth_failed'; - - /** - * Error code used when the email-matched user has two-factor authentication enabled. - * - * @since n.e.x.t - */ + const ERROR_INVALID_REQUEST = 'googlesitekit_auth_invalid_request'; + const ERROR_SIGNIN_FAILED = 'googlesitekit_auth_failed'; const ERROR_TWO_FACTOR_ENABLED = 'googlesitekit_auth_two_factor_enabled'; /** @@ -50,13 +44,6 @@ class Authenticator implements Authenticator_Interface { */ const CREATED_BY_META_KEY = 'googlesitekitpersistent_created_by'; - /** - * User meta key holding the two-factor providers the Two-Factor plugin has enabled for a user. - * - * @since n.e.x.t - */ - const TWO_FACTOR_ENABLED_PROVIDERS_META_KEY = '_two_factor_enabled_providers'; - /** * Nonce action used by the existing-user link flow. * @@ -103,7 +90,7 @@ public function __construct( User_Options $user_options, Profile_Reader_Interfac * @param Input $input Input instance. * @return string Redirect URL. */ - public function authenticate_user( Input $input ): string { + public function authenticate_user( Input $input ) { $credential = $input->filter( INPUT_POST, 'credential' ); $user = null; @@ -195,6 +182,7 @@ protected function get_redirect_url( $user, $input ) { * Signs in the user. * * @since 1.145.0 + * @since n.e.x.t Removes the Two-Factor plugin's login challenge for the request so Sign in with Google can complete. * * @param WP_User $user User object. * @return WP_Error|null WP_Error if an error occurred, null otherwise. @@ -215,8 +203,19 @@ protected function sign_in_user( $user ) { // Set the user to be the current user. wp_set_current_user( $user->ID, $user->user_login ); - // Set the authentication cookies and trigger the wp_login action. + // Set the authentication cookie. wp_set_auth_cookie( $user->ID ); + + // Sign in with Google signs the user in with their Google Account, so + // Google handles two-factor for this login. Remove the Two-Factor plugin's + // login check for this request only, so it doesn't block the sign-in. + 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 ); + } + } + /** This filter is documented in wp-login.php */ do_action( 'wp_login', $user->user_login, $user ); @@ -273,7 +272,6 @@ protected function find_user( $payload ) { * Create a new user using the Google auth payload. * * @since 1.145.0 - * @since n.e.x.t Disables two-factor authentication for the new user when the Two-Factor plugin is active. * * @param array $payload Google auth payload. * @return WP_User|WP_Error User object if found or created, WP_Error otherwise. @@ -307,8 +305,6 @@ protected function create_user( $payload ) { $user_options->set( Hashed_User_ID::OPTION, $google_user_hashed_id ); $user_options->set( self::CREATED_BY_META_KEY, Sign_In_With_Google::MODULE_SLUG ); - $this->disable_two_factor_for_new_user( $user_id ); - // Add the user to the current site if it is a multisite. if ( is_multisite() ) { add_user_to_blog( get_current_blog_id(), $user_id, $default_role ); @@ -330,7 +326,7 @@ protected function create_user( $payload ) { * * @return bool True if the Two-Factor plugin is active, false otherwise. */ - protected function is_two_factor_plugin_active(): bool { + protected function is_two_factor_plugin_active() { return class_exists( 'Two_Factor_Core' ); } @@ -342,30 +338,10 @@ protected function is_two_factor_plugin_active(): bool { * @param int $user_id User ID. * @return bool True if the Two-Factor plugin is active and the user has two-factor authentication enabled, false otherwise. */ - protected function user_has_two_factor( int $user_id ): bool { + 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.) } - /** - * Disables two-factor authentication for a newly created user. - * - * Accounts created by Sign in with Google rely on the two-factor - * authentication of the user's Google Account, so the WordPress-level - * challenge stays off to keep the Sign in with Google flow working. - * - * @since n.e.x.t - * - * @param int $user_id User ID. - */ - protected function disable_two_factor_for_new_user( int $user_id ): void { - if ( ! $this->is_two_factor_plugin_active() ) { - return; - } - - // An empty provider list turns off two-factor authentication in the Two-Factor plugin. - update_user_meta( $user_id, self::TWO_FACTOR_ENABLED_PROVIDERS_META_KEY, array() ); - } - /** * Gets the hashed Google user ID from the provided payload. * diff --git a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php index 75b28f919a8..1745bbb43c8 100644 --- a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php +++ b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php @@ -36,14 +36,14 @@ class FakeTwoFactorAuthenticator extends Authenticator { /** * @inheritDoc */ - protected function is_two_factor_plugin_active(): bool { + protected function is_two_factor_plugin_active() { return $this->is_two_factor_plugin_active; } /** * @inheritDoc */ - protected function user_has_two_factor( int $user_id ): bool { + protected function user_has_two_factor( int $user_id ) { return is_array( $this->user_ids_with_two_factor ) ? in_array( $user_id, $this->user_ids_with_two_factor, true ) : parent::user_has_two_factor( $user_id ); diff --git a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php index d124db7d3b2..c19a724ff47 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php @@ -239,9 +239,12 @@ public function test_authenticate_user__links_email_matched_user_when_two_factor $this->assertEquals( $user->ID, get_current_user_id(), 'Authenticated user ID should match the email-matched user.' ); } - public function test_authenticate_user__disables_two_factor_for_new_user() { + public function test_authenticate_user__removes_two_factor_login_challenge_for_new_user() { add_filter( 'option_users_can_register', '__return_true' ); + // Emulate the Two-Factor plugin's login challenge on the wp_login action. + add_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), PHP_INT_MAX, 2 ); + $authenticator = $this->create_two_factor_authenticator( self::$new_user_payload ); $authenticator->is_two_factor_plugin_active = true; @@ -249,29 +252,33 @@ public function test_authenticate_user__disables_two_factor_for_new_user() { $user_id = get_current_user_id(); $this->assertNotEmpty( $user_id, 'A new user should be created and signed in.' ); - $this->assertTrue( - metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), - 'The two-factor providers meta should be written for the new user.' + $this->assertFalse( + has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ), + 'The Two-Factor login challenge should be removed for the Sign in with Google request.' ); - $this->assertEquals( - array(), - get_user_meta( $user_id, '_two_factor_enabled_providers', true ), - 'The new user should have two-factor authentication disabled.' + $this->assertFalse( + metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), + 'Sign in with Google should not modify the new account\'s two-factor settings.' ); } - public function test_authenticate_user__does_not_write_two_factor_meta_when_plugin_is_inactive() { - add_filter( 'option_users_can_register', '__return_true' ); + public function test_authenticate_user__removes_two_factor_login_challenge_for_returning_user() { + $user = $this->factory()->user->create_and_get( array() ); + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $user_options->set( Hashed_User_ID::OPTION, md5( self::$existing_user_payload['sub'] ) ); - $authenticator = $this->create_two_factor_authenticator( self::$new_user_payload ); + // Emulate the Two-Factor plugin's login challenge on the wp_login action. + add_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), PHP_INT_MAX, 2 ); + + $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); + $authenticator->is_two_factor_plugin_active = true; $authenticator->authenticate_user( new MutableInput() ); - $user_id = get_current_user_id(); - $this->assertNotEmpty( $user_id, 'A new user should be created and signed in.' ); + $this->assertEquals( $user->ID, get_current_user_id(), 'The returning Sign in with Google user should be signed in.' ); $this->assertFalse( - metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), - 'No two-factor providers meta should be written when the Two-Factor plugin is inactive.' + has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ), + 'The Two-Factor login challenge should be removed so the returning user can sign in.' ); } From 59f4287dbd9c3946ff13d9dc3f644aa3e8cc71e4 Mon Sep 17 00:00:00 2001 From: Sherv Date: Tue, 21 Jul 2026 19:56:36 +0300 Subject: [PATCH 7/9] Skip two-factor challenge for Sign in with Google requests. 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. --- .../Sign_In_With_Google/Authenticator.php | 40 ++++++++++++++----- .../Sign_In_With_Google/AuthenticatorTest.php | 39 +++++++++--------- 2 files changed, 48 insertions(+), 31 deletions(-) diff --git a/includes/Modules/Sign_In_With_Google/Authenticator.php b/includes/Modules/Sign_In_With_Google/Authenticator.php index 79fa2b80625..7472d21f58f 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -182,7 +182,7 @@ protected function get_redirect_url( $user, $input ) { * Signs in the user. * * @since 1.145.0 - * @since n.e.x.t Removes the Two-Factor plugin's login challenge for the request so Sign in with Google can complete. + * @since n.e.x.t Skips the Two-Factor plugin's login challenge for the Sign in with Google request. * * @param WP_User $user User object. * @return WP_Error|null WP_Error if an error occurred, null otherwise. @@ -206,15 +206,10 @@ protected function sign_in_user( $user ) { // Set the authentication cookie. wp_set_auth_cookie( $user->ID ); - // Sign in with Google signs the user in with their Google Account, so - // Google handles two-factor for this login. Remove the Two-Factor plugin's - // login check for this request only, so it doesn't block the sign-in. - 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 ); - } - } + // Sign in with Google authenticates the user through their Google Account, + // which handles two-factor itself, so the Two-Factor plugin must not + // challenge this request. + $this->skip_two_factor_for_user( $user ); /** This filter is documented in wp-login.php */ do_action( 'wp_login', $user->user_login, $user ); @@ -342,6 +337,31 @@ 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.) } + /** + * Skips the Two-Factor plugin's challenge for the user's Sign in with Google request. + * + * Sign in with Google authenticates the user through their Google Account, + * which handles two-factor itself. This filters the user's enabled providers + * to an empty array for this request, so the Two-Factor plugin treats them as + * not using two-factor and doesn't challenge the sign-in. It doesn't change + * the user's saved two-factor settings, it leaves every other user's providers + * untouched, and it runs only when the Two-Factor plugin is active. + * + * @since n.e.x.t + * + * @param WP_User $user User signing in with Google. + */ + 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 + ); + } + /** * Gets the hashed Google user ID from the provided payload. * diff --git a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php index c19a724ff47..2d54652fc3c 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php @@ -239,22 +239,17 @@ public function test_authenticate_user__links_email_matched_user_when_two_factor $this->assertEquals( $user->ID, get_current_user_id(), 'Authenticated user ID should match the email-matched user.' ); } - public function test_authenticate_user__removes_two_factor_login_challenge_for_new_user() { + public function test_authenticate_user__skips_two_factor_challenge_for_new_user() { add_filter( 'option_users_can_register', '__return_true' ); - // Emulate the Two-Factor plugin's login challenge on the wp_login action. - add_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), PHP_INT_MAX, 2 ); - - $authenticator = $this->create_two_factor_authenticator( self::$new_user_payload ); - $authenticator->is_two_factor_plugin_active = true; - - $authenticator->authenticate_user( new MutableInput() ); + $this->do_authenticate_user( self::$new_user_payload ); $user_id = get_current_user_id(); $this->assertNotEmpty( $user_id, 'A new user should be created and signed in.' ); - $this->assertFalse( - has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ), - 'The Two-Factor login challenge should be removed for the Sign in with Google request.' + $this->assertEquals( + array(), + apply_filters( 'two_factor_enabled_providers_for_user', array( 'Two_Factor_Email' ), $user_id ), + 'Sign in with Google should clear the two-factor providers for the signed-in user so the challenge is skipped.' ); $this->assertFalse( metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), @@ -262,23 +257,25 @@ public function test_authenticate_user__removes_two_factor_login_challenge_for_n ); } - public function test_authenticate_user__removes_two_factor_login_challenge_for_returning_user() { + public function test_authenticate_user__skips_two_factor_challenge_for_returning_user() { $user = $this->factory()->user->create_and_get( array() ); $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); $user_options->set( Hashed_User_ID::OPTION, md5( self::$existing_user_payload['sub'] ) ); - // Emulate the Two-Factor plugin's login challenge on the wp_login action. - add_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), PHP_INT_MAX, 2 ); - - $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); - $authenticator->is_two_factor_plugin_active = true; + $other_user = $this->factory()->user->create_and_get( array() ); - $authenticator->authenticate_user( new MutableInput() ); + $this->do_authenticate_user( self::$existing_user_payload ); $this->assertEquals( $user->ID, get_current_user_id(), 'The returning Sign in with Google user should be signed in.' ); - $this->assertFalse( - has_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ) ), - 'The Two-Factor login challenge should be removed so the returning user can sign in.' + $this->assertEquals( + array(), + apply_filters( 'two_factor_enabled_providers_for_user', array( 'Two_Factor_Email' ), $user->ID ), + 'Sign in with Google should clear the two-factor providers for the signed-in user so the challenge is skipped.' + ); + $this->assertEquals( + array( 'Two_Factor_Email' ), + apply_filters( 'two_factor_enabled_providers_for_user', array( 'Two_Factor_Email' ), $other_user->ID ), + 'Sign in with Google should leave the two-factor providers of other users untouched.' ); } From e6f295734d2ba0fb4399705205e39eb6f6712d0c Mon Sep 17 00:00:00 2001 From: Sherv Date: Thu, 23 Jul 2026 11:12:42 +0300 Subject: [PATCH 8/9] Address CR feedback. --- includes/Modules/Sign_In_With_Google.php | 9 +- .../Sign_In_With_Google/Authenticator.php | 100 ++++----- .../FakeTwoFactorAuthenticator.php | 51 ----- .../phpunit/includes/Two_Factor_Core_Fake.php | 100 +++++++++ .../includes/Two_Factor_Plugin_Trait.php | 82 ++++++++ .../Sign_In_With_Google/AuthenticatorTest.php | 192 ++++++++++++------ .../Existing_User_AuthenticatorTest.php | 29 +++ .../Modules/Sign_In_With_GoogleTest.php | 10 +- 8 files changed, 389 insertions(+), 184 deletions(-) delete mode 100644 tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php create mode 100644 tests/phpunit/includes/Two_Factor_Core_Fake.php create mode 100644 tests/phpunit/includes/Two_Factor_Plugin_Trait.php diff --git a/includes/Modules/Sign_In_With_Google.php b/includes/Modules/Sign_In_With_Google.php index eebc537e273..6cdb949a6d9 100644 --- a/includes/Modules/Sign_In_With_Google.php +++ b/includes/Modules/Sign_In_With_Google.php @@ -351,7 +351,14 @@ public function handle_login_errors( $error ) { $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' ) ); + $error->add( + self::MODULE_SLUG, + sprintf( + /* translators: %s: Sign in with Google service name */ + __( 'An account with that email address uses two-factor authentication. To use %s, log in with your username and password, then connect your Google account on your profile page.', 'google-site-kit' ), + _x( 'Sign in with Google', 'Service name', 'google-site-kit' ) + ) + ); break; default: break; diff --git a/includes/Modules/Sign_In_With_Google/Authenticator.php b/includes/Modules/Sign_In_With_Google/Authenticator.php index 7472d21f58f..99c97871904 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -182,7 +182,7 @@ protected function get_redirect_url( $user, $input ) { * Signs in the user. * * @since 1.145.0 - * @since n.e.x.t Skips the Two-Factor plugin's login challenge for the Sign in with Google request. + * @since n.e.x.t Skips the Two-Factor plugin's login challenge for this request. * * @param WP_User $user User object. * @return WP_Error|null WP_Error if an error occurred, null otherwise. @@ -203,14 +203,22 @@ protected function sign_in_user( $user ) { // Set the user to be the current user. wp_set_current_user( $user->ID, $user->user_login ); - // Set the authentication cookie. - wp_set_auth_cookie( $user->ID ); - - // Sign in with Google authenticates the user through their Google Account, - // which handles two-factor itself, so the Two-Factor plugin must not - // challenge this request. - $this->skip_two_factor_for_user( $user ); + // Signing in through Google stands in for the second factor, so the + // Two-Factor plugin's challenge would ask for one the user has already + // given. Clear the primary provider, the last value the plugin resolves + // before wp_login decides whether to drop the session. An earlier value + // won't hold, because the plugin restores emailed codes whenever the + // enabled providers come back empty while the user's stored providers + // don't. The user's own two-factor settings stay untouched. + add_filter( + 'two_factor_primary_provider_for_user', + fn ( $provider, $user_id ) => $user_id === $user->ID ? '' : $provider, + 10, + 2 + ); + // Set the authentication cookies and trigger the wp_login action. + wp_set_auth_cookie( $user->ID ); /** This filter is documented in wp-login.php */ do_action( 'wp_login', $user->user_login, $user ); @@ -221,10 +229,10 @@ protected function sign_in_user( $user ) { * Finds an existing user using the Google user ID and email. * * @since 1.145.0 - * @since n.e.x.t Returns a WP_Error when the email-matched user has two-factor authentication enabled. + * @since n.e.x.t Returns a WP_Error when the email-matched user uses two-factor authentication and isn't connected to the Google account. * * @param array $payload Google auth payload. - * @return WP_User|WP_Error|null User object if found, WP_Error if the email-matched user has two-factor authentication enabled, null otherwise. + * @return WP_User|WP_Error|null User object when found, WP_Error when the matched user has to connect their Google account first, null otherwise. */ protected function find_user( $payload ) { // Check if there are any existing WordPress users connected to this Google account. @@ -246,21 +254,22 @@ protected function find_user( $payload ) { // Find an existing user that matches the email and link to their Google account by store their user ID in user meta. $user = get_user_by( 'email', $payload['email'] ); - if ( $user ) { - // 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 ); - } - - $user_options = clone $this->user_options; - $user_options->switch_user( $user->ID ); - $user_options->set( Hashed_User_ID::OPTION, $google_user_hashed_id ); + if ( ! $user ) { + return null; + } - return $user; + // Connecting the accounts here would let a user with two-factor + // authentication sign in without their challenge. They connect from + // their profile page instead, where they sign in first. + if ( $this->user_has_two_factor_enabled( $user->ID ) ) { + return new WP_Error( self::ERROR_TWO_FACTOR_ENABLED ); } - return null; + $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; } /** @@ -311,55 +320,18 @@ protected function create_user( $payload ) { return get_user_by( 'id', $user_id ); } - /** - * Checks whether the Two-Factor plugin is active. - * - * Detects the plugin by its Two_Factor_Core class, so any way of loading - * it (a plugin, an mu-plugin, or a Composer package) counts as active. - * - * @since n.e.x.t - * - * @return bool True if the Two-Factor plugin is active, false otherwise. - */ - protected function is_two_factor_plugin_active() { - return class_exists( 'Two_Factor_Core' ); - } - /** * Checks whether the given user has two-factor authentication enabled. * - * @since n.e.x.t - * - * @param int $user_id User ID. - * @return bool True if the Two-Factor plugin is active and the user has two-factor authentication enabled, false otherwise. - */ - 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.) - } - - /** - * Skips the Two-Factor plugin's challenge for the user's Sign in with Google request. - * - * Sign in with Google authenticates the user through their Google Account, - * which handles two-factor itself. This filters the user's enabled providers - * to an empty array for this request, so the Two-Factor plugin treats them as - * not using two-factor and doesn't challenge the sign-in. It doesn't change - * the user's saved two-factor settings, it leaves every other user's providers - * untouched, and it runs only when the Two-Factor plugin is active. + * Returns false when the optional Two-Factor plugin isn't active. * * @since n.e.x.t * - * @param WP_User $user User signing in with Google. + * @param int $user_id User ID. + * @return bool True when the user has two-factor authentication enabled, false otherwise. */ - 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 - ); + protected function user_has_two_factor_enabled( $user_id ) { + return class_exists( 'Two_Factor_Core' ) && \Two_Factor_Core::is_user_using_two_factor( $user_id ); } /** diff --git a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php b/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php deleted file mode 100644 index 1745bbb43c8..00000000000 --- a/tests/phpunit/includes/Modules/Sign_In_With_Google/FakeTwoFactorAuthenticator.php +++ /dev/null @@ -1,51 +0,0 @@ -is_two_factor_plugin_active; - } - - /** - * @inheritDoc - */ - protected function user_has_two_factor( int $user_id ) { - return is_array( $this->user_ids_with_two_factor ) - ? in_array( $user_id, $this->user_ids_with_two_factor, true ) - : parent::user_has_two_factor( $user_id ); - } -} diff --git a/tests/phpunit/includes/Two_Factor_Core_Fake.php b/tests/phpunit/includes/Two_Factor_Core_Fake.php new file mode 100644 index 00000000000..6da1b1d9ef0 --- /dev/null +++ b/tests/phpunit/includes/Two_Factor_Core_Fake.php @@ -0,0 +1,100 @@ + 'existing-user', 'email' => 'existing-user@example.com', @@ -75,28 +78,17 @@ public function tear_down() { $_POST = $this->post_data; } - private function create_mock_profile_reader( $profile_reader_data ) { + private function do_authenticate_user( $profile_reader_data = array() ) { + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); $mock_profile_reader = $this->getMockBuilder( Profile_Reader_Interface::class ) ->setMethods( array( 'get_profile_data' ) ) ->getMock(); $mock_profile_reader->method( 'get_profile_data' )->willReturn( $profile_reader_data ); - - return $mock_profile_reader; - } - - private function do_authenticate_user( $profile_reader_data = array() ) { - $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); - $authenticator = new Authenticator( $user_options, $this->create_mock_profile_reader( $profile_reader_data ) ); + $authenticator = new Authenticator( $user_options, $mock_profile_reader ); return $authenticator->authenticate_user( new MutableInput() ); } - private function create_two_factor_authenticator( $profile_reader_data ) { - $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); - - return new FakeTwoFactorAuthenticator( $user_options, $this->create_mock_profile_reader( $profile_reader_data ) ); - } - public function test_authenticate_user_fails_when_profile_reader_returns_error() { $expected = add_query_arg( 'error', Authenticator::ERROR_INVALID_REQUEST, wp_login_url() ); $actual = $this->do_authenticate_user( new WP_Error( 'test_error' ) ); @@ -183,99 +175,173 @@ public function test_authenticate_user_creates_new_user_when_registration_is_all ); } - public function test_authenticate_user__errors_when_email_matched_user_has_two_factor() { - $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__blocks_two_factor_user_with_no_connected_account() { + $this->activate_two_factor_plugin(); - $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); - $authenticator->is_two_factor_plugin_active = true; - $authenticator->user_ids_with_two_factor = array( $user->ID ); + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + $this->enable_two_factor_for_user( $user->ID ); $expected = add_query_arg( 'error', 'googlesitekit_auth_two_factor_enabled', wp_login_url() ); - $actual = $authenticator->authenticate_user( new MutableInput() ); + $actual = $this->do_authenticate_user( self::$existing_user_payload ); - $this->assertEquals( $expected, $actual, 'Should redirect to login with the two-factor error when the email-matched user has two-factor authentication enabled.' ); + $this->assertEquals( $expected, $actual, 'Should redirect to the login page with the two-factor error.' ); + $this->assertEquals( 0, get_current_user_id(), 'Should leave the user signed out.' ); $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); $this->assertFalse( metadata_exists( 'user', $user->ID, $user_options->get_meta_key( Hashed_User_ID::OPTION ) ), - 'The Google account should not be linked to the user.' + 'Should leave the Google account unconnected.' ); - $this->assertEquals( 0, get_current_user_id(), 'The user should not be signed in.' ); } - public function test_authenticate_user__links_email_matched_user_without_two_factor() { + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__blocks_two_factor_user_connected_to_another_google_account() { + $this->activate_two_factor_plugin(); + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + $this->enable_two_factor_for_user( $user->ID ); - $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); - $authenticator->is_two_factor_plugin_active = true; - $authenticator->user_ids_with_two_factor = array(); + $connected_account = md5( 'another-google-account' ); + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $user_options->set( Hashed_User_ID::OPTION, $connected_account ); - $expected = admin_url( '/profile.php' ); - $actual = $authenticator->authenticate_user( new MutableInput() ); + $expected = add_query_arg( 'error', 'googlesitekit_auth_two_factor_enabled', wp_login_url() ); + $actual = $this->do_authenticate_user( self::$existing_user_payload ); - $this->assertEquals( $expected, $actual, 'Should redirect to profile when the email-matched user has no two-factor authentication.' ); - $this->assertEquals( $user->ID, get_current_user_id(), 'Authenticated user ID should match the email-matched user.' ); + $this->assertEquals( $expected, $actual, 'Should redirect to the login page with the two-factor error.' ); + $this->assertEquals( 0, get_current_user_id(), 'Should leave the user signed out.' ); + $this->assertEquals( + $connected_account, + $user_options->get( Hashed_User_ID::OPTION ), + 'Should leave the connected Google account untouched.' + ); + } + + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__signs_in_two_factor_user_with_connected_account() { + $this->activate_two_factor_plugin(); + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + $this->enable_two_factor_for_user( $user->ID ); + + // Connecting from the profile page stores the hashed Google user ID. $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $user_options->set( Hashed_User_ID::OPTION, md5( self::$existing_user_payload['sub'] ) ); + + $actual = $this->do_authenticate_user( self::$existing_user_payload ); + + $this->assertEquals( admin_url( '/profile.php' ), $actual, 'Should redirect to the profile page after signing in.' ); + $this->assertEquals( $user->ID, get_current_user_id(), 'Should sign the connected user in.' ); + $this->assertFalse( + $this->two_factor_challenges_user( $user->ID ), + 'Should skip the two-factor challenge for the connected user.' + ); $this->assertEquals( - md5( self::$existing_user_payload['sub'] ), - $user_options->get( Hashed_User_ID::OPTION ), - 'The Google account should be linked to the user.' + array( self::TWO_FACTOR_PROVIDER ), + $this->get_two_factor_providers_for_user( $user->ID ), + 'Should leave the two-factor settings of the connected user alone.' ); } - public function test_authenticate_user__links_email_matched_user_when_two_factor_plugin_is_inactive() { + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__connects_and_signs_in_user_without_two_factor() { + $this->activate_two_factor_plugin(); + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); - update_user_meta( $user->ID, '_two_factor_enabled_providers', array( 'Two_Factor_Email' ) ); - // The plugin-active flag stays false, and no two-factor users are - // faked, so the real user_has_two_factor() runs. - $authenticator = $this->create_two_factor_authenticator( self::$existing_user_payload ); + $actual = $this->do_authenticate_user( self::$existing_user_payload ); - $expected = admin_url( '/profile.php' ); - $actual = $authenticator->authenticate_user( new MutableInput() ); + $this->assertEquals( admin_url( '/profile.php' ), $actual, 'Should redirect to the profile page after signing in.' ); + $this->assertEquals( $user->ID, get_current_user_id(), 'Should sign the matched user in.' ); - $this->assertEquals( $expected, $actual, 'Should redirect to profile when the Two-Factor plugin is inactive.' ); - $this->assertEquals( $user->ID, get_current_user_id(), 'Authenticated user ID should match the email-matched user.' ); + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $this->assertEquals( + md5( self::$existing_user_payload['sub'] ), + $user_options->get( Hashed_User_ID::OPTION ), + 'Should connect the Google account.' + ); } - public function test_authenticate_user__skips_two_factor_challenge_for_new_user() { + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__creates_and_connects_new_user() { + $this->activate_two_factor_plugin(); add_filter( 'option_users_can_register', '__return_true' ); $this->do_authenticate_user( self::$new_user_payload ); $user_id = get_current_user_id(); - $this->assertNotEmpty( $user_id, 'A new user should be created and signed in.' ); $this->assertEquals( - array(), - apply_filters( 'two_factor_enabled_providers_for_user', array( 'Two_Factor_Email' ), $user_id ), - 'Sign in with Google should clear the two-factor providers for the signed-in user so the challenge is skipped.' + self::$new_user_payload['email'], + get_userdata( $user_id )->user_email, + 'Should create the new user and sign them in.' ); - $this->assertFalse( - metadata_exists( 'user', $user_id, '_two_factor_enabled_providers' ), - 'Sign in with Google should not modify the new account\'s two-factor settings.' + + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user_id ); + $this->assertEquals( + md5( self::$new_user_payload['sub'] ), + $user_options->get( Hashed_User_ID::OPTION ), + 'Should connect the new account to the Google account.' + ); + $this->assertEquals( + '', + $this->get_two_factor_providers_for_user( $user_id ), + 'Should leave the two-factor settings of the new account alone.' ); } - public function test_authenticate_user__skips_two_factor_challenge_for_returning_user() { - $user = $this->factory()->user->create_and_get( array() ); + public function test_authenticate_user__connects_user_when_two_factor_plugin_is_inactive() { + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + $this->enable_two_factor_for_user( $user->ID ); + + $actual = $this->do_authenticate_user( self::$existing_user_payload ); + + $this->assertEquals( admin_url( '/profile.php' ), $actual, 'Should redirect to the profile page after signing in.' ); + $this->assertEquals( $user->ID, get_current_user_id(), 'Should sign the matched user in.' ); + + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); + $this->assertEquals( + md5( self::$existing_user_payload['sub'] ), + $user_options->get( Hashed_User_ID::OPTION ), + 'Should connect the Google account.' + ); + } + + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__leaves_the_two_factor_challenge_of_other_users_in_place() { + $this->activate_two_factor_plugin(); + + $user = $this->factory()->user->create_and_get( array( 'user_email' => self::$existing_user_payload['email'] ) ); + $this->enable_two_factor_for_user( $user->ID ); + $user_options = new User_Options( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ), $user->ID ); $user_options->set( Hashed_User_ID::OPTION, md5( self::$existing_user_payload['sub'] ) ); - $other_user = $this->factory()->user->create_and_get( array() ); + $other_user_id = $this->factory()->user->create(); + $this->enable_two_factor_for_user( $other_user_id ); $this->do_authenticate_user( self::$existing_user_payload ); - $this->assertEquals( $user->ID, get_current_user_id(), 'The returning Sign in with Google user should be signed in.' ); - $this->assertEquals( - array(), - apply_filters( 'two_factor_enabled_providers_for_user', array( 'Two_Factor_Email' ), $user->ID ), - 'Sign in with Google should clear the two-factor providers for the signed-in user so the challenge is skipped.' + $this->assertEquals( $user->ID, get_current_user_id(), 'Should sign the connected user in.' ); + $this->assertFalse( + $this->two_factor_challenges_user( $user->ID ), + 'Should skip the two-factor challenge for the user who signed in with Google.' ); - $this->assertEquals( - array( 'Two_Factor_Email' ), - apply_filters( 'two_factor_enabled_providers_for_user', array( 'Two_Factor_Email' ), $other_user->ID ), - 'Sign in with Google should leave the two-factor providers of other users untouched.' + $this->assertTrue( + $this->two_factor_challenges_user( $other_user_id ), + 'Should keep the two-factor challenge for every other user.' ); } diff --git a/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php b/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php index 0a4c98117f4..f6dc9efbb3e 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php @@ -18,6 +18,7 @@ use Google\Site_Kit\Modules\Sign_In_With_Google\Profile_Reader_Interface; use Google\Site_Kit\Tests\MutableInput; use Google\Site_Kit\Tests\TestCase; +use Google\Site_Kit\Tests\Two_Factor_Plugin_Trait; use WP_Error; /** @@ -26,6 +27,8 @@ */ class Existing_User_AuthenticatorTest extends TestCase { + use Two_Factor_Plugin_Trait; + private static $payload = array( 'sub' => 'google-sub-12345', 'email' => 'someone@example.com', @@ -114,6 +117,32 @@ public function test_links_current_user_even_when_email_does_not_match() { ); } + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__links_current_user_who_uses_two_factor() { + $this->activate_two_factor_plugin(); + + $user_id = $this->factory()->user->create(); + wp_set_current_user( $user_id ); + $this->enable_two_factor_for_user( $user_id ); + + $expected = get_edit_user_link( $user_id ); + $actual = $this->do_authenticate_user( self::$payload ); + + $this->assertEquals( $expected, $actual, 'Should redirect to the user edit link after connecting.' ); + $this->assertEquals( + md5( self::$payload['sub'] ), + get_user_option( Hashed_User_ID::OPTION, $user_id ), + 'Should connect the Google account of a user who uses two-factor authentication.' + ); + $this->assertEquals( + array( self::TWO_FACTOR_PROVIDER ), + $this->get_two_factor_providers_for_user( $user_id ), + 'Should leave the two-factor settings of the connected user alone.' + ); + } + public function test_returns_error_redirect_when_google_account_taken_by_other_user() { $other_user_id = $this->factory()->user->create(); update_user_option( $other_user_id, Hashed_User_ID::OPTION, md5( self::$payload['sub'] ) ); diff --git a/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php b/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php index 0fac66a0a33..2eaf8ead147 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_GoogleTest.php @@ -678,18 +678,18 @@ public function test_handle_login_errors__adds_two_factor_error_message() { $error = $this->module->handle_login_errors( new WP_Error() ); $this->assertEquals( - '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.', - $error->get_error_message( Sign_In_With_Google::MODULE_SLUG ), - 'The two-factor error message should be added for the two-factor error code.' + 'An account with that email address uses two-factor authentication. To use Sign in with Google, log in with your username and password, then connect your Google account on your profile page.', + $error->get_error_message( 'sign-in-with-google' ), + 'Should add the two-factor message for the two-factor error code.' ); } - public function test_handle_login_errors__ignores_unrecognized_error_code() { + public function test_handle_login_errors__ignores_an_unrecognized_error_code() { $_GET['error'] = 'unrecognized_error_code'; $error = $this->module->handle_login_errors( new WP_Error() ); - $this->assertFalse( $error->has_errors(), 'An unrecognized error code should leave the error object untouched.' ); + $this->assertFalse( $error->has_errors(), 'Should leave the error object untouched for an unrecognized error code.' ); } protected function create_disconnect_nonce( $user_id ) { From 83e5d1f90aea7d53c6c54bef878db7e77c4f5267 Mon Sep 17 00:00:00 2001 From: Sherv Date: Fri, 24 Jul 2026 10:04:28 +0300 Subject: [PATCH 9/9] Fix the two-factor trait constant for PHP 7.4 and simplify the comments. --- .../Modules/Sign_In_With_Google/Authenticator.php | 12 +++++------- tests/phpunit/includes/Two_Factor_Core_Fake.php | 6 +++--- tests/phpunit/includes/Two_Factor_Plugin_Trait.php | 14 ++++++++++---- .../Sign_In_With_Google/AuthenticatorTest.php | 2 +- .../Existing_User_AuthenticatorTest.php | 2 +- 5 files changed, 20 insertions(+), 16 deletions(-) diff --git a/includes/Modules/Sign_In_With_Google/Authenticator.php b/includes/Modules/Sign_In_With_Google/Authenticator.php index 99c97871904..ebfd48c581b 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -203,13 +203,11 @@ protected function sign_in_user( $user ) { // Set the user to be the current user. wp_set_current_user( $user->ID, $user->user_login ); - // Signing in through Google stands in for the second factor, so the - // Two-Factor plugin's challenge would ask for one the user has already - // given. Clear the primary provider, the last value the plugin resolves - // before wp_login decides whether to drop the session. An earlier value - // won't hold, because the plugin restores emailed codes whenever the - // enabled providers come back empty while the user's stored providers - // don't. The user's own two-factor settings stay untouched. + // Google already checked the second factor, so skip the Two-Factor + // challenge for this login. Setting this user's primary provider to + // empty turns the challenge off for this user only and keeps their + // saved settings. Don't empty the enabled providers instead: the + // plugin turns email codes back on when that list is empty. add_filter( 'two_factor_primary_provider_for_user', fn ( $provider, $user_id ) => $user_id === $user->ID ? '' : $provider, diff --git a/tests/phpunit/includes/Two_Factor_Core_Fake.php b/tests/phpunit/includes/Two_Factor_Core_Fake.php index 6da1b1d9ef0..f22a90bc566 100644 --- a/tests/phpunit/includes/Two_Factor_Core_Fake.php +++ b/tests/phpunit/includes/Two_Factor_Core_Fake.php @@ -17,9 +17,9 @@ * aliases this class to Two_Factor_Core so the plugin counts as active. * * The provider lookup applies the same two filters, in the same order, as - * Two_Factor_Core, including the fallback to emailed codes. The plugin runs its - * challenge or skips it depending on which of the two filters a caller answers, - * so a double that left either one out would pass a test the plugin fails. + * Two_Factor_Core, including the fallback to emailed codes. Both filters decide + * whether the plugin challenges the user, so this fake keeps both. A fake with + * only one would let a test pass that the real plugin fails. * * @since n.e.x.t * @access private diff --git a/tests/phpunit/includes/Two_Factor_Plugin_Trait.php b/tests/phpunit/includes/Two_Factor_Plugin_Trait.php index c28c6ed4343..df85a2e577e 100644 --- a/tests/phpunit/includes/Two_Factor_Plugin_Trait.php +++ b/tests/phpunit/includes/Two_Factor_Plugin_Trait.php @@ -20,12 +20,18 @@ trait Two_Factor_Plugin_Trait { /** - * Two-factor provider the test users turn on. + * Gets the two-factor provider the test users turn on. + * + * A trait can't hold a constant before PHP 8.2, and the plugin supports + * PHP 7.4, so this is a static method. * * @since n.e.x.t - * @var string + * + * @return string Provider key. */ - const TWO_FACTOR_PROVIDER = 'Two_Factor_Totp'; + protected static function two_factor_provider() { + return 'Two_Factor_Totp'; + } /** * Makes the Two-Factor plugin count as active. @@ -53,7 +59,7 @@ class_alias( Two_Factor_Core_Fake::class, 'Two_Factor_Core' ); * @param int $user_id User ID. */ protected function enable_two_factor_for_user( $user_id ) { - update_user_meta( $user_id, Two_Factor_Core_Fake::ENABLED_PROVIDERS_USER_META_KEY, array( self::TWO_FACTOR_PROVIDER ) ); + update_user_meta( $user_id, Two_Factor_Core_Fake::ENABLED_PROVIDERS_USER_META_KEY, array( self::two_factor_provider() ) ); } /** diff --git a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php index 9da3b83181a..6beb2fe9d36 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_Google/AuthenticatorTest.php @@ -244,7 +244,7 @@ public function test_authenticate_user__signs_in_two_factor_user_with_connected_ 'Should skip the two-factor challenge for the connected user.' ); $this->assertEquals( - array( self::TWO_FACTOR_PROVIDER ), + array( self::two_factor_provider() ), $this->get_two_factor_providers_for_user( $user->ID ), 'Should leave the two-factor settings of the connected user alone.' ); diff --git a/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php b/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php index f6dc9efbb3e..778e6aa1b12 100644 --- a/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php +++ b/tests/phpunit/integration/Modules/Sign_In_With_Google/Existing_User_AuthenticatorTest.php @@ -137,7 +137,7 @@ public function test_authenticate_user__links_current_user_who_uses_two_factor() 'Should connect the Google account of a user who uses two-factor authentication.' ); $this->assertEquals( - array( self::TWO_FACTOR_PROVIDER ), + array( self::two_factor_provider() ), $this->get_two_factor_providers_for_user( $user_id ), 'Should leave the two-factor settings of the connected user alone.' );