CrosNetworkConfig: Change VPN device state return conditions

Before this CL, a VPN device state was always being returned from
CrosNetworkConfig::GetDeviceStateList even if no VPN services exist. The
main reason behind that was to use this state to propagate the VPN
disablement state to the UI code.

This CL changes this behavior such that a VPN device state is returned
if a VPN service exists or the VPN is prohibited by policy.

Bug: 1156816
Change-Id: Ie5c894183d205a92a0bedb890bfcd69daeff3e7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2615038Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#841417}

ash/system/network/tray_network_state_model.cc

@@ -174,10 +174,10 @@ void TrayNetworkStateModel::SetNetworkTypeEnabledState(NetworkType type,
   impl_->SetNetworkTypeEnabledState(type, enabled);
 }
 
-bool TrayNetworkStateModel::IsBuiltinVpnEnabled() const {
+bool TrayNetworkStateModel::IsBuiltinVpnProhibited() const {
   return TrayNetworkStateModel::GetDeviceState(
              chromeos::network_config::mojom::NetworkType::kVPN) ==
-         chromeos::network_config::mojom::DeviceStateType::kEnabled;
+         chromeos::network_config::mojom::DeviceStateType::kProhibited;
 }
 
 chromeos::network_config::mojom::CrosNetworkConfig*

ash/system/network/tray_network_state_model.h

@@ -46,9 +46,9 @@ class ASH_EXPORT TrayNetworkStateModel {
       chromeos::network_config::mojom::NetworkType type,
       bool enabled);
 
-  // Returns true if built-in VPN is enabled.
-  // Note: Currently only built-in VPNs can be disabled by policy.
-  bool IsBuiltinVpnEnabled() const;
+  // Returns true if built-in VPN is prohibited.
+  // Note: Currently only built-in VPNs can be prohibited by policy.
+  bool IsBuiltinVpnProhibited() const;
 
   // This used to be inlined but now requires details from the Impl class.
   chromeos::network_config::mojom::CrosNetworkConfig* cros_network_config();

ash/system/network/vpn_feature_pod_controller.cc

@@ -36,7 +36,7 @@ bool IsVPNVisibleInSystemTray() {
     return true;
 
   // Note: At this point, only built-in VPNs are considered.
-  return model->IsBuiltinVpnEnabled() && model->has_vpn();
+  return !model->IsBuiltinVpnProhibited() && model->has_vpn();
 }
 
 }  // namespace

ash/system/network/vpn_list_view.cc

