Minor bug fixing and refactoring in AdvancedProtectionStatusManager

If user stays in advanced protection,  ::OnGetIDToken(..) will not
trigger scheduling next refresh. This CL fixes this issue.

This CL also enables most unit tests on Chrome OS.

Bug: 880931
Change-Id: I59d2c30b7a87bb59b755bea0b31b32081d21ef11
Reviewed-on: https://chromium-review.googlesource.com/1207577Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588993}

chrome/browser/safe_browsing/advanced_protection_status_manager.cc

@@ -91,7 +91,6 @@ void AdvancedProtectionStatusManager::MaybeRefreshOnStartUp() {
     return;
 
   is_under_advanced_protection_ = info.is_under_advanced_protection;
-  primary_account_id_ = info.account_id;
 
   if (profile_->GetPrefs()->HasPrefPath(
           prefs::kAdvancedProtectionLastRefreshInUs)) {
@@ -157,8 +156,8 @@ void AdvancedProtectionStatusManager::OnAccountRemoved(
     return;
 
   // If user signed out primary account, cancel refresh.
-  if (!primary_account_id_.empty() && primary_account_id_ == info.account_id) {
-    primary_account_id_.clear();
+  std::string primary_account_id = GetPrimaryAccountId();
+  if (!primary_account_id.empty() && primary_account_id == info.account_id) {
     is_under_advanced_protection_ = false;
     OnAdvancedProtectionDisabled();
   }
@@ -166,15 +165,12 @@ void AdvancedProtectionStatusManager::OnAccountRemoved(
 
 void AdvancedProtectionStatusManager::GoogleSigninSucceeded(
     const AccountInfo& account_info) {
-  primary_account_id_ = account_info.account_id;
   if (account_info.is_under_advanced_protection)
     OnAdvancedProtectionEnabled();
 }
 
 void AdvancedProtectionStatusManager::GoogleSignedOut(
     const AccountInfo& account_info) {
-  if (primary_account_id_ == account_info.account_id)
-    primary_account_id_.clear();
   OnAdvancedProtectionDisabled();
 }
 
@@ -196,7 +192,8 @@ void AdvancedProtectionStatusManager::RefreshAdvancedProtectionStatus() {
   auto* token_service =
       ProfileOAuth2TokenServiceFactory::GetForProfile(profile_);
 
-  if (!token_service || !IsUserSignedIn() || primary_account_id_.empty())
+  std::string primary_account_id = GetPrimaryAccountId();
+  if (!token_service || primary_account_id.empty())
     return;
   // Refresh OAuth access token.
   OAuth2TokenService::ScopeSet scopes;
@@ -207,9 +204,9 @@ void AdvancedProtectionStatusManager::RefreshAdvancedProtectionStatus() {
     return;
 
   token_consumer_.reset(
-      new AdvancedProtectionTokenConsumer(primary_account_id_, this));
+      new AdvancedProtectionTokenConsumer(primary_account_id, this));
   access_token_request_ = token_service->StartRequest(
-      primary_account_id_, scopes, token_consumer_.get());
+      primary_account_id, scopes, token_consumer_.get());
 }
 
 void AdvancedProtectionStatusManager::ScheduleNextRefresh() {
@@ -251,15 +248,10 @@ bool AdvancedProtectionStatusManager::IsUnderAdvancedProtection(
       ->is_under_advanced_protection();
 }
 
-bool AdvancedProtectionStatusManager::IsUserSignedIn() {
-  return signin_manager_ &&
-         !signin_manager_->GetAuthenticatedAccountInfo().account_id.empty();
-}
-
 bool AdvancedProtectionStatusManager::IsPrimaryAccount(
     const AccountInfo& account_info) {
-  return IsUserSignedIn() && account_info.account_id ==
-                                 signin_manager_->GetAuthenticatedAccountId();
+  return !account_info.account_id.empty() &&
+         account_info.account_id == GetPrimaryAccountId();
 }
 
 void AdvancedProtectionStatusManager::OnGetIDToken(
@@ -267,16 +259,23 @@ void AdvancedProtectionStatusManager::OnGetIDToken(
     const std::string& id_token) {
   // Skips if the ID token is not for the primary account. Or user is no longer
   // signed in.
-  if (account_id != primary_account_id_ || !IsUserSignedIn())
+  std::string primary_account_id = GetPrimaryAccountId();
+  if (primary_account_id.empty() || account_id != primary_account_id)
     return;
 
   gaia::TokenServiceFlags service_flags = gaia::ParseServiceFlags(id_token);
-  // |OnAccountUpdated()| will only be triggered if the advanced protection
-  // status changed. Therefore, we need to call |UpdateLastRefreshTime()| here
-  // to force update and persist last refresh time.
-  account_tracker_service_->SetIsAdvancedProtectionAccount(
-      primary_account_id_, service_flags.is_under_advanced_protection);
-  UpdateLastRefreshTime();
+
+  // If there's a change in advanced protection status, updates account info.
+  // This also triggers |OnAccountUpdated()|.
+  if (is_under_advanced_protection_ !=
+      service_flags.is_under_advanced_protection) {
+    account_tracker_service_->SetIsAdvancedProtectionAccount(
+        GetPrimaryAccountId(), service_flags.is_under_advanced_protection);
+  } else if (service_flags.is_under_advanced_protection) {
+    OnAdvancedProtectionEnabled();
+  } else {
+    OnAdvancedProtectionDisabled();
+  }
 }
 
 void AdvancedProtectionStatusManager::OnTokenRefreshDone(
@@ -310,4 +309,9 @@ AdvancedProtectionStatusManager::AdvancedProtectionStatusManager(
   MaybeRefreshOnStartUp();
 }
 
+std::string AdvancedProtectionStatusManager::GetPrimaryAccountId() const {
+  return signin_manager_ ? signin_manager_->GetAuthenticatedAccountId()
+                         : std::string();
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/advanced_protection_status_manager.h

@@ -63,9 +63,6 @@ class AdvancedProtectionStatusManager : public KeyedService,
 
   bool IsRefreshScheduled();
 
-  // If primary user of this profile is signed in.
-  bool IsUserSignedIn();
-
  private:
   FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
                            NotSignedInOnStartUp);
@@ -82,6 +79,8 @@ class AdvancedProtectionStatusManager : public KeyedService,
   FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
                            SignInAndSignOutEvent);
   FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest, AccountRemoval);
+  FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
+                           StayInAdvancedProtection);
 
   void Initialize();
 
@@ -130,6 +129,10 @@ class AdvancedProtectionStatusManager : public KeyedService,
   // Only called in tests.
   void SetMinimumRefreshDelay(const base::TimeDelta& delay);
 
+  // Gets the account ID of the primary account of |profile_|.
+  // Returns an empty string if user is not signed in.
+  std::string GetPrimaryAccountId() const;
+
   // Only called in tests to set a customized minimum delay.
   AdvancedProtectionStatusManager(Profile* profile,
                                   const base::TimeDelta& min_delay);
@@ -142,10 +145,6 @@ class AdvancedProtectionStatusManager : public KeyedService,
   // Is the profile account under advanced protection.
   bool is_under_advanced_protection_;
 
-  // Account ID of the primary account of |profile_|. If this profile is not
-  // signed in, this field will be empty.
-  std::string primary_account_id_;
-
   std::unique_ptr<AdvancedProtectionTokenConsumer> token_consumer_;
 
   std::unique_ptr<OAuth2TokenService::Request> access_token_request_;

chrome/browser/safe_browsing/advanced_protection_status_manager_unittest.cc

@@ -47,7 +47,6 @@ class AdvancedProtectionStatusManagerTest : public testing::Test {
 
   ~AdvancedProtectionStatusManagerTest() override {}
 
-#if !defined(OS_CHROMEOS)
   std::string SignIn(const std::string& gaia_id,
                      const std::string& email,
                      bool is_under_advanced_protection) {
@@ -57,11 +56,14 @@ class AdvancedProtectionStatusManagerTest : public testing::Test {
     account_info.is_under_advanced_protection = is_under_advanced_protection;
     std::string account_id =
         account_tracker_service_->SeedAccountInfo(account_info);
+#if defined(OS_CHROMEOS)
+    fake_signin_manager_->SignIn(account_id);
+#else
     fake_signin_manager_->SignIn(gaia_id, email, "password");
+#endif
     GetTokenService()->UpdateCredentials(account_id, "refresh_token");
     return account_id;
   }
-#endif
 
   FakeProfileOAuth2TokenService* GetTokenService() {
     ProfileOAuth2TokenService* service =
@@ -104,13 +106,12 @@ class AdvancedProtectionStatusManagerTest : public testing::Test {
 
 }  // namespace
 
-#if !defined(OS_CHROMEOS)
 TEST_F(AdvancedProtectionStatusManagerTest, NotSignedInOnStartUp) {
   ASSERT_FALSE(testing_profile_->GetPrefs()->HasPrefPath(
       prefs::kAdvancedProtectionLastRefreshInUs));
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
-  ASSERT_FALSE(aps_manager.IsUserSignedIn());
+  ASSERT_TRUE(aps_manager.GetPrimaryAccountId().empty());
 
   // If user's not signed-in. No refresh is required.
   EXPECT_FALSE(aps_manager.is_under_advanced_protection());
@@ -132,7 +133,7 @@ TEST_F(AdvancedProtectionStatusManagerTest,
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
   base::RunLoop().RunUntilIdle();
-  ASSERT_TRUE(aps_manager.IsUserSignedIn());
+  ASSERT_FALSE(aps_manager.GetPrimaryAccountId().empty());
 
   // An OAuth2 access token request should be sent.
   ASSERT_TRUE(IsRequestActive());
@@ -158,7 +159,7 @@ TEST_F(AdvancedProtectionStatusManagerTest,
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
   base::RunLoop().RunUntilIdle();
-  ASSERT_TRUE(aps_manager.IsUserSignedIn());
+  ASSERT_FALSE(aps_manager.GetPrimaryAccountId().empty());
 
   // An OAuth2 access token request should be sent.
   ASSERT_TRUE(IsRequestActive());
@@ -183,7 +184,7 @@ TEST_F(AdvancedProtectionStatusManagerTest, SignedInLongTimeAgoNotUnderAP) {
       SignIn("gaia_id", "email", /* is_under_advanced_protection = */ false);
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
-  ASSERT_TRUE(aps_manager.IsUserSignedIn());
+  ASSERT_FALSE(aps_manager.GetPrimaryAccountId().empty());
   base::RunLoop().RunUntilIdle();
   // An OAuth2 access token request should be sent.
   ASSERT_TRUE(IsRequestActive());
@@ -230,7 +231,7 @@ TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndUnderAP) {
       SignIn("gaia_id", "email", /* is_under_advanced_protection = */ true);
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
-  ASSERT_TRUE(aps_manager.IsUserSignedIn());
+  ASSERT_FALSE(aps_manager.GetPrimaryAccountId().empty());
   ASSERT_TRUE(aps_manager.is_under_advanced_protection());
 
   // Since user is already under advanced protection, no need to refresh.
@@ -240,11 +241,37 @@ TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndUnderAP) {
   aps_manager.UnsubscribeFromSigninEvents();
 }
 
+TEST_F(AdvancedProtectionStatusManagerTest, StayInAdvancedProtection) {
+  base::Time last_update = base::Time::Now();
+  testing_profile_->GetPrefs()->SetInt64(
+      prefs::kAdvancedProtectionLastRefreshInUs,
+      last_update.ToDeltaSinceWindowsEpoch().InMicroseconds());
+
+  std::string account_id =
+      SignIn("gaia_id", "email", /* is_under_advanced_protection = */ true);
+  AdvancedProtectionStatusManager aps_manager(
+      testing_profile_.get(), base::TimeDelta() /*no min delay*/);
+  ASSERT_FALSE(aps_manager.GetPrimaryAccountId().empty());
+  ASSERT_TRUE(aps_manager.is_under_advanced_protection());
+
+  // Simulate gets refresh token.
+  aps_manager.OnGetIDToken(account_id, kIdTokenAdvancedProtectionEnabled);
+  EXPECT_GT(
+      base::Time::FromDeltaSinceWindowsEpoch(base::TimeDelta::FromMicroseconds(
+          testing_profile_->GetPrefs()->GetInt64(
+              prefs::kAdvancedProtectionLastRefreshInUs))),
+      last_update);
+  EXPECT_TRUE(aps_manager.IsRefreshScheduled());
+  aps_manager.UnsubscribeFromSigninEvents();
+}
+
+#if !defined(OS_CHROMEOS)
+// Not applicable to Chrome OS.
 TEST_F(AdvancedProtectionStatusManagerTest, SignInAndSignOutEvent) {
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
   ASSERT_FALSE(aps_manager.is_under_advanced_protection());
-  ASSERT_FALSE(aps_manager.IsUserSignedIn());
+  ASSERT_TRUE(aps_manager.GetPrimaryAccountId().empty());
 
   SignIn("gaia_id", "email", /* is_under_advanced_protection = */ true);
   EXPECT_TRUE(aps_manager.is_under_advanced_protection());
@@ -257,12 +284,13 @@ TEST_F(AdvancedProtectionStatusManagerTest, SignInAndSignOutEvent) {
   EXPECT_FALSE(aps_manager.IsRefreshScheduled());
   aps_manager.UnsubscribeFromSigninEvents();
 }
+#endif
 
 TEST_F(AdvancedProtectionStatusManagerTest, AccountRemoval) {
   AdvancedProtectionStatusManager aps_manager(
       testing_profile_.get(), base::TimeDelta() /*no min delay*/);
   ASSERT_FALSE(aps_manager.is_under_advanced_protection());
-  ASSERT_FALSE(aps_manager.IsUserSignedIn());
+  ASSERT_TRUE(aps_manager.GetPrimaryAccountId().empty());
 
   std::string account_id =
       SignIn("gaia_id", "email", /* is_under_advanced_protection = */ false);
@@ -282,5 +310,5 @@ TEST_F(AdvancedProtectionStatusManagerTest, AccountRemoval) {
   EXPECT_FALSE(aps_manager.IsRefreshScheduled());
   aps_manager.UnsubscribeFromSigninEvents();
 }
-#endif
+
 }  // namespace safe_browsing