Wi-Fi Sync: Prevent uploading changes which were just applied from sync.

This change fixes a few places that caused loops of updating sync:
* When sync is applying an update, we now record the network id in a
  map and check before uploading changes from the device.  Previously
  there was only a bit in the metadata store which tracked this, but
  it's not necessarily updated in time.
* When the Network Details page is open and an update is applied, avoid
  re-saving the network from js.
* When applying an update that doesn't include fields that were modified
  by another user, this prevents sync from overwriting the existing
  values (and kicking off another update).  This required adding a new
  option to the proto to leave DNS unspecified.

Bug: 966270
Change-Id: I7a89c0e97b51fbcfd8dea81499fce075a18570eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2209892
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772378}

chrome/browser/resources/settings/chromeos/internet_page/internet_detail_page.js

@@ -219,6 +219,12 @@ Polymer({
   /** @private {?chromeos.networkConfig.mojom.CrosNetworkConfigRemote} */
   networkConfig_: null,
 
+  /**
+   * Prevents re-saving incoming changes.
+   * @private {boolean}
+   */
+  applyingChanges_: false,
+
   /** @override */
   attached() {
     if (loadTimeData.getBoolean('splitSettingsSyncEnabled')) {
@@ -565,7 +571,8 @@ Polymer({
       return;
     }
 
-    this.managedProperties_ = properties;
+    this.updateManagedProperties_(properties);
+
     // Detail page should not be shown when Arc VPN is not connected.
     if (this.isArcVpn_(this.managedProperties_) &&
         !this.isConnectedState_(this.managedProperties_)) {
@@ -579,6 +586,17 @@ Polymer({
     }
   },
 
+  /**
+   * @param {!mojom.ManagedProperties|undefined} properties
+   * @private
+   */
+  updateManagedProperties_(properties) {
+    this.applyingChanges_ = true;
+    this.managedProperties_ = properties;
+    Polymer.RenderStatus.afterNextRender(
+        this, () => this.applyingChanges_ = false);
+  },
+
   /**
    * @param {?OncMojo.NetworkStateProperties} networkState
    * @private
@@ -608,7 +626,7 @@ Polymer({
             networkState.typeState.wifi.signalStrength;
         break;
     }
-    this.managedProperties_ = managedProperties;
+    this.updateManagedProperties_(managedProperties);
 
     this.propertiesReceived_ = true;
     this.outOfRange_ = false;
@@ -638,7 +656,7 @@ Polymer({
    * @private
    */
   setMojoNetworkProperties_(config) {
-    if (!this.propertiesReceived_ || !this.guid) {
+    if (!this.propertiesReceived_ || !this.guid || this.applyingChanges_) {
       return;
     }
     this.networkConfig_.setProperties(this.guid, config).then(response => {

chrome/test/data/webui/chromeos/fake_network_config_mojom.js

@@ -85,7 +85,8 @@ class FakeNetworkConfig {
 
     ['getNetworkState', 'getNetworkStateList', 'getDeviceStateList',
      'getManagedProperties', 'setNetworkTypeEnabledState', 'requestNetworkScan',
-     'getGlobalPolicy', 'getVpnProviders', 'getNetworkCertificates']
+     'getGlobalPolicy', 'getVpnProviders', 'getNetworkCertificates',
+     'setProperties']
         .forEach((methodName) => {
           this.resolverMap_.set(methodName, new PromiseResolver());
         });

chrome/test/data/webui/settings/chromeos/internet_detail_page_tests.js

@@ -324,5 +324,38 @@ suite('InternetDetailPage', function() {
             assertFalse(toggle.checked);
           });
     });
+
+    test.only('Auto Connect updates don\'t trigger a re-save', function() {
+      const mojom = chromeos.networkConfig.mojom;
+      mojoApi_.setNetworkTypeEnabledState(mojom.NetworkType.kWiFi, true);
+      let wifi1 = getManagedProperties(
+          mojom.NetworkType.kWiFi, 'wifi1', mojom.OncSource.kDevice);
+      wifi1.typeProperties.wifi.autoConnect = OncMojo.createManagedBool(true);
+      mojoApi_.setManagedPropertiesForTest(wifi1);
+
+      mojoApi_.whenCalled('setProperties').then(() => assert(false));
+      internetDetailPage.init('wifi1_guid', 'WiFi', 'wifi1');
+      internetDetailPage.onNetworkStateChanged({guid: 'wifi1_guid'});
+      return flushAsync()
+          .then(() => {
+            const toggle = internetDetailPage.$$('#autoConnectToggle');
+            assertTrue(!!toggle);
+            assertTrue(toggle.checked);
+
+            // Rebuild the object to force polymer to recognize a change.
+            wifi1 = getManagedProperties(
+                mojom.NetworkType.kWiFi, 'wifi1', mojom.OncSource.kDevice);
+            wifi1.typeProperties.wifi.autoConnect =
+                OncMojo.createManagedBool(false);
+            mojoApi_.setManagedPropertiesForTest(wifi1);
+            internetDetailPage.onNetworkStateChanged({guid: 'wifi1_guid'});
+            return flushAsync();
+          })
+          .then(() => {
+            const toggle = internetDetailPage.$$('#autoConnectToggle');
+            assertTrue(!!toggle);
+            assertFalse(toggle.checked);
+          });
+    });
   });
 });

chromeos/components/sync_wifi/local_network_collector_impl.cc

@@ -165,21 +165,35 @@ void LocalNetworkCollectorImpl::OnGetManagedPropertiesResult(
   proto.set_automatically_connect(AutomaticallyConnectProtoFromMojo(
       properties->type_properties->get_wifi()->auto_connect));
   proto.set_is_preferred(IsPreferredProtoFromMojo(properties->priority));
-  if (properties->source == network_config::mojom::OncSource::kUser ||
-      !network_metadata_store_->GetIsFieldExternallyModified(
-          properties->guid, shill::kProxyConfigProperty))
-    proto.mutable_proxy_configuration()->CopyFrom(
-        ProxyConfigurationProtoFromMojo(properties->proxy_settings));
 
+  bool is_proxy_modified =
+      network_metadata_store_->GetIsFieldExternallyModified(
+          properties->guid, shill::kProxyConfigProperty);
+  sync_pb::WifiConfigurationSpecifics_ProxyConfiguration proxy_config =
+      ProxyConfigurationProtoFromMojo(properties->proxy_settings,
+                                      /*is_unspecified=*/is_proxy_modified);
+  proto.mutable_proxy_configuration()->CopyFrom(proxy_config);
+
+  bool is_dns_externally_modified =
+      network_metadata_store_->GetIsFieldExternallyModified(
+          properties->guid, shill::kNameServersProperty);
   if (properties->static_ip_config &&
       properties->static_ip_config->name_servers &&
       (properties->source == network_config::mojom::OncSource::kUser ||
-       !network_metadata_store_->GetIsFieldExternallyModified(
-           properties->guid, shill::kNameServersProperty))) {
+       !is_dns_externally_modified)) {
+    proto.set_dns_option(
+        sync_pb::WifiConfigurationSpecifics_DnsOption_DNS_OPTION_CUSTOM);
     for (const std::string& nameserver :
          properties->static_ip_config->name_servers->active_value) {
       proto.add_custom_dns(nameserver);
     }
+  } else if (properties->source == network_config::mojom::OncSource::kDevice &&
+             is_dns_externally_modified) {
+    proto.set_dns_option(
+        sync_pb::WifiConfigurationSpecifics_DnsOption_DNS_OPTION_UNSPECIFIED);
+  } else {
+    proto.set_dns_option(
+        sync_pb::WifiConfigurationSpecifics_DnsOption_DNS_OPTION_DEFAULT_DHCP);
   }
 
   ShillServiceClient::Get()->GetWiFiPassphrase(

chromeos/components/sync_wifi/network_type_conversions.cc

@@ -100,8 +100,9 @@ sync_pb::WifiConfigurationSpecifics_IsPreferredOption IsPreferredProtoFromMojo(
 
 sync_pb::WifiConfigurationSpecifics_ProxyConfiguration_ProxyOption
 ProxyOptionProtoFromMojo(
-    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings) {
-  if (!proxy_settings) {
+    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings,
+    bool is_unspecified) {
+  if (!proxy_settings || is_unspecified) {
     return sync_pb::WifiConfigurationSpecifics_ProxyConfiguration::
         PROXY_OPTION_UNSPECIFIED;
   }
@@ -127,9 +128,11 @@ ProxyOptionProtoFromMojo(
 
 sync_pb::WifiConfigurationSpecifics_ProxyConfiguration
 ProxyConfigurationProtoFromMojo(
-    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings) {
+    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings,
+    bool is_unspecified) {
   sync_pb::WifiConfigurationSpecifics_ProxyConfiguration proto;
-  proto.set_proxy_option(ProxyOptionProtoFromMojo(proxy_settings));
+  proto.set_proxy_option(
+      ProxyOptionProtoFromMojo(proxy_settings, is_unspecified));
 
   if (proto.proxy_option() ==
       sync_pb::WifiConfigurationSpecifics_ProxyConfiguration::
@@ -262,7 +265,14 @@ network_config::mojom::ConfigPropertiesPtr MojoNetworkConfigFromProto(
           ? 1
           : 0);
 
-  if (specifics.custom_dns().size()) {
+  // For backwards compatibility, any available custom nameservers are still
+  // applied when the dns_option is not set.
+  if (specifics.dns_option() ==
+          sync_pb::WifiConfigurationSpecifics_DnsOption_DNS_OPTION_CUSTOM ||
+      (specifics.dns_option() ==
+           sync_pb::
+               WifiConfigurationSpecifics_DnsOption_DNS_OPTION_UNSPECIFIED &&
+       specifics.custom_dns().size())) {
     auto ip_config = network_config::mojom::IPConfigProperties::New();
     std::vector<std::string> custom_dns;
     for (const std::string& nameserver : specifics.custom_dns()) {
@@ -271,7 +281,9 @@ network_config::mojom::ConfigPropertiesPtr MojoNetworkConfigFromProto(
     ip_config->name_servers = std::move(custom_dns);
     config->static_ip_config = std::move(ip_config);
     config->name_servers_config_type = onc::network_config::kIPConfigTypeStatic;
-  } else {
+  } else if (specifics.dns_option() ==
+             sync_pb::
+                 WifiConfigurationSpecifics_DnsOption_DNS_OPTION_DEFAULT_DHCP) {
     config->name_servers_config_type = onc::network_config::kIPConfigTypeDHCP;
   }
 

chromeos/components/sync_wifi/network_type_conversions.h

@@ -35,11 +35,13 @@ sync_pb::WifiConfigurationSpecifics_IsPreferredOption IsPreferredProtoFromMojo(
 
 sync_pb::WifiConfigurationSpecifics_ProxyConfiguration_ProxyOption
 ProxyOptionProtoFromMojo(
-    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings);
+    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings,
+    bool is_unspecified);
 
 sync_pb::WifiConfigurationSpecifics_ProxyConfiguration
 ProxyConfigurationProtoFromMojo(
-    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings);
+    const network_config::mojom::ManagedProxySettingsPtr& proxy_settings,
+    bool is_unspecified);
 
 network_config::mojom::SecurityType MojoSecurityTypeFromString(
     const std::string& security_type);

chromeos/components/sync_wifi/synced_network_updater.h

@@ -27,6 +27,7 @@ class SyncedNetworkUpdater {
   virtual void AddOrUpdateNetwork(
       const sync_pb::WifiConfigurationSpecifics& specifics) = 0;
   virtual void RemoveNetwork(const NetworkIdentifier& id) = 0;
+  virtual bool IsUpdateInProgress(const std::string& network_guid) = 0;
 
  protected:
   SyncedNetworkUpdater() = default;

chromeos/components/sync_wifi/synced_network_updater_impl.cc

@@ -72,6 +72,10 @@ void SyncedNetworkUpdaterImpl::StartAddOrUpdateOperation(
   if (existing_network) {
     NET_LOG(EVENT) << "Updating existing network "
                    << NetworkGuidId(existing_network->guid);
+    if (network_guid_to_updates_counter_.contains(existing_network->guid))
+      network_guid_to_updates_counter_[existing_network->guid]++;
+    else
+      network_guid_to_updates_counter_[existing_network->guid] = 1;
     cros_network_config_->SetProperties(
         existing_network->guid, std::move(config),
         base::BindOnce(&SyncedNetworkUpdaterImpl::OnSetPropertiesResult,
@@ -111,6 +115,12 @@ void SyncedNetworkUpdaterImpl::StartDeleteOperation(
                            weak_ptr_factory_.GetWeakPtr(), change_guid, id));
 }
 
+bool SyncedNetworkUpdaterImpl::IsUpdateInProgress(
+    const std::string& network_guid) {
+  return network_guid_to_updates_counter_.contains(network_guid) &&
+         network_guid_to_updates_counter_[network_guid] > 0;
+}
+
 network_config::mojom::NetworkStatePropertiesPtr
 SyncedNetworkUpdaterImpl::FindMojoNetwork(const NetworkIdentifier& id) {
   for (const network_config::mojom::NetworkStatePropertiesPtr& network :
@@ -182,6 +192,8 @@ void SyncedNetworkUpdaterImpl::OnSetPropertiesResult(
     metrics_logger_->RecordApplyNetworkFailureReason(
         ApplyNetworkFailureReason::kFailedToUpdate, error_message);
   }
+  if (network_guid_to_updates_counter_.contains(network_guid))
+    network_guid_to_updates_counter_[network_guid]--;
   HandleShillResult(change_guid, NetworkIdentifier::FromProto(proto),
                     is_success);
 }

chromeos/components/sync_wifi/synced_network_updater_impl.h

@@ -42,6 +42,8 @@ class SyncedNetworkUpdaterImpl
 
   void RemoveNetwork(const NetworkIdentifier& id) override;
 
+  bool IsUpdateInProgress(const std::string& network_guid) override;
+
   // CrosNetworkConfigObserver:
   void OnNetworkStateListChanged() override;
   void OnActiveNetworksChanged(
@@ -101,6 +103,7 @@ class SyncedNetworkUpdaterImpl
   std::unique_ptr<TimerFactory> timer_factory_;
   base::flat_map<std::string, std::unique_ptr<base::OneShotTimer>>
       change_guid_to_timer_map_;
+  base::flat_map<std::string, int> network_guid_to_updates_counter_;
   SyncedNetworkMetricsLogger* metrics_logger_;
 
   base::WeakPtrFactory<SyncedNetworkUpdaterImpl> weak_ptr_factory_{this};

chromeos/components/sync_wifi/wifi_configuration_bridge.cc

@@ -345,7 +345,8 @@ void WifiConfigurationBridge::OnNetworkUpdate(
   if (!set_properties)
     return;
 
-  if (network_metadata_store_->GetIsConfiguredBySync(guid)) {
+  if (synced_network_updater_->IsUpdateInProgress(guid) ||
+      network_metadata_store_->GetIsConfiguredBySync(guid)) {
     // Don't have to upload a configuration that came from sync.
     NET_LOG(EVENT) << "Not uploading change to " << NetworkGuidId(guid)
                    << ", modified network was configured "

chromeos/components/sync_wifi/wifi_configuration_bridge_unittest.cc

@@ -10,6 +10,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/containers/flat_map.h"
 #include "base/run_loop.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/metrics/histogram_tester.h"
@@ -105,7 +106,14 @@ class TestSyncedNetworkUpdater : public SyncedNetworkUpdater {
 
   const std::vector<NetworkIdentifier>& remove_calls() { return remove_calls_; }
 
- private:
+  void set_update_in_progress(const std::string& network_guid, bool value) {
+    if (value)
+      guid_update_in_progress_.insert(network_guid);
+    else
+      guid_update_in_progress_.erase(network_guid);
+  }
+
+  // SyncedNetworkUpdater:
   void AddOrUpdateNetwork(
       const sync_pb::WifiConfigurationSpecifics& specifics) override {
     add_update_calls_.push_back(specifics);
@@ -115,8 +123,14 @@ class TestSyncedNetworkUpdater : public SyncedNetworkUpdater {
     remove_calls_.push_back(id);
   }
 
+  bool IsUpdateInProgress(const std::string& network_guid) override {
+    return guid_update_in_progress_.contains(network_guid);
+  }
+
+ private:
   std::vector<sync_pb::WifiConfigurationSpecifics> add_update_calls_;
   std::vector<NetworkIdentifier> remove_calls_;
+  base::flat_set<std::string> guid_update_in_progress_;
 };
 
 class WifiConfigurationBridgeTest : public testing::Test {
@@ -437,6 +451,23 @@ TEST_F(WifiConfigurationBridgeTest, LocalUpdate_UntrackedField) {
   histogram_tester.ExpectTotalCount(kTotalCountHistogram, 0);
 }
 
+TEST_F(WifiConfigurationBridgeTest, LocalUpdate_FromSync) {
+  base::HistogramTester histogram_tester;
+  WifiConfigurationSpecifics meow_local =
+      GenerateTestWifiSpecifics(meow_network_id(), kSyncPsk, /*timestamp=*/100);
+  std::string guid = meow_network_id().SerializeToString();
+  local_network_collector()->AddNetwork(meow_local);
+  synced_network_updater()->set_update_in_progress(guid, true);
+
+  EXPECT_CALL(*processor(), Put(_, _, _)).Times(testing::Exactly(0));
+
+  base::DictionaryValue set_properties;
+  set_properties.SetBoolean(shill::kAutoConnectProperty, true);
+  bridge()->OnNetworkUpdate(guid, &set_properties);
+  base::RunLoop().RunUntilIdle();
+  histogram_tester.ExpectTotalCount(kTotalCountHistogram, 0);
+}
+
 TEST_F(WifiConfigurationBridgeTest, LocalRemove) {
   base::HistogramTester histogram_tester;
   WifiConfigurationSpecifics meow_local =

components/sync/protocol/wifi_configuration_specifics.proto

@@ -77,7 +77,13 @@ message WifiConfigurationSpecifics {
     optional ManualProxyConfiguration manual_proxy_configuration = 3;
   }
   optional ProxyConfiguration proxy_configuration = 7;
-  // List of DNS servers to be used.  Up to 4.
+  enum DnsOption {
+    DNS_OPTION_UNSPECIFIED = 0;
+    DNS_OPTION_DEFAULT_DHCP = 1;
+    DNS_OPTION_CUSTOM = 2;
+  }
+  optional DnsOption dns_option = 10;
+  // List of DNS servers to be used when set to DNS_OPTION_CUSTOM.  Up to 4.
   repeated string custom_dns = 8;
   // The last time this configuration was connected to before being synced.  It
   // will only be updated when the configuration is changed. This is represented