Show parse errors in the UI when loading ONC files.

Resubmitting reverted CL with fix
BUG=chromium-os:23757
TEST=manual
Review URL: http://codereview.chromium.org/8883046

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@114558 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 109 insertions, 37 deletions

diff --git a/chrome/browser/chromeos/cros/mock_network_library.h b/chrome/browser/chromeos/cros/mock_network_library.h
index aa0b469..293149b 100644
--- a/chrome/browser/chromeos/cros/mock_network_library.h
+++ b/chrome/browser/chromeos/cros/mock_network_library.h
@@ -155,7 +155,9 @@ class MockNetworkLibrary : public NetworkLibrary {
                                                    HardwareAddressFormat));
   MOCK_METHOD1(SetIPConfig, void(const NetworkIPConfig&));
   MOCK_METHOD0(SwitchToPreferredNetwork, void(void));
-  MOCK_METHOD2(LoadOncNetworks, bool(const std::string&, const std::string&));
+  MOCK_METHOD3(LoadOncNetworks, bool(const std::string&,
+                                     const std::string&,
+                                     std::string*));
   MOCK_METHOD2(SetActiveNetwork, bool(ConnectionType, const std::string&));
 };
 
diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index 43acb7c..d1625d1 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -1745,7 +1745,8 @@ class NetworkLibraryImplBase : public NetworkLibrary  {
   // virtual SetIPConfig implemented in derived classes.
   virtual void SwitchToPreferredNetwork() OVERRIDE;
   virtual bool LoadOncNetworks(const std::string& onc_blob,
-                               const std::string& passcode) OVERRIDE;
+                               const std::string& passcode,
+                               std::string* error) OVERRIDE;
   virtual bool SetActiveNetwork(ConnectionType type,
                                 const std::string& service_path) OVERRIDE;
 
@@ -2834,14 +2835,23 @@ void NetworkLibraryImplBase::SwitchToPreferredNetwork() {
 }
 
 bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
-                                             const std::string& passcode) {
+                                             const std::string& passcode,
+                                             std::string* error) {
   // TODO(gspencer): Add support for decrypting onc files. crbug.com/19397
   OncNetworkParser parser(onc_blob);
 
+  if (!parser.parse_error().empty()) {
+    if (error)
+      *error = parser.parse_error();
+    return false;
+  }
+
   for (int i = 0; i < parser.GetCertificatesSize(); i++) {
     // Insert each of the available certs into the certificate DB.
     if (parser.ParseCertificate(i).get() == NULL) {
       DLOG(WARNING) << "Cannot parse certificate in ONC file";
+      if (error)
+        *error = parser.parse_error();
       return false;
     }
   }
@@ -2851,6 +2861,8 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
     scoped_ptr<Network> network(parser.ParseNetwork(i));
     if (!network.get()) {
       DLOG(WARNING) << "Cannot parse network in ONC file";
+      if (error)
+        *error = parser.parse_error();
       return false;
     }
 
@@ -2868,8 +2880,15 @@ bool NetworkLibraryImplBase::LoadOncNetworks(const std::string& onc_blob,
 
     CallConfigureService(network->unique_id(), &dict);
   }
-  return (parser.GetNetworkConfigsSize() != 0 ||
-          parser.GetCertificatesSize() != 0);
+
+  if (parser.GetNetworkConfigsSize() != 0 ||
+      parser.GetCertificatesSize() != 0) {
+      if (error)
+        *error = l10n_util::GetStringUTF8(
+            IDS_NETWORK_CONFIG_ERROR_NETWORK_IMPORT);
+      return false;
+  }
+  return true;
 }
 
 ////////////////////////////////////////////////////////////////////////////
@@ -5102,7 +5121,7 @@ void NetworkLibraryImplStub::Init() {
         "  ],"
         "  \"Certificates\": []"
         "}");
-  LoadOncNetworks(test_blob, "");
+  LoadOncNetworks(test_blob, "", NULL);
 }
 
 ////////////////////////////////////////////////////////////////////////////
