Added remove functionality for ONC import

BUG=chromium:127124
TEST=Ran new unit test, added a network via ONC, removed it via ONC and checked profiles to make sure it was gone.


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

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

6 files changed, 108 insertions, 58 deletions

diff --git a/chrome/browser/chromeos/cros/network_library_impl_base.cc b/chrome/browser/chromeos/cros/network_library_impl_base.cc
index 3e7ad65..bd4828c 100644
--- a/chrome/browser/chromeos/cros/network_library_impl_base.cc
+++ b/chrome/browser/chromeos/cros/network_library_impl_base.cc
@@ -1151,9 +1151,11 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
   // Parse all networks. Bail out if that fails.
   NetworkOncMap added_onc_map;
   ScopedVector<Network> networks;
+  std::set<std::string> removal_ids;
   for (int i = 0; i < parser.GetNetworkConfigsSize(); i++) {
     // Parse Open Network Configuration blob into a temporary Network object.
-    Network* network = parser.ParseNetwork(i);
+    bool marked_for_removal = false;
+    Network* network = parser.ParseNetwork(i, &marked_for_removal);
     if (!network) {
       DLOG(WARNING) << "Cannot parse network in ONC file";
       if (error)
@@ -1161,7 +1163,7 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
       return false;
     }
 
-    // Disallow anything but WiFi and ethernet for device-level policy (which
+    // Disallow anything but WiFi and Ethernet for device-level policy (which
     // corresponds to shared networks). See also http://crosbug.com/28741.
     if (source == NetworkUIData::ONC_SOURCE_DEVICE_POLICY &&
         network->type() != TYPE_WIFI &&
@@ -1173,7 +1175,12 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
     }
 
     networks.push_back(network);
-    added_onc_map[network->unique_id()] = parser.GetNetworkConfig(i);
+    if (!(source == NetworkUIData::ONC_SOURCE_USER_IMPORT &&
+          marked_for_removal))
+      added_onc_map[network->unique_id()] = parser.GetNetworkConfig(i);
+
+    if (marked_for_removal)
+      removal_ids.insert(network->unique_id());
   }
 
   // Update the ONC map.
