Cache force-installed apps/extensions in device-local accounts

chrome/browser/chromeos/policy/device_local_account_policy_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/policy/device_local_account_policy_service.h"
 
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback.h"
+#include "base/file_util.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/path_service.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/string_number_conversions.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/policy/device_local_account_policy_store.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"

[Joao da Silva, 2013/10/17 14:57:54]

already in .h

[bartfab (slow), 2013/10/18 12:58:39]

Done.

 #include "chrome/browser/chromeos/settings/device_settings_service.h"
 #include "chrome/browser/policy/cloud/cloud_policy_client.h"
 #include "chrome/browser/policy/cloud/cloud_policy_constants.h"
+#include "chrome/browser/policy/cloud/cloud_policy_core.h"
 #include "chrome/browser/policy/cloud/cloud_policy_refresh_scheduler.h"
 #include "chrome/browser/policy/cloud/device_management_service.h"
 #include "chrome/browser/policy/proto/cloud/device_management_backend.pb.h"
+#include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/session_manager_client.h"
 #include "chromeos/settings/cros_settings_names.h"
 #include "chromeos/settings/cros_settings_provider.h"
+#include "content/public/browser/browser_thread.h"
 #include "policy/policy_constants.h"
 
 namespace em = enterprise_management;
 
 namespace policy {
 
 namespace {
 
 // Creates and initializes a cloud policy client. Returns NULL if the device
 // doesn't have credentials in device settings (i.e. is not
@@ skipping 11 matching lines @@
 
   scoped_ptr<CloudPolicyClient> client(
       new CloudPolicyClient(std::string(), std::string(),
                             USER_AFFILIATION_MANAGED,
                             NULL, device_management_service));
   client->SetupRegistration(policy_data->request_token(),
                             policy_data->device_id());
   return client.Pass();
 }
 
+// Get the subdirectory of the cache directory in which force-installed
+// extensions are cached for |account_id|.
+std::string GetCacheSubdirectoryForAccountID(const std::string& account_id) {
+  return base::HexEncode(account_id.c_str(), account_id.size());
+}
+
+// Cleans up the cache directory by removing subdirectories that do not belong
+// to any of the accounts in |account_ids_to_keep| and posts |callback| to the
+// UI thread when done. Only caches belonging to accounts in
+// |account_ids_to_keep| may be running while the clean-up is in progress.
+void DeleteOrphanedExtensionCaches(
+    const std::vector<std::string>& account_ids_to_keep,
+    const base::Closure& callback) {
+  std::set<std::string> subdirectories_to_keep;
+  for (std::vector<std::string>::const_iterator it =
+           account_ids_to_keep.begin();
+       it != account_ids_to_keep.end(); ++it) {
+    subdirectories_to_keep.insert(GetCacheSubdirectoryForAccountID(*it));
+  }
+  base::FilePath cache_root_dir;
+  CHECK(PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_CACHE,
+                         &cache_root_dir));
+  base::FileEnumerator enumerator(cache_root_dir,
+                                  false,
+                                  base::FileEnumerator::DIRECTORIES);
+  for (base::FilePath path = enumerator.Next(); !path.empty();
+       path = enumerator.Next()) {
+    const std::string subdirectory(path.BaseName().MaybeAsASCII());
+    if (subdirectories_to_keep.find(subdirectory) ==
+        subdirectories_to_keep.end()) {
+      base::DeleteFile(path, true);
+    }
+  }
+  content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                   FROM_HERE,
+                                   callback);

[Joao da Silva, 2013/10/17 14:57:54]

See below about avoiding BrowserThread and the UI thread explicitly

[bartfab (slow), 2013/10/18 12:58:39]

Done.

+}
+
+// Removes the subdirectory belonging to |account_id_to_delete| from the cache
+// directory and posts |callback| to the UI thread when done. No cache belonging
+// to |account_id_to_delete| may be running while the removal is in progress.
+void DeleteObsoleteExtensionCache(
+    const std::string& account_id_to_delete,
+    base::Callback<void(const std::string&)> callback) {
+  base::FilePath cache_root_dir;
+  CHECK(PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_CACHE,
+                         &cache_root_dir));
+  const base::FilePath path = cache_root_dir
+      .Append(GetCacheSubdirectoryForAccountID(account_id_to_delete));
+  if (base::DirectoryExists(path))
+    base::DeleteFile(path, true);
+  content::BrowserThread::PostTask(content::BrowserThread::UI,
+                                   FROM_HERE,
+                                   base::Bind(callback, account_id_to_delete));

