diff --git a/includes/Modules/Sign_In_With_Google.php b/includes/Modules/Sign_In_With_Google.php index 23d8fc9b3e0..6cdb949a6d9 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,16 @@ 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, + 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 6b9b430bf35..ebfd48c581b 100644 --- a/includes/Modules/Sign_In_With_Google/Authenticator.php +++ b/includes/Modules/Sign_In_With_Google/Authenticator.php @@ -33,8 +33,9 @@ class Authenticator implements Authenticator_Interface { /** * Error codes. */ - const ERROR_INVALID_REQUEST = 'googlesitekit_auth_invalid_request'; - const ERROR_SIGNIN_FAILED = 'googlesitekit_auth_failed'; + 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'; /** * User meta key marking users created via Sign in with Google. @@ -96,15 +97,15 @@ 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 ( ! $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 ); } } @@ -181,6 +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 this request. * * @param WP_User $user User object. * @return WP_Error|null WP_Error if an error occurred, null otherwise. @@ -201,6 +203,18 @@ protected function sign_in_user( $user ) { // Set the user to be the current user. wp_set_current_user( $user->ID, $user->user_login ); + // 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, + 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 */ @@ -213,9 +227,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 uses two-factor authentication and isn't connected to the Google account. * * @param array $payload Google auth payload. - * @return WP_User|null User object if found, 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. @@ -237,15 +252,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 ) { - $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; } /** @@ -296,6 +318,20 @@ protected function create_user( $payload ) { return get_user_by( 'id', $user_id ); } + /** + * Checks whether the given user has two-factor authentication enabled. + * + * Returns false when the optional Two-Factor plugin isn't active. + * + * @since n.e.x.t + * + * @param int $user_id User ID. + * @return bool True when the user has two-factor authentication enabled, false otherwise. + */ + 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 ); + } + /** * Gets the hashed Google user ID from the provided payload. * 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..f22a90bc566 --- /dev/null +++ b/tests/phpunit/includes/Two_Factor_Core_Fake.php @@ -0,0 +1,100 @@ + 'existing-user', 'email' => 'existing-user@example.com', @@ -172,6 +175,176 @@ public function test_authenticate_user_creates_new_user_when_registration_is_all ); } + /** + * @runInSeparateProcess + */ + public function test_authenticate_user__blocks_two_factor_user_with_no_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 ); + + $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 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 ) ), + 'Should leave the Google account unconnected.' + ); + } + + /** + * @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 ); + + $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 = 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 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( + 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.' + ); + } + + /** + * @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'] ) ); + + $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__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->assertEquals( + self::$new_user_payload['email'], + get_userdata( $user_id )->user_email, + 'Should create the new user and sign them in.' + ); + + $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__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_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(), '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->assertTrue( + $this->two_factor_challenges_user( $other_user_id ), + 'Should keep the two-factor challenge for every other user.' + ); + } + /** * @group ms-required */ 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..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 @@ -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 810fff77b23..2eaf8ead147 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->assertEquals( + '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_an_unrecognized_error_code() { + $_GET['error'] = 'unrecognized_error_code'; + + $error = $this->module->handle_login_errors( new WP_Error() ); + + $this->assertFalse( $error->has_errors(), 'Should leave the error object untouched for an unrecognized error code.' ); + } + protected function create_disconnect_nonce( $user_id ) { return wp_create_nonce( Sign_In_With_Google::ACTION_DISCONNECT . '-' . $user_id ); }