Setup Chromad user policy plumbing

chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc

Index: chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
diff --git a/chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc b/chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
index 36cacfa750a859c9281089f2408867a0c1e59e2b..f7f70c6e88ecabc4b8cf471f8ac1534fe88152d6 100644
--- a/chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
+++ b/chrome/browser/chromeos/policy/user_cloud_policy_manager_factory_chromeos.cc
@@ -10,6 +10,7 @@
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
 #include "base/sequenced_task_runner.h"
@@ -18,11 +19,13 @@
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
+#include "chrome/browser/chromeos/policy/user_active_directory_policy_manager.h"
 #include "chrome/browser/chromeos/policy/user_cloud_external_data_manager.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos.h"
 #include "chrome/browser/chromeos/policy/user_cloud_policy_store_chromeos.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
+#include "chrome/browser/chromeos/settings/install_attributes.h"
 #include "chrome/browser/policy/schema_registry_service.h"
 #include "chrome/browser/policy/schema_registry_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
@@ -33,6 +36,7 @@
 #include "components/policy/core/browser/browser_policy_connector.h"
 #include "components/policy/core/common/cloud/cloud_external_data_manager.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
+#include "components/policy/core/common/configuration_policy_provider.h"
 #include "components/policy/policy_constants.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
@@ -70,14 +74,31 @@ UserCloudPolicyManagerFactoryChromeOS*
 }
 
 // static
+ConfigurationPolicyProvider*
+UserCloudPolicyManagerFactoryChromeOS::GetForProfile(Profile* profile) {
+  ConfigurationPolicyProvider* cloud_provider =
+      GetInstance()->GetCloudPolicyManagerForProfile(profile);
+  if (cloud_provider) {
+    return cloud_provider;
+  }
+  return GetInstance()->GetActiveDirectoryPolicyManagerForProfile(profile);
+}
+
+// static
 UserCloudPolicyManagerChromeOS*