diff --git a/chrome/browser/chromeos/cros/network_library.h b/chrome/browser/chromeos/cros/network_library.h
index b27f4db..5fe3a90 100644
--- a/chrome/browser/chromeos/cros/network_library.h
+++ b/chrome/browser/chromeos/cros/network_library.h
@@ -1789,8 +1789,11 @@ class NetworkLibrary {
   virtual void SwitchToPreferredNetwork() = 0;
 
   // Load networks from an Open Network Configuration blob.
+  // If there was an error, this will return false and |error| will be set to
+  // the error message.
   virtual bool LoadOncNetworks(const std::string& onc_blob,
-                               const std::string& passcode) = 0;
+                               const std::string& passcode,
+                               std::string* error) = 0;
 
   // This sets the active network for the network type. Note: priority order
   // is unchanged (i.e. if a wifi network is set to active, but an ethernet
diff --git a/chrome/browser/chromeos/cros/onc_network_parser.cc b/chrome/browser/chromeos/cros/onc_network_parser.cc
index 670d08a..eedf109 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser.cc
@@ -16,11 +16,13 @@
 #include "chrome/browser/chromeos/cros/native_network_parser.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/common/net/x509_certificate_model.h"
+#include "grit/generated_resources.h"
 #include "net/base/cert_database.h"
 #include "net/base/crypto_module.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_certificate.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
+#include "ui/base/l10n/l10n_util.h"
 
 namespace chromeos {
 
@@ -235,10 +237,13 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
     int cert_index) {
   CHECK(certificates_);
   CHECK(static_cast<size_t>(cert_index) < certificates_->GetSize());
-  CHECK(cert_index >= 0);
+  CHECK_GE(cert_index, 0);
   base::DictionaryValue* certificate = NULL;
-  certificates_->GetDictionary(cert_index, &certificate);
-  CHECK(certificate);
+  if (!certificates_->GetDictionary(cert_index, &certificate)) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
+    return NULL;
+  }
 
   if (VLOG_IS_ON(2)) {
     std::string certificate_json;
@@ -254,6 +259,8 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
   if (!certificate->GetString("GUID", &guid) || guid.empty()) {
     LOG(WARNING) << "ONC File: certificate missing identifier at index"
                  << cert_index;
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_GUID_MISSING);
     return NULL;
   }
 
@@ -262,9 +269,10 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
 
   net::CertDatabase cert_database;
   if (remove) {
-    bool success = DeleteCertAndKeyByNickname(guid);
-    DCHECK(success);
-    // TODO(gspencer): return removed certificate?
+    if (!DeleteCertAndKeyByNickname(guid)) {
+      parse_error_ = l10n_util::GetStringUTF8(
+          IDS_NETWORK_CONFIG_ERROR_CERT_DELETE);
+    }
     return NULL;
   }
 
