Don't augment GUID in ONC merging.

chromeos/network/onc/onc_merger.cc

Index: chromeos/network/onc/onc_merger.cc
diff --git a/chromeos/network/onc/onc_merger.cc b/chromeos/network/onc/onc_merger.cc
index 93bfefe455c56c27bfc7738ef63dab35be4c3f7b..ed81dada7f7fc7d4f2d8000c95ecf490ccde3e8a 100644
--- a/chromeos/network/onc/onc_merger.cc
+++ b/chromeos/network/onc/onc_merger.cc
@@ -20,6 +20,18 @@ namespace {
 
 typedef scoped_ptr<base::DictionaryValue> DictionaryPtr;
 
+// Returns true if the field is the identifier of a configuration, i.e. the GUID
+// of a network or a certificate. These can be special handled during merging
+// because they are always identical for the various setting sources.
+bool IsIdentifierField(const OncValueSignature& value_signature,
+                       const std::string& field_name) {
+  if (&value_signature == &kNetworkConfigurationSignature)
+    return field_name == ::onc::network_config::kGUID;
+  if (&value_signature == &kCertificateSignature)
+    return field_name == ::onc::certificate::kGUID;
+  return false;
+}
+
 // Inserts |true| at every field name in |result| that is recommended in
 // |policy|.
 void MarkRecommendedFieldnames(const base::DictionaryValue& policy,
@@ -329,10 +341,11 @@ class MergeToAugmented : public MergeToEffective {
   virtual scoped_ptr<base::Value> MergeValues(
       const std::string& key,
       const ValueParams& values) OVERRIDE {
-    scoped_ptr<base::DictionaryValue> result(new base::DictionaryValue);
+    scoped_ptr<base::DictionaryValue> augmented_value(
+        new base::DictionaryValue);
     if (values.active_setting) {
-      result->SetWithoutPathExpansion(::onc::kAugmentationActiveSetting,
-                                      values.active_setting->DeepCopy());
+      augmented_value->SetWithoutPathExpansion(
+          ::onc::kAugmentationActiveSetting, values.active_setting->DeepCopy());
     }
 
     const OncFieldSignature* field = NULL;
@@ -343,9 +356,31 @@ class MergeToAugmented : public MergeToEffective {
       // This field is part of the provided ONCSignature, thus it can be
       // controlled by policy.
       std::string which_effective;
-      MergeToEffective::MergeValues(key, values, &which_effective).reset();
+      scoped_ptr<base::Value> effective_value =
+          MergeToEffective::MergeValues(key, values, &which_effective);
+
+      if (IsIdentifierField(*signature_, key)) {
+        // Don't augment the GUID but write the plain value.
+        DCHECK(effective_value);
+
+        // DCHECK that all provided GUIDs are identical.
+        DCHECK(!values.user_policy ||
+               effective_value->Equals(values.user_policy));
+        DCHECK(!values.device_policy ||
+               effective_value->Equals(values.device_policy));
+        DCHECK(!values.user_setting ||
+               effective_value->Equals(values.user_setting));
+        DCHECK(!values.shared_setting ||
+               effective_value->Equals(values.shared_setting));
+        DCHECK(!values.active_setting ||
+               effective_value->Equals(values.active_setting));

[stevenjb, 2014/05/27 15:38:43]

nit: Maybe wrap all of these checks into a helper and DCHECK the result.

[pneubeck (no reviews), 2014/06/03 16:21:29]

Done.

+
+        // Return the un-augmented GUID.
+        return effective_value.Pass();
+      }
+
       if (!which_effective.empty()) {
-        result->SetStringWithoutPathExpansion(
+        augmented_value->SetStringWithoutPathExpansion(
             ::onc::kAugmentationEffectiveSetting, which_effective);
       }
       bool is_credential = onc::FieldIsCredential(*signature_, key);
@@ -355,39 +390,41 @@ class MergeToAugmented : public MergeToEffective {
       // leak here.
       if (!is_credential) {
         if (values.user_policy) {
-          result->SetWithoutPathExpansion(::onc::kAugmentationUserPolicy,
-                                          values.user_policy->DeepCopy());
+          augmented_value->SetWithoutPathExpansion(
+              ::onc::kAugmentationUserPolicy, values.user_policy->DeepCopy());
         }
         if (values.device_policy) {
-          result->SetWithoutPathExpansion(::onc::kAugmentationDevicePolicy,
-                                          values.device_policy->DeepCopy());
+          augmented_value->SetWithoutPathExpansion(
+              ::onc::kAugmentationDevicePolicy,
+              values.device_policy->DeepCopy());
         }
       }
       if (values.user_setting) {
-        result->SetWithoutPathExpansion(::onc::kAugmentationUserSetting,
-                                        values.user_setting->DeepCopy());
+        augmented_value->SetWithoutPathExpansion(
+            ::onc::kAugmentationUserSetting, values.user_setting->DeepCopy());
       }
       if (values.shared_setting) {
-        result->SetWithoutPathExpansion(::onc::kAugmentationSharedSetting,
-                                        values.shared_setting->DeepCopy());
+        augmented_value->SetWithoutPathExpansion(
+            ::onc::kAugmentationSharedSetting,
+            values.shared_setting->DeepCopy());
       }
       if (HasUserPolicy() && values.user_editable) {
-        result->SetBooleanWithoutPathExpansion(::onc::kAugmentationUserEditable,
-                                               true);
+        augmented_value->SetBooleanWithoutPathExpansion(
+            ::onc::kAugmentationUserEditable, true);
       }
       if (HasDevicePolicy() && values.device_editable) {
-        result->SetBooleanWithoutPathExpansion(
+        augmented_value->SetBooleanWithoutPathExpansion(
             ::onc::kAugmentationDeviceEditable, true);
       }
     } else {
       // This field is not part of the provided ONCSignature, thus it cannot be
       // controlled by policy.
-      result->SetStringWithoutPathExpansion(
+      augmented_value->SetStringWithoutPathExpansion(
           ::onc::kAugmentationEffectiveSetting, ::onc::kAugmentationUnmanaged);
     }
-    if (result->empty())
-      result.reset();
-    return result.PassAs<base::Value>();
+    if (augmented_value->empty())
+      augmented_value.reset();
+    return augmented_value.PassAs<base::Value>();
   }
 
   // MergeListOfDictionaries override.