Use the last connected timestamp when merging sync and local networks.

When Wi-Fi sync is first enabled and a list of synced networks
has to be merged with the local networks on a device, use the last
connected timestamp to decide which configuration should be used.

Bug: 966270
Change-Id: Id7688a0fdb9e7922cee8ffec0ef69b804436e3e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2093274Reviewed-by: James Vecore <vecore@google.com>
Commit-Queue: Jon Mann <jonmann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750257}

chromeos/components/sync_wifi/test_data_generator.cc

@@ -17,7 +17,9 @@ NetworkIdentifier GeneratePskNetworkId(const std::string& ssid) {
 }
 
 sync_pb::WifiConfigurationSpecifics GenerateTestWifiSpecifics(
-    const NetworkIdentifier& id) {
+    const NetworkIdentifier& id,
+    const std::string& passphrase = "passphrase",
+    double timestamp = 1) {
   sync_pb::WifiConfigurationSpecifics specifics;
   specifics.set_hex_ssid(id.hex_ssid());
 
@@ -30,7 +32,8 @@ sync_pb::WifiConfigurationSpecifics GenerateTestWifiSpecifics(
   } else {
     NOTREACHED();
   }
-  specifics.set_passphrase("password");
+  specifics.set_passphrase(passphrase);
+  specifics.set_last_update_timestamp(timestamp);
   specifics.set_automatically_connect(
       sync_pb::WifiConfigurationSpecifics::AUTOMATICALLY_CONNECT_ENABLED);
   specifics.set_is_preferred(

chromeos/components/sync_wifi/test_data_generator.h

@@ -19,7 +19,9 @@ NetworkIdentifier GeneratePskNetworkId(const std::string& ssid);
 // Creates a proto with default values and sets the hex_ssid and security_type
 // based on the input |id|.
 sync_pb::WifiConfigurationSpecifics GenerateTestWifiSpecifics(
-    const NetworkIdentifier& id);
+    const NetworkIdentifier& id,
+    const std::string& passphrase = "passphrase",
+    double timestamp = 1);
 
 }  // namespace sync_wifi
 

chromeos/components/sync_wifi/wifi_configuration_bridge.cc

