Skip checking certificate properties for L2TP/IPsec VPN using pre-shared key.

chromeos/network/network_connection_handler.cc

Index: chromeos/network/network_connection_handler.cc
diff --git a/chromeos/network/network_connection_handler.cc b/chromeos/network/network_connection_handler.cc
index 20c1a5b36f168d0c6f43280320b5ed3792e5b592..e0cc7df7f901a7301996b2978be92166587d1b89 100644
--- a/chromeos/network/network_connection_handler.cc
+++ b/chromeos/network/network_connection_handler.cc
@@ -374,7 +374,7 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
   // Get VPN provider type and host (required for configuration) and ensure
   // that required VPN non-cert properties are set.
   const base::DictionaryValue* provider_properties = NULL;
-  std::string vpn_provider_type, vpn_provider_host;
+  std::string vpn_provider_type, vpn_provider_host, vpn_client_cert_id;
   if (type == shill::kTypeVPN) {
     // VPN Provider values are read from the "Provider" dictionary, not the
     // "Provider.Type", etc keys (which are used only to set the values).
@@ -384,6 +384,8 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
           shill::kTypeProperty, &vpn_provider_type);
       provider_properties->GetStringWithoutPathExpansion(
           shill::kHostProperty, &vpn_provider_host);
+      provider_properties->GetStringWithoutPathExpansion(
+          shill::kL2tpIpsecClientCertIdProperty, &vpn_client_cert_id);
     }
     if (vpn_provider_type.empty() || vpn_provider_host.empty()) {
       ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
@@ -395,8 +397,16 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
   if (type == shill::kTypeVPN) {
     if (vpn_provider_type == shill::kProviderOpenVpn)
       client_cert_type = client_cert::CONFIG_TYPE_OPENVPN;
-    else
+    else if (!vpn_client_cert_id.empty()) {

[pneubeck (no reviews), 2014/02/13 09:53:07]

please ignore if my reasoning is incorrect:

this doesn't work when the cert is not set _yet_.
E.g. in case of a unresolved certificate pattern (see line 425), we would
mistakenly assume that the VPN is a PSK one.

Why not instead check for the PSK?

[Ben Chan, 2014/02/13 16:36:13]

In the UI flow, VpnConfigView::CanLogin() already uses ClientCertId to
distinguish PSK from Cert. I guess the ONC flow is kinda different.  We could
potentially rely on the onc::ipsec::kAuthenticationType in such case.  How can I
construct a partial ONC with client cert ID not configured?

+      // If |vpn_client_cert_id| is empty, it's L2TP/IPsec PSK and doesn't
+      // require a certificate.
+
+      // TODO(benchan): Modify shill to include the authentication type,
+      // pre-shared key or certificate, in the VPN provider properties, so that
+      // Chrome doesn't need to deduce the authentication type based on the
+      // kL2tpIpsecClientCertIdProperty here (and also in VPNConfigView).
       client_cert_type = client_cert::CONFIG_TYPE_IPSEC;
+    }
   } else if (type == shill::kTypeWifi && security == shill::kSecurity8021x) {
     client_cert_type = client_cert::CONFIG_TYPE_EAP;
   }
@@ -473,6 +483,13 @@ void NetworkConnectionHandler::VerifyConfiguredAndConnect(
       ErrorCallbackForPendingRequest(service_path, kErrorConfigurationRequired);
       return;
     }
+
+    // If it's L2TP/IPsec PSK, there is no properties to configure, so proceed
+    // to connect.

[pneubeck (no reviews), 2014/02/13 09:53:07]

this should be removed once the connectable property is fixed.

+    if (client_cert_type == client_cert::CONFIG_TYPE_NONE) {
+      CallShillConnect(service_path);
+      return;
+    }
   }
 
   if (!config_properties.empty()) {