[signin] UpdateCredentials() revokes the existing token

When a refresh token was replaced by a new one, it was not revoked, and
essentially leaking.

OnRefreshTokenRevoked() is not called because it is used to signal that
the associated account no longer has a valid token. This is not the case
here, since the token is immediately replaced by a new one.

Bug: 735898
Change-Id: If0f9777543d574a00084d7dd6b67209aeacf150c
Reviewed-on: https://chromium-review.googlesource.com/568303Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Commit-Queue: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485986}

chrome/browser/signin/dice_browsertest.cc

@@ -57,6 +57,7 @@ const char kGoogleSignoutResponseHeader[] = "Google-Accounts-SignOut";
 const char kMainEmail[] = "main_email@example.com";
 const char kMainGaiaID[] = "main_gaia_id";
 const char kOAuth2TokenExchangeURL[] = "/oauth2/v4/token";
+const char kOAuth2TokenRevokeURL[] = "/o/oauth2/revoke";
 const char kSecondaryEmail[] = "secondary_email@example.com";
 const char kSecondaryGaiaID[] = "secondary_gaia_id";
 const char kSigninURL[] = "/signin";
@@ -152,7 +153,7 @@ std::unique_ptr<HttpResponse> HandleOAuth2TokenExchangeURL(
   std::string content =
       "{"
       "  \"access_token\":\"access_token\","
-      "  \"refresh_token\":\"refresh_token\","
+      "  \"refresh_token\":\"new_refresh_token\","
       "  \"expires_in\":9999"
       "}";
 
@@ -162,6 +163,20 @@ std::unique_ptr<HttpResponse> HandleOAuth2TokenExchangeURL(
   return std::move(http_response);
 }
 
+// Handler for OAuth2 token revocation.
+std::unique_ptr<HttpResponse> HandleOAuth2TokenRevokeURL(
+    int* out_token_revoked_count,
+    const HttpRequest& request) {
+  if (!net::test_server::ShouldHandle(request, kOAuth2TokenRevokeURL))
+    return nullptr;
+
+  ++(*out_token_revoked_count);
+
+  std::unique_ptr<BasicHttpResponse> http_response(new BasicHttpResponse);
+  http_response->AddCustomHeader("Cache-Control", "no-store");
+  return std::move(http_response);
+}
+
 }  // namespace FakeGaia
 
 class DiceBrowserTest : public InProcessBrowserTest,
@@ -172,13 +187,17 @@ class DiceBrowserTest : public InProcessBrowserTest,
   DiceBrowserTest()
       : https_server_(net::EmbeddedTestServer::TYPE_HTTPS),
         token_requested_(false),
-        refresh_token_available_(false) {
+        refresh_token_available_(false),
+        token_revoked_notification_count_(0),
+        token_revoked_count_(0) {
     https_server_.RegisterDefaultHandler(
         base::Bind(&FakeGaia::HandleSigninURL));
     https_server_.RegisterDefaultHandler(
         base::Bind(&FakeGaia::HandleSignoutURL));
     https_server_.RegisterDefaultHandler(
         base::Bind(&FakeGaia::HandleOAuth2TokenExchangeURL, &token_requested_));
+    https_server_.RegisterDefaultHandler(base::Bind(
+        &FakeGaia::HandleOAuth2TokenRevokeURL, &token_revoked_count_));
   }
 
   // Navigates to the given path on the test server.
@@ -219,7 +238,7 @@ class DiceBrowserTest : public InProcessBrowserTest,
     // Signin main account.
     SigninManager* signin_manager = GetSigninManager();
     signin_manager->StartSignInWithRefreshToken(
-        "refresh_token", kMainGaiaID, kMainEmail, "password",
+        "existing_refresh_token", kMainGaiaID, kMainEmail, "password",
         SigninManager::OAuthTokenFetchedCallback());
     ASSERT_TRUE(GetTokenService()->RefreshTokenIsAvailable(GetMainAccountID()));
     ASSERT_EQ(GetMainAccountID(), signin_manager->GetAuthenticatedAccountId());
@@ -255,6 +274,7 @@ class DiceBrowserTest : public InProcessBrowserTest,
     const GURL& base_url = https_server_.base_url();
     command_line->AppendSwitchASCII(switches::kGaiaUrl, base_url.spec());
     command_line->AppendSwitchASCII(switches::kGoogleApisUrl, base_url.spec());
+    command_line->AppendSwitchASCII(switches::kLsoUrl, base_url.spec());
     switches::EnableAccountConsistencyDiceForTesting(command_line);
   }
 
@@ -274,9 +294,15 @@ class DiceBrowserTest : public InProcessBrowserTest,
       refresh_token_available_ = true;
   }
 
+  void OnRefreshTokenRevoked(const std::string& account_id) override {
+    ++token_revoked_notification_count_;
+  }
+
   net::EmbeddedTestServer https_server_;
   bool token_requested_;
   bool refresh_token_available_;
