Use hex_ssid+security_type as the unique identifier for synced networks.

TBR=treib@chromium.org

Bug: 966270
Change-Id: I2a210207d19471f56295f2a7e967fdf8ac01214f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1819496
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702495}

chromeos/components/sync_wifi/BUILD.gn

@@ -4,6 +4,8 @@
 
 static_library("sync_wifi") {
   sources = [
+    "network_identifier.cc",
+    "network_identifier.h",
     "pending_network_configuration_tracker.h",
     "pending_network_configuration_tracker_impl.cc",
     "pending_network_configuration_tracker_impl.h",
@@ -17,6 +19,7 @@ static_library("sync_wifi") {
   ]
   deps = [
     "//base",
+    "//chromeos/network:network",
     "//components/keyed_service/core",
     "//components/prefs:prefs",
     "//components/sync",
@@ -27,6 +30,7 @@ static_library("sync_wifi") {
 source_set("unit_tests") {
   testonly = true
   sources = [
+    "network_identifier_unittest.cc",
     "pending_network_configuration_tracker_impl_unittest.cc",
     "test_specifics_generator.h",
     "wifi_configuration_bridge_unittest.cc",
@@ -34,6 +38,7 @@ source_set("unit_tests") {
   deps = [
     ":sync_wifi",
     "//base/test:test_support",
+    "//chromeos/network:network",
     "//components/sync:test_support",
     "//components/sync_preferences:test_support",
     "//testing/gtest",

chromeos/components/sync_wifi/DEPS

 include_rules = [
+  "+chromeos/network",
   "+components/keyed_service/core",
   "+components/prefs",
   "+components/version_info",

chromeos/components/sync_wifi/network_identifier.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/components/sync_wifi/network_identifier.h"
+
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "chromeos/network/network_state.h"
+#include "components/sync/protocol/model_type_state.pb.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+
+namespace {
+
+const char kDelimeter[] = "_";
+
+std::string GetSecurityType(
+    sync_pb::WifiConfigurationSpecificsData_SecurityType security_type) {
+  switch (security_type) {
+    case sync_pb::WifiConfigurationSpecificsData::SECURITY_TYPE_PSK:
+      return shill::kSecurityPsk;
+    case sync_pb::WifiConfigurationSpecificsData::SECURITY_TYPE_WEP:
+      return shill::kSecurityWep;
+    default:
+      NOTREACHED();
+      return "";
+  }
+}
+
+}  // namespace
+
+namespace sync_wifi {
+
+// static
+NetworkIdentifier NetworkIdentifier::FromProto(
+    const sync_pb::WifiConfigurationSpecificsData& specifics) {
+  return NetworkIdentifier(specifics.hex_ssid(),
+                           GetSecurityType(specifics.security_type()));
+}
+
+// static
+NetworkIdentifier NetworkIdentifier::FromNetwork(
+    const chromeos::NetworkState& network) {
+  return NetworkIdentifier(network.GetHexSsid(), network.security_class());
+}
+
+// static
+NetworkIdentifier NetworkIdentifier::DeserializeFromString(
+    const std::string& serialized_string) {
+  std::vector<std::string> pieces =
+      base::SplitString(serialized_string, kDelimeter, base::KEEP_WHITESPACE,
+                        base::SPLIT_WANT_NONEMPTY);
+  DCHECK(pieces.size() == 2);
+  return NetworkIdentifier(pieces[0], pieces[1]);
+}
+
+NetworkIdentifier::NetworkIdentifier(const std::string& hex_ssid,
+                                     const std::string& security_type)
+    : security_type_(security_type) {
+  hex_ssid_ =
+      base::StartsWith(hex_ssid, "0x", base::CompareCase::INSENSITIVE_ASCII)
+          ? base::ToUpperASCII(hex_ssid.substr(2))
+          : base::ToUpperASCII(hex_ssid);
+}
+
+NetworkIdentifier::~NetworkIdentifier() = default;
+
+std::string NetworkIdentifier::SerializeToString() const {
+  return base::StringPrintf("%s%s%s", hex_ssid_.c_str(), kDelimeter,
+                            security_type_.c_str());
+}
+
+bool NetworkIdentifier::operator==(const NetworkIdentifier& o) const {
+  return hex_ssid_ == o.hex_ssid_ && security_type_ == o.security_type_;
+}
+
+}  // namespace sync_wifi

chromeos/components/sync_wifi/network_identifier.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_COMPONENTS_SYNC_WIFI_NETWORK_IDENTIFIER_H_
+#define CHROMEOS_COMPONENTS_SYNC_WIFI_NETWORK_IDENTIFIER_H_
+
+#include <string>
+
+namespace chromeos {
+class NetworkState;
+}
+
+namespace sync_pb {
+class WifiConfigurationSpecificsData;
+}
+
+namespace sync_wifi {
+
+// A unique identifier for synced networks which contains the properties
+// necessary to differentiate a synced network from another with the same name.
+class NetworkIdentifier {
+ public:
+  static NetworkIdentifier FromProto(
+      const sync_pb::WifiConfigurationSpecificsData& specifics);
+  static NetworkIdentifier FromNetwork(const chromeos::NetworkState& network);
+  // |serialized_string| is in the format of hex_ssid and security_type
+  // concatenated with an underscore.  security_type is the shill constant
+  // returned from NetworkState::security_class(). For example, it would be
+  // "2F_psk" if the hex_ssid is 2F and the security_type is psk.
+  static NetworkIdentifier DeserializeFromString(
+      const std::string& serialized_string);
+
+  // |hex_ssid| hex encoded representation of an ssid.  For example, ssid
+  // "network" could be provided as "6E6574776F726B" or "0x6e6574776f726b".
+  // |security_type| is the shill constant that would be returned from
+  // NetworkState::security_class().
+  NetworkIdentifier(const std::string& hex_ssid,
+                    const std::string& security_type);
+  virtual ~NetworkIdentifier();
+
+  bool operator==(const NetworkIdentifier& o) const;
+
+  std::string SerializeToString() const;
+
+  // This will always be returned in a format with upper case letters and no
+  // 0x prefix.
+  const std::string& hex_ssid() const { return hex_ssid_; }
+  const std::string& security_type() const { return security_type_; }
+
+ private:
+  std::string hex_ssid_;
+  std::string security_type_;
+};
+
+}  // namespace sync_wifi
+
+#endif  // CHROMEOS_COMPONENTS_SYNC_WIFI_NETWORK_IDENTIFIER_H_

chromeos/components/sync_wifi/network_identifier_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <map>
+#include <set>
+#include <utility>
+
+#include "base/strings/string_number_conversions.h"
+#include "chromeos/components/sync_wifi/network_identifier.h"
+#include "chromeos/components/sync_wifi/test_specifics_generator.h"
+#include "chromeos/network/network_state.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+
+namespace sync_wifi {
+
+namespace {
+const char kHexSsid[] = "0123456789ABCDEF";
+}  // namespace
+
+class NetworkIdentifierTest : public testing::Test {
+ protected:
+  NetworkIdentifierTest() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(NetworkIdentifierTest);
+};
+
+TEST_F(NetworkIdentifierTest, FromProto) {
+  NetworkIdentifier id =
+      NetworkIdentifier::FromProto(CreateSpecifics(kHexSsid));
+  EXPECT_EQ(kHexSsid, id.hex_ssid());
+  EXPECT_EQ(shill::kSecurityPsk, id.security_type());
+}
+
+TEST_F(NetworkIdentifierTest, FromNetwork) {
+  chromeos::NetworkState network("dummy_path");
+  network.PropertyChanged(shill::kWifiHexSsid, base::Value(kHexSsid));
+  network.PropertyChanged(shill::kSecurityClassProperty,
+                          base::Value(shill::kSecurityPsk));
+  NetworkIdentifier id = NetworkIdentifier::FromNetwork(network);
+  EXPECT_EQ(kHexSsid, id.hex_ssid());
+  EXPECT_EQ(shill::kSecurityPsk, id.security_type());
+}
+
+TEST_F(NetworkIdentifierTest, FromString) {
+  std::string string_id("0123456789ABCDEF_psk");
+  NetworkIdentifier id = NetworkIdentifier::DeserializeFromString(string_id);
+  EXPECT_EQ(kHexSsid, id.hex_ssid());
+  EXPECT_EQ(shill::kSecurityPsk, id.security_type());
+}
+
+TEST_F(NetworkIdentifierTest, DifferentHexFormats) {
+  NetworkIdentifier id("0x2f", shill::kSecurityPsk);
+  EXPECT_EQ("2F", id.hex_ssid());
+
+  id = NetworkIdentifier("0X2F", shill::kSecurityPsk);
+  EXPECT_EQ("2F", id.hex_ssid());
+
+  id = NetworkIdentifier("2f", shill::kSecurityPsk);
+  EXPECT_EQ("2F", id.hex_ssid());
+}
+
+}  // namespace sync_wifi

chromeos/components/sync_wifi/pending_network_configuration_tracker.h

@@ -14,6 +14,8 @@
 
 namespace sync_wifi {
 
+class NetworkIdentifier;
+
 // Tracks updates to the local network stack while they are pending, including
 // how many attempts have been executed.  Updates are stored persistently.
 class PendingNetworkConfigurationTracker {
@@ -22,18 +24,18 @@ class PendingNetworkConfigurationTracker {
   virtual ~PendingNetworkConfigurationTracker() = default;
 
   // Adds an update to the list of in flight changes.  |change_uuid| is a
-  // unique identifier for each update, |ssid| is the identifier for the network
+  // unique identifier for each update, |id| is the identifier for the network
   // which is getting updated, and |specifics| should be nullopt if the network
   // is being deleted.
   virtual void TrackPendingUpdate(
       const std::string& change_guid,
-      const std::string& ssid,
+      const NetworkIdentifier& id,
       const base::Optional<sync_pb::WifiConfigurationSpecificsData>&
           specifics) = 0;
 
   // Removes the given change from the list.
   virtual void MarkComplete(const std::string& change_guid,
-                            const std::string& ssid) = 0;
+                            const NetworkIdentifier& id) = 0;
 
   // Returns all pending updates.
   virtual std::vector<PendingNetworkConfigurationUpdate>
@@ -42,11 +44,11 @@ class PendingNetworkConfigurationTracker {
   // Returns the requested pending update, if it exists.
   virtual base::Optional<PendingNetworkConfigurationUpdate> GetPendingUpdate(
       const std::string& change_guid,
-      const std::string& ssid) = 0;
+      const NetworkIdentifier& id) = 0;
 
   // Increments the number of completed attempts for the given update.
   virtual void IncrementCompletedAttempts(const std::string& change_guid,
-                                          const std::string& ssid) = 0;
+                                          const NetworkIdentifier& id) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(PendingNetworkConfigurationTracker);

chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.cc

@@ -6,6 +6,7 @@
 
 #include "base/optional.h"
 #include "base/strings/stringprintf.h"
+#include "chromeos/components/sync_wifi/network_identifier.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 
@@ -17,13 +18,15 @@ const char kChangeGuidKey[] = "ChangeGuid";
 const char kCompletedAttemptsKey[] = "CompletedAttempts";
 const char kSpecificsKey[] = "Specifics";
 
-std::string GeneratePath(const std::string& ssid, const std::string& subkey) {
-  return base::StringPrintf("%s.%s", ssid.c_str(), subkey.c_str());
+std::string GeneratePath(const sync_wifi::NetworkIdentifier& id,
+                         const std::string& subkey) {
+  return base::StringPrintf("%s.%s", id.SerializeToString().c_str(),
+                            subkey.c_str());
 }
 
 sync_wifi::PendingNetworkConfigurationUpdate ConvertToPendingUpdate(
     base::Value* dict,
-    const std::string ssid) {
+    const sync_wifi::NetworkIdentifier& id) {
   std::string* change_guid = dict->FindStringKey(kChangeGuidKey);
   base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics;
   std::string* specifics_string = dict->FindStringKey(kSpecificsKey);
@@ -39,7 +42,7 @@ sync_wifi::PendingNetworkConfigurationUpdate ConvertToPendingUpdate(
   DCHECK(completed_attempts);
 
   return sync_wifi::PendingNetworkConfigurationUpdate(
-      ssid, *change_guid, specifics, completed_attempts.value());
+      id, *change_guid, specifics, completed_attempts.value());
 }
 
 }  // namespace
@@ -63,7 +66,7 @@ PendingNetworkConfigurationTrackerImpl::
 
 void PendingNetworkConfigurationTrackerImpl::TrackPendingUpdate(
     const std::string& change_guid,
-    const std::string& ssid,
+    const NetworkIdentifier& id,
     const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics) {
   std::string serialized_specifics;
   if (!specifics)
@@ -71,27 +74,27 @@ void PendingNetworkConfigurationTrackerImpl::TrackPendingUpdate(
   else
     CHECK(specifics->SerializeToString(&serialized_specifics));
 
-  dict_.SetPath(GeneratePath(ssid, kChangeGuidKey), base::Value(change_guid));
-  dict_.SetPath(GeneratePath(ssid, kSpecificsKey),
+  dict_.SetPath(GeneratePath(id, kChangeGuidKey), base::Value(change_guid));
+  dict_.SetPath(GeneratePath(id, kSpecificsKey),
                 base::Value(serialized_specifics));
-  dict_.SetPath(GeneratePath(ssid, kCompletedAttemptsKey), base::Value(0));
+  dict_.SetPath(GeneratePath(id, kCompletedAttemptsKey), base::Value(0));
   pref_service_->Set(kPendingNetworkConfigurationsPref, dict_);
 }
 
 void PendingNetworkConfigurationTrackerImpl::MarkComplete(
     const std::string& change_guid,
-    const std::string& ssid) {
-  if (!GetPendingUpdate(change_guid, ssid))
+    const NetworkIdentifier& id) {
+  if (!GetPendingUpdate(change_guid, id))
     return;
 
-  dict_.RemovePath(ssid);
+  dict_.RemovePath(id.SerializeToString());
   pref_service_->Set(kPendingNetworkConfigurationsPref, dict_);
 }
 
 void PendingNetworkConfigurationTrackerImpl::IncrementCompletedAttempts(
     const std::string& change_guid,
-    const std::string& ssid) {
-  std::string path = GeneratePath(ssid, kCompletedAttemptsKey);
+    const NetworkIdentifier& id) {
+  std::string path = GeneratePath(id, kCompletedAttemptsKey);
   base::Optional<int> completed_attempts = dict_.FindIntPath(path);
   dict_.SetIntPath(path, completed_attempts.value() + 1);
 }
@@ -100,21 +103,22 @@ std::vector<PendingNetworkConfigurationUpdate>
 PendingNetworkConfigurationTrackerImpl::GetPendingUpdates() {
   std::vector<PendingNetworkConfigurationUpdate> list;
   for (const auto& entry : dict_.DictItems()) {
-    list.push_back(
-        ConvertToPendingUpdate(/*dict=*/&entry.second, /*ssid=*/entry.first));
+    list.push_back(ConvertToPendingUpdate(
+        /*dict=*/&entry.second,
+        NetworkIdentifier::DeserializeFromString(entry.first)));
   }
   return list;
 }
 base::Optional<PendingNetworkConfigurationUpdate>
 PendingNetworkConfigurationTrackerImpl::GetPendingUpdate(
     const std::string& change_guid,
-    const std::string& ssid) {
+    const NetworkIdentifier& id) {
   std::string* found_id =
-      dict_.FindStringPath(GeneratePath(ssid, kChangeGuidKey));
+      dict_.FindStringPath(GeneratePath(id, kChangeGuidKey));
   if (!found_id || *found_id != change_guid)
     return base::nullopt;
 
-  return ConvertToPendingUpdate(dict_.FindPath(ssid), ssid);
+  return ConvertToPendingUpdate(dict_.FindPath(id.SerializeToString()), id);
 }
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/pending_network_configuration_tracker_impl.h

@@ -28,17 +28,17 @@ class PendingNetworkConfigurationTrackerImpl
   // sync_wifi::PendingNetworkConfigurationTracker::
   void TrackPendingUpdate(
       const std::string& change_guid,
-      const std::string& ssid,
+      const NetworkIdentifier& id,
       const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics)
       override;
   void MarkComplete(const std::string& change_guid,
-                    const std::string& ssid) override;
+                    const NetworkIdentifier& id) override;
   void IncrementCompletedAttempts(const std::string& change_guid,
-                                  const std::string& ssid) override;
+                                  const NetworkIdentifier& id) override;
   std::vector<PendingNetworkConfigurationUpdate> GetPendingUpdates() override;
   base::Optional<PendingNetworkConfigurationUpdate> GetPendingUpdate(
       const std::string& change_guid,
-      const std::string& ssid) override;
+      const NetworkIdentifier& id) override;
 
  private:
   PrefService* pref_service_;

chromeos/components/sync_wifi/pending_network_configuration_tracker_impl_unittest.cc

@@ -12,6 +12,7 @@
 #include "chromeos/components/sync_wifi/test_specifics_generator.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
 
 namespace {
 
@@ -49,9 +50,9 @@ class PendingNetworkConfigurationTrackerImplTest : public testing::Test {
         kPendingNetworkConfigurationsPref);
   }
 
-  bool DoesPrefContainPendingUpdate(const std::string& ssid,
+  bool DoesPrefContainPendingUpdate(const NetworkIdentifier& id,
                                     const std::string& update_guid) const {
-    const base::Value* dict = GetPref()->FindPath(ssid);
+    const base::Value* dict = GetPref()->FindPath(id.SerializeToString());
     if (!dict)
       return false;
 
@@ -61,14 +62,14 @@ class PendingNetworkConfigurationTrackerImplTest : public testing::Test {
 
   void AssertTrackerHasMatchingUpdate(
       const std::string& update_guid,
-      const std::string& ssid,
+      const NetworkIdentifier& id,
       int completed_attempts = 0,
       const base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics =
           base::nullopt) {
     base::Optional<PendingNetworkConfigurationUpdate> update =
-        tracker()->GetPendingUpdate(update_guid, ssid);
+        tracker()->GetPendingUpdate(update_guid, id);
     ASSERT_TRUE(update);
-    ASSERT_EQ(ssid, update->ssid());
+    ASSERT_EQ(id, update->id());
     ASSERT_EQ(completed_attempts, update->completed_attempts());
     std::string serialized_specifics_wants;
     std::string serialized_specifics_has;
@@ -87,103 +88,107 @@ class PendingNetworkConfigurationTrackerImplTest : public testing::Test {
 };
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestMarkComplete) {
-  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
+  NetworkIdentifier id(kFredSsid, shill::kSecurityPsk);
+  tracker()->TrackPendingUpdate(kChangeGuid1, id,
                                 /*specifics=*/base::nullopt);
-  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, id);
   EXPECT_EQ(1u, GetPref()->DictSize());
-  EXPECT_TRUE(DoesPrefContainPendingUpdate(kFredSsid, kChangeGuid1));
-  tracker()->MarkComplete(kChangeGuid1, kFredSsid);
-  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid1, kFredSsid));
+  EXPECT_TRUE(DoesPrefContainPendingUpdate(id, kChangeGuid1));
+  tracker()->MarkComplete(kChangeGuid1, id);
+  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid1, id));
   EXPECT_EQ(0u, GetPref()->DictSize());
 }
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestTwoChangesSameNetwork) {
-  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
+  NetworkIdentifier id(kFredSsid, shill::kSecurityPsk);
+  tracker()->TrackPendingUpdate(kChangeGuid1, id,
                                 /*specifics=*/base::nullopt);
-  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
-  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid,
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, id);
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, id,
                                  /*completed_attempts=*/1);
   EXPECT_EQ(1u, GetPref()->DictSize());
-  EXPECT_EQ(1, tracker()
-                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
-                   ->completed_attempts());
+  EXPECT_EQ(
+      1, tracker()->GetPendingUpdate(kChangeGuid1, id)->completed_attempts());
 
-  tracker()->TrackPendingUpdate(kChangeGuid2, kFredSsid,
+  tracker()->TrackPendingUpdate(kChangeGuid2, id,
                                 /*specifics=*/base::nullopt);
-  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid1, kFredSsid));
-  AssertTrackerHasMatchingUpdate(kChangeGuid2, kFredSsid);
-  EXPECT_EQ(0, tracker()
-                   ->GetPendingUpdate(kChangeGuid2, kFredSsid)
-                   ->completed_attempts());
+  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid1, id));
+  AssertTrackerHasMatchingUpdate(kChangeGuid2, id);
+  EXPECT_EQ(
+      0, tracker()->GetPendingUpdate(kChangeGuid2, id)->completed_attempts());
   EXPECT_EQ(1u, GetPref()->DictSize());
 }
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest,
        TestTwoChangesDifferentNetworks) {
-  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
+  NetworkIdentifier fred_id(kFredSsid, shill::kSecurityPsk);
+  NetworkIdentifier mango_id(kMangoSsid, shill::kSecurityPsk);
+  tracker()->TrackPendingUpdate(kChangeGuid1, fred_id,
                                 /*specifics=*/base::nullopt);
-  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
-  EXPECT_TRUE(DoesPrefContainPendingUpdate(kFredSsid, kChangeGuid1));
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, fred_id);
+  EXPECT_TRUE(DoesPrefContainPendingUpdate(fred_id, kChangeGuid1));
   EXPECT_EQ(1u, GetPref()->DictSize());
