ONC: OpenVPN: Make Password and OTP recommended if auth type unset

In ONC, if a policy provided network configuration does not explicitly
list a property as 'Recommended', the user is not able to set the
property.

In Google's OpenVPN policy configuration (and potentially others),
UserAuthenticationType is not explicitly set, and OTP is not included
in the Recommended list, even though it needs to be provided by the
user.

To work around this technically incorrect policy configuration (and not
break existing policies with the new UI), when UserAuthenticationType
is not explicitly set in the policy, this change adds Password and
OTP to the Recommended list so that they can be provided by the UI.

This also adds a test that catches the failure case and passes with
the new changes.

Bug: 817617
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: If22f34e8943a2b82cf8becc93f257413c3cdcf92
Reviewed-on: https://chromium-review.googlesource.com/961365
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543171}

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

@@ -630,9 +630,6 @@ Polymer({
       this.showTetherDialog_();
       return;
     }
-    // Clear the error state when 'Connect' is clicked to force a connect
-    // attempt instead of showing the configuration UI.
-    this.networkProperties.ErrorState = '';
     this.fire('network-connect', {networkProperties: this.networkProperties});
   },
 

chromeos/network/managed_network_configuration_handler_unittest.cc

@@ -7,6 +7,8 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/json/json_writer.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -18,9 +20,12 @@
 #include "chromeos/network/network_configuration_handler.h"
 #include "chromeos/network/network_policy_observer.h"
 #include "chromeos/network/network_profile_handler.h"
+#include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/onc/onc_signature.h"
 #include "chromeos/network/onc/onc_test_utils.h"
 #include "chromeos/network/onc/onc_utils.h"
+#include "chromeos/network/onc/onc_validator.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
@@ -32,6 +37,21 @@ namespace {
 
 constexpr char kUser1[] = "user1";
 constexpr char kUser1ProfilePath[] = "/profile/user1/shill";
+constexpr char kTestGuid1[] = "{a3860e83-f03d-4cb1-bafa-b22c9e746950}";
+
+std::string PrettyJson(const base::DictionaryValue& value) {
+  std::string pretty;
+  base::JSONWriter::WriteWithOptions(
+      value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &pretty);
+  return pretty;
+}
+
+void ErrorCallback(const std::string& error_name,
+                   std::unique_ptr<base::DictionaryValue> error_data) {
+  ADD_FAILURE() << "Unexpected error: " << error_name
+                << " with associated data: \n"
+                << PrettyJson(*error_data);
+}
 
 class TestNetworkProfileHandler : public NetworkProfileHandler {
  public:
@@ -129,28 +149,42 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
             ? onc::ReadDictionaryFromJson(onc::kEmptyUnencryptedConfiguration)
             : test_utils::ReadTestDictionary(path_to_onc);
 
-    base::ListValue network_configs;
-    {
-      const base::Value* found =
-          policy->FindKeyOfType(::onc::toplevel_config::kNetworkConfigurations,
-                                base::Value::Type::LIST);
-      if (found)
-        network_configs = base::ListValue(found->GetList());
+    base::ListValue validated_network_configs;
+    const base::Value* found_network_configs =
+        policy->FindKeyOfType(::onc::toplevel_config::kNetworkConfigurations,
+                              base::Value::Type::LIST);
+    if (found_network_configs) {
+      for (const auto& network_config : found_network_configs->GetList()) {
+        onc::Validator validator(true,   // error_on_unknown_field
+                                 true,   // error_on_wrong_recommended
+                                 false,  // error_on_missing_field
+                                 true);  // managed_onc
+        validator.SetOncSource(onc_source);
+        onc::Validator::Result validation_result;
+        std::unique_ptr<base::DictionaryValue> validated_network_config =
+            validator.ValidateAndRepairObject(
+                &onc::kNetworkConfigurationSignature, network_config,
+                &validation_result);
+        if (validation_result == onc::Validator::INVALID) {
+          ADD_FAILURE() << "Network configuration invalid.";
+          return;
+        }
+        validated_network_configs.GetList().push_back(
+            std::move(*(validated_network_config)));
+      }
     }
 
     base::DictionaryValue global_config;
-    {
-      const base::Value* found = policy->FindKeyOfType(
-          ::onc::toplevel_config::kGlobalNetworkConfiguration,
-          base::Value::Type::DICTIONARY);
-      if (found) {
-        global_config = std::move(*base::DictionaryValue::From(
-            base::Value::ToUniquePtrValue(found->Clone())));
-      }
+    const base::Value* found_global_config = policy->FindKeyOfType(
+        ::onc::toplevel_config::kGlobalNetworkConfiguration,
+        base::Value::Type::DICTIONARY);
+    if (found_global_config) {
+      global_config = std::move(*base::DictionaryValue::From(
+          base::Value::ToUniquePtrValue(found_global_config->Clone())));
     }
 
     managed_network_configuration_handler_->SetPolicy(
-        onc_source, userhash, network_configs, global_config);
+        onc_source, userhash, validated_network_configs, global_config);
   }
 
   void SetUpEntry(const std::string& path_to_shill_json,
@@ -168,7 +202,7 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
     managed_network_configuration_handler_.reset();
   }
 
- private:
+ protected:
   base::MessageLoop message_loop_;
 
   TestNetworkPolicyObserver policy_observer_;
@@ -240,8 +274,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, EnableManagedCredentialsVPN) {
   base::RunLoop().RunUntilIdle();
 
   const base::DictionaryValue* properties =
-      GetShillServiceClient()->GetServiceProperties(
-          "{a3860e83-f03d-4cb1-bafa-b22c9e746950}");
+      GetShillServiceClient()->GetServiceProperties(kTestGuid1);
   ASSERT_TRUE(properties);
   EXPECT_EQ(*expected_shill_properties, *properties);
 }
@@ -418,16 +451,46 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyUpdateManagedVPN) {
   InitializeStandardProfiles();
   SetUpEntry("policy/shill_managed_vpn.json", kUser1ProfilePath, "entry_path");
 
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_vpn.onc");
+  base::RunLoop().RunUntilIdle();
+
+  const base::DictionaryValue* properties =
+      GetShillServiceClient()->GetServiceProperties(kTestGuid1);
+  ASSERT_TRUE(properties);
   std::unique_ptr<base::DictionaryValue> expected_shill_properties =
       test_utils::ReadTestDictionary("policy/shill_policy_on_managed_vpn.json");
+  EXPECT_EQ(*expected_shill_properties, *properties);
+}
 