+  int token_revoked_notification_count_;
+  int token_revoked_count_;
 };
 
 // Checks that signin on Gaia triggers the fetch for a refresh token.
@@ -329,6 +355,9 @@ IN_PROC_BROWSER_TEST_F(DiceBrowserTest, Reauth) {
   EXPECT_TRUE(refresh_token_available_);
   EXPECT_EQ(GetMainAccountID(),
             GetSigninManager()->GetAuthenticatedAccountId());
+  // Old token must be revoked silently.
+  EXPECT_EQ(0, token_revoked_notification_count_);
+  EXPECT_EQ(1, token_revoked_count_);
 }
 
 // Checks that the Dice signout flow works and deletes all tokens.
@@ -344,6 +373,8 @@ IN_PROC_BROWSER_TEST_F(DiceBrowserTest, SignoutMainAccount) {
   EXPECT_FALSE(GetTokenService()->RefreshTokenIsAvailable(GetMainAccountID()));
   EXPECT_FALSE(
       GetTokenService()->RefreshTokenIsAvailable(GetSecondaryAccountID()));
+  EXPECT_EQ(2, token_revoked_notification_count_);
+  EXPECT_EQ(2, token_revoked_count_);
 }
 
 // Checks that signing out from a secondary account does not delete the main
@@ -362,6 +393,8 @@ IN_PROC_BROWSER_TEST_F(DiceBrowserTest, SignoutSecondaryAccount) {
   EXPECT_TRUE(GetTokenService()->RefreshTokenIsAvailable(GetMainAccountID()));
   EXPECT_FALSE(
       GetTokenService()->RefreshTokenIsAvailable(GetSecondaryAccountID()));
+  EXPECT_EQ(1, token_revoked_notification_count_);
+  EXPECT_EQ(1, token_revoked_count_);
 }
 
 // Checks that the Dice signout flow works and deletes all tokens.
@@ -377,4 +410,6 @@ IN_PROC_BROWSER_TEST_F(DiceBrowserTest, SignoutAllAccounts) {
   EXPECT_FALSE(GetTokenService()->RefreshTokenIsAvailable(GetMainAccountID()));
   EXPECT_FALSE(
       GetTokenService()->RefreshTokenIsAvailable(GetSecondaryAccountID()));
+  EXPECT_EQ(2, token_revoked_notification_count_);
+  EXPECT_EQ(2, token_revoked_count_);
 }

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.cc

@@ -476,7 +476,7 @@ void MutableProfileOAuth2TokenServiceDelegate::UpdateCredentials(
     if (refresh_token_present) {
       VLOG(1) << "MutablePO2TS::UpdateCredentials; Refresh Token was present. "
               << "account_id=" << account_id;
-
+      RevokeCredentialsOnServer(refresh_tokens_[account_id]->refresh_token());
       refresh_tokens_[account_id]->set_refresh_token(refresh_token);
     } else {
       VLOG(1) << "MutablePO2TS::UpdateCredentials; Refresh Token was absent. "

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate.h

@@ -107,6 +107,8 @@ class MutableProfileOAuth2TokenServiceDelegate
       PersistenceLoadCredentialsEmptyPrimaryAccountId_DiceEnabled);
   FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
                            PersistenceLoadCredentials);
+  FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
+                           RevokeOnUpdate);
   FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,
                            GetAccounts);
   FRIEND_TEST_ALL_PREFIXES(MutableProfileOAuth2TokenServiceDelegateTest,

chrome/browser/signin/mutable_profile_oauth2_token_service_delegate_unittest.cc

@@ -414,7 +414,40 @@ TEST_F(MutableProfileOAuth2TokenServiceDelegateTest,
 }
 #endif  // BUILDFLAG(ENABLE_DICE_SUPPORT)
 
-TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, PersistanceNotifications) {
+// Tests that calling UpdateCredentials revokes the old token, without sending
+// the notification.
+TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, RevokeOnUpdate) {
+  // Add a token.
+  ASSERT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+  oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token");
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+  ExpectOneTokenAvailableNotification();
+
+  // Update the token.
+  oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token2");
+  EXPECT_EQ(1u, oauth2_service_delegate_->server_revokes_.size());
+  ExpectOneTokenAvailableNotification();
+
+  // Flush the server revokes.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+
+  // Set the same token again.
+  oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token2");
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+  ExpectNoNotifications();
+
+  // Clear the token.
+  oauth2_service_delegate_->RevokeAllCredentials();
+  EXPECT_EQ(1u, oauth2_service_delegate_->server_revokes_.size());
+  ExpectOneTokenRevokedNotification();
+
+  // Flush the server revokes.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(oauth2_service_delegate_->server_revokes_.empty());
+}
+
+TEST_F(MutableProfileOAuth2TokenServiceDelegateTest, PersistenceNotifications) {
   oauth2_service_delegate_->UpdateCredentials("account_id", "refresh_token");
   ExpectOneTokenAvailableNotification();