Remove token rotation from TokenHandleUtil

The rotation mechanism has been added 20+ milestones ago(in M91) and all
actively used Chromebooks should have it already rotated. In case it
were not it will only lead to a single additional online reauth as a
result of failed token handle check. The metric that captures token
validation response time has been also added to the branch where token
validation fails (because e.g. it has been changed from another device).

Bug: b:274707666
Change-Id: I47cd349ab4fbb71b5cabb254d78910a455f5bd47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4489265
Reviewed-by: Danila Kuzmin <dkuzmin@google.com>
Reviewed-by: Kush Sinha <sinhak@chromium.org>
Commit-Queue: Maciek Slusarczyk <mslus@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@google.com>
Cr-Commit-Position: refs/heads/main@{#1140097}

Author: Maciek Slusarczyk

chrome/browser/ash/login/password_change_browsertest.cc

@@ -589,45 +589,6 @@ IN_PROC_BROWSER_TEST_F(TokenAfterCrash, ValidToken) {
                    ->token_handle_backfill_tried_for_testing());
 }
 
-class RotationTokenTest : public LoginManagerTest {
- public:
-  RotationTokenTest() {
-    login_mixin_.AppendRegularUsers(1);
-    account_id_ = login_mixin_.users()[0].account_id;
-  }
-
- protected:
-  LoginManagerMixin login_mixin_{&mixin_host_};
-  AccountId account_id_;
-};
-
-// Test verifies one-time rotation for the token handle.
-IN_PROC_BROWSER_TEST_F(RotationTokenTest, PRE_Rotated) {
-  TokenHandleUtil::StoreTokenHandle(account_id_, kTokenHandle);
-
-  user_manager::KnownUser known_user(g_browser_process->local_state());
-  // Emulate state before rotation.
-  known_user.RemovePref(account_id_, "TokenHandleRotated");
-
-  // Focus should not trigger online login.
-  LoginScreenTestApi::FocusUser(account_id_);
-  ASSERT_FALSE(LoginScreenTestApi::IsForcedOnlineSignin(account_id_));
-
-  // Should be considered for rotation.
-  EXPECT_TRUE(TokenHandleUtil::ShouldObtainHandle(account_id_));
-
-  login_mixin_.LoginWithDefaultContext(login_mixin_.users().back());
-  login_mixin_.WaitForActiveSession();
-
-  // Emulate obtaining token handle.
-  TokenHandleUtil::StoreTokenHandle(account_id_, kTokenHandle);
-}
-
-IN_PROC_BROWSER_TEST_F(RotationTokenTest, Rotated) {
-  // Token should not be considered for rotation..
-  EXPECT_FALSE(TokenHandleUtil::ShouldObtainHandle(account_id_));
-}
-
 class IgnoreOldTokenTest
     : public LoginManagerTest,
       public LocalStateMixin::Delegate,
@@ -651,10 +612,6 @@ class IgnoreOldTokenTest
       // rotated token.
       return;
     }
-
-    user_manager::KnownUser known_user(g_browser_process->local_state());
-    // Emulate token was not rotated.
-    known_user.RemovePref(account_id_, "TokenHandleRotated");
   }
 
   // LoginManagerTest:
@@ -677,18 +634,18 @@ class IgnoreOldTokenTest
   LocalStateMixin local_state_mixin_{&mixin_host_, this};
 };
 
-// Verify case when a user got token invalidated on a previous version and then
-// updated to the version when not rotated tokens are ignored for managed users.
+// Verify case when a user got token invalidated on a pre-rotated version and
+// then never re-authenticated. Such scenario should now lead to an online
+// sign-in and fetching a new token handle.
 IN_PROC_BROWSER_TEST_P(IgnoreOldTokenTest, PRE_IgnoreNotRotated) {
   ASSERT_TRUE(LoginScreenTestApi::IsForcedOnlineSignin(account_id_));
 }
 