-  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_vpn.onc");
+TEST_F(ManagedNetworkConfigurationHandlerTest,
+       SetPolicyUpdateManagedVPNPlusUi) {
+  InitializeStandardProfiles();
+  SetUpEntry("policy/shill_managed_vpn.json", kUser1ProfilePath, "entry_path");
+
+  // Apply a policy that does not provide an authenticaiton type.
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1,
+            "policy/policy_vpn_no_auth.onc");
+  base::RunLoop().RunUntilIdle();
+
+  // Apply additional configuration (e.g. from the UI). This includes password
+  // and OTP which should be allowed when authentication type is not explicitly
+  // set. See https://crbug.com/817617 for details.
+  const NetworkState* network_state =
+      network_state_handler_->GetNetworkStateFromGuid(kTestGuid1);
+  ASSERT_TRUE(network_state);
+  std::unique_ptr<base::DictionaryValue> ui_config =
+      test_utils::ReadTestDictionary("policy/policy_vpn_ui.json");
+  managed_network_configuration_handler_->SetProperties(
+      network_state->path(), *ui_config, base::DoNothing(),
+      base::Bind(&ErrorCallback));
   base::RunLoop().RunUntilIdle();
 
   const base::DictionaryValue* properties =
-      GetShillServiceClient()->GetServiceProperties(
-          "{a3860e83-f03d-4cb1-bafa-b22c9e746950}");
+      GetShillServiceClient()->GetServiceProperties(kTestGuid1);
   ASSERT_TRUE(properties);
+  std::unique_ptr<base::DictionaryValue> expected_shill_properties =
+      test_utils::ReadTestDictionary(
+          "policy/shill_policy_on_managed_vpn_plus_ui.json");
   EXPECT_EQ(*expected_shill_properties, *properties);
 }
 
@@ -444,8 +507,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest,
   base::RunLoop().RunUntilIdle();
 
   const base::DictionaryValue* properties =
-      GetShillServiceClient()->GetServiceProperties(
-          "{a3860e83-f03d-4cb1-bafa-b22c9e746950}");
+      GetShillServiceClient()->GetServiceProperties(kTestGuid1);
   ASSERT_TRUE(properties);
   EXPECT_EQ(*expected_shill_properties, *properties);
 }

chromeos/network/network_configuration_handler_unittest.cc

@@ -58,7 +58,7 @@ void CopyServiceResult(bool* called,
   *guid_out = guid;
 }
 
