Fix UserAvatarImage and WallpaperImage for Chromad

Both are 'external'-type policies, i.e. the images are not included in
the policy, but downloaded separately. However, the mechanisms to
download external user policies were not hooked up. This CL wires them
up. Interestingly, DeviceWallpaperImage works fine.

BUG=chromium:819561
TEST=out/Release/unit_tests --gtest_filter=ActiveDirectoryPolicyManagerTest.*
     Set UserAvatarImage and WallpaperImage on Windows Server and
     verified that it applies on a domain joined Chromebook.

Change-Id: I8670a3e1ba0edcee31ed5753be92b021e9553bb0
Reviewed-on: https://chromium-review.googlesource.com/952903
Commit-Queue: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543084}

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -268,10 +268,6 @@ ChromeUserManagerImpl::ChromeUserManagerImpl()
     GetMinimumVersionPolicyHandler()->AddObserver(this);
   }
 
-  if (!device_local_account_policy_service) {
-    return;
-  }
-
   avatar_policy_observer_ =
       std::make_unique<policy::CloudExternalDataPolicyObserver>(
           cros_settings_, device_local_account_policy_service,

chrome/browser/chromeos/policy/active_directory_policy_manager.cc

@@ -9,11 +9,14 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "chrome/browser/browser_process.h"
 #include "chromeos/dbus/auth_policy_client.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
+#include "components/policy/core/common/cloud/cloud_external_data_manager.h"
 #include "components/policy/core/common/policy_bundle.h"
 #include "components/policy/core/common/policy_types.h"
 #include "components/policy/policy_constants.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace {
 
@@ -37,9 +40,9 @@ std::unique_ptr<ActiveDirectoryPolicyManager>
 ActiveDirectoryPolicyManager::CreateForDevicePolicy(
     std::unique_ptr<CloudPolicyStore> store) {
   // Can't use MakeUnique<> because the constructor is private.
-  return base::WrapUnique(
-      new ActiveDirectoryPolicyManager(EmptyAccountId(), base::TimeDelta(),
-                                       base::OnceClosure(), std::move(store)));
+  return base::WrapUnique(new ActiveDirectoryPolicyManager(
+      EmptyAccountId(), base::TimeDelta(), base::OnceClosure(),
+      std::move(store), nullptr /* external_data_manager */));
 }
 
 // static
@@ -48,11 +51,12 @@ ActiveDirectoryPolicyManager::CreateForUserPolicy(
     const AccountId& account_id,
     base::TimeDelta initial_policy_fetch_timeout,
     base::OnceClosure exit_session,
-    std::unique_ptr<CloudPolicyStore> store) {
+    std::unique_ptr<CloudPolicyStore> store,
+    std::unique_ptr<CloudExternalDataManager> external_data_manager) {
   // Can't use MakeUnique<> because the constructor is private.
   return base::WrapUnique(new ActiveDirectoryPolicyManager(
       account_id, initial_policy_fetch_timeout, std::move(exit_session),
-      std::move(store)));
+      std::move(store), std::move(external_data_manager)));
 }
 
 void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
@@ -72,9 +76,20 @@ void ActiveDirectoryPolicyManager::Init(SchemaRegistry* registry) {
       base::BindRepeating(&ActiveDirectoryPolicyManager::OnPolicyFetched,
                           weak_ptr_factory_.GetWeakPtr()),
       kFetchInterval);
+
+  if (external_data_manager_) {
+    // Use the system request context here instead of a context derived from the
+    // Profile because Connect() is called before the profile is fully
+    // initialized (required so we can perform the initial policy load).
+    // Note: The request context can be null for tests and for device policy.
+    external_data_manager_->Connect(
+        g_browser_process->system_request_context());
+  }
 }
 
 void ActiveDirectoryPolicyManager::Shutdown() {
+  if (external_data_manager_)
+    external_data_manager_->Disconnect();
   store_->RemoveObserver(this);
   ConfigurationPolicyProvider::Shutdown();
 }
@@ -130,13 +145,15 @@ ActiveDirectoryPolicyManager::ActiveDirectoryPolicyManager(
     const AccountId& account_id,
     base::TimeDelta initial_policy_fetch_timeout,
     base::OnceClosure exit_session,
-    std::unique_ptr<CloudPolicyStore> store)
+    std::unique_ptr<CloudPolicyStore> store,
+    std::unique_ptr<CloudExternalDataManager> external_data_manager)
     : account_id_(account_id),
       waiting_for_initial_policy_fetch_(
           !initial_policy_fetch_timeout.is_zero()),
       initial_policy_fetch_may_fail_(!initial_policy_fetch_timeout.is_max()),
       exit_session_(std::move(exit_session)),
