CrOS: Network: Fix ShillPropertyHandler for ProhibitedTechnologies

When the event listener was added for shill::kProhibitedTechnologies,
it treated the property as a list (like other properties) however
the property is actually stored as a string in Shill:
https://chromium-review.googlesource.com/c/chromium/src/+/2197136

This was causing error spam when running on linux and in tests.

This fixes that and adds a test.

Bug: 1081488
TBR=tbegin@chromium.org

Change-Id: I502bfe3369d31514b0f6f8e338408d3433ad5cef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2283934Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785828}

chromeos/dbus/shill/fake_shill_manager_client.cc

@@ -538,19 +538,30 @@ void FakeShillManagerClient::SetTechnologyInitializing(const std::string& type,
 
 void FakeShillManagerClient::SetTechnologyProhibited(const std::string& type,
                                                      bool prohibited) {
+  std::string prohibited_technologies =
+      GetStringValue(stub_properties_, shill::kProhibitedTechnologiesProperty);
+  std::vector<std::string> prohibited_list =
+      base::SplitString(prohibited_technologies, ",", base::TRIM_WHITESPACE,
+                        base::SPLIT_WANT_NONEMPTY);
+  std::set<std::string> prohibited_set(prohibited_list.begin(),
+                                       prohibited_list.end());
   if (prohibited) {
-    if (GetListProperty(shill::kProhibitedTechnologiesProperty)
-            ->AppendIfNotPresent(std::make_unique<base::Value>(type))) {
-      CallNotifyObserversPropertyChanged(
-          shill::kProhibitedTechnologiesProperty);
-    }
+    auto iter = prohibited_set.find(type);
+    if (iter != prohibited_set.end())
+      return;
+    prohibited_set.insert(type);
   } else {
-    if (GetListProperty(shill::kProhibitedTechnologiesProperty)
-            ->Remove(base::Value(type), nullptr)) {
-      CallNotifyObserversPropertyChanged(
-          shill::kProhibitedTechnologiesProperty);
-    }
+    auto iter = prohibited_set.find(type);
+    if (iter == prohibited_set.end())
+      return;
+    prohibited_set.erase(iter);
   }
+  prohibited_list =
+      std::vector<std::string>(prohibited_set.begin(), prohibited_set.end());
+  prohibited_technologies = base::JoinString(prohibited_list, ",");
+  stub_properties_.SetStringKey(shill::kProhibitedTechnologiesProperty,
+                                prohibited_technologies);
+  CallNotifyObserversPropertyChanged(shill::kProhibitedTechnologiesProperty);
 }
 
 void FakeShillManagerClient::AddGeoNetwork(

chromeos/network/shill_property_handler.cc

@@ -13,6 +13,7 @@
 #include "base/bind_helpers.h"
 #include "base/format_macros.h"
 #include "base/macros.h"
+#include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chromeos/dbus/shill/shill_device_client.h"
@@ -33,10 +34,10 @@ const size_t kMaxObserved = 100;
 
 const base::ListValue* GetListValue(const std::string& key,
                                     const base::Value& value) {
-  const base::ListValue* vlist = NULL;
+  const base::ListValue* vlist = nullptr;
   if (!value.GetAsList(&vlist)) {
     NET_LOG(ERROR) << "Error parsing key as list: " << key;
-    return NULL;
+    return nullptr;
   }
   return vlist;
 }
@@ -375,9 +376,9 @@ void ShillPropertyHandler::ManagerPropertyChanged(const std::string& key,
     if (vlist)
       UpdateUninitializedTechnologies(*vlist);
   } else if (key == shill::kProhibitedTechnologiesProperty) {
-    const base::ListValue* vlist = GetListValue(key, value);
-    if (vlist)
-      UpdateProhibitedTechnologies(*vlist);
+    std::string prohibited_technologies;
+    if (value.GetAsString(&prohibited_technologies))
+      UpdateProhibitedTechnologies(prohibited_technologies);
   } else if (key == shill::kProfilesProperty) {
     listener_->ProfileListChanged();
   } else if (key == shill::kCheckPortalListProperty) {
@@ -516,11 +517,11 @@ void ShillPropertyHandler::UpdateUninitializedTechnologies(
 }
 
 void ShillPropertyHandler::UpdateProhibitedTechnologies(
-    const base::ListValue& technologies) {
-  NET_LOG(EVENT) << "ProhibitedTechnologies:" << technologies;
-  std::set<std::string> new_prohibited_technologies;
-  for (const base::Value& technology : technologies.GetList())
-    new_prohibited_technologies.insert(technology.GetString());
+    const std::string& technologies) {
+  std::vector<std::string> prohibited_list = base::SplitString(
+      technologies, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  std::set<std::string> new_prohibited_technologies(prohibited_list.begin(),
+                                                    prohibited_list.end());
   if (new_prohibited_technologies == prohibited_technologies_)
     return;
   prohibited_technologies_.swap(new_prohibited_technologies);

chromeos/network/shill_property_handler.h

@@ -191,7 +191,7 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) ShillPropertyHandler
   void UpdateAvailableTechnologies(const base::ListValue& technologies);
   void UpdateEnabledTechnologies(const base::ListValue& technologies);
   void UpdateUninitializedTechnologies(const base::ListValue& technologies);
-  void UpdateProhibitedTechnologies(const base::ListValue& technologies);
+  void UpdateProhibitedTechnologies(const std::string& technologies);
 
   void EnableTechnologyFailed(
       const std::string& technology,

chromeos/network/shill_property_handler_unittest.cc

@@ -313,6 +313,24 @@ TEST_F(ShillPropertyHandlerTest, ShillPropertyHandlerTechnologyChanged) {
   EXPECT_EQ(1, listener_->technology_list_updates());
   EXPECT_TRUE(shill_property_handler_->IsTechnologyEnabled(shill::kTypeWifi));
 
+  // Prohibit the technology.
+  listener_->reset_list_updates();
+  manager_test_->SetTechnologyProhibited(shill::kTypeWifi,
+                                         /*prohibited=*/true);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, listener_->technology_list_updates());
+  EXPECT_TRUE(
+      shill_property_handler_->IsTechnologyProhibited(shill::kTypeWifi));
+
+  // Un-prohibit the technology.
+  listener_->reset_list_updates();
+  manager_test_->SetTechnologyProhibited(shill::kTypeWifi,
+                                         /*prohibited=*/false);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(1, listener_->technology_list_updates());
+  EXPECT_FALSE(
+      shill_property_handler_->IsTechnologyProhibited(shill::kTypeWifi));
+
   EXPECT_EQ(0, listener_->errors());
 }