Fix IPsec configuration update using UI

Currently on network configuration updates through UI, there are some
empty fields populated and not omitted before being validated by the ONC
validator. One of these fields is the client certificate type. This CL
ignores this field if it is empty and adds tests with client certificate
type set to empty to make sure that the ONC validator will not complain
and that the VPN configurations will get updated as expected.

Bug: chromium:1043126
Test: chromeos_unittests -gtest_filter=ManagedNetworkConfigurationHandlerTest.*
Change-Id: Ic84e4250e2c01744c6d4632108427d2478f64e16
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2035974
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: Omar Morsi <omorsi@google.com>
Cr-Commit-Position: refs/heads/master@{#740593}

chromeos/network/managed_network_configuration_handler_unittest.cc

@@ -520,7 +520,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyUpdateManagedVPN) {
 }
 
 TEST_F(ManagedNetworkConfigurationHandlerTest,
-       SetPolicyUpdateManagedVPNPlusUi) {
+       SetPolicyUpdateManagedVPNOpenVPNPlusUi) {
   InitializeStandardProfiles();
   SetUpEntry("policy/shill_managed_vpn.json", kUser1ProfilePath, "entry_path");
 
@@ -554,6 +554,42 @@ TEST_F(ManagedNetworkConfigurationHandlerTest,
   EXPECT_EQ(*expected_shill_properties, *properties);
 }
 
+TEST_F(ManagedNetworkConfigurationHandlerTest,
+       SetPolicyUpdateManagedVPNL2TPIPsecPlusUi) {
+  InitializeStandardProfiles();
+  SetUpEntry("policy/shill_managed_vpn_ipsec.json", kUser1ProfilePath,
+             "entry_path");
+
+  // Apply the VPN L2TP-IPsec policy that will be updated.
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1,
+            "policy/policy_vpn_ipsec.onc");
+  base::RunLoop().RunUntilIdle();
+
+  // Update the VPN L2TP-IPsec policy.
+  const NetworkState* network_state =
+      network_state_handler_->GetNetworkStateFromGuid(kTestGuidVpn);
+  ASSERT_TRUE(network_state);
+  std::unique_ptr<base::DictionaryValue> ui_config =
+      test_utils::ReadTestDictionary("policy/policy_vpn_ipsec_ui.json");
+  managed_network_configuration_handler_->SetProperties(
+      network_state->path(), *ui_config, base::DoNothing(),
+      base::Bind(&ErrorCallback));
+  base::RunLoop().RunUntilIdle();
+
+  // Get shill service properties after the update.
+  std::string service_path =
+      GetShillServiceClient()->FindServiceMatchingGUID(kTestGuidVpn);
+  ASSERT_FALSE(service_path.empty());
+  const base::DictionaryValue* properties =
+      GetShillServiceClient()->GetServiceProperties(service_path);
+  ASSERT_TRUE(properties);
+
+  std::unique_ptr<base::DictionaryValue> expected_shill_properties =
+      test_utils::ReadTestDictionary(
+          "policy/shill_policy_on_managed_vpn_ipsec_plus_ui.json");
+  EXPECT_EQ(*expected_shill_properties, *properties);
+}
+
 TEST_F(ManagedNetworkConfigurationHandlerTest,
        SetPolicyUpdateManagedVPNNoUserAuthType) {
   InitializeStandardProfiles();

chromeos/network/onc/onc_validator.cc

@@ -334,12 +334,18 @@ bool Validator::ValidateClientCertFields(bool allow_cert_type_none,
                                                ::onc::client_cert::kPKCS11Id};
   if (allow_cert_type_none)
     valid_cert_types.push_back(::onc::client_cert::kClientCertTypeNone);
-  if (FieldExistsAndHasNoValidValue(
-          *result, ::onc::client_cert::kClientCertType, valid_cert_types))
-    return false;
 
   std::string cert_type =
       GetStringFromDict(*result, ::onc::client_cert::kClientCertType);
+
+  // TODO(https://crbug.com/1049955): Remove the client certificate type empty
+  // check. Ignored fields should be removed by normalizer before validating.
+  if (cert_type.empty())
+    return true;
+
+  if (!IsValidValue(cert_type, valid_cert_types))
+    return false;
+
   bool all_required_exist = true;
 
   if (cert_type == ::onc::client_cert::kPattern)
@@ -849,10 +855,8 @@ bool Validator::ValidateIPsec(base::DictionaryValue* result) {
                        ::onc::ipsec::kServerCARef))
     return false;
 