-      store_(std::move(store)) {
+      store_(std::move(store)),
+      external_data_manager_(std::move(external_data_manager)) {
   // Delaying initialization complete is intended for user policy only.
   DCHECK(account_id != EmptyAccountId() || !waiting_for_initial_policy_fetch_);
   if (waiting_for_initial_policy_fetch_ && initial_policy_fetch_may_fail_) {

chrome/browser/chromeos/policy/active_directory_policy_manager.h

@@ -19,6 +19,8 @@
 
 namespace policy {
 
+class CloudExternalDataManager;
+
 // ConfigurationPolicyProvider for device or user policy from Active Directory.
 // The choice of constructor determines whether device or user policy is
 // provided.
@@ -40,7 +42,8 @@ class ActiveDirectoryPolicyManager : public ConfigurationPolicyProvider,
       const AccountId& account_id,
       base::TimeDelta initial_policy_fetch_timeout,
       base::OnceClosure exit_session,
-      std::unique_ptr<CloudPolicyStore> store);
+      std::unique_ptr<CloudPolicyStore> store,
+      std::unique_ptr<CloudExternalDataManager> external_data_manager);
 
   // ConfigurationPolicyProvider:
   void Init(SchemaRegistry* registry) override;
@@ -71,10 +74,12 @@ class ActiveDirectoryPolicyManager : public ConfigurationPolicyProvider,
   // enforce successful policy fetch. In case the conditions for signaling
   // initialization complete are not met, the user session is aborted by calling
   // |exit_session|.
-  ActiveDirectoryPolicyManager(const AccountId& account_id,
-                               base::TimeDelta initial_policy_fetch_timeout,
-                               base::OnceClosure exit_session,
-                               std::unique_ptr<CloudPolicyStore> store);
+  ActiveDirectoryPolicyManager(
+      const AccountId& account_id,
+      base::TimeDelta initial_policy_fetch_timeout,
+      base::OnceClosure exit_session,
+      std::unique_ptr<CloudPolicyStore> store,
+      std::unique_ptr<CloudExternalDataManager> external_data_manager);
 
   // Publish the policy that's currently cached in the store.
   void PublishPolicy();
@@ -123,7 +128,12 @@ class ActiveDirectoryPolicyManager : public ConfigurationPolicyProvider,
   // Callback to exit the session.
   base::OnceClosure exit_session_;
 
+  // Store used to serialize policy, usually sends data to Session Manager.
   std::unique_ptr<CloudPolicyStore> store_;
+
+  // Manages external data referenced by policies.
+  std::unique_ptr<CloudExternalDataManager> external_data_manager_;
+
   std::unique_ptr<PolicyScheduler> scheduler_;
 
   // Must be last member.

chrome/browser/chromeos/policy/cloud_external_data_policy_observer.cc

@@ -192,7 +192,7 @@ void CloudExternalDataPolicyObserver::OnPolicyUpdated(
   }
 
   if (!device_local_account_policy_service_) {
-    NOTREACHED();
+    // May happen in tests.
     return;
   }
   DeviceLocalAccountPolicyBroker* broker =

chrome/browser/chromeos/policy/cloud_external_data_policy_observer.h

@@ -57,6 +57,8 @@ class CloudExternalDataPolicyObserver
     virtual ~Delegate();
   };
 
+  // |device_local_account_policy_service| may be nullptr if unavailable (e.g.
+  // Active Directory management mode).
   CloudExternalDataPolicyObserver(
       chromeos::CrosSettings* cros_settings,
       DeviceLocalAccountPolicyService* device_local_account_policy_service,
@@ -98,7 +100,7 @@ class CloudExternalDataPolicyObserver
 
   // A map from each logged-in user to the helper that observes |policy_| in the
   // user's PolicyService.
-  typedef std::map<std::string, linked_ptr<PolicyServiceObserver> >
+  typedef std::map<std::string, linked_ptr<PolicyServiceObserver>>
       LoggedInUserObserverMap;
   LoggedInUserObserverMap logged_in_user_observers_;
 
@@ -117,9 +119,8 @@ class CloudExternalDataPolicyObserver
   // A map from user ID to a base::WeakPtr for each external data fetch
   // currently in progress. This allows fetches to be effectively be canceled by
   // invalidating the pointers.
-  typedef base::WeakPtrFactory<CloudExternalDataPolicyObserver>
-      WeakPtrFactory;
-  typedef std::map<std::string, linked_ptr<WeakPtrFactory> > FetchWeakPtrMap;
+  using WeakPtrFactory = base::WeakPtrFactory<CloudExternalDataPolicyObserver>;
+  using FetchWeakPtrMap = std::map<std::string, linked_ptr<WeakPtrFactory>>;
   FetchWeakPtrMap fetch_weak_ptrs_;
 
   base::WeakPtrFactory<CloudExternalDataPolicyObserver> weak_factory_;

chrome/browser/chromeos/policy/user_policy_manager_factory_chromeos.cc

@@ -355,7 +355,8 @@ UserPolicyManagerFactoryChromeOS::CreateManagerForProfile(
     std::unique_ptr<ActiveDirectoryPolicyManager> manager =
         ActiveDirectoryPolicyManager::CreateForUserPolicy(
             account_id, policy_refresh_timeout,
-            base::BindOnce(&chrome::AttemptUserExit), std::move(store));
+            base::BindOnce(&chrome::AttemptUserExit), std::move(store),
+            std::move(external_data_manager));
     manager->Init(
         SchemaRegistryServiceFactory::GetForContext(profile)->registry());