Provide a GUID for existing network configurations

Prior to using mojo for network configuration, when creating a new
configuration for a visible network, the GUID of the visible network
would be provided so that it was preserved.

Some UI depends on this (e.g. OOBE flow) and generally it is less
confusing for logging, debugging, etc.

This restores that behavior by providing an optional guid property
in the mojo ConfigProperties and translating it to ONC.

Bug: 1009406
Change-Id: Ia6fc22afa16b514a767dc97274bc216f28112ab8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1951558Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722577}

chromeos/services/network_config/cros_network_config.cc

@@ -1459,7 +1459,8 @@ base::Value GetEAPProperties(const mojom::EAPConfigProperties& eap) {
 }
 
 std::unique_ptr<base::DictionaryValue> GetOncFromConfigProperties(
-    const mojom::ConfigProperties* properties) {
+    const mojom::ConfigProperties* properties,
+    base::Optional<std::string> guid) {
   auto onc = std::make_unique<base::DictionaryValue>();
 
   // Process |properties->network_type| and set |type|. Configurations have only
@@ -1467,6 +1468,15 @@ std::unique_ptr<base::DictionaryValue> GetOncFromConfigProperties(
   mojom::NetworkType type = mojom::NetworkType::kAll;  // Invalid type
   base::Value type_dict(base::Value::Type::DICTIONARY);
 
+  if (properties->guid && !properties->guid->empty()) {
+    if (guid && *guid != *properties->guid) {
+      NET_LOG(ERROR) << "GUID does not match: " << *guid
+                     << " != " << *properties->guid;
+      return nullptr;
+    }
+    SetString(::onc::network_config::kGUID, *properties->guid, onc.get());
+  }
+
   if (properties->type_config->is_cellular()) {
     type = mojom::NetworkType::kCellular;
     const mojom::CellularConfigProperties& cellular =
@@ -1969,7 +1979,7 @@ void CrosNetworkConfig::SetProperties(const std::string& guid,
   }
 
   std::unique_ptr<base::DictionaryValue> onc =
-      GetOncFromConfigProperties(properties.get());
+      GetOncFromConfigProperties(properties.get(), guid);
   if (!onc) {
     NET_LOG(ERROR) << "Bad ONC Configuration for " << guid;
     std::move(callback).Run(false, kErrorInvalidONCConfiguration);
@@ -2052,7 +2062,7 @@ void CrosNetworkConfig::ConfigureNetwork(mojom::ConfigPropertiesPtr properties,
   }
 
   std::unique_ptr<base::DictionaryValue> onc =
-      GetOncFromConfigProperties(properties.get());
+      GetOncFromConfigProperties(properties.get(), /*guid=*/base::nullopt);
   if (!onc) {
     std::move(callback).Run(/*guid=*/base::nullopt,
                             kErrorInvalidONCConfiguration);

chromeos/services/network_config/cros_network_config_unittest.cc

@@ -756,12 +756,14 @@ TEST_F(CrosNetworkConfigTest, SetProperties) {
   ASSERT_TRUE(properties->priority);
   EXPECT_EQ(1, properties->priority->active_value);
 
-  // Set auto connect only. Priority should remain unchanged.
+  // Set auto connect only. Priority should remain unchanged. Also provide a
+  // matching |guid|.
   config = mojom::ConfigProperties::New();
   config->type_config = mojom::NetworkTypeConfigProperties::NewWifi(
       mojom::WiFiConfigProperties::New());
   config->auto_connect = mojom::AutoConnectConfig::New();
   config->auto_connect->value = true;
+  config->guid = kGUID;
   success = SetProperties(kGUID, std::move(config));
   ASSERT_TRUE(success);
   properties = GetManagedProperties(kGUID);
@@ -774,6 +776,16 @@ TEST_F(CrosNetworkConfigTest, SetProperties) {
   EXPECT_TRUE(wifi->auto_connect->active_value);
   ASSERT_TRUE(properties->priority);
   EXPECT_EQ(1, properties->priority->active_value);
+
+  // Set auto connect with a mismatched guid; call should fail.
+  config = mojom::ConfigProperties::New();
+  config->type_config = mojom::NetworkTypeConfigProperties::NewWifi(
+      mojom::WiFiConfigProperties::New());
+  config->auto_connect = mojom::AutoConnectConfig::New();
+  config->auto_connect->value = false;
+  config->guid = "Mismatched guid";
+  success = SetProperties(kGUID, std::move(config));
+  EXPECT_FALSE(success);
 }
 
 TEST_F(CrosNetworkConfigTest, ConfigureNetwork) {
@@ -782,6 +794,7 @@ TEST_F(CrosNetworkConfigTest, ConfigureNetwork) {
   const std::string ssid = "new_wifi_ssid";
   // Configure a new wifi network.
   auto config = mojom::ConfigProperties::New();
+  config->name = ssid;
   auto wifi = mojom::WiFiConfigProperties::New();
   wifi->ssid = ssid;
   config->type_config =
@@ -800,6 +813,24 @@ TEST_F(CrosNetworkConfigTest, ConfigureNetwork) {
   EXPECT_EQ(ssid, network->type_state->get_wifi()->ssid);
 }
 
+TEST_F(CrosNetworkConfigTest, ConfigureNetworkExistingGuid) {
+  // Note: shared = false requires a UserManager instance.
+  bool shared = true;
+  const std::string guid = "new_wifi_guid";
+  const std::string ssid = "new_wifi_ssid";
+  // Configure a new wifi network with an existing guid.
+  auto config = mojom::ConfigProperties::New();
+  config->guid = guid;
+  config->name = ssid;
+  auto wifi = mojom::WiFiConfigProperties::New();
+  wifi->ssid = ssid;
+  config->type_config =
+      mojom::NetworkTypeConfigProperties::NewWifi(std::move(wifi));
+  std::string config_guid = ConfigureNetwork(std::move(config), shared);
+  // The new guid should be the same as the existing guid.
+  EXPECT_EQ(config_guid, guid);
+}
+
 TEST_F(CrosNetworkConfigTest, ForgetNetwork) {
   // Use a non visible configured network.
   const std::string kGUID = "wifi3_guid";

chromeos/services/network_config/public/mojom/cros_network_config.mojom

@@ -775,6 +775,9 @@ union NetworkTypeConfigProperties {
 
 struct ConfigProperties {
   AutoConnectConfig? auto_connect;
+  // An optional guid may be provided when configuring a visible network to
+  // preserve the existing guid.
+  string? guid;
   string? ip_address_config_type;
   // Name is only required for new configurations.
   string? name;
@@ -885,18 +888,21 @@ interface CrosNetworkConfig {
   // available returns null.
   GetManagedProperties(string guid) => (ManagedProperties? result);
 
-  // Sets properties for the network matching |guid|. If |success| is false,
-  // |error_message| provides a string identifying the error. The string may
-  // be a Shill error, or an error token provided by the NetworkHandler layer.
-  // JS localization for a common subset of these is provided by
-  // network_element::AddErrorLocalizedStrings. TODO(1004434): Consider
-  // enumerating these.
+  // Sets properties for the network matching |guid|. |properties.guid| must be
+  // null or match |guid|, otherwise it will fail with an invalid input error.
+  // If |success| is false, |error_message| provides a string identifying the
+  // error. The string may be a Shill error, or an error token provided by the
+  // NetworkHandler layer. JS localization for a common subset of these is
+  // provided by network_element::AddErrorLocalizedStrings.
+  // TODO(1004434): Consider enumerating these.
   SetProperties(string guid, ConfigProperties properties) =>
     (bool success, string error_message);
 
-  // Configures a new network with |properties|. Returns the guid of the new
-  // network configuration on success. A null guid indicates that the properties
-  // were invalid or incomplete. See SetProperties for notes on errorMessage.
+  // Configures a new network with |properties|. If |properties.guid| is
+  // provided, an existing configuration must exist and match the provided
+  // |properties|. Returns the guid of the new network configuration on success.
+  // A null guid indicates that the properties were invalid or incomplete. See
+  // SetProperties for notes on errorMessage.
   ConfigureNetwork(ConfigProperties properties, bool shared) =>
     (string? guid, string error_message);