AuthPolicyCredentialsManager: Postpone service creation.

Changes to create the service automatically after BrowserContext is initialized.

BUG=chromium:845829
TEST=manual

Change-Id: Id1c830f0ad33c39e9dbbe676ded9d5044d1df242
Reviewed-on: https://chromium-review.googlesource.com/1068184
Commit-Queue: Roman Sorokin <rsorokin@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563577}

chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.cc

@@ -99,7 +99,6 @@ AuthPolicyCredentialsManager::AuthPolicyCredentialsManager(Profile* profile)
   StartObserveNetwork();
   account_id_ = user->GetAccountId();
   GetUserStatus();
-  GetUserKerberosFiles();
 
   // Setting environment variables for GSSAPI library.
   std::unique_ptr<base::Environment> env(base::Environment::Create());
@@ -374,17 +373,6 @@ AuthPolicyCredentialsManagerFactory::GetInstance() {
   return base::Singleton<AuthPolicyCredentialsManagerFactory>::get();
 }
 
-// static
-KeyedService*
-AuthPolicyCredentialsManagerFactory::BuildForProfileIfActiveDirectory(
-    Profile* profile) {
-  const user_manager::User* user =
-      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
-  if (!user || !user->IsActiveDirectoryUser())
-    return nullptr;
-  return GetInstance()->GetServiceForBrowserContext(profile, true /* create */);
-}
-
 AuthPolicyCredentialsManagerFactory::AuthPolicyCredentialsManagerFactory()
     : BrowserContextKeyedServiceFactory(
           "AuthPolicyCredentialsManager",
@@ -392,9 +380,21 @@ AuthPolicyCredentialsManagerFactory::AuthPolicyCredentialsManagerFactory()
 
 AuthPolicyCredentialsManagerFactory::~AuthPolicyCredentialsManagerFactory() {}
 
+bool AuthPolicyCredentialsManagerFactory::ServiceIsCreatedWithBrowserContext()
+    const {
+  return true;
+}
+
 KeyedService* AuthPolicyCredentialsManagerFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
+  // UserManager is usually not initialized in tests.
+  if (!user_manager::UserManager::IsInitialized())
+    return nullptr;
   Profile* profile = Profile::FromBrowserContext(context);
+  const user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
+  if (!user || !user->IsActiveDirectoryUser())
+    return nullptr;
   return new AuthPolicyCredentialsManager(profile);
 }
 

chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager.h

@@ -131,18 +131,18 @@ class AuthPolicyCredentialsManagerFactory
  public:
   static AuthPolicyCredentialsManagerFactory* GetInstance();
 
-  // Returns nullptr in case profile is not Active Directory. Otherwise returns
-  // valid AuthPolicyCredentialsManager. Lifetime is managed by
-  // BrowserContextKeyedServiceFactory.
-  static KeyedService* BuildForProfileIfActiveDirectory(Profile* profile);
-
  private:
   friend struct base::DefaultSingletonTraits<
       AuthPolicyCredentialsManagerFactory>;
+  friend class AuthPolicyCredentialsManagerTest;
 
   AuthPolicyCredentialsManagerFactory();
   ~AuthPolicyCredentialsManagerFactory() override;
 
+  bool ServiceIsCreatedWithBrowserContext() const override;
+
+  // Returns nullptr in case profile is not Active Directory. Otherwise returns
+  // valid AuthPolicyCredentialsManager.
   KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* context) const override;
 

chrome/browser/chromeos/authpolicy/auth_policy_credentials_manager_unittest.cc