@@ -280,15 +288,22 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseCertificate(
 
   LOG(WARNING) << "ONC File: certificate of unknown type: " << cert_type
                << " at index " << cert_index;
+  parse_error_ = l10n_util::GetStringUTF8(
+      IDS_NETWORK_CONFIG_ERROR_CERT_TYPE_MISSING);
   return NULL;
 }
 
 Network* OncNetworkParser::ParseNetwork(int n) {
-  if (!network_configs_)
-    return NULL;
+  CHECK(network_configs_);
+  CHECK(static_cast<size_t>(n) < network_configs_->GetSize());
+  CHECK_GE(n, 0);
   DictionaryValue* info = NULL;
-  if (!network_configs_->GetDictionary(n, &info))
+  if (!network_configs_->GetDictionary(n, &info)) {
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
     return NULL;
+  }
+
   if (VLOG_IS_ON(2)) {
     std::string network_json;
     base::JSONWriter::Write(static_cast<base::Value*>(info),
@@ -304,8 +319,11 @@ Network* OncNetworkParser::CreateNetworkFromInfo(
     const std::string& service_path,
     const DictionaryValue& info) {
   ConnectionType type = ParseTypeFromDictionary(info);
-  if (type == TYPE_UNKNOWN)  // Return NULL if cannot parse network type.
+  if (type == TYPE_UNKNOWN) {  // Return NULL if cannot parse network type.
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_NETWORK_TYPE_MISSING);
     return NULL;
+  }
   scoped_ptr<Network> network(CreateNewNetwork(type, service_path));
   if (!ParseNestedObject(network.get(),
                          "NetworkConfiguration",
@@ -313,6 +331,9 @@ Network* OncNetworkParser::CreateNetworkFromInfo(
                          network_configuration_signature,
                          ParseNetworkConfigurationValue)) {
     LOG(WARNING) << "Network " << network->name() << " failed to parse.";
+    if (parse_error_.empty())
+      parse_error_ = l10n_util::GetStringUTF8(
+          IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
     return NULL;
   }
   if (VLOG_IS_ON(2)) {
@@ -373,6 +394,8 @@ OncNetworkParser::ParseServerOrCaCertificate(
       if (!trust_list->GetString(i, &trust_type)) {
         LOG(WARNING) << "ONC File: certificate trust is invalid at index "
                      << cert_index;
+        parse_error_ = l10n_util::GetStringUTF8(
+            IDS_NETWORK_CONFIG_ERROR_CERT_TRUST_INVALID);
         return NULL;
       }
       if (trust_type == "Web") {
@@ -381,6 +404,8 @@ OncNetworkParser::ParseServerOrCaCertificate(
         LOG(WARNING) << "ONC File: certificate contains unknown "
                      << "trust type: " << trust_type
                      << " at index " << cert_index;
+        parse_error_ = l10n_util::GetStringUTF8(
+            IDS_NETWORK_CONFIG_ERROR_CERT_TRUST_UNKNOWN);
         return NULL;
       }
     }
@@ -391,6 +416,8 @@ OncNetworkParser::ParseServerOrCaCertificate(
     LOG(WARNING) << "ONC File: certificate missing appropriate "
                  << "certificate data for type: " << cert_type
                  << " at index " << cert_index;
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MISSING);
     return NULL;
   }
 
@@ -398,6 +425,8 @@ OncNetworkParser::ParseServerOrCaCertificate(
   if (!base::Base64Decode(x509_data, &decoded_x509)) {
     LOG(WARNING) << "Unable to base64 decode X509 data: \""
                  << x509_data << "\".";
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
     return NULL;
   }
 
@@ -408,6 +437,8 @@ OncNetworkParser::ParseServerOrCaCertificate(
           guid.c_str());
   if (!x509_cert.get()) {
     LOG(WARNING) << "Unable to create X509 certificate from bytes.";
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
     return NULL;
   }
 
@@ -417,7 +448,7 @@ OncNetworkParser::ParseServerOrCaCertificate(
   bool success = false;
   if (cert_type == "Server") {
     success = cert_database.ImportServerCert(cert_list, &failures);
-  } else { // Authority cert
+  } else {  // Authority cert
     net::CertDatabase::TrustBits trust = web_trust ?
                                          net::CertDatabase::TRUSTED_SSL :
                                          net::CertDatabase::UNTRUSTED;
@@ -428,11 +459,15 @@ OncNetworkParser::ParseServerOrCaCertificate(
                  << net::ErrorToString(failures[0].net_error)
                  << ") importing " << cert_type << " certificate at index "
                  << cert_index;
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_IMPORT);
     return NULL;
   }
   if (!success) {
     LOG(WARNING) << "ONC File: Unknown error importing " << cert_type
                  << " certificate at index " << cert_index;
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_UNKNOWN);
     return NULL;
   }
   VLOG(2) << "Successfully imported server/ca certificate at index "
@@ -451,6 +486,8 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
       pkcs12_data.empty()) {
     LOG(WARNING) << "ONC File: PKCS12 data is missing for Client "
                  << "certificate at index " << cert_index;
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MISSING);
     return NULL;
   }
 
@@ -458,6 +495,8 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
   if (!base::Base64Decode(pkcs12_data, &decoded_pkcs12)) {
     LOG(WARNING) << "Unable to base64 decode PKCS#12 data: \""
                  << pkcs12_data << "\".";
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_DATA_MALFORMED);
     return NULL;
   }
 
@@ -471,6 +510,8 @@ scoped_refptr<net::X509Certificate> OncNetworkParser::ParseClientCertificate(
     LOG(WARNING) << "ONC File: Unable to import Client certificate at index "
                  << cert_index
                  << " (error " << net::ErrorToString(result) << ").";
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_CERT_IMPORT);
     return NULL;
   }
 
@@ -515,6 +556,8 @@ bool OncNetworkParser::ParseNestedObject(Network* network,
   bool any_errors = false;
   if (!value.IsType(base::Value::TYPE_DICTIONARY)) {
     VLOG(1) << network->name() << ": expected object of type " << onc_type;
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
     return false;
   }
   VLOG(2) << "Parsing nested object of type " << onc_type;
@@ -560,6 +603,9 @@ bool OncNetworkParser::ParseNestedObject(Network* network,
               << "(" << index << ")] = " << value_json;
     }
   }
+  if (any_errors)
+    parse_error_ = l10n_util::GetStringUTF8(
+        IDS_NETWORK_CONFIG_ERROR_NETWORK_PROP_DICT_MALFORMED);
   return !any_errors;
 }
 
diff --git a/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc b/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
index ec5907a..9b3ece0 100644
--- a/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
+++ b/chrome/browser/chromeos/cros/onc_network_parser_unittest.cc
@@ -263,7 +263,6 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifi1) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  EXPECT_FALSE(parser.ParseNetwork(1));
   scoped_ptr<Network> network(parser.ParseNetwork(0));
   ASSERT_TRUE(network.get());
 
@@ -299,7 +298,6 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifiEAP1) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  EXPECT_FALSE(parser.ParseNetwork(1));
   scoped_ptr<Network> network(parser.ParseNetwork(0));
   ASSERT_TRUE(network.get());
 
