Add similar service matching logic to shill stub

Mimic shill's similar service matching logic in fake_shill_* fakes.
This made it necessary to adapt a few test cases which assume that this
logic is not present in the stubs, and to fix a few test cases that
confuse GUIDs with service paths.

Bug: 936677, 943102
Test: chromeos_unittests, chromeos_components_unittests, browser_tests
Change-Id: I44ff36ee147ef90d9b756698ed1eddf5e0c0fff9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1539053
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667368}

chromeos/components/tether/host_scan_scheduler_impl_unittest.cc

@@ -36,12 +36,13 @@ const char kWifiServiceGuid[] = "wifiServiceGuid";
 const char kTetherGuid[] = "tetherGuid";
 
 std::string CreateConfigurationJsonString(const std::string& guid,
-                                          const std::string& type) {
+                                          const std::string& type,
+                                          const std::string& state) {
   std::stringstream ss;
   ss << "{"
      << "  \"GUID\": \"" << guid << "\","
      << "  \"Type\": \"" << type << "\","
-     << "  \"State\": \"" << shill::kStateReady << "\""
+     << "  \"State\": \"" << state << "\""
      << "}";
   return ss.str();
 }
@@ -86,10 +87,12 @@ class HostScanSchedulerImplTest : public testing::Test {
     host_scan_scheduler_->ScanRequested(type);
   }
 
-  void InitializeEthernet() {
+  void InitializeEthernet(bool is_initially_connected) {
+    std::string state =
+        is_initially_connected ? shill::kStateReady : shill::kStateIdle;
     ethernet_service_path_ =
         helper_->ConfigureService(CreateConfigurationJsonString(
-            kEthernetServiceGuid, shill::kTypeEthernet));
+            kEthernetServiceGuid, shill::kTypeEthernet, state));
     helper_->manager_test()->SetManagerProperty(
         shill::kDefaultServiceProperty, base::Value(ethernet_service_path_));
   }