-// Old tokens should be ignored for managed users. Regular users should be
-// forced to go through online signin.
+// If any pre-rotated token handle is still left for either regular or managed
+// user it will verified as invalid and lead to online re-authenication.
 IN_PROC_BROWSER_TEST_P(IgnoreOldTokenTest, IgnoreNotRotated) {
-  ASSERT_NE(TokenHandleUtil::HasToken(account_id_), IsManagedUser());
-  ASSERT_NE(LoginScreenTestApi::IsForcedOnlineSignin(account_id_),
-            IsManagedUser());
+  ASSERT_TRUE(TokenHandleUtil::HasToken(account_id_));
+  ASSERT_TRUE(LoginScreenTestApi::IsForcedOnlineSignin(account_id_));
 }
 
 INSTANTIATE_TEST_SUITE_P(All, IgnoreOldTokenTest, testing::Bool());

chrome/browser/ash/login/signin/token_handle_fetcher.cc

@@ -7,7 +7,7 @@
 #include <memory>
 
 #include "base/functional/bind.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_functions.h"
 #include "chrome/browser/ash/login/signin/token_handle_util.h"
 #include "chrome/browser/ash/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
@@ -146,7 +146,7 @@ void TokenHandleFetcher::OnGetTokenInfoResponse(
   }
   const base::TimeDelta duration =
       base::TimeTicks::Now() - tokeninfo_response_start_time_;
-  UMA_HISTOGRAM_TIMES("Login.TokenObtainResponseTime", duration);
+  base::UmaHistogramTimes("Login.TokenObtainResponseTime", duration);
   std::move(callback_).Run(account_id_, success);
 }
 

chrome/browser/ash/login/signin/token_handle_util.cc

@@ -6,7 +6,7 @@
 
 #include "base/json/values_util.h"
 #include "base/memory/weak_ptr.h"