[Joao da Silva, 2013/10/17 14:57:54]

Same here, see below

[bartfab (slow), 2013/10/18 12:58:39]

Done.

+}
+
 }  // namespace
 
 DeviceLocalAccountPolicyBroker::DeviceLocalAccountPolicyBroker(
-    const std::string& user_id,
+    const DeviceLocalAccount& account,
     scoped_ptr<DeviceLocalAccountPolicyStore> store,
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
-    : user_id_(user_id),
-      store_(store.Pass()),
-      core_(PolicyNamespaceKey(dm_protocol::kChromePublicAccountPolicyType,
-                               store_->account_id()),
-            store_.get(),
-            task_runner) {}
+    : account_id_(account.account_id),
+      user_id_(account.user_id),
+      store_(store.Pass()) {
+  base::FilePath cache_root_dir;
+  CHECK(PathService::Get(chromeos::DIR_DEVICE_LOCAL_ACCOUNT_CACHE,
+                         &cache_root_dir));
+  extension_loader_ = new chromeos::DeviceLocalAccountExternalPolicyLoader(
+      store_.get(),
+      cache_root_dir.Append(
+          GetCacheSubdirectoryForAccountID(account.account_id)));
+  core_.reset(new CloudPolicyCore(
+      PolicyNamespaceKey(dm_protocol::kChromePublicAccountPolicyType,
+                         store_->account_id()),
+      store_.get(),
+      task_runner));

[Joao da Silva, 2013/10/17 14:57:54]

Why is the core in a scoped_ptr now?

[bartfab (slow), 2013/10/18 12:58:39]

That was a leftover from some earlier iteration of the construction sequence.
Done.

+  store_->Load();
+}
 
-DeviceLocalAccountPolicyBroker::~DeviceLocalAccountPolicyBroker() {}
+DeviceLocalAccountPolicyBroker::~DeviceLocalAccountPolicyBroker() {
+}
 
-void DeviceLocalAccountPolicyBroker::Connect(
-    scoped_ptr<CloudPolicyClient> client) {
-  core_.Connect(client.Pass());
-  core_.StartRefreshScheduler();
+void DeviceLocalAccountPolicyBroker::ConnectIfPossible(
+    chromeos::DeviceSettingsService* device_settings_service,
+    DeviceManagementService* device_management_service) {
+  if (core_->client())
+    return;
+
+  scoped_ptr<CloudPolicyClient> client(CreateClient(device_settings_service,
+                                                    device_management_service));
+  if (!client)
+    return;
+
+  core_->Connect(client.Pass());
+  core_->StartRefreshScheduler();
   UpdateRefreshDelay();
 }
 
 void DeviceLocalAccountPolicyBroker::Disconnect() {
-  core_.Disconnect();
+  core_->Disconnect();
 }
 
 void DeviceLocalAccountPolicyBroker::UpdateRefreshDelay() {
-  if (core_.refresh_scheduler()) {
+  if (core_->refresh_scheduler()) {
     const Value* policy_value =
         store_->policy_map().GetValue(key::kPolicyRefreshRate);
     int delay = 0;
     if (policy_value && policy_value->GetAsInteger(&delay))
-      core_.refresh_scheduler()->SetRefreshDelay(delay);
+      core_->refresh_scheduler()->SetRefreshDelay(delay);
   }
 }
 
 std::string DeviceLocalAccountPolicyBroker::GetDisplayName() const {
   std::string display_name;
   const base::Value* display_name_value =
       store_->policy_map().GetValue(policy::key::kUserDisplayName);
   if (display_name_value)
     display_name_value->GetAsString(&display_name);
   return display_name;
 }
 
-DeviceLocalAccountPolicyService::PolicyBrokerWrapper::PolicyBrokerWrapper()
-    : parent(NULL), broker(NULL) {}
-
-DeviceLocalAccountPolicyBroker*
-    DeviceLocalAccountPolicyService::PolicyBrokerWrapper::GetBroker() {
-  if (!broker) {
-    scoped_ptr<DeviceLocalAccountPolicyStore> store(
-        new DeviceLocalAccountPolicyStore(account_id,
-                                          parent->session_manager_client_,
-                                          parent->device_settings_service_));
-    broker = new DeviceLocalAccountPolicyBroker(
-        user_id, store.Pass(), base::MessageLoopProxy::current());
-    broker->core()->store()->AddObserver(parent);
-    broker->core()->store()->Load();
-  }
-  return broker;
-}
-
-void DeviceLocalAccountPolicyService::PolicyBrokerWrapper::ConnectIfPossible() {
-  if (broker && broker->core()->client())
-    return;
-  scoped_ptr<CloudPolicyClient> client(CreateClient(
-      parent->device_settings_service_,
-      parent->device_management_service_));
-  if (client)
-    GetBroker()->Connect(client.Pass());
-}
-
-void DeviceLocalAccountPolicyService::PolicyBrokerWrapper::Disconnect() {
-  if (broker)
-    broker->Disconnect();
-}
-
-void DeviceLocalAccountPolicyService::PolicyBrokerWrapper::DeleteBroker() {
-  if (!broker)
-    return;
-  broker->core()->store()->RemoveObserver(parent);
-  delete broker;
-  broker = NULL;
-}
-
 DeviceLocalAccountPolicyService::DeviceLocalAccountPolicyService(
     chromeos::SessionManagerClient* session_manager_client,
     chromeos::DeviceSettingsService* device_settings_service,
-    chromeos::CrosSettings* cros_settings)
+    chromeos::CrosSettings* cros_settings,
+    scoped_refptr<base::SequencedTaskRunner> extension_cache_task_runner)
     : session_manager_client_(session_manager_client),
       device_settings_service_(device_settings_service),
       cros_settings_(cros_settings),
       device_management_service_(NULL),
-      cros_settings_callback_factory_(this) {
-  local_accounts_subscription_ = cros_settings_->AddSettingsObserver(
-      chromeos::kAccountsPrefDeviceLocalAccounts,
-      base::Bind(&DeviceLocalAccountPolicyService::
-                     UpdateAccountListIfNonePending,
-                 base::Unretained(this)));
+      waiting_for_cros_settings_(false),
+      orphaned_cache_deletion_started_(false),
+      orphaned_cache_deletion_finished_(false),
+      extension_cache_task_runner_(extension_cache_task_runner),
+      local_accounts_subscription_(cros_settings_->AddSettingsObserver(
+          chromeos::kAccountsPrefDeviceLocalAccounts,
+          base::Bind(&DeviceLocalAccountPolicyService::
+                         UpdateAccountListIfNonePending,
+                     base::Unretained(this)))),
+      weak_factory_(this) {
   UpdateAccountList();
 }
 
 DeviceLocalAccountPolicyService::~DeviceLocalAccountPolicyService() {
   DeleteBrokers(&policy_brokers_);
 }
 
 void DeviceLocalAccountPolicyService::Connect(
     DeviceManagementService* device_management_service) {
   DCHECK(!device_management_service_);
   device_management_service_ = device_management_service;
 
   // Connect the brokers.
   for (PolicyBrokerMap::iterator it(policy_brokers_.begin());
        it != policy_brokers_.end(); ++it) {
-    it->second.ConnectIfPossible();
+    it->second->ConnectIfPossible(device_settings_service_,
+                                  device_management_service_);
   }
 }
 
 void DeviceLocalAccountPolicyService::Disconnect() {
   DCHECK(device_management_service_);
   device_management_service_ = NULL;
 
   // Disconnect the brokers.
   for (PolicyBrokerMap::iterator it(policy_brokers_.begin());
        it != policy_brokers_.end(); ++it) {
-    it->second.Disconnect();
+    it->second->Disconnect();
   }
 }
 
 DeviceLocalAccountPolicyBroker*
     DeviceLocalAccountPolicyService::GetBrokerForUser(
         const std::string& user_id) {
   PolicyBrokerMap::iterator entry = policy_brokers_.find(user_id);
   if (entry == policy_brokers_.end())
     return NULL;
 
-  return entry->second.GetBroker();
+  return entry->second;
 }
 
 bool DeviceLocalAccountPolicyService::IsPolicyAvailableForUser(
     const std::string& user_id) {
   DeviceLocalAccountPolicyBroker* broker = GetBrokerForUser(user_id);
   return broker && broker->core()->store()->is_managed();
 }
 
 void DeviceLocalAccountPolicyService::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
@@ skipping 13 matching lines @@
 }
 
 void DeviceLocalAccountPolicyService::OnStoreError(CloudPolicyStore* store) {
   DeviceLocalAccountPolicyBroker* broker = GetBrokerForStore(store);
   DCHECK(broker);
   if (!broker)
     return;
   FOR_EACH_OBSERVER(Observer, observers_, OnPolicyUpdated(broker->user_id()));
 }
 
+bool DeviceLocalAccountPolicyService::IsExtensionCacheDirectoryBusy(
+    const std::string& account_id) {
+  return busy_extension_cache_directories_.find(account_id) !=
+            busy_extension_cache_directories_.end();
+}
+
+void DeviceLocalAccountPolicyService::StartExtensionCachesIfPossible() {
+  for (PolicyBrokerMap::iterator it = policy_brokers_.begin();
+       it != policy_brokers_.end(); ++it) {
+    if (!it->second->extension_loader()->IsCacheRunning() &&
+        !IsExtensionCacheDirectoryBusy(it->second->account_id())) {
+      it->second->extension_loader()->StartCache(extension_cache_task_runner_);
+    }
+  }
+}
+
+bool DeviceLocalAccountPolicyService::StartExtensionCacheForAccountIfPresent(
+    const std::string& account_id) {
+  for (PolicyBrokerMap::iterator it = policy_brokers_.begin();
+       it != policy_brokers_.end(); ++it) {
+    if (it->second->account_id() == account_id) {
+      DCHECK(!it->second->extension_loader()->IsCacheRunning());
+      it->second->extension_loader()->StartCache(extension_cache_task_runner_);
+      return true;
+    }
+  }
+  return false;
+}
+
+void DeviceLocalAccountPolicyService::OnOrphanedExtensionCachesDeleted() {
+  DCHECK(orphaned_cache_deletion_started_);
+  DCHECK(!orphaned_cache_deletion_finished_);
+
+  orphaned_cache_deletion_finished_ = true;
+  StartExtensionCachesIfPossible();
+}
+
+void DeviceLocalAccountPolicyService::OnObsoleteExensionCacheShutdown(
+    const std::string& account_id) {
+  DCHECK(orphaned_cache_deletion_started_);
+  DCHECK(IsExtensionCacheDirectoryBusy(account_id));
+
+  // The account with |account_id| was deleted and the broker for it has shut
+  // down completely.
+
+  if (StartExtensionCacheForAccountIfPresent(account_id)) {
+    // If another account with the same ID was created in the meantime, its
+    // extension cache is started, reusing the cache directory. The directory no
+    // longer needs to be marked as busy in this case.
+    busy_extension_cache_directories_.erase(account_id);
+    return;
+  }
+
+  // If no account with |account_id| exists anymore, the cache directory should
+  // be removed. The directory must stay marked as busy while the removal is in
+  // progress.
+  extension_cache_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &DeleteObsoleteExtensionCache,
+      account_id,
+      base::Bind(&DeviceLocalAccountPolicyService::
+                     OnObsoleteExtensionCacheDeleted,
+                 weak_factory_.GetWeakPtr())));

[Joao da Silva, 2013/10/17 14:57:54]

Instead of posting this callback to UI, why don't you use PostTaskAndReply? It
achieves the same & avoids having to use BrowserThread here, which I've been
trying to avoid (even though this code won't move into the component)

See also PostTaskAndReplyWithResult in task_runner_utils.h for the callback with
the string.

[bartfab (slow), 2013/10/18 12:58:39]

Done. No need for PostTaskAndReplyWithResult() either as I can just bind
|account_id| to the completion callback straight away.

+}
+
+void DeviceLocalAccountPolicyService::OnObsoleteExtensionCacheDeleted(
+    const std::string& account_id) {
+  DCHECK(orphaned_cache_deletion_started_);
+  DCHECK(orphaned_cache_deletion_finished_);
+  DCHECK(IsExtensionCacheDirectoryBusy(account_id));
+
+  // The cache directory for |account_id| has been deleted. The directory no
+  // longer needs to be marked as busy.
+  busy_extension_cache_directories_.erase(account_id);
+
+  // If another account with the same ID was created in the meantime, start its
+  // extension cache, creating a new cache directory.
+  StartExtensionCacheForAccountIfPresent(account_id);
+}
+
 void DeviceLocalAccountPolicyService::UpdateAccountListIfNonePending() {
   // Avoid unnecessary calls to UpdateAccountList(): If an earlier call is still
   // pending (because the |cros_settings_| are not trusted yet), the updated
   // account list will be processed by that call when it eventually runs.
-  if (!cros_settings_callback_factory_.HasWeakPtrs())
+  if (!waiting_for_cros_settings_)
     UpdateAccountList();
 }
 
 void DeviceLocalAccountPolicyService::UpdateAccountList() {
-  if (chromeos::CrosSettingsProvider::TRUSTED !=
-          cros_settings_->PrepareTrustedValues(
-              base::Bind(&DeviceLocalAccountPolicyService::UpdateAccountList,
-                         cros_settings_callback_factory_.GetWeakPtr()))) {
-    return;
+  chromeos::CrosSettingsProvider::TrustedStatus status =
+      cros_settings_->PrepareTrustedValues(
+          base::Bind(&DeviceLocalAccountPolicyService::UpdateAccountList,
+                     weak_factory_.GetWeakPtr()));
+  switch (status) {
+    case chromeos::CrosSettingsProvider::TRUSTED:
+      waiting_for_cros_settings_ = false;
+      break;
+    case chromeos::CrosSettingsProvider::TEMPORARILY_UNTRUSTED:
+      waiting_for_cros_settings_ = true;
+      return;
+    case chromeos::CrosSettingsProvider::PERMANENTLY_UNTRUSTED:
+      waiting_for_cros_settings_ = false;
+      return;
   }
 
   // Update |policy_brokers_|, keeping existing entries.
   PolicyBrokerMap old_policy_brokers;
   policy_brokers_.swap(old_policy_brokers);
+  std::vector<std::string> account_ids;
   const std::vector<DeviceLocalAccount> device_local_accounts =
       GetDeviceLocalAccounts(cros_settings_);
   for (std::vector<DeviceLocalAccount>::const_iterator it =
            device_local_accounts.begin();
        it != device_local_accounts.end(); ++it) {
-    PolicyBrokerWrapper& wrapper = policy_brokers_[it->user_id];
-    wrapper.user_id = it->user_id;
-    wrapper.account_id = it->account_id;
-    wrapper.parent = this;
-
-    // Reuse the existing broker if present.
-    PolicyBrokerWrapper& existing_wrapper = old_policy_brokers[it->user_id];
-    wrapper.broker = existing_wrapper.broker;
-    existing_wrapper.broker = NULL;
+    PolicyBrokerMap::iterator broker_it = old_policy_brokers.find(it->user_id);
+
+    scoped_ptr<DeviceLocalAccountPolicyBroker> broker;
+    if (broker_it != old_policy_brokers.end()) {
+      // Reuse the existing broker if present.
+      broker.reset(broker_it->second);
+      old_policy_brokers.erase(broker_it);
+    } else {
+      scoped_ptr<DeviceLocalAccountPolicyStore> store(
+          new DeviceLocalAccountPolicyStore(it->account_id,
+                                            session_manager_client_,
+                                            device_settings_service_));
+      store->AddObserver(this);
+      broker.reset(new DeviceLocalAccountPolicyBroker(
+          *it,
+          store.Pass(),
+          base::MessageLoopProxy::current()));
+    }
 
     // Fire up the cloud connection for fetching policy for the account from
     // the cloud if this is an enterprise-managed device.
-    wrapper.ConnectIfPossible();
-  }
-  DeleteBrokers(&old_policy_brokers);
+    broker->ConnectIfPossible(device_settings_service_,
+                              device_management_service_);
+
+    policy_brokers_[it->user_id] = broker.release();
+    account_ids.push_back(it->account_id);
+  }
+
+  std::set<std::string> obsolete_account_ids;
+  for (PolicyBrokerMap::const_iterator it = old_policy_brokers.begin();
+       it != old_policy_brokers.end(); ++it) {
+    obsolete_account_ids.insert(it->second->account_id());
+  }
+
+  if (!orphaned_cache_deletion_started_) {
+    DCHECK(old_policy_brokers.empty());
+    DCHECK(busy_extension_cache_directories_.empty());
+
+    // If this method is running for the first time, no extension caches have
+    // been started yet. Take this opportunity to do a clean-up by removing
+    // orphaned cache directories that do not belong to any of the accounts in
+    // |account_ids| from the cache directory.
+    orphaned_cache_deletion_started_ = true;
+    extension_cache_task_runner_->PostTask(FROM_HERE, base::Bind(
+        &DeleteOrphanedExtensionCaches,
+        account_ids,

[Joao da Silva, 2013/10/17 14:57:54]

|account_ids| only seem to be used here, and then
DeleteOrphanedExtensionCaches() goes over them and builds a set with the encoded
names. Why not just build the encoded names set from here?

[bartfab (slow), 2013/10/18 12:58:39]

Done.

+        base::Bind(&DeviceLocalAccountPolicyService::
+                       OnOrphanedExtensionCachesDeleted,
+                   weak_factory_.GetWeakPtr())));

[Joao da Silva, 2013/10/17 14:57:54]

PostTaskAndReply?

[bartfab (slow), 2013/10/18 12:58:39]

Done.

+
+    // Start the extension caches for all brokers. These belong to accounts in
+    // |account_ids| and are not affected by the clean-up.
+    StartExtensionCachesIfPossible();
+

[Joao da Silva, 2013/10/17 14:57:54]

remove newline

[bartfab (slow), 2013/10/18 12:58:39]

Done.

+  } else {
+    // If this method has run before, obsolete brokers may exist. Shut down
+    // their extension caches and delete the brokers.
+    DeleteBrokers(&old_policy_brokers);
+
+    if (orphaned_cache_deletion_finished_) {
+      // If the initial clean-up of orphaned cache directories has been
+      // complete, start any extension caches that are not running yet but can
+      // be started now because their cache directories are not busy.
+      StartExtensionCachesIfPossible();
+    }
+  }
 
   FOR_EACH_OBSERVER(Observer, observers_, OnDeviceLocalAccountsChanged());
 }
 
 void DeviceLocalAccountPolicyService::DeleteBrokers(PolicyBrokerMap* map) {
-  for (PolicyBrokerMap::iterator it = map->begin(); it != map->end(); ++it)
-    it->second.DeleteBroker();
+  for (PolicyBrokerMap::iterator it = map->begin(); it != map->end(); ++it) {
+    it->second->core()->store()->RemoveObserver(this);
+    scoped_refptr<chromeos::DeviceLocalAccountExternalPolicyLoader>
+        extension_loader = it->second->extension_loader();
+    if (extension_loader->IsCacheRunning()) {
+      DCHECK(!IsExtensionCacheDirectoryBusy(it->second->account_id()));
+      busy_extension_cache_directories_.insert(it->second->account_id());
+      extension_loader->StopCache(base::Bind(
+          &DeviceLocalAccountPolicyService::OnObsoleteExensionCacheShutdown,
+          weak_factory_.GetWeakPtr(),
+          it->second->account_id()));
+    }
+    delete it->second;
+  }
   map->clear();
 }
 
 DeviceLocalAccountPolicyBroker*
     DeviceLocalAccountPolicyService::GetBrokerForStore(
         CloudPolicyStore* store) {
   for (PolicyBrokerMap::iterator it(policy_brokers_.begin());
        it != policy_brokers_.end(); ++it) {
-    if (it->second.broker && it->second.broker->core()->store() == store)
-      return it->second.broker;
+    if (it->second->core()->store() == store)
+      return it->second;
   }
   return NULL;
 }
 
 }  // namespace policy