@@ -339,7 +337,6 @@ TEST_F(OncNetworkParserTest, TestCreateNetworkWifiEAP2) {
 
   EXPECT_EQ(1, parser.GetNetworkConfigsSize());
   EXPECT_EQ(0, parser.GetCertificatesSize());
-  EXPECT_FALSE(parser.ParseNetwork(1));
   scoped_ptr<Network> network(parser.ParseNetwork(0));
   ASSERT_TRUE(network.get());
 
diff --git a/chrome/browser/policy/network_configuration_updater.cc b/chrome/browser/policy/network_configuration_updater.cc
index 788a010..a1597ca 100644
--- a/chrome/browser/policy/network_configuration_updater.cc
+++ b/chrome/browser/policy/network_configuration_updater.cc
@@ -56,8 +56,11 @@ void NetworkConfigurationUpdater::ApplyNetworkConfiguration(
 
   if (*cached_value != new_network_config) {
     *cached_value = new_network_config;
-    if (!network_library_->LoadOncNetworks(new_network_config, ""))
-      LOG(WARNING) << "Network library failed to load ONC configuration.";
+    std::string error;
+    if (!network_library_->LoadOncNetworks(new_network_config, "", &error)) {
+      LOG(WARNING) << "Network library failed to load ONC configuration:"
+          << error;
+    }
   }
 }
 
diff --git a/chrome/browser/policy/network_configuration_updater_unittest.cc b/chrome/browser/policy/network_configuration_updater_unittest.cc
index d1e71a2..82db2e9 100644
--- a/chrome/browser/policy/network_configuration_updater_unittest.cc
+++ b/chrome/browser/policy/network_configuration_updater_unittest.cc
@@ -11,6 +11,7 @@
 
 using testing::Mock;
 using testing::Return;
+using testing::_;
 
 namespace policy {
 
@@ -26,7 +27,7 @@ class NetworkConfigurationUpdaterTest
 TEST_P(NetworkConfigurationUpdaterTest, InitialUpdate) {
   provider_.AddPolicy(GetParam(), Value::CreateStringValue(kFakeONC));
 
-  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, ""))
+  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, "", _))
       .WillRepeatedly(Return(true));
 
   NetworkConfigurationUpdater updater(&provider_, &network_library_);
@@ -37,20 +38,20 @@ TEST_P(NetworkConfigurationUpdaterTest, PolicyChange) {
   NetworkConfigurationUpdater updater(&provider_, &network_library_);
 
   // We should update if policy changes.
-  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, ""))
+  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, "", _))
       .WillOnce(Return(true));
   provider_.AddPolicy(GetParam(), Value::CreateStringValue(kFakeONC));
   provider_.NotifyPolicyUpdated();
   Mock::VerifyAndClearExpectations(&network_library_);
 
   // No update if the set the same value again.
