Handle prohibited technologies in device policy ONC

chromeos/network/prohibited_technologies_handler.cc

Index: chromeos/network/prohibited_technologies_handler.cc
diff --git a/chromeos/network/prohibited_technologies_handler.cc b/chromeos/network/prohibited_technologies_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d248197e463af1627eefd8820e4b3eece37ce26f
--- /dev/null
+++ b/chromeos/network/prohibited_technologies_handler.cc
@@ -0,0 +1,112 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/network/prohibited_technologies_handler.h"
+
+#include "chromeos/network/managed_network_configuration_handler.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_util.h"
+#include "third_party/cros_system_api/dbus/service_constants.h"
+
+namespace chromeos {
+
+ProhibitedTechnologiesHandler::ProhibitedTechnologiesHandler(){};

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

' ' before {} (and generally run 'git cl format')

[fqj, 2015/11/13 13:53:49]

Done.

+
+ProhibitedTechnologiesHandler::~ProhibitedTechnologiesHandler() {
+  if (managed_network_configuration_handler_)
+    managed_network_configuration_handler_->RemoveObserver(this);
+  if (LoginState::IsInitialized())
+    LoginState::Get()->RemoveObserver(this);
+}
+
+void ProhibitedTechnologiesHandler::Init(
+    ManagedNetworkConfigurationHandler* managed_network_configuration_handler,
+    NetworkStateHandler* network_state_handler) {
+  if (LoginState::IsInitialized())
+    LoginState::Get()->AddObserver(this);
+
+  managed_network_configuration_handler_ =
+      managed_network_configuration_handler;
+  if (managed_network_configuration_handler_)
+    managed_network_configuration_handler_->AddObserver(this);
+  network_state_handler_ = network_state_handler;
+
+  // Clear the list of prohibited network technologies. As a user logout always
+  // triggers a browser process restart, Init() is always invoked to reallow any
+  // network technology forbidden for the previous user.
+  network_state_handler_->SetProhibitedTechnologies(
+      std::vector<std::string>(), chromeos::network_handler::ErrorCallback());
+
+  if (LoginState::IsInitialized())
+    LoggedInStateChanged();
+}
+
+void ProhibitedTechnologiesHandler::LoggedInStateChanged() {
+  user_logged_in_ = LoginState::Get()->IsUserLoggedIn();
+  EnforceProhibitedTechnologies();
+}
+
+void ProhibitedTechnologiesHandler::PoliciesChanged(
+    const std::string& userhash) {}
+
+void ProhibitedTechnologiesHandler::PoliciesApplied(
+    const std::string& userhash) {
+  if (userhash.empty())
+    return;
+  user_policy_applied_ = true;
+  EnforceProhibitedTechnologies();
+}
+
+void ProhibitedTechnologiesHandler::SetProhibitedTechnologies(
+    const base::ListValue* prohibited_list) {
+  // Build up prohibited network type list and saves it for furthur use when

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

s/saves/save/

[fqj, 2015/11/13 13:53:49]

Done.

+  // enforced
+  prohibited_technologies_.clear();
+  for (const base::Value* item : *prohibited_list) {
+    std::string prohibited_technology;
+    item->GetAsString(&prohibited_technology);
+    std::string translated_tech =
+        network_util::TranslateONCTypeToShill(prohibited_technology);
+    if (!translated_tech.empty())
+      prohibited_technologies_.push_back(translated_tech);
+  }
+  EnforceProhibitedTechnologies();
+}
+
+void ProhibitedTechnologiesHandler::EnforceProhibitedTechnologies() {
+  bool enable_ethernet = true;
+  if (user_logged_in_ && user_policy_applied_) {
+    network_state_handler_->SetProhibitedTechnologies(
+        prohibited_technologies_, network_handler::ErrorCallback());
+    if (std::find(prohibited_technologies_.begin(),
+                  prohibited_technologies_.end(),
+                  shill::kTypeEthernet) != prohibited_technologies_.end())
+      enable_ethernet = false;

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

Why not just return here? Then you can remove the 'else' also.

[fqj, 2015/11/13 13:53:49]

Done with "return"

I don't think else can be removed;
The return only returns on Ethernet is prohibited (no need to enable ethernet
back)
but else here means no technologies are prohibited.

[stevenjb, 2015/11/13 18:26:56]

Right again, sorry, missed the nested if. Too many code reviews of late.

+  } else {

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

nit: Add a comment that this is done to ensure that prohibited technologies are
cleared before the user policy is applied. It would be nice to only do this when
necessary (.e.g in Init()), but I geuss there is no harm in doing this more
frequently.

[fqj, 2015/11/13 13:53:49]

Done.

+    network_state_handler_->SetProhibitedTechnologies(
+        std::vector<std::string>(), network_handler::ErrorCallback());
+  }
+
+  // Enable ethernet back as user doesn't have a place to enable it back
+  // if user shuts down directly in a user session. As shill will persist
+  // ProhibitedTechnologies which may include ethernet, making users can
+  // not find Ethernet at next boot or logging out unless user log out first
+  // and then shutdown.
+  if (enable_ethernet &&
+      network_state_handler_->IsTechnologyAvailable(
+          NetworkTypePattern::Ethernet()) &&
+      !network_state_handler_->IsTechnologyEnabled(
+          NetworkTypePattern::Ethernet()))
+    network_state_handler_->SetTechnologyEnabled(
+        NetworkTypePattern::Ethernet(), true, network_handler::ErrorCallback());
+}
+
+std::vector<std::string>
+ProhibitedTechnologiesHandler::GetCurrentlyProhibitedTechnologies() {
+  if (user_logged_in_ && user_policy_applied_)
+    return prohibited_technologies_;
+  return std::vector<std::string>();
+}
+
+}  // namespace chromeos