Post launch clean up for AdvancedProtectionStatus

Also made some adjustment to child_account_service_unittest and
user_policy_signin_service_unittest to better simulate the sign-in
state to avoid hitting a DCHECK in IdentityManager.

Bug: 890882
Change-Id: Id76e2cd962904352bf0bdf86ff1fb5d6714573ae
Reviewed-on: https://chromium-review.googlesource.com/c/1258162Reviewed-by: Sergey Poromov <poromov@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Jialiu Lin <jialiul@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601162}

chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_unittest.cc

@@ -24,7 +24,9 @@
 #include "chrome/browser/chromeos/settings/stub_install_attributes.h"
 #include "chrome/browser/policy/cloud/cloud_policy_test_utils.h"
 #include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
+#include "chrome/browser/signin/fake_signin_manager_builder.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/common/chrome_constants.h"
@@ -45,7 +47,9 @@
 #include "components/policy/proto/device_management_backend.pb.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
+#include "components/signin/core/browser/account_tracker_service.h"
 #include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
+#include "components/signin/core/browser/fake_signin_manager.h"
 #include "components/signin/core/browser/profile_oauth2_token_service.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "components/sync_preferences/pref_service_syncable.h"
@@ -156,6 +160,9 @@ class UserCloudPolicyManagerChromeOSTest : public testing::Test {
     factories.emplace_back(
         ProfileOAuth2TokenServiceFactory::GetInstance(),
         base::BindRepeating(&BuildFakeProfileOAuth2TokenService));
+    factories.emplace_back(
+        SigninManagerFactory::GetInstance(),
+        base::BindRepeating(&BuildFakeSigninManagerForTesting));
     profile_ = profile_manager_->CreateTestingProfile(
         chrome::kInitialProfile,
         std::unique_ptr<sync_preferences::PrefServiceSyncable>(),
@@ -708,10 +715,14 @@ TEST_F(UserCloudPolicyManagerChromeOSTest, NonBlockingFirstFetch) {
     static_cast<FakeProfileOAuth2TokenService*>(
         ProfileOAuth2TokenServiceFactory::GetForProfile(profile_));
   ASSERT_TRUE(token_service);
-  SigninManagerBase* signin_manager =
-      SigninManagerFactory::GetForProfile(profile_);
+  const std::string& account_id =
+      AccountTrackerServiceFactory::GetForProfile(profile_)->SeedAccountInfo(
+          kTestGaiaId, kAccountId);
+  FakeSigninManagerForTesting* signin_manager =
+      static_cast<FakeSigninManagerForTesting*>(
+          SigninManagerFactory::GetForProfile(profile_));
   ASSERT_TRUE(signin_manager);
-  const std::string& account_id = signin_manager->GetAuthenticatedAccountId();
+  signin_manager->SignIn(account_id);
   EXPECT_FALSE(token_service->RefreshTokenIsAvailable(account_id));
   token_service->UpdateCredentials(account_id, "refresh_token");
   EXPECT_TRUE(token_service->RefreshTokenIsAvailable(account_id));

chrome/browser/policy/cloud/user_policy_signin_service_unittest.cc

@@ -20,6 +20,7 @@
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/signin/account_fetcher_service_factory.h"
+#include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/chrome_signin_client_factory.h"
 #include "chrome/browser/signin/fake_account_fetcher_service_builder.h"
 #include "chrome/browser/signin/fake_profile_oauth2_token_service_builder.h"
@@ -440,8 +441,9 @@ TEST_F(UserPolicySigninServiceTest, InitRefreshTokenAvailableBeforeSignin) {
   ASSERT_FALSE(IsRequestActive());
 
   // Make oauth token available.
-  std::string account_id = AccountTrackerService::PickAccountIdForAccount(
-      profile_.get()->GetPrefs(), kTestGaiaId, kTestUser);
+  std::string account_id =
+      AccountTrackerServiceFactory::GetForProfile(profile_.get())
+          ->SeedAccountInfo(kTestGaiaId, kTestUser);
   GetTokenService()->UpdateCredentials(account_id, "oauth_login_refresh_token");
 
   // Not signed in yet, so client registration should be deferred.

chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

@@ -172,7 +172,6 @@
 #endif
 
 #if defined(FULL_SAFE_BROWSING)
-#include "chrome/browser/safe_browsing/advanced_protection_status_manager.h"
 #include "chrome/browser/safe_browsing/advanced_protection_status_manager_factory.h"
 #endif
 
@@ -360,8 +359,7 @@ void ChromeBrowserMainExtraPartsProfiles::
   resource_coordinator::LocalSiteCharacteristicsDataStoreFactory::GetInstance();
 #endif
 #if defined(FULL_SAFE_BROWSING)
-  if (safe_browsing::AdvancedProtectionStatusManager::IsEnabled())
-    safe_browsing::AdvancedProtectionStatusManagerFactory::GetInstance();
+  safe_browsing::AdvancedProtectionStatusManagerFactory::GetInstance();
 #endif
 #if defined(OS_ANDROID)
   SearchPermissionsService::Factory::GetInstance();

chrome/browser/safe_browsing/advanced_protection_status_manager.cc

@@ -12,7 +12,6 @@
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/common/safe_browsing_prefs.h"
-#include "components/safe_browsing/features.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/gaia/gaia_constants.h"
@@ -81,11 +80,6 @@ void AdvancedProtectionStatusManager::MaybeRefreshOnStartUp() {
   }
 }
 
-// static
-bool AdvancedProtectionStatusManager::IsEnabled() {
-  return base::FeatureList::IsEnabled(kAdvancedProtectionStatusFeature);
-}
-
 void AdvancedProtectionStatusManager::Shutdown() {
   CancelFutureRefresh();
   UnsubscribeFromSigninEvents();

chrome/browser/safe_browsing/advanced_protection_status_manager.h

@@ -33,9 +33,6 @@ class AdvancedProtectionStatusManager
   explicit AdvancedProtectionStatusManager(Profile* profile);
   ~AdvancedProtectionStatusManager() override;
 
-  // If |kAdvancedProtectionStatusFeature| is enabled.
-  static bool IsEnabled();
-
   // If the primary account of |profile| is under advanced protection.
   static bool IsUnderAdvancedProtection(Profile* profile);
 
@@ -68,6 +65,10 @@ class AdvancedProtectionStatusManager
   FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest, AccountRemoval);
   FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
                            StayInAdvancedProtection);
+  FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
+                           AlreadySignedInAndUnderAPIncognito);
+  FRIEND_TEST_ALL_PREFIXES(AdvancedProtectionStatusManagerTest,
+                           AlreadySignedInAndNotUnderAPIncognito);
 
   void Initialize();
 

chrome/browser/safe_browsing/advanced_protection_status_manager_factory.cc

@@ -8,6 +8,7 @@
 #include "chrome/browser/signin/account_tracker_service_factory.h"
 #include "chrome/browser/signin/identity_manager_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/signin/core/browser/signin_manager.h"
 #include "content/public/browser/browser_context.h"
 
 namespace safe_browsing {

chrome/browser/safe_browsing/advanced_protection_status_manager_unittest.cc

@@ -244,17 +244,8 @@ TEST_F(AdvancedProtectionStatusManagerTest, AlreadySignedInAndUnderAP) {
   aps_manager.UnsubscribeFromSigninEvents();
 }
 
-#if defined(OS_CHROMEOS)
-// https://crbug.com/892117
-#define MAYBE_AlreadySignedInAndUnderAPIncognito \
-  DISABLED_AlreadySignedInAndUnderAPIncognito
-#else
-#define MAYBE_AlreadySignedInAndUnderAPIncognito \
-  AlreadySignedInAndUnderAPIncognito
-#endif
-// Rediret to the actual download URL.
 TEST_F(AdvancedProtectionStatusManagerTest,
-       MAYBE_AlreadySignedInAndUnderAPIncognito) {
+       AlreadySignedInAndUnderAPIncognito) {
   testing_profile_->GetPrefs()->SetInt64(
       prefs::kAdvancedProtectionLastRefreshInUs,
       base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
@@ -263,6 +254,9 @@ TEST_F(AdvancedProtectionStatusManagerTest,
   // under advanced protection.
   std::string account_id =
       SignIn("gaia_id", "email", /* is_under_advanced_protection = */ true);
+  AdvancedProtectionStatusManagerFactory::GetForBrowserContext(
+      Profile::FromBrowserContext(testing_profile_.get()))
+      ->MaybeRefreshOnStartUp();
 
   // Incognito profile should share the advanced protection status with the
   // original profile.
@@ -272,16 +266,8 @@ TEST_F(AdvancedProtectionStatusManagerTest,
       testing_profile_.get()));
 }
 
-#if defined(OS_CHROMEOS)
-// https://crbug.com/892117
-#define MAYBE_AlreadySignedInAndNotUnderAPIncognito \
-  DISABLED_AlreadySignedInAndNotUnderAPIncognito
-#else
-#define MAYBE_AlreadySignedInAndNotUnderAPIncognito \
-  AlreadySignedInAndNotUnderAPIncognito
-#endif
 TEST_F(AdvancedProtectionStatusManagerTest,
-       MAYBE_AlreadySignedInAndNotUnderAPIncognito) {
+       AlreadySignedInAndNotUnderAPIncognito) {
   testing_profile_->GetPrefs()->SetInt64(
       prefs::kAdvancedProtectionLastRefreshInUs,
       base::Time::Now().ToDeltaSinceWindowsEpoch().InMicroseconds());
@@ -290,6 +276,9 @@ TEST_F(AdvancedProtectionStatusManagerTest,
   // NOT under advanced protection.
   std::string account_id =
       SignIn("gaia_id", "email", /* is_under_advanced_protection = */ false);
+  AdvancedProtectionStatusManagerFactory::GetForBrowserContext(
+      Profile::FromBrowserContext(testing_profile_.get()))
+      ->MaybeRefreshOnStartUp();
 
   // Incognito profile should share the advanced protection status with the
   // original profile.

chrome/browser/supervised_user/child_accounts/child_account_service_unittest.cc

@@ -29,17 +29,19 @@ class ChildAccountServiceTest : public ::testing::Test {
   ChildAccountServiceTest() = default;
 
   void SetUp() override {
-    ChromeSigninClientFactory::GetInstance()->SetTestingFactory(
-        &profile_, base::BindRepeating(&BuildTestSigninClient));
-    GaiaCookieManagerServiceFactory::GetInstance()->SetTestingFactory(
-        &profile_, base::BindRepeating(&BuildFakeGaiaCookieManagerService));
+    TestingProfile::Builder builder;
+    builder.AddTestingFactory(ChromeSigninClientFactory::GetInstance(),
+                              BuildTestSigninClient);
+    builder.AddTestingFactory(GaiaCookieManagerServiceFactory::GetInstance(),
+                              BuildFakeGaiaCookieManagerService);
+    profile_ = builder.Build();
     gaia_cookie_manager_service_ = static_cast<FakeGaiaCookieManagerService*>(
-        GaiaCookieManagerServiceFactory::GetForProfile(&profile_));
+        GaiaCookieManagerServiceFactory::GetForProfile(profile_.get()));
   }
 
  protected:
   content::TestBrowserThreadBundle thread_bundle_;
-  TestingProfile profile_;
+  std::unique_ptr<TestingProfile> profile_;
   FakeGaiaCookieManagerService* gaia_cookie_manager_service_ = nullptr;
 };
 
@@ -47,7 +49,7 @@ TEST_F(ChildAccountServiceTest, GetGoogleAuthState) {
   gaia_cookie_manager_service_->SetListAccountsResponseNoAccounts();
 
   ChildAccountService* child_account_service =
-      ChildAccountServiceFactory::GetForProfile(&profile_);
+      ChildAccountServiceFactory::GetForProfile(profile_.get());
 
   // Initial state should be PENDING.
   EXPECT_EQ(ChildAccountService::AuthState::PENDING,

components/safe_browsing/features.cc

@@ -22,9 +22,6 @@ namespace safe_browsing {
 const base::Feature kAdSamplerTriggerFeature{"SafeBrowsingAdSamplerTrigger",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
 
-const base::Feature kAdvancedProtectionStatusFeature{
-    "AdvancedProtectionStatus", base::FEATURE_DISABLED_BY_DEFAULT};
-
 // Controls the billing interstitial UI.
 const base::Feature kBillingInterstitial{"BillingInterstitial",
                                          base::FEATURE_DISABLED_BY_DEFAULT};
@@ -68,7 +65,6 @@ constexpr struct {
   bool probabilistically_enabled;
 } kExperimentalFeatures[]{
     {&kAdSamplerTriggerFeature, false},
-    {&kAdvancedProtectionStatusFeature, true},
     {&kBillingInterstitial, true},
     {&kCheckByURLLoaderThrottle, true},
     {&kForceEnableResetPasswordWebUI, true},

components/safe_browsing/features.h

@@ -21,9 +21,6 @@ namespace safe_browsing {
 // Features list
 extern const base::Feature kAdSamplerTriggerFeature;
 
-// Controls the safe browsing protection for advanced protection program.
-extern const base::Feature kAdvancedProtectionStatusFeature;
-
 // Controls the billing interstitial UI.
 extern const base::Feature kBillingInterstitial;
 

testing/variations/fieldtrial_testing_config.json

@@ -87,24 +87,6 @@
             ]
         }
     ],
-    "AdvancedProtectionStatusRollout": [
-        {
-            "platforms": [
-                "chromeos",
-                "linux",
-                "mac",
-                "windows"
-            ],
-            "experiments": [
-                {
-                    "name": "Enabled",
-                    "enable_features": [
-                        "AdvancedProtectionStatus"
-                    ]
-                }
-            ]
-        }
-    ],
     "AllowClientHintsToThirdParty": [
         {
             "platforms": [