@@ -1194,6 +1201,14 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
   for (std::vector<Network*>::iterator iter(networks.begin());
        iter != networks.end(); ++iter) {
     Network* network = *iter;
+
+    // Don't configure a network that is supposed to be removed. For
+    // policy-managed networks, the "remove" functionality of ONC is ignored.
+    if (source == NetworkUIData::ONC_SOURCE_USER_IMPORT &&
+        removal_ids.find(network->unique_id()) != removal_ids.end()) {
+      continue;
+    }
+
     DictionaryValue dict;
     for (Network::PropertyMap::const_iterator props =
              network->property_map_.begin();
@@ -1210,14 +1225,14 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
     if (!profile_path.empty())
       dict.SetString(flimflam::kProfileProperty, profile_path);
 
-    // For ethernet networks, apply them to the current ethernet service.
+    // For Ethernet networks, apply them to the current Ethernet service.
     if (network->type() == TYPE_ETHERNET) {
       const EthernetNetwork* ethernet = ethernet_network();
       if (ethernet) {
         CallConfigureService(ethernet->unique_id(), &dict);
       } else {
-        DLOG(WARNING) << "Tried to import ONC with an ethernet network when "
-                      << "there is no active ethernet connection.";
+        DLOG(WARNING) << "Tried to import ONC with an Ethernet network when "
+                      << "there is no active Ethernet connection.";
       }
     } else {
       CallConfigureService(network->unique_id(), &dict);
@@ -1232,40 +1247,23 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
     // networks and clean out the ones that no longer have a definition in the
     // ONC blob. We first collect the networks and do the actual deletion later
     // because ForgetNetwork() changes the remembered network vectors.
-    std::vector<std::string> to_be_deleted;
-    for (WifiNetworkVector::iterator i(remembered_wifi_networks_.begin());
-         i != remembered_wifi_networks_.end(); ++i) {
-      WifiNetwork* network = *i;
-      if (network->ui_data().onc_source() == source &&
-          network_ids.find(network->unique_id()) == network_ids.end()) {
-        to_be_deleted.push_back(network->service_path());
-      }
-    }
-
-    for (VirtualNetworkVector::iterator i(remembered_virtual_networks_.begin());
-         i != remembered_virtual_networks_.end(); ++i) {
-      VirtualNetwork* network = *i;
-      if (network->ui_data().onc_source() == source &&
-          network_ids.find(network->unique_id()) == network_ids.end()) {
-        to_be_deleted.push_back(network->service_path());
-      }
-    }
-
-    for (std::vector<std::string>::const_iterator i(to_be_deleted.begin());
-         i != to_be_deleted.end(); ++i) {
-      ForgetNetwork(*i);
-    }
+    ForgetNetworksById(source, network_ids, false);
   } else if (source == NetworkUIData::ONC_SOURCE_USER_IMPORT) {
     // User-imported files should always contain something.
     if (parser.GetNetworkConfigsSize() == 0 &&
         parser.GetCertificatesSize() == 0) {
-      LOG(ERROR) << "Onc file missing networks and certs.";
+      LOG(ERROR) << "ONC file contains no networks or certificates.";
       if (error) {
         *error = l10n_util::GetStringUTF8(
             IDS_NETWORK_CONFIG_ERROR_NETWORK_IMPORT);
       }
       return false;
     }
+
+    if (removal_ids.empty())
+      return true;
+
+    ForgetNetworksById(source, removal_ids, true);
   }
 
   return true;
@@ -1416,6 +1414,33 @@ void NetworkLibraryImplBase::DeleteNetwork(Network* network) {
   delete network;
 }
 
+void NetworkLibraryImplBase::ForgetNetworksById(
+    NetworkUIData::ONCSource source,
+    std::set<std::string> ids,
+    bool if_found) {
+  std::vector<std::string> to_be_forgotten;
+  for (WifiNetworkVector::iterator i = remembered_wifi_networks_.begin();
+       i != remembered_wifi_networks_.end(); ++i) {
+    WifiNetwork* wifi_network = *i;
+    if (wifi_network->ui_data().onc_source() == source &&
+        if_found == (ids.find(wifi_network->unique_id()) != ids.end()))
+      to_be_forgotten.push_back(wifi_network->service_path());
+  }
+
+  for (VirtualNetworkVector::iterator i = remembered_virtual_networks_.begin();
+       i != remembered_virtual_networks_.end(); ++i) {
+    VirtualNetwork* virtual_network = *i;
+    if (virtual_network->ui_data().onc_source() == source &&
+        if_found == (ids.find(virtual_network->unique_id()) != ids.end()))
+      to_be_forgotten.push_back(virtual_network->service_path());
+  }
+
+  for (std::vector<std::string>::const_iterator i = to_be_forgotten.begin();
+       i != to_be_forgotten.end(); ++i) {
+    ForgetNetwork(*i);
+  }
+}
+
 bool NetworkLibraryImplBase::ValidateRememberedNetwork(Network* network) {
   std::pair<NetworkMap::iterator, bool> result =
       remembered_network_map_.insert(
diff --git a/chrome/browser/chromeos/cros/network_library_impl_base.h b/chrome/browser/chromeos/cros/network_library_impl_base.h
index 24c885b..bdbc543 100644
--- a/chrome/browser/chromeos/cros/network_library_impl_base.h
+++ b/chrome/browser/chromeos/cros/network_library_impl_base.h
@@ -335,6 +335,14 @@ class NetworkLibraryImplBase : public NetworkLibrary {
   void AddNetwork(Network* network);
   void DeleteNetwork(Network* network);
 
+  // Calls ForgetNetwork for remembered wifi and virtual networks based on id.
+  // When |if_found| is true, then it forgets networks that appear in |ids|.
+  // When |if_found| is false, it removes networks that do NOT appear in |ids|.
+  // |source| is the import source of the data.
+  void ForgetNetworksById(NetworkUIData::ONCSource source,
+                          std::set<std::string> ids,
+                          bool if_found);
+
   // Checks whether |network| has meanwhile been pruned by ONC policy. If so,
   // instructs flimflam to remove the network, deletes |network| and returns
   // false.
diff --git a/chrome/browser/chromeos/cros/network_library_unittest.cc b/chrome/browser/chromeos/cros/network_library_unittest.cc
index 5b36950..d97e885 100644
--- a/chrome/browser/chromeos/cros/network_library_unittest.cc
+++ b/chrome/browser/chromeos/cros/network_library_unittest.cc
@@ -355,7 +355,7 @@ TEST_F(NetworkLibraryStubTest, NetworkConnectOncWifi) {
   ASSERT_TRUE(parser.parse_error().empty());
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(2, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
   EXPECT_EQ(CLIENT_CERT_TYPE_PATTERN, network->client_cert_type());
 
@@ -386,7 +386,7 @@ TEST_F(NetworkLibraryStubTest, NetworkConnectOncVPN) {
   ASSERT_TRUE(parser.parse_error().empty());
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(2, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
   EXPECT_EQ(CLIENT_CERT_TYPE_PATTERN, network->client_cert_type());
 
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.cc b/chrome/browser/chromeos/cros/onc_network_parser.cc
index 0193142..05ef0ec 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser.cc
@@ -14,6 +14,7 @@
 #include "base/stringprintf.h"
 #include "base/values.h"
 #include "chrome/browser/chromeos/cros/certificate_pattern.h"
+#include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/cros/native_network_constants.h"
 #include "chrome/browser/chromeos/cros/native_network_parser.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
@@ -55,7 +56,6 @@ OncValueSignature network_configuration_signature[] = {
   { onc::kGUID, PROPERTY_INDEX_GUID, TYPE_STRING },
   { onc::kProxySettings, PROPERTY_INDEX_ONC_PROXY_SETTINGS, TYPE_DICTIONARY },
   { onc::kName, PROPERTY_INDEX_NAME, TYPE_STRING },
-  // TODO(crosbug.com/23604): Handle removing networks.
   { onc::kRemove, PROPERTY_INDEX_ONC_REMOVE, TYPE_BOOLEAN },
   { onc::kType, PROPERTY_INDEX_TYPE, TYPE_STRING },
   { onc::kEthernet, PROPERTY_INDEX_ONC_ETHERNET, TYPE_DICTIONARY },
@@ -459,7 +459,7 @@ const base::DictionaryValue* OncNetworkParser::GetNetworkConfig(int n) {
   return info;
 }
 
-Network* OncNetworkParser::ParseNetwork(int n) {
+Network* OncNetworkParser::ParseNetwork(int n, bool* marked_for_removal) {
   const base::DictionaryValue* info = GetNetworkConfig(n);
   if (!info)
     return NULL;
@@ -469,8 +469,12 @@ Network* OncNetworkParser::ParseNetwork(int n) {
     base::JSONWriter::WriteWithOptions(static_cast<const base::Value*>(info),
                                        base::JSONWriter::OPTIONS_PRETTY_PRINT,
                                        &network_json);
-    VLOG(2) << "Parsing network at index " << n
-            << ": " << network_json;
+    VLOG(2) << "Parsing network at index " << n << ": " << network_json;
+  }
+
+  if (marked_for_removal) {
+    if (!info->GetBoolean(onc::kRemove, marked_for_removal))
+      *marked_for_removal = false;
   }
 
   return CreateNetworkFromInfo(std::string(), *info);
@@ -731,17 +735,15 @@ bool OncNetworkParser::ParseNetworkConfigurationValue(
     const base::Value& value,
     Network* network) {
   switch (index) {
-    case PROPERTY_INDEX_ONC_ETHERNET: {
+    case PROPERTY_INDEX_ONC_ETHERNET:
       return parser->ParseNestedObject(network, "Ethernet", value,
           ethernet_signature, OncEthernetNetworkParser::ParseEthernetValue);
-    }
-    case PROPERTY_INDEX_ONC_WIFI: {
+    case PROPERTY_INDEX_ONC_WIFI:
       return parser->ParseNestedObject(network,
                                        onc::kWiFi,
                                        value,
                                        wifi_signature,
                                        OncWifiNetworkParser::ParseWifiValue);
-    }
     case PROPERTY_INDEX_ONC_VPN: {
       if (!CheckNetworkType(network, TYPE_VPN, onc::kVPN))
         return false;
@@ -765,12 +767,11 @@ bool OncNetworkParser::ParseNetworkConfigurationValue(
                                        vpn_signature,
                                        OncVirtualNetworkParser::ParseVPNValue);
     }
-    case PROPERTY_INDEX_ONC_PROXY_SETTINGS: {
-       return ProcessProxySettings(parser, value, network);
-    }
+    case PROPERTY_INDEX_ONC_PROXY_SETTINGS:
+      return ProcessProxySettings(parser, value, network);
     case PROPERTY_INDEX_ONC_REMOVE:
-      VLOG(1) << network->name() << ": Remove field not yet implemented";
-      return false;
+      // Removal is handled in ParseNetwork, and so is ignored here.
+      return true;
     case PROPERTY_INDEX_TYPE: {
       // Update property with native value for type.
       std::string str =
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.h b/chrome/browser/chromeos/cros/onc_network_parser.h
index 2414cf8..c6d562c 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.h
+++ b/chrome/browser/chromeos/cros/onc_network_parser.h
@@ -70,7 +70,9 @@ class OncNetworkParser : public NetworkParser {
 
   // Call to create the network by parsing network config in the nth position.
   // (0-based). CHECKs if |n| is out of range and returns NULL on parse errors.
-  Network* ParseNetwork(int n);
+  // |removed| is set to true if the network should be removed.  |removed| may
+  // be NULL.
+  Network* ParseNetwork(int n, bool* marked_for_removal);
 
   // Returns the number of certificates in the "Certificates" list.
   int GetCertificatesSize() const;
diff --git a/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc b/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
index dccdb22..35cfad5 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
@@ -186,7 +186,7 @@ void OncNetworkParserTest::TestProxySettings(const std::string test_blob,
                                              net::ProxyConfig* net_config) {
   // Parse Network Configuration including ProxySettings dictionary.
   OncNetworkParser parser(test_blob, "", NetworkUIData::ONC_SOURCE_USER_IMPORT);
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
   EXPECT_FALSE(network->proxy_config().empty());
 
@@ -232,7 +232,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifi) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_WIFI, network->type());
@@ -252,7 +252,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkEthernet) {
   OncNetworkParser parser(test_blob, "", NetworkUIData::ONC_SOURCE_USER_IMPORT);
 
   EXPECT_GE(parser.GetNetworkConfigsSize(), 1);
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_ETHERNET, network->type());
@@ -269,7 +269,7 @@ TEST_F(OncNetworkParserTest, TestLoadEncryptedOnc) {
   ASSERT_TRUE(parser.parse_error().empty());
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_WIFI, network->type());
@@ -287,7 +287,7 @@ TEST_F(OncNetworkParserTest, TestLoadWifiCertificatePattern) {
   ASSERT_TRUE(parser.parse_error().empty());
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(2, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_WIFI, network->type());
@@ -315,7 +315,7 @@ TEST_F(OncNetworkParserTest, TestLoadVPNCertificatePattern) {
   ASSERT_TRUE(parser.parse_error().empty());
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(2, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_VPN, network->type());
@@ -339,7 +339,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifiEAP1) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_WIFI, network->type());
@@ -363,7 +363,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifiEAP2) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_WIFI, network->type());
@@ -386,7 +386,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkUnknownFields) {
   GetTestData("network-unknown-fields.onc", &test_blob);
 
   OncNetworkParser parser(test_blob, "", NetworkUIData::ONC_SOURCE_USER_IMPORT);
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get());
 
   EXPECT_EQ(chromeos::TYPE_WIFI, network->type());
@@ -404,7 +404,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkOpenVPN) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(1, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get() != NULL);
 
   EXPECT_EQ(chromeos::TYPE_VPN, network->type());
@@ -463,7 +463,7 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkL2TPIPsec) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network != NULL);
 
   EXPECT_EQ(chromeos::TYPE_VPN, network->type());
@@ -807,7 +807,7 @@ TEST_F(OncNetworkParserTest, TestNetworkAndCertificate) {
   EXPECT_TRUE(parser.ParseCertificate(0));
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
-  scoped_ptr<Network> network(parser.ParseNetwork(0));
+  scoped_ptr<Network> network(parser.ParseNetwork(0, NULL));
   ASSERT_TRUE(network.get() != NULL);
   EXPECT_EQ(chromeos::TYPE_VPN, network->type());
   VirtualNetwork* vpn = static_cast<VirtualNetwork*>(network.get());
@@ -940,4 +940,18 @@ TEST(OncNetworkParserUserExpansionTest, GetUserExpandedValue) {
                 login_email_pattern, source));
 }
 
+TEST_F(OncNetworkParserTest, TestRemoveNetworkWifi) {
+  std::string test_blob;
+  GetTestData("network-wifi-remove.onc", &test_blob);
+  OncNetworkParser parser(test_blob, "",
+                                 NetworkUIData::ONC_SOURCE_USER_IMPORT);
+  EXPECT_EQ(1, parser.GetNetworkConfigsSize());
+  EXPECT_EQ(0, parser.GetCertificatesSize());
+  bool marked_for_removal = false;
+  scoped_ptr<Network> network(parser.ParseNetwork(0, &marked_for_removal));
+
+  EXPECT_TRUE(marked_for_removal);
+  EXPECT_EQ("{485d6076-dd44-6b6d-69787465725f5045}", network->unique_id());
+}
+
 }  // namespace chromeos