Don't augment GUID in ONC merging.

The GUID property is used to pick user settings, policies and active settings for the same network. Thus it's more an identifier than a property and should not be exposed with additional meta information as it is the case for other properties.

BUG=372337

Review URL: https://codereview.chromium.org/291553006

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

4 files changed, 87 insertions, 30 deletions

diff --git a/chromeos/network/onc/onc_merger.cc b/chromeos/network/onc/onc_merger.cc
index 93bfefe..61053db 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,
@@ -302,6 +314,26 @@ class MergeToEffective : public MergeSettingsAndPolicies {
   DISALLOW_COPY_AND_ASSIGN(MergeToEffective);
 };
 
+namespace {
+
+// Returns true if all not-null values in |values| are equal to |value|.
+bool AllPresentValuesEqual(const MergeSettingsAndPolicies::ValueParams& values,
+                           const base::Value& value) {
+  if (values.user_policy && !value.Equals(values.user_policy))
+    return false;
+  if (values.device_policy && !value.Equals(values.device_policy))
+    return false;
+  if (values.user_setting && !value.Equals(values.user_setting))
+    return false;
+  if (values.shared_setting && !value.Equals(values.shared_setting))
+    return false;
+  if (values.active_setting && !value.Equals(values.active_setting))
+    return false;
+  return true;
+}
+
+}  // namespace
+
 // Call MergeDictionaries to merge policies and settings to an augmented
 // dictionary which contains a dictionary for each value in the original
 // dictionaries. See the description of MergeSettingsAndPoliciesToAugmented.
@@ -329,10 +361,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 +376,22 @@ 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(AllPresentValuesEqual(values, *effective_value));
+
+        // 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 +401,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.
diff --git a/chromeos/network/onc/onc_merger_unittest.cc b/chromeos/network/onc/onc_merger_unittest.cc
index 066803b..64ba338 100644
--- a/chromeos/network/onc/onc_merger_unittest.cc
+++ b/chromeos/network/onc/onc_merger_unittest.cc
@@ -53,6 +53,7 @@ class ONCMergerTest : public testing::Test {
   scoped_ptr<const base::DictionaryValue> policy_;
   scoped_ptr<const base::DictionaryValue> policy_without_recommended_;
   scoped_ptr<const base::DictionaryValue> device_policy_;
+  scoped_ptr<const base::DictionaryValue> active_;
 
   virtual void SetUp() {
     policy_ = test_utils::ReadTestDictionary("managed_vpn.onc");
@@ -60,6 +61,7 @@ class ONCMergerTest : public testing::Test {
         test_utils::ReadTestDictionary("managed_vpn_without_recommended.onc");
     user_ = test_utils::ReadTestDictionary("user.onc");
     device_policy_ = test_utils::ReadTestDictionary("device_policy.onc");
+    active_ = test_utils::ReadTestDictionary("vpn_active_settings.onc");
   }
 };
 
@@ -148,7 +150,7 @@ TEST_F(ONCMergerTest, MergeToAugmented) {
       test_utils::ReadTestDictionary("augmented_merge.json");
   scoped_ptr<base::DictionaryValue> merged(MergeSettingsAndPoliciesToAugmented(
       kNetworkConfigurationSignature, policy_.get(), device_policy_.get(),
-      user_.get(), NULL, NULL));
+      user_.get(), NULL, active_.get()));
   EXPECT_TRUE(test_utils::Equals(expected_augmented.get(), merged.get()));
 }
 
diff --git a/chromeos/test/data/network/augmented_merge.json b/chromeos/test/data/network/augmented_merge.json
index d7987ae..6ebbf04 100644
--- a/chromeos/test/data/network/augmented_merge.json
+++ b/chromeos/test/data/network/augmented_merge.json
@@ -1,9 +1,9 @@
 {
-   "GUID": {
-      "DevicePolicy": "123",
-      "Effective": "UserPolicy",
-      "UserPolicy": "123"
+   "ConnectionState": {
+      "Active": "Connected",
+      "Effective": "Unmanaged"
    },
+   "GUID": "123",
    "IPConfigs": {
       "DevicePolicy": [ {
          "IPAddress": "127.0.0.1",
@@ -25,6 +25,7 @@
       } ]
    },
    "Name": {
+      "Active": "testopenvpn",
       "DevicePolicy": "testopenvpn",
       "Effective": "UserPolicy",
       "UserPolicy": "testopenvpn",
@@ -37,6 +38,7 @@
       }
    },
    "Type": {
+      "Active": "VPN",
       "DevicePolicy": "VPN",
       "Effective": "UserPolicy",
       "UserPolicy": "VPN",
@@ -61,7 +63,7 @@
             "UserPolicy": 1
          },
          "PSK": {
-            "Effective": "UserPolicy",
+            "Effective": "UserPolicy"
          }
       },
       "OpenVPN": {
@@ -96,15 +98,15 @@
             "UserPolicy": 1194,
             "UserSetting": 1195
          },
+         "ServerCARefs": {
+            "Effective": "UserPolicy",
+            "UserPolicy": ["ref1", "ref2"]
+         },
          "Username": {
             "DevicePolicy": "device user",
             "Effective": "DevicePolicy",
             "UserEditable": true,
             "UserPolicy": "policy user"
-         },
-         "ServerCARefs": {
-            "UserPolicy": ["ref1", "ref2"],
-            "Effective": "UserPolicy"
          }
       },
       "Type": {
diff --git a/chromeos/test/data/network/vpn_active_settings.onc b/chromeos/test/data/network/vpn_active_settings.onc
new file mode 100644
index 0000000..62b3c21
--- /dev/null
+++ b/chromeos/test/data/network/vpn_active_settings.onc
@@ -0,0 +1,5 @@
+{ "Type": "VPN",
+  "Name": "testopenvpn",
+  "ConnectionState": "Connected",
+  "GUID": "123"
+}