@@ -135,8 +138,9 @@ class HostScanSchedulerImplTest : public testing::Test {
     helper_->network_state_handler()->AddTetherNetworkState(
         kTetherGuid, "name", "carrier", 100 /* battery_percentage */,
         100 /* signal strength */, false /* has_connected_to_host */);
-    std::string wifi_service_path = helper_->ConfigureService(
-        CreateConfigurationJsonString(kWifiServiceGuid, shill::kTypeWifi));
+    std::string wifi_service_path =
+        helper_->ConfigureService(CreateConfigurationJsonString(
+            kWifiServiceGuid, shill::kTypeWifi, shill::kStateReady));
     helper_->network_state_handler()
         ->AssociateTetherNetworkStateWithWifiNetwork(kTetherGuid,
                                                      kWifiServiceGuid);
@@ -210,7 +214,7 @@ TEST_F(HostScanSchedulerImplTest, TestDeviceLockAndUnlock_Offline) {
 
 TEST_F(HostScanSchedulerImplTest, TestDeviceLockAndUnlock_Online) {
   // Simulate the device being online.
-  InitializeEthernet();
+  InitializeEthernet(true /* is_initially_connected */);
 
   // Lock the screen. This should never trigger a scan.
   SetScreenLockedState(true /* is_locked */);
@@ -337,7 +341,7 @@ TEST_F(HostScanSchedulerImplTest, HostScanBatchMetric) {
 }
 
 TEST_F(HostScanSchedulerImplTest, DefaultNetworkChanged) {
-  InitializeEthernet();
+  InitializeEthernet(false /* is_initially_connected */);
 
   // When no Tether network is present, a scan should start when the default
   // network is disconnected.

chromeos/components/tether/tether_network_disconnection_handler_unittest.cc

@@ -170,7 +170,7 @@ TEST_F(TetherNetworkDisconnectionHandlerTest,
   SetWiFiTechnologyStateEnabled(false);
 
   std::unique_ptr<NetworkState> network =
-      std::make_unique<NetworkState>(kWifiNetworkGuid);
+      std::make_unique<NetworkState>(wifi_service_path_);
   network->SetGuid(kWifiNetworkGuid);
   handler_->NetworkConnectionStateChanged(network.get());
 

chromeos/components/tether/wifi_hotspot_disconnector_impl_unittest.cc

@@ -230,7 +230,7 @@ TEST_F(WifiHotspotDisconnectorImplTest, NetworkNotActuallyConnected) {
   SimulateConnectionToWifiNetwork();
   SetWifiNetworkToDisconnected();
 
-  CallDisconnect(wifi_service_path_);
+  CallDisconnect(kWifiNetworkGuid);
   EXPECT_EQ(NetworkConnectionHandler::kErrorNotConnected, GetResultAndReset());
 
   // Configuration should not have been removed.
@@ -243,7 +243,7 @@ TEST_F(WifiHotspotDisconnectorImplTest, WifiDisconnectionFails) {
 
   should_disconnect_successfully_ = false;
 
-  CallDisconnect(wifi_service_path_);
+  CallDisconnect(kWifiNetworkGuid);
   EXPECT_EQ(NetworkConnectionHandler::kErrorDisconnectFailed,
             GetResultAndReset());
 
@@ -260,7 +260,7 @@ TEST_F(WifiHotspotDisconnectorImplTest, WifiDisconnectionFails) {
 TEST_F(WifiHotspotDisconnectorImplTest, WifiDisconnectionSucceeds) {
   SimulateConnectionToWifiNetwork();
 
-  CallDisconnect(wifi_service_path_);
+  CallDisconnect(kWifiNetworkGuid);
   EXPECT_EQ(kSuccessResult, GetResultAndReset());
 
   // The Wi-Fi network should be disconnected.

chromeos/dbus/shill/fake_shill_manager_client.cc

@@ -357,26 +357,26 @@ void FakeShillManagerClient::ConfigureService(
     return;
   }
 
-  // For the purposes of this stub, we're going to assume that the GUID property
-  // is set to the service path because we don't want to re-implement Shill's
-  // property matching magic here.
-  std::string service_path = guid;
-
   std::string ipconfig_path;
   properties.GetString(shill::kIPConfigProperty, &ipconfig_path);
 
-  // Merge the new properties with existing properties, if any.
-  const base::DictionaryValue* existing_properties =
-      service_client->GetServiceProperties(service_path);
-  if (!existing_properties) {
-    // Add a new service to the service client stub because none exists, yet.
-    // This calls AddManagerService.
+  std::string service_path = service_client->FindServiceMatchingGUID(guid);
+  if (service_path.empty())
+    service_path = service_client->FindSimilarService(properties);
+  if (service_path.empty()) {
+    // In the stub, service paths don't have to be DBus paths, so build
+    // something out of the GUID as service path.
+    // Don't use the GUID itself, so tests are forced to distinguish between
+    // service paths and GUIDs instead of assuming that service path == GUID.
+    service_path = "service_path_for_" + guid;
     service_client->AddServiceWithIPConfig(
         service_path, guid /* guid */, guid /* name */, type, shill::kStateIdle,
         ipconfig_path, true /* visible */);
-    existing_properties = service_client->GetServiceProperties(service_path);
   }
 
+  // Merge the new properties with existing properties.
+  const base::DictionaryValue* existing_properties =
+      service_client->GetServiceProperties(service_path);
   std::unique_ptr<base::DictionaryValue> merged_properties(
       existing_properties->DeepCopy());
   merged_properties->MergeDictionary(&properties);
@@ -602,6 +602,9 @@ void FakeShillManagerClient::SortManagerServices(bool notify) {
     complete_dict_list.emplace_back(std::move(properties_copy));
   }
 
+  if (complete_dict_list.empty())
+    return;
+
   // Sort the service list using the same logic as Shill's Service::Compare.
   std::sort(complete_dict_list.begin(), complete_dict_list.end(),
             CompareNetworks);

chromeos/dbus/shill/fake_shill_profile_client.cc

@@ -102,8 +102,10 @@ void FakeShillProfileClient::DeleteEntry(const dbus::ObjectPath& profile_path,
                                          const base::Closure& callback,
                                          const ErrorCallback& error_callback) {
   ProfileProperties* profile = GetProfile(profile_path, error_callback);
-  if (!profile)
+  if (!profile) {
+    error_callback.Run("Error.InvalidProfile", profile_path.value());
     return;
+  }
 
   if (!profile->entries.RemoveWithoutPathExpansion(entry_path, nullptr)) {
     error_callback.Run("Error.InvalidProfileEntry", entry_path);

chromeos/dbus/shill/fake_shill_service_client.cc

@@ -14,6 +14,7 @@
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
+#include "base/task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chromeos/dbus/shill/shill_device_client.h"
@@ -44,6 +45,81 @@ base::TimeDelta GetInteractiveDelay() {
   return ShillManagerClient::Get()->GetTestInterface()->GetInteractiveDelay();
 }
 
+// Extracts the hex SSID from shill |service_properties|.
+std::string GetHexSSID(const base::Value& service_properties) {
+  const std::string* hex_ssid =
+      service_properties.FindStringKey(shill::kWifiHexSsid);
+  if (hex_ssid)
+    return *hex_ssid;
+  const std::string* ssid =
+      service_properties.FindStringKey(shill::kSSIDProperty);
+  if (ssid)
+    return base::HexEncode(ssid->c_str(), ssid->size());
+  return std::string();
+}
+
+std::string GetSecurityClass(const base::Value& service_properties) {
+  // Mimics shill's WiFiProvider::GetServiceParametersFromStorage with
+  // WiFiService::ComputeSecurityClass  .
+  const std::string* security_class =
+      service_properties.FindStringKey(shill::kSecurityClassProperty);
+  const std::string* security =
+      service_properties.FindStringKey(shill::kSecurityProperty);
+  if (security_class && security && *security_class != *security) {
+    LOG(ERROR) << "Mismatch between SecurityClass " << *security_class
+               << " and Security " << *security;
+  }
+
+  if (security_class)
+    return *security_class;
+
+  if (security) {
+    if (*security == shill::kSecurityRsn || *security == shill::kSecurityWpa)
+      return shill::kSecurityPsk;
+    return *security;
+  }
+
+  return shill::kSecurityNone;
+}
+
+// Returns true if both |template_service_properties| and |service_properties|
+// have the key |key| and both have the same value for it.
+bool HaveSameValueForKey(const base::Value& template_service_properties,
+                         const base::Value& service_properties,
+                         base::StringPiece key) {
+  const base::Value* template_service_value =
+      template_service_properties.FindKey(key);
+  const base::Value* service_value = service_properties.FindKey(key);
+  return template_service_value && service_value &&
+         *template_service_value == *service_value;
+}
+
+// Mimics shill's similar service matching logic. This is only invoked if
+// |template_service_properties| and |service_properties| refer to a service of
+// the same type.
+bool IsSimilarService(const std::string& service_type,
+                      const base::Value& template_service_properties,
+                      const base::Value& service_properties) {
+  if (service_type == shill::kTypeWifi) {
+    // Mimics shill's WiFiProvider::FindSimilarService.
+    return GetHexSSID(template_service_properties) ==
+               GetHexSSID(service_properties) &&
+           HaveSameValueForKey(template_service_properties, service_properties,
+                               shill::kModeProperty) &&
+           GetSecurityClass(template_service_properties) ==
+               GetSecurityClass(service_properties);
+  }
+
+  // Assume that Ethernet / EthernetEAP services are always similar.
+  if (service_type == shill::kTypeEthernet ||
+      service_type == shill::kTypeEthernetEap) {
+    // Mimics shill's EthernetProvider::FindSimilarService.
+    return true;
+  }
+
+  return false;
+}
+
 }  // namespace
 
 FakeShillServiceClient::FakeShillServiceClient() : weak_ptr_factory_(this) {}
@@ -85,10 +161,14 @@ void FakeShillServiceClient::GetProperties(
     call_status = DBUS_METHOD_CALL_FAILURE;
   }
 
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
+  base::OnceClosure property_update =
       base::BindOnce(&PassStubServiceProperties, callback, call_status,
-                     base::Owned(result_properties.release())));
+                     base::Owned(result_properties.release()));
+  if (hold_back_service_property_updates_)
+    recorded_property_updates_.push_back(std::move(property_update));
+  else
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  std::move(property_update));
 }
 
 void FakeShillServiceClient::SetProperty(const dbus::ObjectPath& service_path,
@@ -461,11 +541,15 @@ bool FakeShillServiceClient::SetServiceProperty(const std::string& service_path,
   }
 
   // Notifiy Chrome of the property change.
-  base::ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE,
+  base::OnceClosure property_update =
       base::BindOnce(&FakeShillServiceClient::NotifyObserversPropertyChanged,
                      weak_ptr_factory_.GetWeakPtr(),
-                     dbus::ObjectPath(service_path), changed_property));
+                     dbus::ObjectPath(service_path), changed_property);
+  if (hold_back_service_property_updates_)
+    recorded_property_updates_.push_back(std::move(property_update));
+  else
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  std::move(property_update));
   return true;
 }
 
@@ -476,6 +560,46 @@ const base::DictionaryValue* FakeShillServiceClient::GetServiceProperties(
   return properties;
 }
 
+std::string FakeShillServiceClient::FindServiceMatchingGUID(
+    const std::string& guid) {
+  for (const auto& service_pair : stub_services_.DictItems()) {
+    const auto& service_path = service_pair.first;
+    const auto& service_properties = service_pair.second;
+
+    const std::string* service_guid =
+        service_properties.FindStringKey(shill::kGuidProperty);
+    if (service_guid && *service_guid == guid)
+      return service_path;
+  }
+
+  return std::string();
+}
+
+std::string FakeShillServiceClient::FindSimilarService(
+    const base::Value& template_service_properties) {
+  const std::string* template_type =
+      template_service_properties.FindStringKey(shill::kTypeProperty);
+  if (!template_type)
+    return std::string();
+
+  for (const auto& service_pair : stub_services_.DictItems()) {
+    const auto& service_path = service_pair.first;
+    const auto& service_properties = service_pair.second;
+
+    const std::string* service_type =
+        service_properties.FindStringKey(shill::kTypeProperty);
+    if (!service_type || *service_type != *template_type)
+      continue;
+
+    if (IsSimilarService(*service_type, template_service_properties,
+                         service_properties)) {
+      return service_path;
+    }
+  }
+
+  return std::string();
+}
+
 void FakeShillServiceClient::ClearServices() {
   ShillManagerClient::Get()->GetTestInterface()->ClearManagerServices();
   stub_services_.Clear();
@@ -487,6 +611,19 @@ void FakeShillServiceClient::SetConnectBehavior(const std::string& service_path,
   connect_behavior_[service_path] = behavior;
 }
 
+void FakeShillServiceClient::SetHoldBackServicePropertyUpdates(bool hold_back) {
+  hold_back_service_property_updates_ = hold_back;
+  std::vector<base::OnceClosure> property_updates;
+  recorded_property_updates_.swap(property_updates);
+
+  if (hold_back_service_property_updates_)
+    return;
+
+  for (auto& property_update : property_updates)
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  std::move(property_update));
+}
+
 void FakeShillServiceClient::NotifyObserversPropertyChanged(
     const dbus::ObjectPath& service_path,
     const std::string& property) {

chromeos/dbus/shill/fake_shill_service_client.h

@@ -100,9 +100,13 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillServiceClient
                           const base::Value& value) override;
   const base::DictionaryValue* GetServiceProperties(
       const std::string& service_path) const override;
+  std::string FindServiceMatchingGUID(const std::string& guid) override;
+  std::string FindSimilarService(
+      const base::Value& template_service_properties) override;
   void ClearServices() override;
   void SetConnectBehavior(const std::string& service_path,
                           const base::Closure& behavior) override;
+  void SetHoldBackServicePropertyUpdates(bool hold_back) override;
 
  private:
   typedef base::ObserverList<ShillPropertyChangedObserver>::Unchecked
@@ -130,6 +134,15 @@ class COMPONENT_EXPORT(SHILL_CLIENT) FakeShillServiceClient
   std::map<dbus::ObjectPath, std::unique_ptr<PropertyObserverList>>
       observer_list_;
 
+  // If this is true, the FakeShillServiceClient is recording service property
+  // updates and will only send them when SetHoldBackServicePropertyUpdates is
+  // called with false again.
+  bool hold_back_service_property_updates_ = false;
+
+  // Property updates that were held back while
+  // |hold_back_service_property_updates_| was true.
+  std::vector<base::OnceClosure> recorded_property_updates_;
+
   // Note: This should remain the last member so it'll be destroyed and
   // invalidate its weak pointers before any other members are destroyed.
   base::WeakPtrFactory<FakeShillServiceClient> weak_ptr_factory_;

chromeos/dbus/shill/shill_manager_client.cc

@@ -4,6 +4,7 @@
 
 #include "chromeos/dbus/shill/shill_manager_client.h"
 
+#include <ios>
 #include <memory>
 
 #include "base/bind.h"

chromeos/dbus/shill/shill_service_client.h

@@ -76,12 +76,28 @@ class COMPONENT_EXPORT(SHILL_CLIENT) ShillServiceClient {
     virtual const base::DictionaryValue* GetServiceProperties(
         const std::string& service_path) const = 0;
 
+    // Returns the service path for the service which has the GUID property set
+    // to |guid|. If no such service exists, returns the empty string.
+    virtual std::string FindServiceMatchingGUID(const std::string& guid) = 0;
+
+    // Returns the service path for a service which is similar to the service
+    // described by |template_service_properties|. For Wifi, this means that
+    // security and mode match. Returns the empty string if no similar service
+    // is found.
+    virtual std::string FindSimilarService(
+        const base::Value& template_service_properties) = 0;
+
     // Clears all Services from the Manager and Service stubs.
     virtual void ClearServices() = 0;
 
     virtual void SetConnectBehavior(const std::string& service_path,
                                     const base::Closure& behavior) = 0;
 
+    // If |hold_back| is set to true, stops sending service property updates to
+    // observers and records them instead. Then if this is called again with
+    // |hold_back| == false, sends all recorded property updates.
+    virtual void SetHoldBackServicePropertyUpdates(bool hold_back) = 0;
+
    protected:
     virtual ~TestInterface() {}
   };

chromeos/network/network_configuration_handler_unittest.cc

@@ -473,9 +473,8 @@ TEST_F(NetworkConfigurationHandlerTest, CreateConfiguration) {
   base::RunLoop().RunUntilIdle();
 
   ASSERT_TRUE(success);
-  // In FakeShillManagerClient, instead of re-implementing shill's behavior,
-  // guid is used for service_path.
-  EXPECT_EQ(service_path, kGuid);
+  EXPECT_EQ(service_path,
+            GetShillServiceClient()->FindServiceMatchingGUID(kGuid));
   EXPECT_EQ(guid, kGuid);
 }
 

chromeos/test/data/network/policy/policy_ethernet_eap.onc

@@ -13,7 +13,7 @@
           "Outer":"EAP-TLS",
         }
       },
-      "GUID":"guid",
+      "GUID":"policy_ethernet_eap",
       "Name":"name",
       "Type":"Ethernet"
     }

chromeos/test/data/network/policy/shill_policy_autoconnect_on_unconfigured_wifi1.json

@@ -4,13 +4,11 @@
   "GUID": "policy_wifi1",
   "ManagedCredentials": true,
   "Mode": "managed",
-  "Name": "policy_wifi1",
   "Passphrase": "policy's passphrase",
   "PassphraseRequired": false,
   "Profile": "/profile/user1/shill",
   "SSID": "policy_wifi1",
   "SecurityClass": "psk",
-  "State": "idle",
   "Type": "wifi",
   "UIData": "{\"onc_source\":\"user_policy\"}",
   "Visible": true,

chromeos/test/data/network/policy/shill_policy_on_unconfigured_wifi1.json

@@ -2,13 +2,11 @@
   "Device": "/device/wifi1",
   "GUID": "policy_wifi1",
   "Mode": "managed",
-  "Name": "policy_wifi1",
   "Passphrase": "policy's passphrase",
   "PassphraseRequired": false,
   "Profile": "/profile/user1/shill",
   "SSID": "policy_wifi1",
   "SecurityClass": "psk",
-  "State": "idle",
   "Type": "wifi",
   "UIData": "{\"onc_source\":\"user_policy\"}",
   "Visible": true,

chromeos/test/data/network/policy/shill_policy_on_unmanaged_ethernet_eap.json

 {
   "Device": "",
   "EAP.EAP": "TLS",
-  "GUID": "guid",
-  "Name": "guid",
+  "GUID": "policy_ethernet_eap",
   "Profile": "/profile/user1/shill",
-  "SSID": "guid",
-  "State": "idle",
   "Type": "etherneteap",
   "UIData": "{\"onc_source\":\"user_policy\"}",
-  "Visible": true,
-  "WiFi.HexSSID": "67756964"
+  "Visible": true
 }

chromeos/test/data/network/policy/shill_policy_on_unmanaged_wifi1.json

@@ -2,13 +2,11 @@
   "Device": "/device/wifi1",
   "GUID": "policy_wifi1",
   "Mode": "managed",
-  "Name": "policy_wifi1",
   "Passphrase": "user's passphrase",
   "PassphraseRequired": false,
   "Profile": "/profile/user1/shill",
   "SSID": "policy_wifi1",
   "SecurityClass": "psk",
-  "State": "idle",
   "Type": "wifi",
   "UIData": "{\"onc_source\":\"user_policy\",\"user_settings\":{\"WiFi\":{\"Passphrase\":\"FAKE_CREDENTIAL_VPaJDV9x\"}}}",
   "Visible":true,