-static std::string PrettyJson(const base::DictionaryValue& value) {
+std::string PrettyJson(const base::DictionaryValue& value) {
   std::string pretty;
   base::JSONWriter::WriteWithOptions(
       value, base::JSONWriter::OPTIONS_PRETTY_PRINT, &pretty);

chromeos/network/onc/onc_translator_onc_to_shill.cc

@@ -20,6 +20,7 @@
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "chromeos/network/client_cert_util.h"
+#include "chromeos/network/network_event_log.h"
 #include "chromeos/network/onc/onc_signature.h"
 #include "chromeos/network/onc/onc_translation_tables.h"
 #include "chromeos/network/onc/onc_translator.h"
@@ -175,7 +176,7 @@ void LocalTranslator::TranslateOpenVPN() {
   // identical to kPasswordAndOTP.
   if (user_auth_type.empty())
     user_auth_type = ::onc::openvpn_user_auth_type::kPasswordAndOTP;
-
+  NET_LOG(DEBUG) << "USER AUTH TYPE: " << user_auth_type;
   if (user_auth_type == ::onc::openvpn_user_auth_type::kPassword ||
       user_auth_type == ::onc::openvpn_user_auth_type::kPasswordAndOTP) {
     CopyFieldFromONCToShill(::onc::openvpn::kPassword,
@@ -394,12 +395,12 @@ void LocalTranslator::CopyFieldFromONCToShill(
     base::Value::Type expected_type =
         field_signature->value_signature->onc_type;
     if (value->type() != expected_type) {
-      LOG(ERROR) << "Found field " << onc_field_name << " of type "
-                 << value->type() << " but expected type " << expected_type;
+      NET_LOG(ERROR) << "Found field " << onc_field_name << " of type "
+                     << value->type() << " but expected type " << expected_type;
       return;
     }
   } else {
-    LOG(ERROR)
+    NET_LOG(ERROR)
         << "Attempt to translate a field that is not part of the ONC format.";
     return;
   }
@@ -433,9 +434,9 @@ void LocalTranslator::TranslateWithTableAndSet(
   // As we previously validate ONC, this case should never occur. If it still
   // occurs, we should check here. Otherwise the failure will only show up much
   // later in Shill.
-  LOG(ERROR) << "Value '" << onc_value
-             << "' cannot be translated to Shill property: "
-             << shill_property_name;
+  NET_LOG(ERROR) << "Value '" << onc_value
+                 << "' cannot be translated to Shill property: "
+                 << shill_property_name;
 }
 
 // Iterates recursively over |onc_object| and its |signature|. At each object

chromeos/network/onc/onc_validator.cc

@@ -13,6 +13,7 @@
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ -48,7 +49,7 @@ Validator::~Validator() = default;
 
 std::unique_ptr<base::DictionaryValue> Validator::ValidateAndRepairObject(
     const OncValueSignature* object_signature,
-    const base::DictionaryValue& onc_object,
+    const base::Value& onc_object,
     Result* result) {
   CHECK(object_signature);
   *result = VALID;
@@ -806,6 +807,26 @@ bool Validator::ValidateOpenVPN(base::DictionaryValue* result) {
     return false;
   }
 
+  if ((onc_source_ == ::onc::ONC_SOURCE_DEVICE_POLICY ||
+       onc_source_ == ::onc::ONC_SOURCE_USER_POLICY) &&
+      !result->FindKeyOfType(::onc::openvpn::kUserAuthenticationType,
+                             base::Value::Type::STRING)) {
+    // If kUserAuthenticationType is unspecified for a policy controlled
+    // network, allow Password and OTP to be specified by the user by adding
+    // them to the recommended list.
+    base::Value* recommended =
+        result->FindKeyOfType(::onc::kRecommended, base::Value::Type::LIST);
+    if (!recommended)
+      recommended = result->SetKey(::onc::kRecommended, base::ListValue());
+    base::Value::ListStorage& result_list = recommended->GetList();
+    base::Value password_value(::onc::openvpn::kPassword);
+    if (!base::ContainsValue(result_list, password_value))
+      result_list.push_back(std::move(password_value));
+    base::Value otp_value(::onc::openvpn::kOTP);
+    if (!base::ContainsValue(result_list, otp_value))
+      result_list.push_back(std::move(otp_value));
+  }
+
   if (result->HasKey(kServerCARefs) && result->HasKey(kServerCARef)) {
     error_or_warning_found_ = true;
     LOG(ERROR) << MessageHeader() << "At most one of " << kServerCARefs

chromeos/network/onc/onc_validator.h

@@ -81,8 +81,8 @@ class CHROMEOS_EXPORT Validator : public Mapper {
     onc_source_ = source;
   }
 
-  // Validate the given |onc_object| according to |object_signature|. The
-  // |object_signature| has to be a pointer to one of the signatures in
+  // Validate the given |onc_object| dictionary according to |object_signature|.
+  // The |object_signature| has to be a pointer to one of the signatures in
   // |onc_signature.h|. If an error is found, the function returns NULL and sets
   // |result| to INVALID. If possible (no error encountered) a DeepCopy is
   // created that contains all but the invalid fields and values and returns
@@ -96,7 +96,7 @@ class CHROMEOS_EXPORT Validator : public Mapper {
   // For details, see the class comment.
   std::unique_ptr<base::DictionaryValue> ValidateAndRepairObject(
       const OncValueSignature* object_signature,
-      const base::DictionaryValue& onc_object,
+      const base::Value& onc_object,
       Result* result);
 
  private:

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

+{
+  "NetworkConfigurations":[
+    {
+      "GUID":"{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+      "Name":"my vpn",
+      "Type":"VPN",
+      "VPN":{
+        "AutoConnect":false,
+        "Host":"vpn.my.domain.com",
+        "OpenVPN":{
+          "Port":443,
+          "Proto":"udp",
+          "Recommended": ["Username", "Password", "SaveCredentials"],
+          "SaveCredentials":false,
+        },
+        "Type":"OpenVPN"
+      }
+    }
+  ],
+  "Type":"UnencryptedConfiguration"
+}

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

+{
+  "GUID":"{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+  "Name":"my vpn",
+  "Type":"VPN",
+  "VPN":{
+    "AutoConnect":false,
+    "Host":"vpn.my.domain.com",
+    "OpenVPN":{
+      "OTP":"user OTP",
+      "Password":"user password",
+      "SaveCredentials":true,
+      "UserAuthenticationType":"PasswordAndOTP",
+      "Username":"user name"
+    },
+    "Type":"OpenVPN"
+  }
+}

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

@@ -17,7 +17,6 @@
   "SaveCredentials": false,
   "State": "idle",
   "Type": "vpn",
-  // Password is not 'Recommended' by policy, thus removed here.
   "UIData": "{\"onc_source\":\"user_policy\",\"user_settings\":{}}",
   "Visible": true,
   "WiFi.HexSSID": "7B61333836306538332D663033642D346362312D626166612D6232326339653734363935307D"

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

+{
+  "AutoConnect": false,
+  "Device": "",
+  "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+  "Name": "my vpn",
+  "Profile": "/profile/user1/shill",
+  "Provider": {
+    "Host": "vpn.my.domain.com",
+    "OpenVPN.OTP": "user OTP",
+    "OpenVPN.Password": "user password",
+    "OpenVPN.Port": "443",
+    "OpenVPN.Proto": "udp",
+    "OpenVPN.RemoteCertKU": "",
+    "OpenVPN.User": "user name",
+    "Type": "openvpn"
+  },
+  "SSID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+  "SaveCredentials": true,
+  "State": "idle",
+  "Type": "vpn",
+  "UIData": "{\"onc_source\":\"user_policy\",\"user_settings\":{\"GUID\":\"{a3860e83-f03d-4cb1-bafa-b22c9e746950}\",\"Name\":\"my vpn\",\"Type\":\"VPN\",\"VPN\":{\"AutoConnect\":false,\"Host\":\"vpn.my.domain.com\",\"OpenVPN\":{\"OTP\":\"user OTP\",\"Password\":\"FAKE_CREDENTIAL_VPaJDV9x\",\"SaveCredentials\":true,\"UserAuthenticationType\":\"PasswordAndOTP\",\"Username\":\"user name\"},\"Type\":\"OpenVPN\"}}}",
+  "Visible": true,
+  "WiFi.HexSSID": "7B61333836306538332D663033642D346362312D626166612D6232326339653734363935307D"
+}

chromeos/test/data/network/repaired_toplevel_partially_invalid.onc

@@ -15,7 +15,8 @@
           "OpenVPN": {
               "Port": 1194,
               "Username": "abc fake email def",
-              "Recommended": [ "Username", "Password" ],
+              // "UNKNOWN_FIELD" is removed and "OTP" is added (issue 817617).
+              "Recommended": [ "Username", "Password", "OTP" ],
               "ClientCertType": "Pattern",
               "ClientCertPattern": {
                   "IssuerCAPEMs": [ "-----BEGIN CERTIFICATE-----\nMIIG\n-----END CERTIFICATE-----\n" ],