-  tracker()->TrackPendingUpdate(kChangeGuid2, kMangoSsid,
+  tracker()->TrackPendingUpdate(kChangeGuid2, mango_id,
                                 /*specifics=*/base::nullopt);
-  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
-  AssertTrackerHasMatchingUpdate(kChangeGuid2, kMangoSsid);
-  EXPECT_TRUE(DoesPrefContainPendingUpdate(kFredSsid, kChangeGuid1));
-  EXPECT_TRUE(DoesPrefContainPendingUpdate(kMangoSsid, kChangeGuid2));
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, fred_id);
+  AssertTrackerHasMatchingUpdate(kChangeGuid2, mango_id);
+  EXPECT_TRUE(DoesPrefContainPendingUpdate(fred_id, kChangeGuid1));
+  EXPECT_TRUE(DoesPrefContainPendingUpdate(mango_id, kChangeGuid2));
   EXPECT_EQ(2u, GetPref()->DictSize());
 }
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestGetPendingUpdates) {
-  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
+  NetworkIdentifier fred_id(kFredSsid, shill::kSecurityPsk);
+  NetworkIdentifier mango_id(kMangoSsid, shill::kSecurityPsk);
+  tracker()->TrackPendingUpdate(kChangeGuid1, fred_id,
                                 /*specifics=*/base::nullopt);
-  tracker()->TrackPendingUpdate(kChangeGuid2, kMangoSsid,
+  tracker()->TrackPendingUpdate(kChangeGuid2, mango_id,
                                 /*specifics=*/base::nullopt);
   std::vector<PendingNetworkConfigurationUpdate> list =
       tracker()->GetPendingUpdates();
   EXPECT_EQ(2u, list.size());
   EXPECT_EQ(kChangeGuid1, list[0].change_guid());
-  EXPECT_EQ(kFredSsid, list[0].ssid());
+  EXPECT_EQ(fred_id, list[0].id());
   EXPECT_EQ(kChangeGuid2, list[1].change_guid());
-  EXPECT_EQ(kMangoSsid, list[1].ssid());
+  EXPECT_EQ(mango_id, list[1].id());
 
-  tracker()->MarkComplete(kChangeGuid1, kFredSsid);
+  tracker()->MarkComplete(kChangeGuid1, fred_id);
   list = tracker()->GetPendingUpdates();
   EXPECT_EQ(1u, list.size());
   EXPECT_EQ(kChangeGuid2, list[0].change_guid());
-  EXPECT_EQ(kMangoSsid, list[0].ssid());
+  EXPECT_EQ(mango_id, list[0].id());
 }
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestGetPendingUpdate) {
   sync_pb::WifiConfigurationSpecificsData specifics =
       CreateSpecifics(kFredSsid);
-  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid, specifics);
+  NetworkIdentifier fred_id(kFredSsid, shill::kSecurityPsk);
+  NetworkIdentifier mango_id(kMangoSsid, shill::kSecurityPsk);
+  tracker()->TrackPendingUpdate(kChangeGuid1, fred_id, specifics);
 