@@ -440,8 +440,8 @@ void VPNListView::AddProviderAndNetworks(VpnProviderPtr vpn_provider,
   views::View* provider_view = nullptr;
 
   // Note: Currently only built-in VPNs can be disabled by policy.
-  bool vpn_enabled =
-      vpn_provider->type != VpnType::kOpenVPN || model()->IsBuiltinVpnEnabled();
+  bool vpn_enabled = vpn_provider->type != VpnType::kOpenVPN ||
+                     !model()->IsBuiltinVpnProhibited();
 
   provider_view =
       new VPNListProviderEntry(vpn_provider, list_empty_, vpn_name, vpn_enabled,

chrome/browser/chromeos/net/network_health/network_health_unittest.cc

@@ -60,20 +60,14 @@ class NetworkHealthTest : public ::testing::Test {
     const auto& initial_network_health_state =
         network_health_.GetNetworkHealthState();
 
-    ASSERT_EQ(std::size_t(2), initial_network_health_state->networks.size());
-
-    // Check that VPN device state is always reported even if no VPNs exist.
-    ASSERT_EQ(network_config::mojom::NetworkType::kVPN,
-              initial_network_health_state->networks[0]->type);
-    ASSERT_EQ(network_health::mojom::NetworkState::kNotConnected,
-              initial_network_health_state->networks[0]->state);
+    ASSERT_EQ(std::size_t(1), initial_network_health_state->networks.size());
 
     // Check that the default wifi device created by CrosNetworkConfigTestHelper
     // exists.
     ASSERT_EQ(network_config::mojom::NetworkType::kWiFi,
-              initial_network_health_state->networks[1]->type);
+              initial_network_health_state->networks[0]->type);
     ASSERT_EQ(network_health::mojom::NetworkState::kNotConnected,
-              initial_network_health_state->networks[1]->state);
+              initial_network_health_state->networks[0]->state);
   }
 
   mojom::NetworkPtr GetNetworkHealthStateByType(

chrome/browser/resources/settings/chromeos/internet_page/internet_page.html

@@ -71,13 +71,13 @@
                       aria-labelledby="add-wifi-label"></cr-icon-button>
                 </div>
               </template>
-              <div actionable$="[[vpnIsEnabled_]]" class="list-item"
+              <div actionable$="[[!vpnIsProhibited_]]" class="list-item"
                   on-click="onAddVPNTap_">
                 <div class="start settings-box-text"
                     id="add-vpn-label" aria-hidden="true">
                   $i18n{internetAddVPN}
                 </div>
-                <template is="dom-if" if="[[!vpnIsEnabled_]]">
+                <template is="dom-if" if="[[vpnIsProhibited_]]">
                   <cr-policy-indicator id="vpnPolicyIndicator"
                       icon-aria-label="$i18n{networkVpnBuiltin}"
                       indicator-type="devicePolicy"
@@ -86,7 +86,7 @@
                 </template>
                 <cr-icon-button class="icon-add-circle"
                     aria-labelledby="add-vpn-label"
-                    disabled="[[!vpnIsEnabled_]]">
+                    disabled="[[vpnIsProhibited_]]">
                 </cr-icon-button>
               </div>
               <template is="dom-repeat" items="[[vpnProviders_]]">

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

@@ -81,10 +81,10 @@ Polymer({
     },
 
     /**
-     * False if VPN is disabled by policy.
+     * True if VPN is prohibited by policy.
      * @private {boolean}
      */
-    vpnIsEnabled_: {
+    vpnIsProhibited_: {
       type: Boolean,
       value: false,
     },
@@ -502,9 +502,9 @@ Polymer({
     }
 
     const vpn = this.deviceStates[mojom.NetworkType.kVPN];
-    this.vpnIsEnabled_ = !!vpn &&
+    this.vpnIsProhibited_ = !!vpn &&
         vpn.deviceState ===
-            chromeos.networkConfig.mojom.DeviceStateType.kEnabled;
+            chromeos.networkConfig.mojom.DeviceStateType.kProhibited;
 
     if (this.detailType_ && !this.deviceStates[this.detailType_]) {
       // If the device type associated with the current network has been
@@ -541,7 +541,7 @@ Polymer({
 
   /** @private */
   onAddVPNTap_() {
-    if (this.vpnIsEnabled_) {
+    if (!this.vpnIsProhibited_) {
       this.showConfig_(
           true /* configAndConnect */,
           chromeos.networkConfig.mojom.NetworkType.kVPN);

chromeos/services/network_config/cros_network_config.cc

@@ -274,21 +274,24 @@ const std::string& GetVpnProviderName(
   return base::EmptyString();
 }
 
-mojom::DeviceStatePropertiesPtr GetVpnState() {
-  auto result = mojom::DeviceStateProperties::New();
-  result->type = mojom::NetworkType::kVPN;
-
-  bool vpn_disabled = false;
+bool IsVpnProhibited() {
+  bool vpn_prohibited = false;
   if (NetworkHandler::IsInitialized()) {
     std::vector<std::string> prohibited_technologies =
         NetworkHandler::Get()
             ->prohibited_technologies_handler()
             ->GetCurrentlyProhibitedTechnologies();
-    vpn_disabled = base::Contains(prohibited_technologies, shill::kTypeVPN);
+    vpn_prohibited = base::Contains(prohibited_technologies, shill::kTypeVPN);
   }
+  return vpn_prohibited;
+}
 
-  result->device_state = vpn_disabled ? mojom::DeviceStateType::kProhibited
-                                      : mojom::DeviceStateType::kEnabled;
+mojom::DeviceStatePropertiesPtr GetVpnState() {
+  auto result = mojom::DeviceStateProperties::New();
+  result->type = mojom::NetworkType::kVPN;
+
+  result->device_state = IsVpnProhibited() ? mojom::DeviceStateType::kProhibited
+                                           : mojom::DeviceStateType::kEnabled;
   return result;
 }
 
@@ -1861,7 +1864,15 @@ void CrosNetworkConfig::GetDeviceStateList(
   // Handle VPN state separately because VPN is not considered a device by shill
   // and thus will not be included in the |devices| list returned by network
   // state handler. In the UI code, it is treated as a "device" for consistency.
-  result.emplace_back(GetVpnState());
+  // In the UI code, knowing whether a device is prohibited or not is done by
+  // checking |device_state| field of the DeviceStateProperties of the
+  // corresponding device. A VPN device state is returned if built-in VPN
+  // services are prohibited by policy even if no VPN services exist in order to
+  // indicate that adding a VPN is prohibited in the UI.
+  if (network_state_handler_->FirstNetworkByType(NetworkTypePattern::VPN()) ||
+      IsVpnProhibited()) {
+    result.emplace_back(GetVpnState());
+  }
 
   std::move(callback).Run(std::move(result));
 }

chromeos/services/network_config/cros_network_config_unittest.cc

@@ -25,6 +25,7 @@
 #include "chromeos/network/network_state_test_helper.h"
 #include "chromeos/network/network_type_pattern.h"
 #include "chromeos/network/onc/onc_utils.h"
+#include "chromeos/network/prohibited_technologies_handler.h"
 #include "chromeos/network/proxy/ui_proxy_config_service.h"
 #include "chromeos/services/network_config/public/cpp/cros_network_config_test_observer.h"
 #include "components/onc/onc_constants.h"
@@ -485,6 +486,16 @@ class CrosNetworkConfigTest : public testing::Test {
     run_loop.Run();
   }
 
+  bool ContainsVpnDeviceState(
+      std::vector<mojom::DeviceStatePropertiesPtr> devices) {
+    for (auto& device : devices) {
+      if (device->type == mojom::NetworkType::kVPN) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   NetworkStateTestHelper& helper() { return helper_; }
   CrosNetworkConfigTestObserver* observer() { return observer_.get(); }
   CrosNetworkConfig* cros_network_config() {
@@ -684,6 +695,32 @@ TEST_F(CrosNetworkConfigTest, GetDeviceStateList) {
   EXPECT_EQ(mojom::DeviceStateType::kDisabled, devices[0]->device_state);
 }
 
+// Tests that no VPN device state is returned by GetDeviceStateList if no VPN
+// services exist and built-in VPN is not prohibited.
+TEST_F(CrosNetworkConfigTest, GetDeviceStateListNoVpnServices) {
+  helper().ClearServices();
+
+  std::vector<std::string> prohibited_technologies =
+      NetworkHandler::Get()
+          ->prohibited_technologies_handler()
+          ->GetCurrentlyProhibitedTechnologies();
+  ASSERT_FALSE(base::Contains(prohibited_technologies, shill::kTypeVPN));
+
+  EXPECT_FALSE(ContainsVpnDeviceState(GetDeviceStateList()));
+}
+
+// Tests that a VPN device state is returned by GetDeviceStateList if built-in
+// VPN is not prohibited even if no VPN services exist.
+TEST_F(CrosNetworkConfigTest, GetDeviceStateListNoVpnServicesAndVpnProhibited) {
+  helper().ClearServices();
+
+  NetworkHandler::Get()
+      ->prohibited_technologies_handler()
+      ->AddGloballyProhibitedTechnology(shill::kTypeVPN);
+
+  EXPECT_TRUE(ContainsVpnDeviceState(GetDeviceStateList()));
+}
+
 // Test a sampling of properties, ensuring that string property types are
 // translated as strings and not enum values (See ManagedProperties definition
 // in cros_network_config.mojom for details).