-  if (!ValidateClientCertFields(false,  // don't allow ClientCertType None
-                                result)) {
+  if (!ValidateClientCertFields(/*allow_cert_type_none=*/false, result))
     return false;
-  }
 
   bool all_required_exist =
       RequireField(*result, ::onc::ipsec::kAuthenticationType) &&

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

@@ -0,0 +1,31 @@
+{
+  "NetworkConfigurations": [
+    {
+      "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+      "Name": "my vpn",
+      "Type": "VPN",
+      "VPN": {
+        "AutoConnect": false,
+        "Host": "vpn.my.domain.com",
+        "Type": "L2TP-IPsec",
+        "IPsec": {
+          "AuthenticationType": "PSK",
+          "IKEVersion": 1,
+          "PSK": "my_psk",
+          "Recommended": [
+            "PSK"
+          ]
+        },
+        "L2TP": {
+          "Username": "my_username",
+          "Password": "my_password",
+          "Recommended": [
+            "Password"
+          ],
+          "SaveCredentials": true
+        }
+      }
+    }
+  ],
+  "Type": "UnencryptedConfiguration"
+}

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

@@ -0,0 +1,20 @@
+{
+  "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+  "Name": "my vpn",
+  "Type": "VPN",
+  "VPN": {
+    "AutoConnect": false,
+    "Host": "vpn.my.domain.com",
+    "Type": "L2TP-IPsec",
+    "IPsec": {
+      "AuthenticationType": "PSK",
+      "PSK": "my_updated_psk",
+      // ClientCertType Should be ignored if the AuthenticationType is PSK
+      // (see onc_spec.md).
+      "ClientCertType": ""
+    },
+    "L2TP": {
+      "Password": "my_updated_password"
+    }
+  }
+}

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

@@ -0,0 +1,11 @@
+{
+  "AutoConnect": false,
+  "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+  "Name": "my vpn",
+  "Provider": {
+    "Host": "vpn.my.domain.com",
+    "Type": "l2tpipsec"
+  },
+  "UIData": "{\"onc_source\":\"user_policy\",\"user_settings\":{}}",
+  "Type": "vpn"
+}

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

@@ -0,0 +1,22 @@
+{
+   "AutoConnect": false,
+   "Device": "",
+   "GUID": "{a3860e83-f03d-4cb1-bafa-b22c9e746950}",
+   "Name": "my vpn",
+   "Profile": "/profile/user1/shill",
+   "Provider": {
+      "Host": "vpn.my.domain.com",
+      "L2TPIPsec.ClientCertID": "",
+      "L2TPIPsec.ClientCertSlot": "",
+      "L2TPIPsec.PIN": "",
+      "L2TPIPsec.PSK": "my_updated_psk",
+      "L2TPIPsec.Password": "my_updated_password",
+      "L2TPIPsec.User": "my_username",
+      "Type": "l2tpipsec"
+   },
+   "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\",\"IPsec\":{\"AuthenticationType\":\"PSK\",\"ClientCertType\":\"\",\"PSK\":\"FAKE_CREDENTIAL_VPaJDV9x\"},\"L2TP\":{\"Password\":\"FAKE_CREDENTIAL_VPaJDV9x\"},\"Type\":\"L2TP-IPsec\"}}}",
+   "Visible": true
+}