-  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid,
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, fred_id,
                                  /*completed_attempts=*/0, specifics);
 
-  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid2, kMangoSsid));
+  EXPECT_FALSE(tracker()->GetPendingUpdate(kChangeGuid2, mango_id));
 }
 
 TEST_F(PendingNetworkConfigurationTrackerImplTest, TestRetryCounting) {
-  tracker()->TrackPendingUpdate(kChangeGuid1, kFredSsid,
+  NetworkIdentifier id(kFredSsid, shill::kSecurityPsk);
+  tracker()->TrackPendingUpdate(kChangeGuid1, id,
                                 /*specifics=*/base::nullopt);
-  AssertTrackerHasMatchingUpdate(kChangeGuid1, kFredSsid);
+  AssertTrackerHasMatchingUpdate(kChangeGuid1, id);
   EXPECT_EQ(1u, GetPref()->DictSize());
-  EXPECT_EQ(0, tracker()
-                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
-                   ->completed_attempts());
-  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
-  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
-  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
-  EXPECT_EQ(3, tracker()
-                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
-                   ->completed_attempts());
-  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
-  tracker()->IncrementCompletedAttempts(kChangeGuid1, kFredSsid);
-  EXPECT_EQ(5, tracker()
-                   ->GetPendingUpdate(kChangeGuid1, kFredSsid)
-                   ->completed_attempts());
+  EXPECT_EQ(
+      0, tracker()->GetPendingUpdate(kChangeGuid1, id)->completed_attempts());
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, id);
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, id);
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, id);
+  EXPECT_EQ(
+      3, tracker()->GetPendingUpdate(kChangeGuid1, id)->completed_attempts());
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, id);
+  tracker()->IncrementCompletedAttempts(kChangeGuid1, id);
+  EXPECT_EQ(
+      5, tracker()->GetPendingUpdate(kChangeGuid1, id)->completed_attempts());
 }
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/pending_network_configuration_update.cc