-  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, ""))
+  EXPECT_CALL(network_library_, LoadOncNetworks(kFakeONC, "", _))
       .Times(0);
   provider_.NotifyPolicyUpdated();
   Mock::VerifyAndClearExpectations(&network_library_);
 
   // Another update is expected if the policy goes away.
-  EXPECT_CALL(network_library_, LoadOncNetworks("", ""))
+  EXPECT_CALL(network_library_, LoadOncNetworks("", "", _))
       .WillOnce(Return(true));
   provider_.RemovePolicy(GetParam());
   provider_.NotifyPolicyUpdated();
diff --git a/chrome/browser/resources/net_internals/browser_bridge.js b/chrome/browser/resources/net_internals/browser_bridge.js
index 6a2d308..032b2e1 100644
--- a/chrome/browser/resources/net_internals/browser_bridge.js
+++ b/chrome/browser/resources/net_internals/browser_bridge.js
@@ -316,9 +316,9 @@ var BrowserBridge = (function() {
         this.hstsObservers_[i].onHSTSQueryResult(info);
     },
 
-    receivedONCFileParse: function(status) {
+    receivedONCFileParse: function(error) {
       for (var i = 0; i < this.crosONCFileParseObservers_.length; i++)
-        this.crosONCFileParseObservers_[i].onONCFileParse(status);
+        this.crosONCFileParseObservers_[i].onONCFileParse(error);
     },
 
     receivedHttpCacheInfo: function(info) {
@@ -498,7 +498,7 @@ var BrowserBridge = (function() {
      * Adds a listener for ONC file parse status. The observer will be called
      * back with:
      *
-     *   observer.onONCFileParse(status);
+     *   observer.onONCFileParse(error);
      */
     addCrosONCFileParseObserver: function(observer) {
       this.crosONCFileParseObservers_.push(observer);
diff --git a/chrome/browser/resources/net_internals/chromeos_view.js b/chrome/browser/resources/net_internals/chromeos_view.js
index b560d82..a3d2a35 100644
--- a/chrome/browser/resources/net_internals/chromeos_view.js
+++ b/chrome/browser/resources/net_internals/chromeos_view.js
@@ -20,7 +20,7 @@ var CrosView = (function() {
     if (fileContent)
       g_browser.importONCFile(fileContent, passcode);
     else
-      setParseStatus_(false);
+      setParseStatus_('ONC file parse failed: cannot read file');
   }
 
   /**
@@ -69,11 +69,11 @@ var CrosView = (function() {
    *
    *  @private
    */
-  function setParseStatus_(success) {
+  function setParseStatus_(error) {
     var parseStatus = $(CrosView.PARSE_STATUS_ID);
     parseStatus.hidden = false;
-    parseStatus.textContent = success ?
-        "ONC file successfully parsed" : "ONC file parse failed";
+    parseStatus.textContent = error ?
+        'ONC file parse failed: ' + error : 'ONC file successfully parsed';
     reset_();
   }
 
diff --git a/chrome/browser/ui/webui/net_internals_ui.cc b/chrome/browser/ui/webui/net_internals_ui.cc
index 8660432..09851c0 100644
--- a/chrome/browser/ui/webui/net_internals_ui.cc
+++ b/chrome/browser/ui/webui/net_internals_ui.cc
@@ -1295,10 +1295,11 @@ void NetInternalsMessageHandler::OnImportONCFile(const ListValue* list) {
     NOTREACHED();
   }
 
-  const bool success = chromeos::CrosLibrary::Get()->GetNetworkLibrary()->
-      LoadOncNetworks(onc_blob, passcode);
+  std::string error;
+  chromeos::CrosLibrary::Get()->GetNetworkLibrary()->
+      LoadOncNetworks(onc_blob, passcode, &error);
   SendJavascriptCommand("receivedONCFileParse",
-                        Value::CreateBooleanValue(success));
+                        Value::CreateStringValue(error));
 }
 #endif