From 42656a631cca6a9875ac5819c036ef04f7f7d06d Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Sun, 10 Nov 2019 17:03:31 +0530 Subject: [PATCH 1/9] Ensure that an invalid_grant error on token refresh results in that token being revoked and deleted. --- .../Core/Authentication/Authentication.php | 17 +--------- .../Authentication/Clients/OAuth_Client.php | 34 ++++++++++++------- 2 files changed, 23 insertions(+), 28 deletions(-) diff --git a/includes/Core/Authentication/Authentication.php b/includes/Core/Authentication/Authentication.php index 647f2f6562e..51c0dda0b03 100644 --- a/includes/Core/Authentication/Authentication.php +++ b/includes/Core/Authentication/Authentication.php @@ -279,14 +279,7 @@ public function get_oauth_client() { public function disconnect() { $this->get_oauth_client()->revoke_token(); - $this->user_options->delete( Clients\OAuth_Client::OPTION_ACCESS_TOKEN ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_ACCESS_TOKEN_EXPIRES_IN ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_ACCESS_TOKEN_CREATED ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_REFRESH_TOKEN ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_REDIRECT_URL ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_AUTH_SCOPES ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_ERROR_CODE ); - $this->user_options->delete( Clients\OAuth_Client::OPTION_PROXY_ACCESS_CODE ); + // Delete additional user data. $this->user_options->delete( Verification::OPTION ); $this->user_options->delete( Verification_Tag::OPTION ); $this->user_options->delete( Profile::OPTION ); @@ -461,14 +454,6 @@ private function refresh_auth_token_on_login() { // Refresh auth token. $auth_client->refresh_token(); - - // If 'invalid_grant' error, disconnect the account. - if ( 'invalid_grant' === $this->user_options->get( Clients\OAuth_Client::OPTION_ERROR_CODE ) ) { - $this->disconnect(); - - // We need to re-set this error so that it is displayed to the user. - $this->user_options->set( Clients\OAuth_Client::OPTION_ERROR_CODE, 'invalid_grant' ); - } } /** diff --git a/includes/Core/Authentication/Clients/OAuth_Client.php b/includes/Core/Authentication/Clients/OAuth_Client.php index f93d48080ce..d51a4fbfc42 100644 --- a/includes/Core/Authentication/Clients/OAuth_Client.php +++ b/includes/Core/Authentication/Clients/OAuth_Client.php @@ -250,16 +250,14 @@ public function refresh_token() { if ( $this->using_proxy() ) { // Only the Google_Proxy_Client exposes the real error response. $error_code = $e->getMessage(); } + // Revoke and delete user connection data if the refresh token is invalid or expired. + if ( 'invalid_grant' === $error_code ) { + $this->revoke_token(); + } $this->user_options->set( self::OPTION_ERROR_CODE, $error_code ); return; } - // Refresh token is expired or revoked. - if ( ! empty( $authentication_token['error'] ) ) { - $this->user_options->set( self::OPTION_ERROR_CODE, $authentication_token['error'] ); - return; - } - if ( ! isset( $authentication_token['access_token'] ) ) { $this->user_options->set( self::OPTION_ERROR_CODE, 'access_token_not_received' ); return; @@ -287,6 +285,8 @@ public function revoke_token() { } $this->google_client->revokeToken(); + + $this->delete_token(); } /** @@ -504,12 +504,6 @@ public function authorize_user() { exit(); } - if ( ! empty( $authentication_token['error'] ) ) { - $this->user_options->set( self::OPTION_ERROR_CODE, $authentication_token['error'] ); - wp_safe_redirect( admin_url() ); - exit(); - } - if ( ! isset( $authentication_token['access_token'] ) ) { $this->user_options->set( self::OPTION_ERROR_CODE, 'access_token_not_received' ); wp_safe_redirect( admin_url() ); @@ -768,6 +762,22 @@ public function get_error_message( $error_code ) { return $message; } + /** + * Deletes the current user's token and all associated data. + * + * @since 1.0.3 + */ + private function delete_token() { + $this->user_options->delete( self::OPTION_ACCESS_TOKEN ); + $this->user_options->delete( self::OPTION_ACCESS_TOKEN_EXPIRES_IN ); + $this->user_options->delete( self::OPTION_ACCESS_TOKEN_CREATED ); + $this->user_options->delete( self::OPTION_REFRESH_TOKEN ); + $this->user_options->delete( self::OPTION_REDIRECT_URL ); + $this->user_options->delete( self::OPTION_AUTH_SCOPES ); + $this->user_options->delete( self::OPTION_ERROR_CODE ); + $this->user_options->delete( self::OPTION_PROXY_ACCESS_CODE ); + } + /** * Gets the OAuth redirect URI that listens to the callback request. * From def8029de0e63bc7eb88428e4899bed6134b6469 Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Sun, 10 Nov 2019 17:07:18 +0530 Subject: [PATCH 2/9] Ensure that even on a token revokation failure the user credentials are wiped. --- includes/Core/Authentication/Clients/OAuth_Client.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/includes/Core/Authentication/Clients/OAuth_Client.php b/includes/Core/Authentication/Clients/OAuth_Client.php index d51a4fbfc42..6078a23200b 100644 --- a/includes/Core/Authentication/Clients/OAuth_Client.php +++ b/includes/Core/Authentication/Clients/OAuth_Client.php @@ -284,7 +284,11 @@ public function revoke_token() { return; } - $this->google_client->revokeToken(); + try { + $this->google_client->revokeToken(); + } catch ( \Exception $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement + // No special handling, we just need to make sure this goes through. + } $this->delete_token(); } From 96d88bb8407dcef730f56e384b215b395325760f Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Sun, 10 Nov 2019 17:47:01 +0530 Subject: [PATCH 3/9] Fix failing test and update them for better coverage of new functionality. --- .../Authentication/AuthenticationTest.php | 32 +++++++------------ .../Clients/OAuth_ClientTest.php | 27 +++++++++++++++- 2 files changed, 37 insertions(+), 22 deletions(-) diff --git a/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php b/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php index 8179bf84a9e..0661d964165 100644 --- a/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php +++ b/tests/phpunit/integration/Core/Authentication/AuthenticationTest.php @@ -165,31 +165,21 @@ public function test_register_wp_login() { 'fetchAccessTokenWithRefreshToken', 'revokeToken' ) ); - $mock_google_client->expects( $this->once() )->method( 'fetchAccessTokenWithRefreshToken' )->with( 'test-refresh-token' ); + $mock_google_client->expects( $this->once() )->method( 'fetchAccessTokenWithRefreshToken' ) + ->with( 'test-refresh-token' ) + ->willThrowException( new \Exception( 'invalid_grant' ) ); $mock_google_client->expects( $this->once() )->method( 'revokeToken' ); $this->force_set_property( $client, 'google_client', $mock_google_client ); - // Force invalid_grant error to trigger disconnect. - add_filter( - 'get_user_metadata', - function ( $given, $object_id, $meta_key, $single ) use ( $context, $user_id ) { - if ( $context->is_network_mode() ) { - $error_meta_key = OAuth_Client::OPTION_ERROR_CODE; - } else { - $error_meta_key = $GLOBALS['wpdb']->get_blog_prefix() . OAuth_Client::OPTION_ERROR_CODE; - } - - if ( (int) $object_id === (int) $user_id && $error_meta_key === $meta_key ) { - return $single ? 'invalid_grant' : array( 'invalid_grant' ); - } - - return $given; - }, - 10, - 4 - ); - do_action( 'wp_login' ); + + if ( $context->is_network_mode() ) { + $error_meta_key = OAuth_Client::OPTION_ERROR_CODE; + } else { + $error_meta_key = $GLOBALS['wpdb']->get_blog_prefix() . OAuth_Client::OPTION_ERROR_CODE; + } + + $this->assertSame( 'invalid_grant', get_user_meta( $user_id, $error_meta_key, true ) ); } public function test_get_oauth_client() { diff --git a/tests/phpunit/integration/Core/Authentication/Clients/OAuth_ClientTest.php b/tests/phpunit/integration/Core/Authentication/Clients/OAuth_ClientTest.php index 5ba62345a92..26c71368fd6 100644 --- a/tests/phpunit/integration/Core/Authentication/Clients/OAuth_ClientTest.php +++ b/tests/phpunit/integration/Core/Authentication/Clients/OAuth_ClientTest.php @@ -13,6 +13,7 @@ use Google\Site_Kit\Context; use Google\Site_Kit\Core\Authentication\Clients\OAuth_Client; use Google\Site_Kit\Tests\Exception\RedirectException; +use Google\Site_Kit\Core\Storage\User_Options; use Google\Site_Kit\Tests\FakeHttpClient; use Google\Site_Kit\Tests\TestCase; @@ -62,11 +63,23 @@ public function test_refresh_token() { public function test_revoke_token() { $user_id = $this->factory()->user->create(); wp_set_current_user( $user_id ); - $client = new OAuth_Client( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ); + + $context = new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ); + $user_options = new User_Options( $context, $user_id ); + $client = new OAuth_Client( $context, null, $user_options ); + + foreach ( $this->get_user_credential_keys() as $key ) { + $user_options->set( $key, "test-$key-value" ); + } + // Initialize Google Client $client->get_client(); // Nothing to assert here other than to make sure no errors are raised or exceptions thrown. $client->revoke_token(); + + foreach ( $this->get_user_credential_keys() as $key ) { + $this->assertFalse( $user_options->get( $key ) ); + } } public function test_get_required_scopes() { @@ -371,4 +384,16 @@ protected function fake_proxy_authentication() { ) ); } ); } + + protected function get_user_credential_keys() { + return array( + OAuth_Client::OPTION_ACCESS_TOKEN, + OAuth_Client::OPTION_ACCESS_TOKEN_CREATED, + OAuth_Client::OPTION_ACCESS_TOKEN_EXPIRES_IN, + OAuth_Client::OPTION_AUTH_SCOPES, + OAuth_Client::OPTION_ERROR_CODE, + OAuth_Client::OPTION_REDIRECT_URL, + OAuth_Client::OPTION_REFRESH_TOKEN, + ); + } } From f4debc6bb63aa3d5834fbf5420283204bda15a70 Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Tue, 12 Nov 2019 11:43:07 +0530 Subject: [PATCH 4/9] Properly set refresh token and ensure refreshing token on-the-fly works correctly. --- .../Clients/Google_Proxy_Client.php | 32 +++++++++++++++++++ .../Authentication/Clients/OAuth_Client.php | 18 +++++++---- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/includes/Core/Authentication/Clients/Google_Proxy_Client.php b/includes/Core/Authentication/Clients/Google_Proxy_Client.php index 687b62ecf16..24ae77d1d90 100644 --- a/includes/Core/Authentication/Clients/Google_Proxy_Client.php +++ b/includes/Core/Authentication/Clients/Google_Proxy_Client.php @@ -16,6 +16,7 @@ use Google\Site_Kit_Dependencies\Google\Auth\HttpHandler\HttpClientCache; use Google\Site_Kit_Dependencies\GuzzleHttp\Psr7; use Google\Site_Kit_Dependencies\GuzzleHttp\Psr7\Request; +use Google\Site_Kit_Dependencies\GuzzleHttp\ClientInterface; use Exception; use InvalidArgumentException; use LogicException; @@ -140,6 +141,37 @@ public function revokeToken( $token = null ) { return 200 === (int) $response->getStatusCode(); } + /** + * Adds auth listeners to the HTTP client based on the credentials set in the Google API Client object. + * + * @since 1.0.3 + * + * @param ClientInterface $http The HTTP client object. + * @return ClientInterface The HTTP client object + */ + public function authorize( ClientInterface $http = null ) { + if ( $this->isUsingApplicationDefaultCredentials() ) { + return parent::authorize( $http ); + } + + $token = $this->getAccessToken(); + if ( isset( $token['refresh_token'] ) && $this->isAccessTokenExpired() ) { + $callback = $this->config['token_callback']; + + try { + $creds = $this->fetchAccessTokenWithRefreshToken( $token['refresh_token'] ); + if ( $callback ) { + // Due to original callback signature this can only accept the token itself. + call_user_func( $callback, '', $creds['access_token'] ); + } + } catch ( \Exception $e ) { // phpcs:ignore Generic.CodeAnalysis.EmptyStatement + // Ignore exceptions. + } + } + + return parent::authorize( $http ); + } + /** * Creates a Google auth object for the authentication proxy. * diff --git a/includes/Core/Authentication/Clients/OAuth_Client.php b/includes/Core/Authentication/Clients/OAuth_Client.php index 6078a23200b..e388eb4ad09 100644 --- a/includes/Core/Authentication/Clients/OAuth_Client.php +++ b/includes/Core/Authentication/Clients/OAuth_Client.php @@ -205,16 +205,22 @@ public function get_client() { } $token = array( - 'access_token' => $access_token, - 'expires_in' => $this->user_options->get( self::OPTION_ACCESS_TOKEN_EXPIRES_IN ), - 'created' => $this->user_options->get( self::OPTION_ACCESS_TOKEN_CREATED ), + 'access_token' => $access_token, + 'expires_in' => $this->user_options->get( self::OPTION_ACCESS_TOKEN_EXPIRES_IN ), + 'created' => $this->user_options->get( self::OPTION_ACCESS_TOKEN_CREATED ), + 'refresh_token' => $this->get_refresh_token(), ); - if ( ! $this->using_proxy() ) { - $token['refresh_token'] = $this->get_refresh_token(); - } $this->google_client->setAccessToken( $token ); + // This is called when the client refreshes the access token on-the-fly. + $this->google_client->setTokenCallback( + function( $cache_key, $access_token ) { + // All we can do here is assume an hour as it usually is. + $this->set_access_token( $access_token, HOUR_IN_SECONDS ); + } + ); + // If the token expired or is going to expire in the next 30 seconds. if ( $this->google_client->isAccessTokenExpired() ) { $this->refresh_token(); From be8113c9dab95667aecd45ebcf51b6ddad41987e Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Tue, 12 Nov 2019 11:43:40 +0530 Subject: [PATCH 5/9] Only refresh access token on login if actually needed. --- includes/Core/Authentication/Authentication.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/includes/Core/Authentication/Authentication.php b/includes/Core/Authentication/Authentication.php index 51c0dda0b03..4af01cb2615 100644 --- a/includes/Core/Authentication/Authentication.php +++ b/includes/Core/Authentication/Authentication.php @@ -449,11 +449,11 @@ private function refresh_auth_token_on_login() { $auth_client = $this->get_oauth_client(); - // Initiates Google Client object. - $auth_client->get_client(); - - // Refresh auth token. - $auth_client->refresh_token(); + // Make sure to refresh the access token if necessary. + $google_client = $auth_client->get_client(); + if ( $auth_client->get_access_token() && $google_client->isAccessTokenExpired() ) { + $auth_client->refresh_token(); + } } /** From 8c10decdc8db893911327ce901d594056ff7a60f Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Tue, 12 Nov 2019 11:50:16 +0530 Subject: [PATCH 6/9] Revoke token when refreshing token fails. --- includes/Core/Authentication/Clients/OAuth_Client.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/includes/Core/Authentication/Clients/OAuth_Client.php b/includes/Core/Authentication/Clients/OAuth_Client.php index e388eb4ad09..d374184a81d 100644 --- a/includes/Core/Authentication/Clients/OAuth_Client.php +++ b/includes/Core/Authentication/Clients/OAuth_Client.php @@ -257,9 +257,7 @@ public function refresh_token() { $error_code = $e->getMessage(); } // Revoke and delete user connection data if the refresh token is invalid or expired. - if ( 'invalid_grant' === $error_code ) { - $this->revoke_token(); - } + $this->revoke_token(); $this->user_options->set( self::OPTION_ERROR_CODE, $error_code ); return; } From 20d711b92f3cc8f1aad0fbe715a8a6573776852d Mon Sep 17 00:00:00 2001 From: Felix Arntz Date: Tue, 12 Nov 2019 12:56:17 +0530 Subject: [PATCH 7/9] Only revoke token if no or invalid refresh token. --- includes/Core/Authentication/Clients/OAuth_Client.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/includes/Core/Authentication/Clients/OAuth_Client.php b/includes/Core/Authentication/Clients/OAuth_Client.php index d374184a81d..4e6b8c27651 100644 --- a/includes/Core/Authentication/Clients/OAuth_Client.php +++ b/includes/Core/Authentication/Clients/OAuth_Client.php @@ -237,7 +237,9 @@ function( $cache_key, $access_token ) { public function refresh_token() { $refresh_token = $this->get_refresh_token(); if ( empty( $refresh_token ) ) { + $this->revoke_token(); $this->user_options->set( self::OPTION_ERROR_CODE, 'refresh_token_not_exist' ); + return; } // Stop if google_client not initialized yet. @@ -257,7 +259,9 @@ public function refresh_token() { $error_code = $e->getMessage(); } // Revoke and delete user connection data if the refresh token is invalid or expired. - $this->revoke_token(); + if ( 'invalid_grant' === $error_code ) { + $this->revoke_token(); + } $this->user_options->set( self::OPTION_ERROR_CODE, $error_code ); return; } From fc78f2322445b454afecd37d22d144ac0b92c426 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Tue, 12 Nov 2019 12:23:33 +0200 Subject: [PATCH 8/9] e2e: set access token with oauth client --- tests/e2e/mu-plugins/e2e-rest-access-token.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/tests/e2e/mu-plugins/e2e-rest-access-token.php b/tests/e2e/mu-plugins/e2e-rest-access-token.php index 7c695707664..763c5972b4e 100644 --- a/tests/e2e/mu-plugins/e2e-rest-access-token.php +++ b/tests/e2e/mu-plugins/e2e-rest-access-token.php @@ -9,8 +9,9 @@ * @link https://sitekit.withgoogle.com */ +use Google\Site_Kit\Context; +use Google\Site_Kit\Core\Authentication\Clients\OAuth_Client; use Google\Site_Kit\Core\REST_API\REST_Routes; -use Google\Site_Kit\Core\Storage\Data_Encryption; add_action( 'rest_api_init', function () { if ( ! defined( 'GOOGLESITEKIT_PLUGIN_MAIN_FILE' ) ) { @@ -23,13 +24,8 @@ array( 'methods' => WP_REST_Server::EDITABLE, 'callback' => function ( WP_REST_Request $request ) { - update_user_option( - get_current_user_id(), - 'googlesitekit_access_token', - ( new Data_Encryption() )->encrypt( - serialize( array( 'access_token' => $request['token'] ) ) - ) - ); + ( new OAuth_Client( new Context( GOOGLESITEKIT_PLUGIN_MAIN_FILE ) ) ) + ->set_access_token( $request['token'], HOUR_IN_SECONDS ); return array( 'success' => true, 'token' => $request['token'] ); } From 73957d972dbee799f64a86f066e8ae029360fb62 Mon Sep 17 00:00:00 2001 From: Evan Mattson Date: Tue, 12 Nov 2019 12:24:06 +0200 Subject: [PATCH 9/9] tweak auth-flow test --- tests/e2e/specs/auth-flow.test.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/e2e/specs/auth-flow.test.js b/tests/e2e/specs/auth-flow.test.js index b1fa4222ed6..0a265a8f37c 100644 --- a/tests/e2e/specs/auth-flow.test.js +++ b/tests/e2e/specs/auth-flow.test.js @@ -51,12 +51,9 @@ const disconnectFromSiteKit = async () => { }; describe( 'Site Kit set up flow for the first time', () => { - beforeAll( async () => { - await setSearchConsoleProperty(); - } ); - beforeEach( async () => { await activatePlugin( 'e2e-tests-gcp-credentials-plugin' ); + await setSearchConsoleProperty(); } ); afterEach( async () => { @@ -80,7 +77,6 @@ describe( 'Site Kit set up flow for the first time', () => { it( 'disconnects user from Site Kit', async () => { await setAuthToken(); await setSiteVerification(); - await setSearchConsoleProperty(); await visitAdminPage( 'admin.php', 'page=googlesitekit-dashboard' ); await disconnectFromSiteKit();