ONC policy: Support SSID and Security change.

If the SSID of a ONC policy is changed without changing the GUID, this lead to a partial policy application where everything except the SSID is changed.
This happened because Shill doesn't allow changes of the SSID property after the network creation but the policy application code didn't respect that.

With this change, it's ensured that the respective network configuration (profile entry), is deleted and rewritten on each SSID change.

The same applies to the Security type of the network.

BUG=319362
R=bartfab@chromium.org

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

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

4 files changed, 50 insertions, 27 deletions

diff --git a/chromeos/network/policy_applicator.cc b/chromeos/network/policy_applicator.cc
index 462caee..1b01f67 100644
--- a/chromeos/network/policy_applicator.cc
+++ b/chromeos/network/policy_applicator.cc
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/stl_util.h"
 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
@@ -167,22 +168,32 @@ void PolicyApplicator::GetEntryCallback(
       VLOG(1) << "Not updating existing managed configuration with guid "
               << new_guid << " because the policy didn't change.";
     } else {
-      // Delete the entry to ensure that no old configuration remains.
-      // Don't do this if a policy is reapplied (e.g. after reboot) or updated
-      // (i.e. the GUID didn't change), in order to keep implicit state of
-      // Shill like "connected successfully before".
-      if (old_guid == new_guid) {
+      const base::DictionaryValue* user_settings =
+          ui_data ? ui_data->user_settings() : NULL;
+      scoped_ptr<base::DictionaryValue> new_shill_properties =
+          policy_util::CreateShillConfiguration(
+              profile_, new_guid, new_policy, user_settings);
+      // A new policy has to be applied to this profile entry. In order to keep
+      // implicit state of Shill like "connected successfully before", keep the
+      // entry if a policy is reapplied (e.g. after reboot) or is updated.
+      // However, some Shill properties are used to identify the network and
+      // cannot be modified after initial configuration, so we have to delete
+      // the profile entry in these cases. Also, keeping Shill's state if the
+      // SSID changed might not be a good idea anyways. If the policy GUID
+      // changed, or there was no policy before, we delete the entry at first to
+      // ensure that no old configuration remains.
+      if (old_guid == new_guid &&
+          shill_property_util::DoIdentifyingPropertiesMatch(
+              *new_shill_properties, entry_properties)) {
         VLOG(1) << "Updating previously managed configuration with the "
                 << "updated policy " << new_guid << ".";
       } else {
+        VLOG(1) << "Deleting profile entry before writing new policy "
+                << new_guid << " because of identifying properties changed.";
         DeleteEntry(entry);
       }
 
-      const base::DictionaryValue* user_settings =
-          ui_data ? ui_data->user_settings() : NULL;
-
-      // Write the new configuration.
-      CreateAndWriteNewShillConfiguration(new_guid, *new_policy, user_settings);
+      WriteNewShillConfiguration(*new_shill_properties, *new_policy);
       remaining_policies_.erase(new_guid);
     }
   } else if (was_managed) {
@@ -196,7 +207,6 @@ void PolicyApplicator::GetEntryCallback(
   } else {
     // The entry wasn't managed and doesn't match any current policy. Global
     // network settings have to be applied.
-
     base::DictionaryValue shill_properties_to_update;
     GetPropertiesForUnmanagedEntry(entry_properties,
                                    &shill_properties_to_update);
@@ -219,10 +229,9 @@ void PolicyApplicator::DeleteEntry(const std::string& entry) {
       base::Bind(&LogErrorMessage, FROM_HERE));
 }
 
-void PolicyApplicator::CreateAndWriteNewShillConfiguration(
-    const std::string& guid,
-    const base::DictionaryValue& policy,
-    const base::DictionaryValue* user_settings) {
+void PolicyApplicator::WriteNewShillConfiguration(
+    const base::DictionaryValue& shill_dictionary,
+    const base::DictionaryValue& policy) {
   // Ethernet (non EAP) settings, like GUID or UIData, cannot be stored per
   // user. Abort in that case.
   std::string type;
@@ -239,10 +248,7 @@ void PolicyApplicator::CreateAndWriteNewShillConfiguration(
       return;
   }
 
-  scoped_ptr<base::DictionaryValue> shill_dictionary =
-      policy_util::CreateShillConfiguration(
-          profile_, guid, &policy, user_settings);
-  handler_->CreateConfigurationFromPolicy(*shill_dictionary);
+  handler_->CreateConfigurationFromPolicy(shill_dictionary);
 }
 
 void PolicyApplicator::GetPropertiesForUnmanagedEntry(
@@ -304,8 +310,9 @@ void PolicyApplicator::ApplyRemainingPolicies() {
     VLOG(1) << "Creating new configuration managed by policy " << *it
             << " in profile " << profile_.ToDebugString() << ".";
 
-    CreateAndWriteNewShillConfiguration(
-        *it, *policy, NULL /* no user settings */);
+    scoped_ptr<base::DictionaryValue> shill_dictionary =
+        policy_util::CreateShillConfiguration(profile_, *it, policy, NULL);
+    WriteNewShillConfiguration(*shill_dictionary, *policy);
   }
 }
 
diff --git a/chromeos/network/policy_applicator.h b/chromeos/network/policy_applicator.h
index facb378..a98aae7 100644
--- a/chromeos/network/policy_applicator.h
+++ b/chromeos/network/policy_applicator.h
@@ -69,12 +69,9 @@ class PolicyApplicator : public base::RefCounted<PolicyApplicator> {
   // Sends Shill the command to delete profile entry |entry| from |profile_|.
   void DeleteEntry(const std::string& entry);
 
-  // Creates a Shill configuration from the given parameters and sends them to
-  // Shill. |user_settings| can be NULL if none exist.
-  void CreateAndWriteNewShillConfiguration(
-      const std::string& guid,
-      const base::DictionaryValue& policy,
-      const base::DictionaryValue* user_settings);
+  // Sends the Shill configuration |shill_dictionary| to Shill.
+  void WriteNewShillConfiguration(const base::DictionaryValue& shill_dictionary,
+                                  const base::DictionaryValue& policy);
 
   // Adds properties to |properties_to_update|, which are enforced on an
   // unamaged network by the global network config of the policy.
diff --git a/chromeos/network/shill_property_util.cc b/chromeos/network/shill_property_util.cc
index 3077218..31709b3 100644
--- a/chromeos/network/shill_property_util.cc
+++ b/chromeos/network/shill_property_util.cc
@@ -251,6 +251,18 @@ 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))
+    return false;
+  base::DictionaryValue identifying_b;
+  if (!CopyIdentifyingProperties(properties_b, &identifying_b))
+    return false;
+
+  return identifying_a.Equals(&identifying_b);
+}
+
 }  // namespace shill_property_util
 
 namespace {
diff --git a/chromeos/network/shill_property_util.h b/chromeos/network/shill_property_util.h
index dc1f45f..466b967 100644
--- a/chromeos/network/shill_property_util.h
+++ b/chromeos/network/shill_property_util.h
@@ -59,6 +59,13 @@ void SetUIData(const NetworkUIData& ui_data,
 bool CopyIdentifyingProperties(const base::DictionaryValue& service_properties,
                                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);
+
 }  // namespace shill_property_util
 
 class CHROMEOS_EXPORT NetworkTypePattern {