Merge pull request #830 from google/fix/revoke-token-on-invalid-grant

Ensure that user credentials are consistently wiped on refresh token failure
google · Nov 12, 2019 · 9ebdf1b · 9ebdf1b
2 parents 66f7cda + 73957d9
commit 9ebdf1b
Show file tree

Hide file tree

Showing 7 changed files with 120 additions and 74 deletions.
diff --git 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 );
@@ -456,18 +449,10 @@ 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();
-
-		// 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' );
+		// 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();
 		}
 	}
 

diff --git 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
@@ -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();
@@ -231,7 +237,9 @@ public function get_client() {
 	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.
@@ -250,16 +258,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;
@@ -286,7 +292,13 @@ 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();
 	}
 
 	/**
@@ -504,12 +516,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 +774,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.
 	 *

diff --git 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'] );
 			}

diff --git 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();

diff --git 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
@@ -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,
+		);
+	}
 }