-    UserCloudPolicyManagerFactoryChromeOS::GetForProfile(
-        Profile* profile) {
-  return GetInstance()->GetManagerForProfile(profile);
+UserCloudPolicyManagerFactoryChromeOS::GetCloudPolicyManagerForProfile(
+    Profile* profile) {
+  return GetInstance()->GetCloudPolicyManager(profile);
 }
 
 // static
-std::unique_ptr<UserCloudPolicyManagerChromeOS>
+UserActiveDirectoryPolicyManager* UserCloudPolicyManagerFactoryChromeOS::
+    GetActiveDirectoryPolicyManagerForProfile(Profile* profile) {
+  return GetInstance()->GetActiveDirectoryPolicyManager(profile);
+}
+
+// static
+std::unique_ptr<ConfigurationPolicyProvider>
 UserCloudPolicyManagerFactoryChromeOS::CreateForProfile(
     Profile* profile,
     bool force_immediate_load,
@@ -97,15 +118,24 @@ UserCloudPolicyManagerFactoryChromeOS::
     ~UserCloudPolicyManagerFactoryChromeOS() {}
 
 UserCloudPolicyManagerChromeOS*
-    UserCloudPolicyManagerFactoryChromeOS::GetManagerForProfile(
-        Profile* profile) {
+UserCloudPolicyManagerFactoryChromeOS::GetCloudPolicyManager(Profile* profile) {
   // Get the manager for the original profile, since the PolicyService is
   // also shared between the incognito Profile and the original Profile.
-  ManagerMap::const_iterator it = managers_.find(profile->GetOriginalProfile());
-  return it != managers_.end() ? it->second : NULL;
+  auto const it = cloud_managers_.find(profile->GetOriginalProfile());

[emaxx, 2016/12/28 19:09:19]

nit: The existing code in this file uses the style with "const" being put first.

[Thiemo Nagel, 2016/12/29 15:08:27]

Done.

+  return it != cloud_managers_.end() ? it->second : nullptr;
 }
 
-std::unique_ptr<UserCloudPolicyManagerChromeOS>
+UserActiveDirectoryPolicyManager*
+UserCloudPolicyManagerFactoryChromeOS::GetActiveDirectoryPolicyManager(
+    Profile* profile) {
+  // Get the manager for the original profile, since the PolicyService is
+  // also shared between the incognito Profile and the original Profile.
+  auto const it =
+      active_directory_managers_.find(profile->GetOriginalProfile());
+  return it != active_directory_managers_.end() ? it->second : nullptr;
+}
+
+std::unique_ptr<ConfigurationPolicyProvider>
 UserCloudPolicyManagerFactoryChromeOS::CreateManagerForProfile(
     Profile* profile,
     bool force_immediate_load,
@@ -114,41 +144,65 @@ UserCloudPolicyManagerFactoryChromeOS::CreateManagerForProfile(
       base::CommandLine::ForCurrentProcess();
   // Don't initialize cloud policy for the signin profile.
   if (chromeos::ProfileHelper::IsSigninProfile(profile))
-    return std::unique_ptr<UserCloudPolicyManagerChromeOS>();
+    return std::unique_ptr<ConfigurationPolicyProvider>();
 
-  // |user| should never be NULL except for the signin profile. This object is
-  // created as part of the Profile creation, which happens right after
+  // |user| should never be nullptr except for the signin profile. This object
+  // is created as part of the Profile creation, which happens right after
   // sign-in. The just-signed-in User is the active user during that time.
   const user_manager::User* user =
       chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
   CHECK(user);
 
   // User policy exists for enterprise accounts only:
-  // - For regular enterprise users (those who have a GAIA account), a
+  // - For regular cloud-managed users (those who have a GAIA account), a
   //   |UserCloudPolicyManagerChromeOS| is created here.
+  // - For Active Directory managed users, a |UserActiveDirectoryPolicyManager|
+  //   is created.
   // - For device-local accounts, policy is provided by
   //   |DeviceLocalAccountPolicyService|.
   // All other user types do not have user policy.
-  const AccountId account_id = user->GetAccountId();
-  if (!user->HasGaiaAccount() || user->IsSupervised() ||
+  const AccountId& account_id = user->GetAccountId();
+  if (user->IsSupervised() ||
       BrowserPolicyConnector::IsNonEnterpriseUser(account_id.GetUserEmail())) {
-    return std::unique_ptr<UserCloudPolicyManagerChromeOS>();
+    return std::unique_ptr<ConfigurationPolicyProvider>();
   }
 
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
+  bool is_active_directory;

[emaxx, 2016/12/28 19:09:19]

nit: I think usually in Chrome source code it's preferred to initialize all
variables immediately. The false value would be suitable here.

[Thiemo Nagel, 2016/12/29 15:08:27]

Thanks, that seems prudent.  I'd assume that the compiler most probably will
optimize away duplicate assignments anyways.

+  switch (account_id.GetAccountType()) {
+    case AccountType::UNKNOWN:
+      return std::unique_ptr<ConfigurationPolicyProvider>();

[emaxx, 2016/12/28 19:09:19]

nit: Not insisting, but C++11 allows to shorten this to:
  return {};

[Thiemo Nagel, 2016/12/29 15:08:27]

Very interesting, thanks.  Done.

+      break;

[emaxx, 2016/12/28 19:09:19]

nit: May be removed.

[Thiemo Nagel, 2016/12/29 15:08:27]

If you don't mind, I'd suggest to keep it for symmetry (and maybe also to
prevent havoc in case the return statement is ever replaced by something else).

+    case AccountType::GOOGLE:
+      if (!user->HasGaiaAccount()) {
+        return std::unique_ptr<ConfigurationPolicyProvider>();
+      }
+      is_active_directory = false;
+      break;
+    case AccountType::ACTIVE_DIRECTORY:
+      // Ensure install attributes are locked into Active Directory mode before
+      // allowing Active Directory policy which is not signed.
+      if (!connector->GetInstallAttributes()->IsActiveDirectoryManaged()) {
+        return std::unique_ptr<ConfigurationPolicyProvider>();
+      }
+      is_active_directory = true;
+      break;
+  }
+
   const bool is_browser_restart =
       command_line->HasSwitch(chromeos::switches::kLoginUser);
   const user_manager::UserManager* const user_manager =
       user_manager::UserManager::Get();
 
-  // We want to block for policy in a few situations: if the user is new, or
-  // if we are forcing an online signin. An online signin will be forced if
-  // there has been a credential error, or if the initial session creation
-  // was not completed (the oauth_token_status is not set to valid by
-  // OAuth2LoginManager until profile creation/session restore is complete).
+  // We want to block for policy in a few situations: if the user is new, or if
+  // we are forcing an online signin. An online signin will be forced if there
+  // has been a credential error, or if the initial session creation was not
+  // completed (the oauth_token_status is not set to valid by OAuth2LoginManager
+  // until profile creation/session restore is complete).
+  // TODO(tnagel): Don't limit blocking to cloud managed users.
   const bool block_forever_for_policy =
-      !user_manager->IsLoggedInAsStub() &&
+      !is_active_directory && !user_manager->IsLoggedInAsStub() &&
       (user_manager->IsCurrentUserNew() ||
        user_manager->GetActiveUser()->force_online_signin() ||
        user_manager->GetActiveUser()->oauth_token_status() !=
@@ -182,7 +236,8 @@ UserCloudPolicyManagerFactoryChromeOS::CreateManagerForProfile(
       new UserCloudPolicyStoreChromeOS(

[emaxx, 2016/12/28 19:09:19]

nit: Use base::MakeUnique here too while you're here?

[Thiemo Nagel, 2016/12/29 15:08:27]

Done.

           chromeos::DBusThreadManager::Get()->GetCryptohomeClient(),
           chromeos::DBusThreadManager::Get()->GetSessionManagerClient(),
-          background_task_runner, account_id, policy_key_dir));
+          background_task_runner, account_id, policy_key_dir,
+          is_active_directory));
 
   scoped_refptr<base::SequencedTaskRunner> backend_task_runner =
       content::BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
@@ -201,30 +256,48 @@ UserCloudPolicyManagerFactoryChromeOS::CreateManagerForProfile(
       content::BrowserThread::GetTaskRunnerForThread(
           content::BrowserThread::FILE);
 
-  std::unique_ptr<UserCloudPolicyManagerChromeOS> manager(
-      new UserCloudPolicyManagerChromeOS(
-          std::move(store), std::move(external_data_manager),
-          component_policy_cache_dir, wait_for_policy_fetch,
-          initial_policy_fetch_timeout, base::ThreadTaskRunnerHandle::Get(),
-          file_task_runner, io_task_runner));
-
-  bool wildcard_match = false;
-  if (connector->IsEnterpriseManaged() &&
-      chromeos::CrosSettings::IsWhitelisted(account_id.GetUserEmail(),
-                                            &wildcard_match) &&
-      wildcard_match &&
-      !connector->IsNonEnterpriseUser(account_id.GetUserEmail())) {
-    manager->EnableWildcardLoginCheck(account_id.GetUserEmail());
+  if (is_active_directory) {
+    std::unique_ptr<UserActiveDirectoryPolicyManager> manager =
+        base::MakeUnique<UserActiveDirectoryPolicyManager>(account_id,
+                                                           std::move(store));
+    manager->Init(
+        SchemaRegistryServiceFactory::GetForContext(profile)->registry());
+
+    DCHECK(cloud_managers_.find(profile) == cloud_managers_.end());
+    DCHECK(active_directory_managers_.find(profile) ==
+           active_directory_managers_.end());
+    active_directory_managers_[profile] = manager.get();
+    return manager;
+  } else {
+    std::unique_ptr<UserCloudPolicyManagerChromeOS> manager =
+        base::MakeUnique<UserCloudPolicyManagerChromeOS>(
+            std::move(store), std::move(external_data_manager),
+            component_policy_cache_dir, wait_for_policy_fetch,
+            initial_policy_fetch_timeout, base::ThreadTaskRunnerHandle::Get(),
+            file_task_runner, io_task_runner);
+
+    // TODO(tnagel): Enable whitelist for Active Directory.
+    bool wildcard_match = false;
+    if (connector->IsEnterpriseManaged() &&
+        chromeos::CrosSettings::IsWhitelisted(account_id.GetUserEmail(),
+                                              &wildcard_match) &&
+        wildcard_match &&
+        !connector->IsNonEnterpriseUser(account_id.GetUserEmail())) {
+      manager->EnableWildcardLoginCheck(account_id.GetUserEmail());
+    }
+
+    manager->Init(
+        SchemaRegistryServiceFactory::GetForContext(profile)->registry());
+    manager->Connect(g_browser_process->local_state(),
+                     device_management_service,
+                     g_browser_process->system_request_context());
+
+    DCHECK(cloud_managers_.find(profile) == cloud_managers_.end());

[emaxx, 2016/12/28 19:09:19]

You could move these two DCHECKS out of the condition to rid of duplicating
them.

[Thiemo Nagel, 2016/12/29 15:08:27]

Good point, they look much better at the top of the method.

+    DCHECK(active_directory_managers_.find(profile) ==
+           active_directory_managers_.end());
+    cloud_managers_[profile] = manager.get();
+    return manager;
   }
-
-  manager->Init(
-      SchemaRegistryServiceFactory::GetForContext(profile)->registry());
-  manager->Connect(g_browser_process->local_state(), device_management_service,
-                   g_browser_process->system_request_context());
-
-  DCHECK(managers_.find(profile) == managers_.end());
-  managers_[profile] = manager.get();
-  return manager;
 }
 
 void UserCloudPolicyManagerFactoryChromeOS::BrowserContextShutdown(
@@ -232,15 +305,21 @@ void UserCloudPolicyManagerFactoryChromeOS::BrowserContextShutdown(
   Profile* profile = static_cast<Profile*>(context);
   if (profile->IsOffTheRecord())
     return;
-  UserCloudPolicyManagerChromeOS* manager = GetManagerForProfile(profile);
-  if (manager)
-    manager->Shutdown();
+  UserCloudPolicyManagerChromeOS* cloud_manager =
+      GetCloudPolicyManager(profile);
+  if (cloud_manager)
+    cloud_manager->Shutdown();
+  UserActiveDirectoryPolicyManager* active_directory_manager =
+      GetActiveDirectoryPolicyManager(profile);
+  if (active_directory_manager)
+    active_directory_manager->Shutdown();
 }
 
 void UserCloudPolicyManagerFactoryChromeOS::BrowserContextDestroyed(
     content::BrowserContext* context) {
   Profile* profile = static_cast<Profile*>(context);
-  managers_.erase(profile);
+  cloud_managers_.erase(profile);
+  active_directory_managers_.erase(profile);
   BrowserContextKeyedBaseFactory::BrowserContextDestroyed(context);
 }