@@ -28,9 +28,10 @@ namespace chromeos {
 
 namespace {
 
-const char kProfileSigninNotificationId[] = "chrome://settings/signin/";
-const char kDisplayName[] = "DisplayName";
-const char kGivenName[] = "Given Name";
+constexpr char kProfileSigninNotificationId[] = "chrome://settings/signin/";
+constexpr char kProfileEmail[] = "user@example.com";
+constexpr char kDisplayName[] = "DisplayName";
+constexpr char kGivenName[] = "Given Name";
 
 MATCHER_P(UserAccountDataEq, value, "Compares two UserAccountData") {
   const user_manager::UserManager::UserAccountData& expected_data = value;
@@ -54,22 +55,24 @@ class AuthPolicyCredentialsManagerTest : public testing::Test {
     fake_auth_policy_client()->DisableOperationDelayForTesting();
 
     TestingProfile::Builder profile_builder;
-    profile_builder.SetProfileName("user@gmail.com");
-    profile_ = profile_builder.Build();
-    account_id_ = AccountId::AdFromUserEmailObjGuid(
-        profile()->GetProfileUserName(), "1234567890");
+    profile_builder.SetProfileName(kProfileEmail);
+    account_id_ =
+        AccountId::AdFromUserEmailObjGuid(kProfileEmail, "1234567890");
     mock_user_manager()->AddUser(account_id_);
-    display_service_ =
-        std::make_unique<NotificationDisplayServiceTester>(profile());
 
     base::RunLoop run_loop;
     fake_auth_policy_client()->set_on_get_status_closure(
         run_loop.QuitClosure());
 
-    auth_policy_credentials_manager_ = static_cast<
-        AuthPolicyCredentialsManager*>(
-        AuthPolicyCredentialsManagerFactory::BuildForProfileIfActiveDirectory(
-            profile()));
+    profile_ = profile_builder.Build();
+    display_service_ =
+        std::make_unique<NotificationDisplayServiceTester>(profile());
+
+    auth_policy_credentials_manager_ =
+        static_cast<AuthPolicyCredentialsManager*>(
+            AuthPolicyCredentialsManagerFactory::GetInstance()
+                ->GetServiceForBrowserContext(profile(), false /* create */));
+    EXPECT_TRUE(auth_policy_credentials_manager_);
 
     EXPECT_CALL(*mock_user_manager(),
                 SaveForceOnlineSignin(account_id(), false));

chrome/browser/chromeos/login/existing_user_controller_browsertest.cc

@@ -116,6 +116,75 @@ void WaitForPermanentlyUntrustedStatusAndRun(const base::Closure& callback) {
   }
 }
 
+std::string GetKerberosConfigFileName() {
+  std::unique_ptr<base::Environment> env(base::Environment::Create());
+  std::string config_file;
+  EXPECT_TRUE(env->GetVar("KRB5_CONFIG", &config_file));
+  return config_file;
+}
+
+std::string GetKerberosCredentialsCacheFileName() {
+  std::unique_ptr<base::Environment> env(base::Environment::Create());
+  std::string creds_file;
+  EXPECT_TRUE(env->GetVar("KRB5CCNAME", &creds_file));
+  EXPECT_EQ(kKrb5CCFilePrefix, creds_file.substr(0, strlen(kKrb5CCFilePrefix)));
+  return creds_file.substr(strlen(kKrb5CCFilePrefix));
+}
+
+// Helper class to wait when both Kerberos credentials cache and config file
+// changed.
+class KerberosFilesChangeWaiter {
+ public:
+  // If |files_must_exist| is true and a file already exists the class does not
+  // wait when it changes.
+  explicit KerberosFilesChangeWaiter(bool files_must_exist) {
+    barrier_closure_ = base::BarrierClosure(2, loop_.QuitClosure());
+
+    watch_callback_ = base::BindRepeating(
+        [](const base::RepeatingClosure& barrier_closure,
+           const base::FilePath& path, bool error) -> void {
+          EXPECT_FALSE(error);
+          barrier_closure.Run();
+        },
+        barrier_closure_);
+
+    config_watcher_ = std::make_unique<base::FilePathWatcher>();
+    MaybeStartWatch(&config_watcher_,
+                    base::FilePath(GetKerberosConfigFileName()),
+                    files_must_exist);
+
+    creds_watcher_ = std::make_unique<base::FilePathWatcher>();
+    MaybeStartWatch(&creds_watcher_,
+                    base::FilePath(GetKerberosCredentialsCacheFileName()),
+                    files_must_exist);
+  }
+
+  // Should be called once.
+  void Wait() {
+    loop_.Run();
+    config_watcher_.reset();
+    creds_watcher_.reset();
+  }
+
+ private:
+  void MaybeStartWatch(std::unique_ptr<base::FilePathWatcher>* watcher,
+                       const base::FilePath& path,
+                       bool files_must_exist) {
+    (*watcher)->Watch(path, false /* recursive */, watch_callback_);
+    if (!files_must_exist && base::PathExists(path)) {
+      watch_callback_.Run(path, false /* error */);
+      watcher->reset();
+    }
+  }
+  base::RunLoop loop_;
+  base::RepeatingClosure barrier_closure_;
+
+  base::RepeatingCallback<void(const base::FilePath& path, bool error)>
+      watch_callback_;
+  std::unique_ptr<base::FilePathWatcher> config_watcher_;
+  std::unique_ptr<base::FilePathWatcher> creds_watcher_;
+};
+
 }  // namespace
 
 class ExistingUserControllerTest : public policy::DevicePolicyCrosBrowserTest {
@@ -822,22 +891,6 @@ class ExistingUserControllerActiveDirectoryTest
     return config;
   }
 
-  std::string GetKerberosConfigFileName() {
-    std::unique_ptr<base::Environment> env(base::Environment::Create());
-    std::string config_file;
-    EXPECT_TRUE(env->GetVar("KRB5_CONFIG", &config_file));
-    return config_file;
-  }
-
-  std::string GetKerberosCredentialsCacheFileName() {
-    std::unique_ptr<base::Environment> env(base::Environment::Create());
-    std::string creds_file;
-    EXPECT_TRUE(env->GetVar("KRB5CCNAME", &creds_file));
-    EXPECT_EQ(kKrb5CCFilePrefix,
-              creds_file.substr(0, strlen(kKrb5CCFilePrefix)));
-    return creds_file.substr(strlen(kKrb5CCFilePrefix));
-  }
-
   void CheckKerberosFiles(bool enable_dns_cname_lookup) {
     std::string file_contents;
     EXPECT_TRUE(base::ReadFileToString(
@@ -852,27 +905,7 @@ class ExistingUserControllerActiveDirectoryTest
 
   // Applies policy and waits until both config and credentials files changed.
   void ApplyPolicyAndWaitFilesChanged(bool enable_dns_cname_lookup) {
-    base::RunLoop loop;
-    base::RepeatingClosure barrier_closure(
-        base::BarrierClosure(2, loop.QuitClosure()));
-
-    auto watch_callback = base::BindRepeating(
-        [](const base::RepeatingClosure& barrier_closure,
-           const base::FilePath& path, bool error) -> void {
-          LOG(ERROR) << "Changed " << path.value();
-          EXPECT_FALSE(error);
-          barrier_closure.Run();
-        },
-        barrier_closure);
-
-    base::FilePathWatcher config_watcher;
-    config_watcher.Watch(base::FilePath(GetKerberosConfigFileName()),
-                         false /* recursive */, watch_callback);
-
-    base::FilePathWatcher creds_watcher;
-    creds_watcher.Watch(base::FilePath(GetKerberosCredentialsCacheFileName()),
-                        false /* recursive */, watch_callback);
-
+    KerberosFilesChangeWaiter files_change_waiter(true /* files_must_exist */);
     policy::PolicyMap policies;
     policies.Set(policy::key::kDisableAuthNegotiateCnameLookup,
                  policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
@@ -880,7 +913,7 @@ class ExistingUserControllerActiveDirectoryTest
                  std::make_unique<base::Value>(!enable_dns_cname_lookup),
                  nullptr);
     UpdateProviderPolicy(policies);
-    loop.Run();
+    files_change_waiter.Wait();
   }
 
  private:
@@ -932,6 +965,8 @@ IN_PROC_BROWSER_TEST_F(ExistingUserControllerActiveDirectoryTest,
   existing_user_controller()->CompleteLogin(user_context);
 
   profile_prepared_observer.Wait();
+  KerberosFilesChangeWaiter files_change_waiter(false /* files_must_exist */);
+  files_change_waiter.Wait();
   CheckKerberosFiles(true /* enable_dns_cname_lookup */);
 }
 
@@ -953,6 +988,8 @@ IN_PROC_BROWSER_TEST_F(ExistingUserControllerActiveDirectoryTest,
   existing_user_controller()->CompleteLogin(user_context);
 
   profile_prepared_observer.Wait();
+  KerberosFilesChangeWaiter files_change_waiter(false /* files_must_exist */);
+  files_change_waiter.Wait();
   CheckKerberosFiles(true /* enable_dns_cname_lookup */);
 
   ApplyPolicyAndWaitFilesChanged(false /* enable_dns_cname_lookup */);
@@ -978,8 +1015,9 @@ IN_PROC_BROWSER_TEST_F(ExistingUserControllerActiveDirectoryTest,
       chrome::NOTIFICATION_LOGIN_USER_PROFILE_PREPARED,
       content::NotificationService::AllSources());
   existing_user_controller()->Login(user_context, SigninSpecifics());
-
   profile_prepared_observer.Wait();
+  KerberosFilesChangeWaiter files_change_waiter(false /* files_must_exist */);
+  files_change_waiter.Wait();
   CheckKerberosFiles(true /* enable_dns_cname_lookup */);
 }
 

chrome/browser/profiles/profile_impl.cc

@@ -459,8 +459,6 @@ ProfileImpl::ProfileImpl(
   configuration_policy_provider_ =
       policy::UserPolicyManagerFactoryChromeOS::CreateForProfile(
           this, force_immediate_policy_load, io_task_runner_);
-  chromeos::AuthPolicyCredentialsManagerFactory::
-      BuildForProfileIfActiveDirectory(this);
 #else
   configuration_policy_provider_ =
       policy::UserCloudPolicyManagerFactory::CreateForOriginalBrowserContext(