@@ -7,11 +7,11 @@
 namespace sync_wifi {
 
 PendingNetworkConfigurationUpdate::PendingNetworkConfigurationUpdate(
-    const std::string& ssid,
+    const NetworkIdentifier& id,
     const std::string& change_guid,
     const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics,
     int completed_attempts)
-    : ssid_(ssid),
+    : id_(id),
       change_guid_(change_guid),
       specifics_(specifics),
       completed_attempts_(completed_attempts) {}

chromeos/components/sync_wifi/pending_network_configuration_update.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "base/unguessable_token.h"
+#include "chromeos/components/sync_wifi/network_identifier.h"
 #include "components/sync/protocol/wifi_configuration_specifics.pb.h"
 
 namespace sync_wifi {
@@ -19,7 +20,7 @@ namespace sync_wifi {
 class PendingNetworkConfigurationUpdate {
  public:
   PendingNetworkConfigurationUpdate(
-      const std::string& ssid,
+      const NetworkIdentifier& id,
       const std::string& change_guid,
       const base::Optional<sync_pb::WifiConfigurationSpecificsData>& specifics,
       int completed_attempts);
@@ -27,8 +28,8 @@ class PendingNetworkConfigurationUpdate {
       const PendingNetworkConfigurationUpdate& update);
   virtual ~PendingNetworkConfigurationUpdate();
 
-  // The SSID of the network.
-  const std::string& ssid() const { return ssid_; }
+  // The identifier for the network.
+  const NetworkIdentifier& id() const { return id_; }
 
   // A unique ID for each change.
   const std::string& change_guid() const { return change_guid_; }
@@ -46,7 +47,7 @@ class PendingNetworkConfigurationUpdate {
   bool IsDeleteOperation() const;
 
  private:
-  const std::string ssid_;
+  const NetworkIdentifier id_;
   const std::string change_guid_;
   const base::Optional<sync_pb::WifiConfigurationSpecificsData> specifics_;
   int completed_attempts_;

chromeos/components/sync_wifi/synced_network_updater.h

@@ -15,6 +15,8 @@ class WifiConfigurationSpecificsData;
 
 namespace sync_wifi {
 
+class NetworkIdentifier;
+
 // Applies updates to synced networks to the local networking stack.
 class SyncedNetworkUpdater {
  public:
@@ -22,7 +24,7 @@ class SyncedNetworkUpdater {
 
   virtual void AddOrUpdateNetwork(
       const sync_pb::WifiConfigurationSpecificsData& specifics) = 0;
-  virtual void RemoveNetwork(const std::string& ssid) = 0;
+  virtual void RemoveNetwork(const NetworkIdentifier& id) = 0;
 
  protected:
   SyncedNetworkUpdater() = default;

chromeos/components/sync_wifi/test_specifics_generator.h

@@ -14,7 +14,7 @@ namespace {
 sync_pb::WifiConfigurationSpecificsData CreateSpecifics(
     const std::string& ssid) {
   sync_pb::WifiConfigurationSpecificsData specifics;
-  specifics.set_ssid(ssid);
+  specifics.set_hex_ssid(ssid);
   specifics.set_security_type(
       sync_pb::WifiConfigurationSpecificsData::SECURITY_TYPE_PSK);
   specifics.set_passphrase("password");

chromeos/components/sync_wifi/wifi_configuration_bridge.cc

@@ -13,6 +13,7 @@
 #include "base/optional.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
+#include "chromeos/components/sync_wifi/network_identifier.h"
 #include "chromeos/components/sync_wifi/synced_network_updater.h"
 #include "components/sync/model/entity_change.h"
 #include "components/sync/model/metadata_batch.h"
@@ -31,7 +32,7 @@ std::unique_ptr<syncer::EntityData> GenerateWifiEntityData(
   entity_data->specifics.mutable_wifi_configuration()
       ->mutable_client_only_encrypted_data()
       ->CopyFrom(data);
-  entity_data->name = data.ssid();
+  entity_data->name = NetworkIdentifier::FromProto(data).SerializeToString();
   return entity_data;
 }
 }  // namespace
@@ -75,7 +76,8 @@ base::Optional<syncer::ModelError> WifiConfigurationBridge::ApplySyncChanges(
       if (it != entries_.end()) {
         entries_.erase(it);
         batch->DeleteData(change->storage_key());
-        synced_network_updater_->RemoveNetwork(change->storage_key());
+        synced_network_updater_->RemoveNetwork(
+            NetworkIdentifier::DeserializeFromString(change->storage_key()));
       }
       continue;
     }
@@ -124,9 +126,9 @@ std::string WifiConfigurationBridge::GetClientTag(
 
 std::string WifiConfigurationBridge::GetStorageKey(
     const syncer::EntityData& entity_data) {
-  return entity_data.specifics.wifi_configuration()
-      .client_only_encrypted_data()
-      .ssid();
+  return NetworkIdentifier::FromProto(entity_data.specifics.wifi_configuration()
+                                          .client_only_encrypted_data())
+      .SerializeToString();
 }
 
 void WifiConfigurationBridge::OnStoreCreated(
@@ -190,7 +192,7 @@ void WifiConfigurationBridge::Commit(
 std::vector<std::string> WifiConfigurationBridge::GetAllSsidsForTesting() {
   std::vector<std::string> ssids;
   for (const auto& entry : entries_)
-    ssids.push_back(entry.second.ssid());
+    ssids.push_back(entry.second.hex_ssid());
 
   return ssids;
 }

chromeos/components/sync_wifi/wifi_configuration_bridge_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/test/task_environment.h"
+#include "chromeos/components/sync_wifi/network_identifier.h"
 #include "chromeos/components/sync_wifi/synced_network_updater.h"
 #include "chromeos/components/sync_wifi/test_specifics_generator.h"
 #include "components/sync/model/entity_change.h"
@@ -46,7 +47,7 @@ std::unique_ptr<syncer::EntityData> GenerateWifiEntityData(
   entity_data->specifics.mutable_wifi_configuration()
       ->mutable_client_only_encrypted_data()
       ->CopyFrom(data);
-  entity_data->name = data.ssid();
+  entity_data->name = data.hex_ssid();
   return entity_data;
 }
 
@@ -58,7 +59,7 @@ bool VectorContainsSsid(
     const std::vector<sync_pb::WifiConfigurationSpecificsData>& v,
     std::string s) {
   for (sync_pb::WifiConfigurationSpecificsData specifics : v) {
-    if (specifics.ssid() == s)
+    if (specifics.hex_ssid() == s)
       return true;
   }
   return false;
@@ -74,7 +75,7 @@ class TestSyncedNetworkUpdater : public SyncedNetworkUpdater {
     return add_update_calls_;
   }
 
-  const std::vector<std::string>& remove_calls() { return remove_calls_; }
+  const std::vector<NetworkIdentifier>& remove_calls() { return remove_calls_; }
 
  private:
   void AddOrUpdateNetwork(
@@ -82,12 +83,12 @@ class TestSyncedNetworkUpdater : public SyncedNetworkUpdater {
     add_update_calls_.push_back(specifics);
   }
 
-  void RemoveNetwork(const std::string& ssid) override {
-    remove_calls_.push_back(ssid);
+  void RemoveNetwork(const NetworkIdentifier& id) override {
+    remove_calls_.push_back(id);
   }
 
   std::vector<sync_pb::WifiConfigurationSpecificsData> add_update_calls_;
-  std::vector<std::string> remove_calls_;
+  std::vector<NetworkIdentifier> remove_calls_;
 };
 
 class WifiConfigurationBridgeTest : public testing::Test {
@@ -117,10 +118,10 @@ class WifiConfigurationBridgeTest : public testing::Test {
       specifics.mutable_client_only_encrypted_data()->CopyFrom(data);
       entity_data->specifics.mutable_wifi_configuration()->CopyFrom(specifics);
 
-      entity_data->name = data.ssid();
+      entity_data->name = data.hex_ssid();
 
-      changes.push_back(
-          syncer::EntityChange::CreateAdd(data.ssid(), std::move(entity_data)));
+      changes.push_back(syncer::EntityChange::CreateAdd(
+          data.hex_ssid(), std::move(entity_data)));
     }
     return changes;
   }
@@ -217,11 +218,12 @@ TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneAdd) {
 
 TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneDeletion) {
   WifiConfigurationSpecificsData entry = CreateSpecifics(kSsidMeow);
+  NetworkIdentifier id = NetworkIdentifier::FromProto(entry);
 
   syncer::EntityChangeList add_changes;
 
   add_changes.push_back(syncer::EntityChange::CreateAdd(
-      kSsidMeow, GenerateWifiEntityData(entry)));
+      id.SerializeToString(), GenerateWifiEntityData(entry)));
 
   bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
                              std::move(add_changes));
@@ -235,16 +237,17 @@ TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneDeletion) {
   EXPECT_TRUE(VectorContainsSsid(networks, kSsidMeow));
 
   syncer::EntityChangeList delete_changes;
-  delete_changes.push_back(syncer::EntityChange::CreateDelete(kSsidMeow));
+  delete_changes.push_back(
+      syncer::EntityChange::CreateDelete(id.SerializeToString()));
 
   bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
                              std::move(delete_changes));
   EXPECT_TRUE(bridge()->GetAllSsidsForTesting().empty());
 
-  const std::vector<std::string>& removed_networks =
+  const std::vector<NetworkIdentifier>& removed_networks =
       synced_network_updater()->remove_calls();
   EXPECT_EQ(1u, removed_networks.size());
-  EXPECT_TRUE(VectorContainsString(removed_networks, kSsidMeow));
+  EXPECT_EQ(removed_networks[0], id);
 }
 
 }  // namespace

components/sync/protocol/proto_visitors.h

@@ -1100,7 +1100,7 @@ VISIT_PROTO_FIELDS(
 }
 
 VISIT_PROTO_FIELDS(const sync_pb::WifiConfigurationSpecificsData& proto) {
-  VISIT_BYTES(ssid);
+  VISIT_BYTES(hex_ssid);
   VISIT_ENUM(security_type);
   VISIT_BYTES(passphrase);
   VISIT_ENUM(automatically_connect);

components/sync/protocol/wifi_configuration_specifics.proto

@@ -19,8 +19,9 @@ package sync_pb;
 import "encryption.proto";
 
 message WifiConfigurationSpecificsData {
-  // May contain NUL, not necessarily UTF-8 encoded.
-  optional bytes ssid = 1;
+  // SSID encoded to hex, letters should be upper case and 0x prefix should be
+  // omitted. For example, ssid "network" would be provided as "6E6574776F726B".
+  optional bytes hex_ssid = 1;
   enum SecurityType {
     SECURITY_TYPE_UNSPECIFIED = 0;
     SECURITY_TYPE_NONE = 1;