cros login: Do not trigger online signin on login and in the session.

This CL fixes the issue when one invalid "password changed" token
triggers online reauth two times - on the login screen and in the
session. Check in the session happens before the new token is backfilled
with the online login triggered on the login screen.
So this CL adds backoff time between two consecutive checks.
Also moved more relative logic into TokenHandleUtil.

Fixed: 1094030
Change-Id: Ica0a9c547f14e8f0bc743fdab87fff7bf49c56c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2300441
Auto-Submit: Roman Sorokin [CET] <rsorokin@chromium.org>
Commit-Queue: Alexander Alekseev <alemate@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792655}

chrome/browser/chromeos/login/password_change_browsertest.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "ash/public/cpp/login_screen_test_api.h"
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
@@ -20,6 +21,7 @@
 #include "chrome/browser/chromeos/login/reauth_stats.h"
 #include "chrome/browser/chromeos/login/session/user_session_manager.h"
 #include "chrome/browser/chromeos/login/session/user_session_manager_test_api.h"
+#include "chrome/browser/chromeos/login/signin/signin_error_notifier_ash.h"
 #include "chrome/browser/chromeos/login/signin_specifics.h"
 #include "chrome/browser/chromeos/login/test/js_checker.h"
 #include "chrome/browser/chromeos/login/test/login_manager_mixin.h"
@@ -262,6 +264,8 @@ class PasswordChangeTokenCheck : public PasswordChangeTestBase {
   PasswordChangeTokenCheck() {
     login_mixin_.AppendRegularUsers(1);
     user_with_invalid_token_ = login_mixin_.users().back().account_id;
+    ignore_sync_errors_for_test_ =
+        SigninErrorNotifier::IgnoreSyncErrorsForTesting();
   }
 
  protected:
@@ -283,6 +287,7 @@ class PasswordChangeTokenCheck : public PasswordChangeTestBase {
   }
 
   AccountId user_with_invalid_token_;
+  std::unique_ptr<base::AutoReset<bool>> ignore_sync_errors_for_test_;
 };
 
 IN_PROC_BROWSER_TEST_F(PasswordChangeTokenCheck, LoginScreenPasswordChange) {
@@ -345,6 +350,9 @@ IN_PROC_BROWSER_TEST_F(PasswordChangeTokenCheck, PRE_Session) {
 
   // Store invalid token to triger notification in the session.
   TokenHandleUtil::StoreTokenHandle(user_with_invalid_token_, kTokenHandle);
+  // Make token not "checked recently".
+  TokenHandleUtil::SetLastCheckedPrefForTesting(user_with_invalid_token_,
+                                                base::Time());
 
   ProfileWaiter waiter;
   login_mixin_.LoginWithDefaultContext(login_mixin_.users().back());
@@ -381,4 +389,26 @@ IN_PROC_BROWSER_TEST_F(PasswordChangeTokenCheck, Session) {
                                      ReauthReason::INVALID_TOKEN_HANDLE, 1);
 }
 
+// Notification should not be triggered because token was checked on the login
+// screen - recently.
+IN_PROC_BROWSER_TEST_F(PasswordChangeTokenCheck, TokenRecentlyChecked) {
+  TokenHandleUtil::StoreTokenHandle(user_with_invalid_token_, kTokenHandle);
+  // Focus triggers token check.
+  ash::LoginScreenTestApi::FocusUser(user_with_invalid_token_);
+  OpenGaiaDialog(user_with_invalid_token_);
+
+  ProfileWaiter waiter;
+  login_mixin_.LoginWithDefaultContext(login_mixin_.users().back());
+  // We need to replace notification service very early to intercept reauth
+  // notification.
+  auto display_service_tester = waiter.Wait();
+
+  login_mixin_.WaitForActiveSession();
+
+  std::vector<message_center::Notification> notifications =
+      display_service_tester->GetDisplayedNotificationsForType(
+          NotificationHandler::Type::TRANSIENT);
+  ASSERT_EQ(notifications.size(), 0u);
+}
+
 }  // namespace chromeos

chrome/browser/chromeos/login/screens/user_selection_screen.cc