-#include "base/metrics/histogram_macros.h"
+#include "base/metrics/histogram_functions.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
@@ -20,7 +20,6 @@ namespace {
 const char kTokenHandlePref[] = "PasswordTokenHandle";
 const char kTokenHandleStatusPref[] = "TokenHandleStatus";
 const char kTokenHandleLastCheckedPref[] = "TokenHandleLastChecked";
-const char kTokenHandleRotated[] = "TokenHandleRotated";
 
 const char kHandleStatusValid[] = "valid";
 const char kHandleStatusInvalid[] = "invalid";
@@ -102,15 +101,6 @@ TokenHandleUtil::~TokenHandleUtil() = default;
 // static
 bool TokenHandleUtil::HasToken(const AccountId& account_id) {
   user_manager::KnownUser known_user(g_browser_process->local_state());
-  bool token_rotated =
-      known_user.FindBoolPath(account_id, kTokenHandleRotated).value_or(false);
-  if (!token_rotated && known_user.GetIsEnterpriseManaged(account_id)) {
-    // Ignore not rotated token starting from M94 for enterprise users to avoid
-    // blocking them on the login screen. Rotation started in M91.
-    ClearTokenHandle(account_id);
-    return false;
-  }
-
   const std::string* token =
       known_user.FindStringPath(account_id, kTokenHandlePref);
   return token && !token->empty();
@@ -134,11 +124,7 @@ bool TokenHandleUtil::IsRecentlyChecked(const AccountId& account_id) {
 
 // static
 bool TokenHandleUtil::ShouldObtainHandle(const AccountId& account_id) {
-  user_manager::KnownUser known_user(g_browser_process->local_state());
-  bool token_rotated =
-      known_user.FindBoolPath(account_id, kTokenHandleRotated).value_or(false);
-  return !HasToken(account_id) || HasTokenStatusInvalid(account_id) ||
-         !token_rotated;
+  return !HasToken(account_id) || HasTokenStatusInvalid(account_id);
 }
 
 // static
@@ -186,21 +172,10 @@ void TokenHandleUtil::StoreTokenHandle(const AccountId& account_id,
   known_user.SetStringPref(account_id, kTokenHandlePref, handle);
   known_user.SetStringPref(account_id, kTokenHandleStatusPref,
                            kHandleStatusValid);
-  known_user.SetBooleanPref(account_id, kTokenHandleRotated, true);
   known_user.SetPath(account_id, kTokenHandleLastCheckedPref,
                      base::TimeToValue(base::Time::Now()));
 }
 
-// static
-void TokenHandleUtil::ClearTokenHandle(const AccountId& account_id) {
-  user_manager::KnownUser known_user(g_browser_process->local_state());
-
-  known_user.RemovePref(account_id, kTokenHandlePref);
-  known_user.RemovePref(account_id, kTokenHandleStatusPref);
-  known_user.RemovePref(account_id, kTokenHandleRotated);
-  known_user.RemovePref(account_id, kTokenHandleLastCheckedPref);
-}
-
 // static
 void TokenHandleUtil::SetInvalidTokenForTesting(const char* token) {
   g_invalid_token_for_testing = token;
@@ -237,18 +212,27 @@ TokenHandleUtil::TokenDelegate::~TokenDelegate() = default;
 
 void TokenHandleUtil::TokenDelegate::OnOAuthError() {
   std::move(callback_).Run(account_id_, INVALID);
-  NotifyDone();
+  NotifyDone(/*request_completed=*/true);
 }
 
 // Warning: NotifyDone() deletes `this`
-void TokenHandleUtil::TokenDelegate::NotifyDone() {
+void TokenHandleUtil::TokenDelegate::NotifyDone(bool request_completed) {
+  if (request_completed) {
+    RecordTokenCheckResponseTime();
+  }
   if (owner_)
     owner_->OnValidationComplete(token_);
 }
 
 void TokenHandleUtil::TokenDelegate::OnNetworkError(int response_code) {
   std::move(callback_).Run(account_id_, UNKNOWN);
-  NotifyDone();
+  NotifyDone(/*request_completed=*/response_code != -1);
+}
+
+void TokenHandleUtil::TokenDelegate::RecordTokenCheckResponseTime() {
+  const base::TimeDelta duration =
+      base::TimeTicks::Now() - tokeninfo_response_start_time_;
+  base::UmaHistogramTimes("Login.TokenCheckResponseTime", duration);
 }
 
 void TokenHandleUtil::TokenDelegate::OnGetTokenInfoResponse(
@@ -260,11 +244,8 @@ void TokenHandleUtil::TokenDelegate::OnGetTokenInfoResponse(
       outcome = (*expires_in < 0) ? INVALID : VALID;
   }
 
-  const base::TimeDelta duration =
-      base::TimeTicks::Now() - tokeninfo_response_start_time_;
-  UMA_HISTOGRAM_TIMES("Login.TokenCheckResponseTime", duration);
   std::move(callback_).Run(account_id_, outcome);
-  NotifyDone();
+  NotifyDone(/*request_completed=*/true);
 }
 
 }  // namespace ash

chrome/browser/ash/login/signin/token_handle_util.h

@@ -58,8 +58,6 @@ class TokenHandleUtil {
   static void StoreTokenHandle(const AccountId& account_id,
                                const std::string& handle);
 
-  static void ClearTokenHandle(const AccountId& account_id);
-
   static void SetInvalidTokenForTesting(const char* token);
 
   static void SetLastCheckedPrefForTesting(const AccountId& account_id,
@@ -81,12 +79,18 @@ class TokenHandleUtil {
 
     ~TokenDelegate() override;
 
+    // gaia::GaiaOAuthClient::Delegate overrides.
     void OnOAuthError() override;
     void OnNetworkError(int response_code) override;
     void OnGetTokenInfoResponse(const base::Value::Dict& token_info) override;
-    void NotifyDone();
+
+    // Completes the validation request at the owner TokenHandleUtil. The bool
+    // flag signals if we actually got any data from the Gaia endpoint.
+    void NotifyDone(bool request_completed);
 
    private:
+    void RecordTokenCheckResponseTime();
+
     base::WeakPtr<TokenHandleUtil> owner_;
     AccountId account_id_;
     std::string token_;

components/user_manager/known_user.cc

@@ -109,6 +109,9 @@ const char kOnboardingCompletedVersion[] = "onboarding_completed_version";
 // Last screen shown in the onboarding flow.
 const char kPendingOnboardingScreen[] = "onboarding_screen_pending";
 
+// Key of the obsolete token handle rotation flag.
+const char kTokenHandleRotatedObsolete[] = "TokenHandleRotated";
+
 // List containing all the known user preferences keys.
 const char* kReservedKeys[] = {kCanonicalEmail,
                                kGAIAIdKey,
@@ -139,6 +142,7 @@ const char* kObsoleteKeys[] = {
     kMinimalMigrationAttemptedObsolete,
     kGaiaIdMigrationObsolete,
     kOfflineSigninLimitObsolete,
+    kTokenHandleRotatedObsolete,
 };
 
 // Checks if values in |dict| correspond with |account_id| identity.