Handle prohibited technologies in device policy ONC

chromeos/network/shill_property_handler.cc

Index: chromeos/network/shill_property_handler.cc
diff --git a/chromeos/network/shill_property_handler.cc b/chromeos/network/shill_property_handler.cc
index 02b620d38e6e2895d65b0cccbf3752c4a1bbd5ca..8b44ad238b5c8948847132383fca549044443f8a 100644
--- a/chromeos/network/shill_property_handler.cc
+++ b/chromeos/network/shill_property_handler.cc
@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/stl_util.h"
+#include "base/strings/string_util.h"

[stevenjb, 2015/11/11 18:07:31]

not needed?

[fqj, 2015/11/12 10:23:01]

Needed.

  // Send updated prohibited technology list to shill.
  const std::string prohibited_list =
      base::JoinString(prohibited_technologies, ",");

 #include "base/values.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_device_client.h"
@@ -16,6 +17,9 @@
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/managed_network_configuration_handler.h"
+#include "chromeos/network/network_handler.h"
+#include "chromeos/network/network_handler_callbacks.h"

[stevenjb, 2015/11/11 18:07:31]

Not needed

[fqj, 2015/11/12 10:23:01]

Done.

 #include "chromeos/network/network_state.h"
 #include "components/device_event_log/device_event_log.h"
 #include "dbus/object_path.h"
@@ -104,8 +108,7 @@ class ShillPropertyObserver : public ShillPropertyChangedObserver {
 
 ShillPropertyHandler::ShillPropertyHandler(Listener* listener)
     : listener_(listener),
-      shill_manager_(DBusThreadManager::Get()->GetShillManagerClient()) {
-}
+      shill_manager_(DBusThreadManager::Get()->GetShillManagerClient()) {}
 
 ShillPropertyHandler::~ShillPropertyHandler() {
   // Delete network service observers.
@@ -153,6 +156,14 @@ void ShillPropertyHandler::SetTechnologyEnabled(
     bool enabled,
     const network_handler::ErrorCallback& error_callback) {
   if (enabled) {
+    if (prohibited_technologies_.find(technology) !=
+        prohibited_technologies_.end()) {
+      chromeos::network_handler::RunErrorCallback(
+          error_callback, "", "prohibited_technologies",
+          "Ignored: Attempt to enable prohibited network technology " +
+              technology);

[stevenjb, 2015/11/11 18:07:31]

Align

[fqj, 2015/11/12 10:23:01]

Not modified.
Actually, git cl format made it like this....
If I aligns it, presubmit gives warning to ask me to run git cl format chromeos.
And then clang-format just make it like this again..

[stevenjb, 2015/11/12 18:20:38]

Sorry, you're right, didn't notice there was a + on the prev line.

+      return;
+    }
     enabling_technologies_.insert(technology);
     shill_manager_->EnableTechnology(
         technology, base::Bind(&base::DoNothing),
@@ -169,6 +180,43 @@ void ShillPropertyHandler::SetTechnologyEnabled(
   }
 }
 
+void ShillPropertyHandler::SetProhibitedTechnologies(
+    const std::vector<std::string>& prohibited_technologies,
+    const network_handler::ErrorCallback& error_callback) {
+  // Remove technologies from the other lists.

[stevenjb, 2015/11/11 18:07:31]

Update / move comment

[fqj, 2015/11/12 10:23:01]

Done.

+  for (const auto& technology : prohibited_technologies)
+    prohibited_technologies_.insert(technology);
+  for (auto iter_prohibited_technology = prohibited_technologies_.begin();
+       iter_prohibited_technology != prohibited_technologies_.end();) {
+    if (std::find(prohibited_technologies.begin(),
+                  prohibited_technologies.end(),
+                  *iter_prohibited_technology) == prohibited_technologies.end())
+      iter_prohibited_technology =
+          prohibited_technologies_.erase(iter_prohibited_technology);
+    else
+      iter_prohibited_technology++;

[stevenjb, 2015/11/11 18:07:31]

{}

[fqj, 2015/11/12 10:23:01]

189-197 no longer used.

+  }

[stevenjb, 2015/11/11 18:07:31]

Why not just clear prohibited_technologies_ and replace it with
prohibited_technologies?

[fqj, 2015/11/12 10:23:01]

Done.
But is it possible trying SetTechnologyEnabled being called in another thread
while manipulating prohibited_technologies_ here?

[stevenjb, 2015/11/12 18:20:38]

No. DBus calls must all be made from the UI thread, so it is important that none
of this code be called from any other thread. (There are checks in lower level
code to catch that).

+
+  for (const auto& technology : prohibited_technologies) {
+    enabling_technologies_.erase(technology);
+    enabled_technologies_.erase(technology);
+    shill_manager_->DisableTechnology(
+        technology, base::Bind(&base::DoNothing),
+        base::Bind(&network_handler::ShillErrorCallbackFunction,
+                   "DisableTechnology Failed", technology, error_callback));
+  }
+
+  // Send updated prohibited technology list to shill.
+  const std::string prohibited_list =
+      base::JoinString(prohibited_technologies, ",");
+  base::StringValue value(prohibited_list);
+  shill_manager_->SetProperty(
+      "ProhibitedTechnologies", value, base::Bind(&base::DoNothing),
+      base::Bind(&network_handler::ShillErrorCallbackFunction,
+                 "SetTechnologiesProhibited Failed", prohibited_list,
+                 error_callback));
+}
+
 void ShillPropertyHandler::SetCheckPortalList(
     const std::string& check_portal_list) {
   base::StringValue value(check_portal_list);