@@ -734,7 +734,6 @@ void UserSelectionScreen::OnUserStatusChecked(
     TokenHandleUtil::TokenHandleStatus status) {
   if (status == TokenHandleUtil::INVALID) {
     RecordReauthReason(account_id, ReauthReason::INVALID_TOKEN_HANDLE);
-    token_handle_util_->MarkHandleInvalid(account_id);
     SetAuthType(account_id, proximity_auth::mojom::AuthType::ONLINE_SIGN_IN,
                 base::string16());
   }

chrome/browser/chromeos/login/signin/signin_error_notifier_ash.cc

@@ -54,6 +54,8 @@ namespace {
 constexpr char kProfileSigninNotificationId[] = "chrome://settings/signin/";
 constexpr char kSecondaryAccountNotificationIdSuffix[] = "/secondary-account";
 
+bool g_ignore_sync_errors_for_test_ = false;
+
 void HandleDeviceAccountReauthNotificationClick(
     base::Optional<int> button_index) {
   chrome::AttemptUserExit();
@@ -90,7 +92,8 @@ SigninErrorNotifier::SigninErrorNotifier(SigninErrorController* controller,
   error_controller_->AddObserver(this);
   const AccountId account_id =
       multi_user_util::GetAccountIdFromProfile(profile_);
-  if (TokenHandleUtil::HasToken(account_id)) {
+  if (TokenHandleUtil::HasToken(account_id) &&
+      !TokenHandleUtil::IsRecentlyChecked(account_id)) {
     token_handle_util_ = std::make_unique<TokenHandleUtil>();
     token_handle_util_->CheckToken(
         account_id, profile->GetURLLoaderFactory(),
@@ -114,12 +117,22 @@ SigninErrorNotifier::~SigninErrorNotifier() {
       << "SigninErrorNotifier::Shutdown() was not called";
 }
 
+// static
+std::unique_ptr<base::AutoReset<bool>>
+SigninErrorNotifier::IgnoreSyncErrorsForTesting() {
+  return std::make_unique<base::AutoReset<bool>>(
+      &g_ignore_sync_errors_for_test_, true);
+}
+
 void SigninErrorNotifier::Shutdown() {
   error_controller_->RemoveObserver(this);
   error_controller_ = nullptr;
 }
 
 void SigninErrorNotifier::OnErrorChanged() {
+  if (g_ignore_sync_errors_for_test_)
+    return;
+
   if (!error_controller_->HasError()) {
     NotificationDisplayService::GetForProfile(profile_)->Close(
         NotificationHandler::Type::TRANSIENT, device_account_notification_id_);
@@ -178,10 +191,6 @@ void SigninErrorNotifier::HandleDeviceAccountError() {
   user_manager::UserManager::Get()->SaveForceOnlineSignin(
       account_id, true /* force_online_signin */);
 
-  // We need to remove the handle so it won't be checked next time session is
-  // started.
-  TokenHandleUtil::DeleteHandle(account_id);
-
   // Add an accept button to sign the user out.
   message_center::RichNotificationData data;
   data.buttons.push_back(message_center::ButtonInfo(

chrome/browser/chromeos/login/signin/signin_error_notifier_ash.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/auto_reset.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
@@ -31,6 +32,8 @@ class SigninErrorNotifier : public SigninErrorController::Observer,
   SigninErrorNotifier(SigninErrorController* controller, Profile* profile);
   ~SigninErrorNotifier() override;
 
+  static std::unique_ptr<base::AutoReset<bool>> IgnoreSyncErrorsForTesting();
+
   // KeyedService:
   void Shutdown() override;
 

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

@@ -6,6 +6,7 @@
 
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/util/values/values_util.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
@@ -17,6 +18,7 @@ namespace {
 
 const char kTokenHandlePref[] = "PasswordTokenHandle";
 const char kTokenHandleStatusPref[] = "TokenHandleStatus";
+const char kTokenHandleLastCheckedPref[] = "TokenHandleLastChecked";
 
 const char kHandleStatusValid[] = "valid";
 const char kHandleStatusInvalid[] = "invalid";
@@ -24,8 +26,49 @@ const char* const kDefaultHandleStatus = kHandleStatusValid;
 
 constexpr int kMaxRetries = 3;
 
+constexpr base::TimeDelta kCacheStatusTime = base::TimeDelta::FromHours(1);
+
 const char* g_invalid_token_for_testing = nullptr;
 
+bool MaybeReturnCachedStatus(
+    const AccountId& account_id,
+    const TokenHandleUtil::TokenValidationCallback& callback) {
+  std::string saved_status;
+  if (!user_manager::known_user::GetStringPref(
+          account_id, kTokenHandleStatusPref, &saved_status)) {
+    return false;
+  }
+
+  if (saved_status == kHandleStatusValid) {
+    callback.Run(account_id, TokenHandleUtil::VALID);
+    return true;
+  }
+
+  if (saved_status == kHandleStatusInvalid) {
+    callback.Run(account_id, TokenHandleUtil::INVALID);
+    return true;
+  }
+
+  NOTREACHED();
+  return false;
+}
+
+void OnStatusChecked(const TokenHandleUtil::TokenValidationCallback& callback,
+                     const AccountId& account_id,
+                     TokenHandleUtil::TokenHandleStatus status) {
+  if (status != TokenHandleUtil::UNKNOWN) {
+    // Update last checked timestamp.
+    user_manager::known_user::SetPref(account_id, kTokenHandleLastCheckedPref,
+                                      util::TimeToValue(base::Time::Now()));
+  }
+
+  if (status == TokenHandleUtil::INVALID) {
+    user_manager::known_user::SetStringPref(account_id, kTokenHandleStatusPref,
+                                            kHandleStatusInvalid);
+  }
+  callback.Run(account_id, status);
+}
+
 }  // namespace
 
 TokenHandleUtil::TokenHandleUtil() {}
@@ -43,6 +86,22 @@ bool TokenHandleUtil::HasToken(const AccountId& account_id) {
   return !token.empty();
 }
 
+// static
+bool TokenHandleUtil::IsRecentlyChecked(const AccountId& account_id) {
+  const base::Value* value;
+  if (!user_manager::known_user::GetPref(account_id,
+                                         kTokenHandleLastCheckedPref, &value)) {
+    return false;
+  }
+
+  base::Optional<base::Time> last_checked = util::ValueToTime(value);
+  if (!last_checked.has_value()) {
+    return false;
+  }
+
+  return base::Time::Now() - last_checked.value() < kCacheStatusTime;
+}
+
 // static
 bool TokenHandleUtil::ShouldObtainHandle(const AccountId& account_id) {
   const base::DictionaryValue* dict = nullptr;
@@ -58,24 +117,6 @@ bool TokenHandleUtil::ShouldObtainHandle(const AccountId& account_id) {
   return kHandleStatusInvalid == status;
 }
 
-// static
-void TokenHandleUtil::DeleteHandle(const AccountId& account_id) {
-  const base::DictionaryValue* dict = nullptr;
-  if (!user_manager::known_user::FindPrefs(account_id, &dict))
-    return;
-  std::unique_ptr<base::DictionaryValue> dict_copy(dict->DeepCopy());
-  dict_copy->Remove(kTokenHandlePref, nullptr);
-  dict_copy->Remove(kTokenHandleStatusPref, nullptr);
-  user_manager::known_user::UpdatePrefs(account_id, *dict_copy.get(),
-                                        /* replace values */ true);
-}
-
-// static
-void TokenHandleUtil::MarkHandleInvalid(const AccountId& account_id) {
-  user_manager::known_user::SetStringPref(account_id, kTokenHandleStatusPref,
-                                          kHandleStatusInvalid);
-}
-
 // static
 void TokenHandleUtil::CheckToken(
     const AccountId& account_id,
@@ -97,10 +138,15 @@ void TokenHandleUtil::CheckToken(
     return;
   }
 
+  if (IsRecentlyChecked(account_id) &&
+      MaybeReturnCachedStatus(account_id, callback)) {
+    return;
+  }
+
   // Constructor starts validation.
   validation_delegates_[token] = std::make_unique<TokenDelegate>(
       weak_factory_.GetWeakPtr(), account_id, token,
-      std::move(url_loader_factory), callback);
+      std::move(url_loader_factory), base::Bind(&OnStatusChecked, callback));
 }
 
 // static
@@ -109,6 +155,8 @@ void TokenHandleUtil::StoreTokenHandle(const AccountId& account_id,
   user_manager::known_user::SetStringPref(account_id, kTokenHandlePref, handle);
   user_manager::known_user::SetStringPref(account_id, kTokenHandleStatusPref,
                                           kHandleStatusValid);
+  user_manager::known_user::SetPref(account_id, kTokenHandleLastCheckedPref,
+                                    util::TimeToValue(base::Time::Now()));
 }
 
 // static
@@ -116,6 +164,13 @@ void TokenHandleUtil::SetInvalidTokenForTesting(const char* token) {
   g_invalid_token_for_testing = token;
 }
 
+// static
+void TokenHandleUtil::SetLastCheckedPrefForTesting(const AccountId& account_id,
+                                                   base::Time time) {
+  user_manager::known_user::SetPref(account_id, kTokenHandleLastCheckedPref,
+                                    util::TimeToValue(time));
+}
+
 void TokenHandleUtil::OnValidationComplete(const std::string& token) {
   validation_delegates_.erase(token);
 }

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

@@ -38,12 +38,9 @@ class TokenHandleUtil {
   // Returns true if UserManager has token handle associated with |account_id|.
   static bool HasToken(const AccountId& account_id);
 
-  // Removes token handle for |account_id| from UserManager storage.
-  static void DeleteHandle(const AccountId& account_id);
-
-  // Marks current handle as invalid, new one should be obtained at next sign
-  // in.
-  static void MarkHandleInvalid(const AccountId& account_id);
+  // Returns true if the token status for |account_id| was checked recently
+  // (within kCacheStatusTime).
+  static bool IsRecentlyChecked(const AccountId& account_id);
 
   // Indicates if token handle for |account_id| is missing or marked as invalid.
   static bool ShouldObtainHandle(const AccountId& account_id);
@@ -61,6 +58,9 @@ class TokenHandleUtil {
 
   static void SetInvalidTokenForTesting(const char* token);
 
+  static void SetLastCheckedPrefForTesting(const AccountId& account_id,
+                                           base::Time time);
+
  private:
   // Associates GaiaOAuthClient::Delegate with User ID and Token.
   class TokenDelegate : public gaia::GaiaOAuthClient::Delegate {