@@ -76,20 +76,86 @@ base::Optional<syncer::ModelError> WifiConfigurationBridge::MergeSyncData(
 void WifiConfigurationBridge::OnGetAllSyncableNetworksResult(
     std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
     syncer::EntityChangeList change_list,
-    std::vector<sync_pb::WifiConfigurationSpecifics> list) {
-  NET_LOG(DEBUG) << list.size() << " local networks eligible for sync.";
+    std::vector<sync_pb::WifiConfigurationSpecifics> local_network_list) {
+  // To merge local and synced networks we add all local networks that don't
+  // exist in sync to the server and all synced networks that don't exist
+  // locally to Shill.  For networks which exist on both lists, we compare the
+  // last connected timestamp and take the newer configuration.
+
+  NET_LOG(DEBUG) << local_network_list.size()
+                 << " local networks eligible for sync.";
+  base::flat_map<NetworkIdentifier, sync_pb::WifiConfigurationSpecifics>
+      sync_networks;
+  base::flat_map<NetworkIdentifier, sync_pb::WifiConfigurationSpecifics>
+      local_networks;
+
+  // Iterate through incoming changes from sync and populate the sync_networks
+  // map.
+  for (std::unique_ptr<syncer::EntityChange>& change : change_list) {
+    if (change->type() == syncer::EntityChange::ACTION_DELETE) {
+      // Don't delete any local networks during the initial merge when sync is
+      // first enabled.
+      continue;
+    }
 
-  for (sync_pb::WifiConfigurationSpecifics& proto : list) {
-    // TODO(jonmann) Don't override local configurations that are newer.
+    const sync_pb::WifiConfigurationSpecifics& proto =
+        change->data().specifics.wifi_configuration();
+    NetworkIdentifier id = NetworkIdentifier::FromProto(proto);
+    if (sync_networks.contains(id) &&
+        sync_networks[id].last_update_timestamp() >
+            proto.last_update_timestamp()) {
+      continue;
+    }
+    sync_networks[id] = proto;
+  }
+
+  // Iterate through local networks and add to sync where appropriate.
+  for (sync_pb::WifiConfigurationSpecifics& proto : local_network_list) {
+    NetworkIdentifier id = NetworkIdentifier::FromProto(proto);
+    if (sync_networks.contains(id) &&
+        sync_networks[id].last_update_timestamp() >
+            proto.last_update_timestamp()) {
+      continue;
+    }
+
+    local_networks[id] = proto;
     std::unique_ptr<syncer::EntityData> entity_data =
         GenerateWifiEntityData(proto);
     std::string storage_key = GetStorageKey(*entity_data);
+
+    // Upload the local network configuration to sync.  This could be a new
+    // configuration or an update to an existing one.
     change_processor()->Put(storage_key, std::move(entity_data),
                             metadata_change_list.get());
     entries_[storage_key] = proto;
   }
 
-  ApplySyncChanges(std::move(metadata_change_list), std::move(change_list));
+  std::unique_ptr<syncer::ModelTypeStore::WriteBatch> batch =
+      store_->CreateWriteBatch();
+  // Iterate through synced networks and update local stack where appropriate.
+  for (auto& kv : sync_networks) {
+    NetworkIdentifier& id = kv.first;
+    sync_pb::WifiConfigurationSpecifics& proto = kv.second;
+
+    if (local_networks.contains(id) &&
+        local_networks[id].last_update_timestamp() >
+            proto.last_update_timestamp()) {
+      continue;
+    }
+
+    // Update the local network stack to have the synced network configuration.
+    synced_network_updater_->AddOrUpdateNetwork(proto);
+
+    // Save the proto to the sync data store to keep track of all synced
+    // networks on device.  This gets loaded into |entries_| next time the
+    // bridge is initialized.
+    batch->WriteData(id.SerializeToString(), proto.SerializeAsString());
+    entries_[id.SerializeToString()] = proto;
+  }
+
+  // Mark the changes as processed.
+  batch->TakeMetadataChangesFrom(std::move(metadata_change_list));
+  Commit(std::move(batch));
 }
 
 base::Optional<syncer::ModelError> WifiConfigurationBridge::ApplySyncChanges(

chromeos/components/sync_wifi/wifi_configuration_bridge.h

@@ -72,7 +72,7 @@ class WifiConfigurationBridge : public syncer::ModelTypeSyncBridge {
   void OnGetAllSyncableNetworksResult(
       std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
       syncer::EntityChangeList change_list,
-      std::vector<sync_pb::WifiConfigurationSpecifics> list);
+      std::vector<sync_pb::WifiConfigurationSpecifics> local_network_list);
 
   // An in-memory list of the proto's that mirrors what is on the sync server.
   // This gets updated when changes are received from the server and after local

chromeos/components/sync_wifi/wifi_configuration_bridge_unittest.cc

@@ -45,6 +45,7 @@ using testing::UnorderedElementsAre;
 
 const char kSsidMeow[] = "meow";
 const char kSsidWoof[] = "woof";
+const char kSsidHonk[] = "honk";
 
 syncer::EntityData GenerateWifiEntityData(
     const sync_pb::WifiConfigurationSpecifics& data) {
@@ -55,13 +56,17 @@ syncer::EntityData GenerateWifiEntityData(
   return entity_data;
 }
 
-bool ProtoVectorContainsId(
+bool VectorContainsProto(
     const std::vector<sync_pb::WifiConfigurationSpecifics>& protos,
-    NetworkIdentifier id) {
+    const sync_pb::WifiConfigurationSpecifics& proto) {
   return std::find_if(
              protos.begin(), protos.end(),
-             [&id](const sync_pb::WifiConfigurationSpecifics& specifics) {
-               return NetworkIdentifier::FromProto(specifics) == id;
+             [&proto](const sync_pb::WifiConfigurationSpecifics& specifics) {
+               return NetworkIdentifier::FromProto(specifics) ==
+                          NetworkIdentifier::FromProto(proto) &&
+                      specifics.last_update_timestamp() ==
+                          proto.last_update_timestamp() &&
+                      specifics.passphrase() == proto.passphrase();
              }) != protos.end();
 }
 
@@ -164,6 +169,8 @@ class WifiConfigurationBridgeTest : public testing::Test {
 
   const NetworkIdentifier& meow_network_id() const { return meow_network_id_; }
 
+  const NetworkIdentifier& honk_network_id() const { return honk_network_id_; }
+
  private:
   base::test::TaskEnvironment task_environment_;
 
@@ -181,21 +188,25 @@ class WifiConfigurationBridgeTest : public testing::Test {
 
   const NetworkIdentifier meow_network_id_ = GeneratePskNetworkId(kSsidMeow);
 
+  const NetworkIdentifier honk_network_id_ = GeneratePskNetworkId(kSsidHonk);
+
   DISALLOW_COPY_AND_ASSIGN(WifiConfigurationBridgeTest);
 };
 
 TEST_F(WifiConfigurationBridgeTest, InitWithTwoNetworksFromServer) {
   syncer::EntityChangeList remote_input;
 
-  WifiConfigurationSpecifics entry1 =
+  WifiConfigurationSpecifics meow_network =
       GenerateTestWifiSpecifics(meow_network_id());
-  WifiConfigurationSpecifics entry2 =
+  WifiConfigurationSpecifics woof_network =
       GenerateTestWifiSpecifics(woof_network_id());
 
-  remote_input.push_back(syncer::EntityChange::CreateAdd(
-      meow_network_id().SerializeToString(), GenerateWifiEntityData(entry1)));
-  remote_input.push_back(syncer::EntityChange::CreateAdd(
-      woof_network_id().SerializeToString(), GenerateWifiEntityData(entry2)));
+  remote_input.push_back(
+      syncer::EntityChange::CreateAdd(meow_network_id().SerializeToString(),
+                                      GenerateWifiEntityData(meow_network)));
+  remote_input.push_back(
+      syncer::EntityChange::CreateAdd(woof_network_id().SerializeToString(),
+                                      GenerateWifiEntityData(woof_network)));
 
   bridge()->MergeSyncData(
       std::make_unique<syncer::InMemoryMetadataChangeList>(),
@@ -209,19 +220,19 @@ TEST_F(WifiConfigurationBridgeTest, InitWithTwoNetworksFromServer) {
   const std::vector<sync_pb::WifiConfigurationSpecifics>& networks =
       synced_network_updater()->add_or_update_calls();
   EXPECT_EQ(2u, networks.size());
-  EXPECT_TRUE(ProtoVectorContainsId(networks, meow_network_id()));
-  EXPECT_TRUE(ProtoVectorContainsId(networks, woof_network_id()));
+  EXPECT_TRUE(VectorContainsProto(networks, meow_network));
+  EXPECT_TRUE(VectorContainsProto(networks, woof_network));
 }
 
 TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesAddTwoSpecifics) {
-  const WifiConfigurationSpecifics specifics1 =
+  const WifiConfigurationSpecifics meow_network =
       GenerateTestWifiSpecifics(meow_network_id());
-  const WifiConfigurationSpecifics specifics2 =
+  const WifiConfigurationSpecifics woof_network =
       GenerateTestWifiSpecifics(woof_network_id());
 
-  base::Optional<syncer::ModelError> error =
-      bridge()->ApplySyncChanges(bridge()->CreateMetadataChangeList(),
-                                 CreateEntityAddList({specifics1, specifics2}));
+  base::Optional<syncer::ModelError> error = bridge()->ApplySyncChanges(
+      bridge()->CreateMetadataChangeList(),
+      CreateEntityAddList({meow_network, woof_network}));
   EXPECT_FALSE(error);
   std::vector<NetworkIdentifier> ids = bridge()->GetAllIdsForTesting();
   EXPECT_EQ(2u, ids.size());
@@ -231,8 +242,8 @@ TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesAddTwoSpecifics) {
   const std::vector<sync_pb::WifiConfigurationSpecifics>& networks =
       synced_network_updater()->add_or_update_calls();
   EXPECT_EQ(2u, networks.size());
-  EXPECT_TRUE(ProtoVectorContainsId(networks, meow_network_id()));
-  EXPECT_TRUE(ProtoVectorContainsId(networks, woof_network_id()));
+  EXPECT_TRUE(VectorContainsProto(networks, woof_network));
+  EXPECT_TRUE(VectorContainsProto(networks, meow_network));
 }
 
 TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneAdd) {
@@ -254,7 +265,7 @@ TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneAdd) {
   const std::vector<sync_pb::WifiConfigurationSpecifics>& networks =
       synced_network_updater()->add_or_update_calls();
   EXPECT_EQ(1u, networks.size());
-  EXPECT_TRUE(ProtoVectorContainsId(networks, meow_network_id()));
+  EXPECT_TRUE(VectorContainsProto(networks, entry));
 }
 
 TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneDeletion) {
@@ -276,7 +287,7 @@ TEST_F(WifiConfigurationBridgeTest, ApplySyncChangesOneDeletion) {
   const std::vector<sync_pb::WifiConfigurationSpecifics>& networks =
       synced_network_updater()->add_or_update_calls();
   EXPECT_EQ(1u, networks.size());
-  EXPECT_TRUE(ProtoVectorContainsId(networks, meow_network_id()));
+  EXPECT_TRUE(VectorContainsProto(networks, entry));
 
   syncer::EntityChangeList delete_changes;
   delete_changes.push_back(
@@ -296,16 +307,31 @@ TEST_F(WifiConfigurationBridgeTest, MergeSyncData) {
   auto metadata_change_list =
       std::make_unique<syncer::InMemoryMetadataChangeList>();
   syncer::EntityChangeList entity_data;
-
-  WifiConfigurationSpecifics meow_network =
-      GenerateTestWifiSpecifics(meow_network_id());
+  const char kSyncPsk[] = "sync_psk";
+  const char kLocalPsk[] = "local_psk";
+
+  WifiConfigurationSpecifics meow_sync =
+      GenerateTestWifiSpecifics(meow_network_id(), kSyncPsk, /*timestamp=*/100);
+  WifiConfigurationSpecifics woof_sync =
+      GenerateTestWifiSpecifics(woof_network_id(), kSyncPsk, /*timestamp=*/100);
+  WifiConfigurationSpecifics honk_sync =
+      GenerateTestWifiSpecifics(honk_network_id(), kSyncPsk, /*timestamp=*/100);
   entity_data.push_back(
       syncer::EntityChange::CreateAdd(meow_network_id().SerializeToString(),
-                                      GenerateWifiEntityData(meow_network)));
+                                      GenerateWifiEntityData(meow_sync)));
+  entity_data.push_back(
+      syncer::EntityChange::CreateAdd(woof_network_id().SerializeToString(),
+                                      GenerateWifiEntityData(woof_sync)));
+  entity_data.push_back(
+      syncer::EntityChange::CreateAdd(honk_network_id().SerializeToString(),
+                                      GenerateWifiEntityData(honk_sync)));
 
-  WifiConfigurationSpecifics woof_network =
-      GenerateTestWifiSpecifics(woof_network_id());
-  local_network_collector()->AddNetwork(woof_network);
+  WifiConfigurationSpecifics woof_local =
+      GenerateTestWifiSpecifics(woof_network_id(), kLocalPsk, /*timestamp=*/1);
+  WifiConfigurationSpecifics meow_local = GenerateTestWifiSpecifics(
+      meow_network_id(), kLocalPsk, /*timestamp=*/1000);
+  local_network_collector()->AddNetwork(woof_local);
+  local_network_collector()->AddNetwork(meow_local);
 
   std::string storage_key;
   EXPECT_CALL(*processor(), Put(_, _, _))
@@ -316,17 +342,21 @@ TEST_F(WifiConfigurationBridgeTest, MergeSyncData) {
   base::RunLoop().RunUntilIdle();
 
   // Verify local network was added to sync.
-  EXPECT_EQ(storage_key, woof_network_id().SerializeToString());
+  EXPECT_EQ(storage_key, meow_network_id().SerializeToString());
 
   // Verify sync network was added to local stack.
   const std::vector<sync_pb::WifiConfigurationSpecifics>&
       updated_local_networks = synced_network_updater()->add_or_update_calls();
-  EXPECT_EQ(1u, updated_local_networks.size());
-  EXPECT_TRUE(ProtoVectorContainsId(updated_local_networks, meow_network_id()));
+  EXPECT_EQ(2u, updated_local_networks.size());
+  EXPECT_TRUE(VectorContainsProto(updated_local_networks, woof_sync));
+  EXPECT_TRUE(VectorContainsProto(updated_local_networks, honk_sync));
 
   std::vector<sync_pb::WifiConfigurationSpecifics> sync_networks =
       GetAllSyncedData();
-  EXPECT_EQ(2u, sync_networks.size());
+  EXPECT_EQ(3u, sync_networks.size());
+  EXPECT_TRUE(VectorContainsProto(sync_networks, meow_local));
+  EXPECT_TRUE(VectorContainsProto(sync_networks, woof_sync));
+  EXPECT_TRUE(VectorContainsProto(sync_networks, honk_sync));
 }
 
 }  // namespace