Fix VPN identifying properties during policy application.

The policy application code didn't respect the fact that VPN properties are written differently than being read.

BUG=365392

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

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

4 files changed, 82 insertions, 30 deletions

diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc
index 9bcf193..21c225a 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -488,8 +488,10 @@ void ManagedNetworkConfigurationHandlerImpl::
   shill_properties.SetStringWithoutPathExpansion(shill::kProfileProperty,
                                                  profile);
 
-  if (!shill_property_util::CopyIdentifyingProperties(existing_properties,
-                                                      &shill_properties)) {
+  if (!shill_property_util::CopyIdentifyingProperties(
+          existing_properties,
+          true /* properties were read from Shill */,
+          &shill_properties)) {
     NET_LOG_ERROR("Missing identifying properties",
                   shill_property_util::GetNetworkIdFromProperties(
                       existing_properties));
diff --git a/chromeos/network/managed_network_configuration_handler_unittest.cc b/chromeos/network/managed_network_configuration_handler_unittest.cc
index b880437..c3cc5c4 100644
--- a/chromeos/network/managed_network_configuration_handler_unittest.cc
+++ b/chromeos/network/managed_network_configuration_handler_unittest.cc
@@ -485,6 +485,32 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyUpdateManagedNewGUID) {
   message_loop_.RunUntilIdle();
 }
 
+TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyUpdateManagedVPN) {
+  InitializeStandardProfiles();
+  SetUpEntry("policy/shill_managed_vpn.json", kUser1ProfilePath, "entry_path");
+
+  scoped_ptr<base::DictionaryValue> expected_shill_properties =
+      test_utils::ReadTestDictionary(
+          "policy/shill_policy_on_managed_vpn.json");
+
+  EXPECT_CALL(*mock_profile_client_,
+              GetProperties(dbus::ObjectPath(kUser1ProfilePath), _, _));
+
+  EXPECT_CALL(
+      *mock_profile_client_,
+      GetEntry(dbus::ObjectPath(kUser1ProfilePath), "entry_path", _, _));
+
+  EXPECT_CALL(*mock_manager_client_,
+              ConfigureServiceForProfile(
+                  dbus::ObjectPath(kUser1ProfilePath),
+                  IsEqualTo(expected_shill_properties.get()),
+                  _, _));
+
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_vpn.onc");
+  message_loop_.RunUntilIdle();
+  VerifyAndClearExpectations();
+}
+
 TEST_F(ManagedNetworkConfigurationHandlerTest,
        SetPolicyUpdateManagedEquivalentSecurity) {
   InitializeStandardProfiles();
diff --git a/chromeos/network/shill_property_util.cc b/chromeos/network/shill_property_util.cc
index d9e1054..3104a5e 100644
--- a/chromeos/network/shill_property_util.cc
+++ b/chromeos/network/shill_property_util.cc
@@ -238,6 +238,7 @@ void SetUIData(const NetworkUIData& ui_data,
 }
 
 bool CopyIdentifyingProperties(const base::DictionaryValue& service_properties,
+                               const bool properties_read_from_shill,
                                base::DictionaryValue* dest) {
   bool success = true;
 
@@ -265,25 +266,33 @@ bool CopyIdentifyingProperties(const base::DictionaryValue& service_properties,
   } else if (type == shill::kTypeVPN) {
     success &= CopyStringFromDictionary(
         service_properties, shill::kNameProperty, dest);
+
     // VPN Provider values are read from the "Provider" dictionary, but written
     // with the keys "Provider.Type" and "Provider.Host".
-    const base::DictionaryValue* provider_properties = NULL;
-    if (!service_properties.GetDictionaryWithoutPathExpansion(
-            shill::kProviderProperty, &provider_properties)) {
-      NET_LOG_ERROR("Missing VPN provider dict",
-                    GetNetworkIdFromProperties(service_properties));
-      return false;
-    }
+    // TODO(pneubeck): Simplify this once http://crbug.com/381135 is fixed.
     std::string vpn_provider_type;
-    provider_properties->GetStringWithoutPathExpansion(shill::kTypeProperty,
-                                                       &vpn_provider_type);
+    std::string vpn_provider_host;
+    if (properties_read_from_shill) {
+      const base::DictionaryValue* provider_properties = NULL;
+      if (!service_properties.GetDictionaryWithoutPathExpansion(
+              shill::kProviderProperty, &provider_properties)) {
+        NET_LOG_ERROR("Missing VPN provider dict",
+                      GetNetworkIdFromProperties(service_properties));
+      }
+      provider_properties->GetStringWithoutPathExpansion(shill::kTypeProperty,
+                                                         &vpn_provider_type);
+      provider_properties->GetStringWithoutPathExpansion(shill::kHostProperty,
+                                                         &vpn_provider_host);
+    } else {
+      service_properties.GetStringWithoutPathExpansion(
+          shill::kProviderTypeProperty, &vpn_provider_type);
+      service_properties.GetStringWithoutPathExpansion(
+          shill::kProviderHostProperty, &vpn_provider_host);
+    }
     success &= !vpn_provider_type.empty();
     dest->SetStringWithoutPathExpansion(shill::kProviderTypeProperty,
                                         vpn_provider_type);
 
-    std::string vpn_provider_host;
-    provider_properties->GetStringWithoutPathExpansion(shill::kHostProperty,
-                                                       &vpn_provider_host);
     success &= !vpn_provider_host.empty();
     dest->SetStringWithoutPathExpansion(shill::kProviderHostProperty,
                                         vpn_provider_host);
@@ -301,16 +310,23 @@ bool CopyIdentifyingProperties(const base::DictionaryValue& service_properties,
   return success;
 }
 
-bool DoIdentifyingPropertiesMatch(const base::DictionaryValue& properties_a,
-                                  const base::DictionaryValue& properties_b) {
-  base::DictionaryValue identifying_a;
-  if (!CopyIdentifyingProperties(properties_a, &identifying_a))
+bool DoIdentifyingPropertiesMatch(const base::DictionaryValue& new_properties,
+                                  const base::DictionaryValue& old_properties) {
+  base::DictionaryValue new_identifying;
+  if (!CopyIdentifyingProperties(
+          new_properties,
+          false /* properties were not read from Shill */,
+          &new_identifying)) {
     return false;
-  base::DictionaryValue identifying_b;
-  if (!CopyIdentifyingProperties(properties_b, &identifying_b))
+  }
+  base::DictionaryValue old_identifying;
+  if (!CopyIdentifyingProperties(old_properties,
+                                 true /* properties were read from Shill */,
+                                 &old_identifying)) {
     return false;
+  }
 
-  return identifying_a.Equals(&identifying_b);
+  return new_identifying.Equals(&old_identifying);
 }
 
 bool IsPassphraseKey(const std::string& key) {
diff --git a/chromeos/network/shill_property_util.h b/chromeos/network/shill_property_util.h
index ca0aa5c..152e779 100644
--- a/chromeos/network/shill_property_util.h
+++ b/chromeos/network/shill_property_util.h
@@ -57,19 +57,27 @@ scoped_ptr<NetworkUIData> GetUIDataFromProperties(
 void SetUIData(const NetworkUIData& ui_data,
                base::DictionaryValue* shill_dictionary);
 
-// Copy configuration properties required by Shill to identify a network.
+// Copy configuration properties required by Shill to identify a network in the
+// format that Shill expects on writes.
 // Only WiFi, VPN, Ethernet and EthernetEAP are supported. Wimax and Cellular
-// are not supported. Returns true only if all required properties could be
-// copied.
+// are not supported.
+// If |properties_read_from_shill| is true, it is assumed that
+// |service_properties| has the format that Shill exposes on reads, as opposed
+// to property dictionaries which are sent to Shill. Returns true only if all
+// required properties could be copied.
 bool CopyIdentifyingProperties(const base::DictionaryValue& service_properties,
+                               const bool properties_read_from_shill,
                                base::DictionaryValue* dest);
 
-// Compares the identifying configuration properties of |properties_a| and
-// |properties_b|, returns true if they are identical. See also
-// CopyIdentifyingProperties. Only WiFi, VPN, Ethernet and EthernetEAP are
-// supported. Wimax and Cellular are not supported.
-bool DoIdentifyingPropertiesMatch(const base::DictionaryValue& properties_a,
-                                  const base::DictionaryValue& properties_b);
+// Compares the identifying configuration properties of |new_properties| and
+// |old_properties|, returns true if they are identical. |new_properties| must
+// have the form that Shill expects on writes. |old_properties| must have the
+// form that Shill exposes on reads. See also CopyIdentifyingProperties. Only
+// WiFi, VPN, Ethernet and EthernetEAP are supported. Wimax and Cellular are not
+// supported.
+bool DoIdentifyingPropertiesMatch(
+    const base::DictionaryValue& new_properties,
+    const base::DictionaryValue& old_properties);
 
 // Returns true if |key| corresponds to a passphrase property.
 bool IsPassphraseKey(const std::string& key);