Gracefully handle the situation of duplicate public accounts.

Make the code in DeviceLocalAccountPolicyService robust against this
case, i.e. don't crash and just ignore the duplicates. This situation
should not show up in theory, but since the data is coming from the
server being robust is better.

BUG=chromium:170538
TEST=unit test


Review URL: https://chromiumcodereview.appspot.com/12026003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@177662 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 38 insertions, 4 deletions

diff --git a/chrome/browser/policy/device_local_account_policy_service.cc b/chrome/browser/policy/device_local_account_policy_service.cc
index 26955bf..c214279 100644
--- a/chrome/browser/policy/device_local_account_policy_service.cc
+++ b/chrome/browser/policy/device_local_account_policy_service.cc
@@ -165,12 +165,19 @@ void DeviceLocalAccountPolicyService::UpdateAccountList(
   RepeatedPtrField<em::DeviceLocalAccountInfoProto>::const_iterator entry;
   for (entry = accounts.begin(); entry != accounts.end(); ++entry) {
     if (entry->has_id()) {
-      // Reuse the existing broker if present.
-      DeviceLocalAccountPolicyBroker*& broker = policy_brokers_[entry->id()];
+      // Sanity check for whether this account ID has already been processed.
       DeviceLocalAccountPolicyBroker*& new_broker =
           new_policy_brokers[entry->id()];
-      new_broker = broker;
-      broker = NULL;
+      if (new_broker) {
+        LOG(WARNING) << "Duplicate public account " << entry->id();
+        continue;
+      }
+
+      // Reuse the existing broker if present.
+      DeviceLocalAccountPolicyBroker*& existing_broker =
+          policy_brokers_[entry->id()];
+      new_broker = existing_broker;
+      existing_broker = NULL;
 
       // Fire up the cloud connection for fetching policy for the account from
       // the cloud if this is an enterprise-managed device.
diff --git a/chrome/browser/policy/device_local_account_policy_service_unittest.cc b/chrome/browser/policy/device_local_account_policy_service_unittest.cc
index b32b35e..37e617b 100644
--- a/chrome/browser/policy/device_local_account_policy_service_unittest.cc
+++ b/chrome/browser/policy/device_local_account_policy_service_unittest.cc
@@ -245,6 +245,33 @@ TEST_F(DeviceLocalAccountPolicyServiceTest, DevicePolicyChange) {
   Mock::VerifyAndClearExpectations(&service_observer_);
 }
 
+TEST_F(DeviceLocalAccountPolicyServiceTest, DuplicateAccounts) {
+  InstallDevicePolicy();
+  DeviceLocalAccountPolicyBroker* broker =
+      service_.GetBrokerForAccount(PolicyBuilder::kFakeUsername);
+  ASSERT_TRUE(broker);
+
+  // Add a second entry with a duplicate account name to device policy.
+  device_policy_.payload().mutable_device_local_accounts()->add_account()->
+      set_id(PolicyBuilder::kFakeUsername);
+  device_policy_.Build();
+  device_settings_test_helper_.set_device_local_account_policy_blob(
+      PolicyBuilder::kFakeUsername, device_local_account_policy_.GetBlob());
+  device_settings_test_helper_.set_policy_blob(device_policy_.GetBlob());
+
+  EXPECT_CALL(service_observer_, OnDeviceLocalAccountsChanged());
+  EXPECT_CALL(service_observer_, OnPolicyUpdated(PolicyBuilder::kFakeUsername));
+  device_settings_service_.PropertyChangeComplete(true);
+  FlushDeviceSettings();
+  Mock::VerifyAndClearExpectations(&service_observer_);
+
+  // Make sure the broker is accessible and policy got loaded.
+  broker = service_.GetBrokerForAccount(PolicyBuilder::kFakeUsername);
+  ASSERT_TRUE(broker);
+  EXPECT_EQ(PolicyBuilder::kFakeUsername, broker->account_id());
+  EXPECT_TRUE(broker->core()->store()->policy());
+}
+
 TEST_F(DeviceLocalAccountPolicyServiceTest, FetchPolicy) {
   device_settings_test_helper_.set_device_local_account_policy_blob(
       PolicyBuilder::kFakeUsername, device_local_account_policy_.GetBlob());