Replace non-localized errors with message ID variants

Replace non-localized AddError(std::string) instances with the variant which accepts a message ID (and optionally placeholder arguments). Remove AddError(std::string) variants to encourage developers to define localization-supported policy strings instead of hard-coding errors.

Bug: 1153894
Change-Id: I014522a16e6bff74832c2de797763623037c7000
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2566692
Commit-Queue: Igor Ruvinov <igorruvinov@chromium.org>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Yann Dago <ydago@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833905}

chrome/browser/chromeos/policy/device_policy_decoder_chromeos.cc

@@ -71,7 +71,8 @@ void SetJsonDevicePolicy(
                 POLICY_SOURCE_CLOUD, std::move(value_to_set),
                 std::move(external_data_fetcher));
   if (!error.empty())
-    policies->AddError(policy_name, error);
+    policies->AddError(policy_name, IDS_POLICY_PROTO_PARSING_ERROR,
+                       {base::UTF8ToUTF16(error)});
 }
 
 // Returns true and sets |level| to a PolicyLevel if the policy has been set
@@ -156,9 +157,8 @@ std::unique_ptr<base::Value> DecodeConnectionType(int value) {
 void AddDeprecationWarning(const std::string& old_name,
                            const std::string& new_name,
                            PolicyMap* policies) {
-  policies->AddError(old_name,
-                     l10n_util::GetStringFUTF8(IDS_POLICY_MIGRATED_OLD_POLICY,
-                                               base::UTF8ToUTF16(new_name)));
+  policies->AddError(old_name, IDS_POLICY_MIGRATED_OLD_POLICY,
+                     {base::UTF8ToUTF16(new_name)});
 }
 
 void DecodeLoginPolicies(const em::ChromeDeviceSettingsProto& policy,

chrome/browser/chromeos/policy/device_policy_decoder_chromeos_unittest.cc

@@ -5,9 +5,11 @@
 #include "chrome/browser/chromeos/policy/device_policy_decoder_chromeos.h"
 
 #include "base/bind.h"
+#include "base/strings/utf_string_conversions.h"
 #include "components/policy/core/common/policy_bundle.h"
 #include "components/policy/policy_constants.h"
 #include "components/policy/proto/chrome_device_policy.pb.h"
+#include "components/strings/grit/components_strings.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 
@@ -73,9 +75,13 @@ TEST_F(DevicePolicyDecoderChromeOSTest,
   std::string error;
   base::Optional<base::Value> decoded_json = DecodeJsonStringAndNormalize(
       kInvalidJson, key::kDeviceWallpaperImage, &error);
+  std::string localized_error = l10n_util::GetStringFUTF8(
+      IDS_POLICY_PROTO_PARSING_ERROR, base::UTF8ToUTF16(error));
   EXPECT_FALSE(decoded_json.has_value());
-  EXPECT_NE(std::string::npos,
-            error.find("Invalid JSON string: Line: 1, column: 14"));
+  EXPECT_EQ(
+      "Policy parsing error: Invalid JSON string: Line: 1, column: 14, Syntax "
+      "error.",
+      localized_error);
 }
 
 #if GTEST_HAS_DEATH_TEST
@@ -94,10 +100,12 @@ TEST_F(DevicePolicyDecoderChromeOSTest,
   base::Optional<base::Value> decoded_json = DecodeJsonStringAndNormalize(
       kWallpaperJsonInvalidValue, key::kDeviceWallpaperImage, &error);
   EXPECT_FALSE(decoded_json.has_value());
+  std::string localized_error = l10n_util::GetStringFUTF8(
+      IDS_POLICY_PROTO_PARSING_ERROR, base::UTF8ToUTF16(error));
   EXPECT_EQ(
-      "Invalid policy value: The value type doesn't match the schema type. (at "
-      "url)",
-      error);
+      "Policy parsing error: Invalid policy value: The value type doesn't "
+      "match the schema type. (at url)",
+      localized_error);
 }
 
 TEST_F(DevicePolicyDecoderChromeOSTest,
@@ -105,11 +113,13 @@ TEST_F(DevicePolicyDecoderChromeOSTest,
   std::string error;
   base::Optional<base::Value> decoded_json = DecodeJsonStringAndNormalize(
       kWallpaperJsonUnknownProperty, key::kDeviceWallpaperImage, &error);
+  std::string localized_error = l10n_util::GetStringFUTF8(
+      IDS_POLICY_PROTO_PARSING_ERROR, base::UTF8ToUTF16(error));
   EXPECT_EQ(*GetWallpaperDict(), decoded_json.value());
   EXPECT_EQ(
-      "Dropped unknown properties: Unknown property: unknown-field (at "
-      "toplevel)",
-      error);
+      "Policy parsing error: Dropped unknown properties: Unknown property: "
+      "unknown-field (at toplevel)",
+      localized_error);
 }
 
 TEST_F(DevicePolicyDecoderChromeOSTest, DecodeJsonStringAndNormalizeSuccess) {

components/policy/core/common/policy_map.cc

@@ -23,10 +23,9 @@ namespace {
 
 const base::string16 GetLocalizedString(
     PolicyMap::Entry::L10nLookupFunction lookup,
-    const base::string16& initial_string,
     const std::map<int, base::Optional<std::vector<base::string16>>>&
         localized_string_ids) {
-  base::string16 result = initial_string;
+  base::string16 result = base::string16();
   base::string16 line_feed = base::UTF8ToUTF16("\n");
   for (const auto& string_pairs : localized_string_ids) {
     if (string_pairs.second)
@@ -70,7 +69,6 @@ PolicyMap::Entry PolicyMap::Entry::DeepCopy() const {
                  ? std::make_unique<ExternalDataFetcher>(*external_data_fetcher)
                  : nullptr);
   copy.ignored_ = ignored_;
-  copy.error_strings_ = error_strings_;
   copy.error_message_ids_ = error_message_ids_;
   copy.warning_message_ids_ = warning_message_ids_;
   copy.is_default_value_ = is_default_value_;
@@ -100,7 +98,6 @@ bool PolicyMap::Entry::Equals(const PolicyMap::Entry& other) const {
       conflicts_are_equal && level == other.level && scope == other.scope &&
       source == other.source &&  // Necessary for PolicyUIHandler observers.
                                  // They have to update when sources change.
-      error_strings_ == other.error_strings_ &&
       error_message_ids_ == other.error_message_ids_ &&
       warning_message_ids_ == other.warning_message_ids_ &&
       is_default_value_ == other.is_default_value_ &&
@@ -111,10 +108,6 @@ bool PolicyMap::Entry::Equals(const PolicyMap::Entry& other) const {
   return equals;
 }
 
-void PolicyMap::Entry::AddError(base::StringPiece error) {
-  base::StrAppend(&error_strings_, {error, "\n"});
-}
-
 void PolicyMap::Entry::AddError(int message_id) {
   error_message_ids_.emplace(message_id, base::nullopt);
 }
@@ -152,13 +145,12 @@ void PolicyMap::Entry::ClearConflicts() {
 
 base::string16 PolicyMap::Entry::GetLocalizedErrors(
     L10nLookupFunction lookup) const {
-  return GetLocalizedString(lookup, base::UTF8ToUTF16(error_strings_),
-                            error_message_ids_);
+  return GetLocalizedString(lookup, error_message_ids_);
 }
 
 base::string16 PolicyMap::Entry::GetLocalizedWarnings(
     L10nLookupFunction lookup) const {
-  return GetLocalizedString(lookup, base::string16(), warning_message_ids_);
+  return GetLocalizedString(lookup, warning_message_ids_);
 }
 
 bool PolicyMap::Entry::ignored() const {
@@ -254,10 +246,6 @@ void PolicyMap::Set(const std::string& policy, Entry entry) {
   map_[policy] = std::move(entry);
 }
 
-void PolicyMap::AddError(const std::string& policy, const std::string& error) {
-  map_[policy].AddError(error);
-}
-
 void PolicyMap::AddError(const std::string& policy, int message_id) {
   map_[policy].AddError(message_id);
 }

components/policy/core/common/policy_map.h

@@ -70,8 +70,6 @@ class POLICY_EXPORT PolicyMap {
     // Returns true if |this| equals |other|.
     bool Equals(const Entry& other) const;
 
-    // Add a non-localized error given its UTF-8 string contents.
-    void AddError(base::StringPiece error);
     // Add a localized error given its l10n message ID.
     void AddError(int message_id);
     // Add a localized error given its l10n message ID and placeholder args.
@@ -128,7 +126,6 @@ class POLICY_EXPORT PolicyMap {
     base::Optional<base::Value> value_;
     bool ignored_ = false;
     bool is_default_value_ = false;
-    std::string error_strings_;
     std::map<int, base::Optional<std::vector<base::string16>>>
         error_message_ids_;
     std::map<int, base::Optional<std::vector<base::string16>>>
@@ -164,11 +161,6 @@ class POLICY_EXPORT PolicyMap {
 
   void Set(const std::string& policy, Entry entry);
 
-  // Adds non-localized |error| to the map for the key |policy| that should be
-  // shown to the user alongside the value in the policy UI. This should only be
-  // called for policies that are already stored in this map.
-  void AddError(const std::string& policy, const std::string& error);
-
   // Adds a localized error with |message_id| to the map for the key |policy|
   // that should be shown to the user alongisde the value in the policy UI. This
   // should only be called for policies that are already stored in the map.

components/policy/core/common/policy_map_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
+#include "base/strings/strcat.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/policy/core/common/external_data_manager.h"
@@ -81,16 +82,18 @@ TEST_F(PolicyMapTest, SetAndGet) {
   base::Value expected_b("bbb");
   EXPECT_TRUE(expected_b.Equals(map.GetValue(kTestPolicyName1)));
   SetPolicy(&map, kTestPolicyName1, CreateExternalDataFetcher("dummy"));
-  map.AddError(kTestPolicyName1, kTestError);
+  map.AddError(kTestPolicyName1, IDS_POLICY_STORE_STATUS_VALIDATION_ERROR,
+               {base::UTF8ToUTF16(kTestError)});
   EXPECT_FALSE(map.GetValue(kTestPolicyName1));
   const PolicyMap::Entry* entry = map.Get(kTestPolicyName1);
   ASSERT_TRUE(entry != nullptr);
   EXPECT_EQ(POLICY_LEVEL_MANDATORY, entry->level);
   EXPECT_EQ(POLICY_SCOPE_USER, entry->scope);
   EXPECT_EQ(POLICY_SOURCE_CLOUD, entry->source);
+  std::string error_string = base::StrCat({"Validation error: ", kTestError});
   PolicyMap::Entry::L10nLookupFunction lookup = base::BindRepeating(
       static_cast<base::string16 (*)(int)>(&base::NumberToString16));
-  EXPECT_EQ(base::UTF8ToUTF16(kTestError), entry->GetLocalizedErrors(lookup));
+  EXPECT_EQ(base::UTF8ToUTF16(error_string), entry->GetLocalizedErrors(lookup));
   EXPECT_TRUE(
       ExternalDataFetcher::Equals(entry->external_data_fetcher.get(),
                                   CreateExternalDataFetcher("dummy").get()));
@@ -118,9 +121,6 @@ TEST_F(PolicyMapTest, AddError) {
   map.AddError(kTestPolicyName1, 5678);
   EXPECT_EQ(base::UTF8ToUTF16("1234\n5678"),
             entry1->GetLocalizedErrors(lookup));
-  map.AddError(kTestPolicyName1, "abcd");
-  EXPECT_EQ(base::UTF8ToUTF16("abcd\n1234\n5678"),
-            entry1->GetLocalizedErrors(lookup));
 
   // Add second entry to make sure errors are added individually.
   SetPolicy(&map, kTestPolicyName2, base::Value(0));
@@ -128,13 +128,13 @@ TEST_F(PolicyMapTest, AddError) {
   // Test AddError with placeholder replacement (one arg)
   map.AddError(kTestPolicyName2, IDS_POLICY_MIGRATED_OLD_POLICY,
                {base::UTF8ToUTF16("SomeNewPolicy")});
-  EXPECT_EQ(base::UTF8ToUTF16("abcd\n1234\n5678"),
+  EXPECT_EQ(base::UTF8ToUTF16("1234\n5678"),
             entry1->GetLocalizedErrors(lookup));
   EXPECT_EQ(base::UTF8ToUTF16("This policy is deprecated. You should use the "
                               "SomeNewPolicy policy instead."),
             entry2->GetLocalizedErrors(lookup));
   map.AddError(kTestPolicyName2, 1357);
-  EXPECT_EQ(base::UTF8ToUTF16("abcd\n1234\n5678"),
+  EXPECT_EQ(base::UTF8ToUTF16("1234\n5678"),
             entry1->GetLocalizedErrors(lookup));
   EXPECT_EQ(base::UTF8ToUTF16("1357\nThis policy is deprecated. You should use "
                               "the SomeNewPolicy policy instead."),
@@ -143,10 +143,9 @@ TEST_F(PolicyMapTest, AddError) {
   map.AddError(
       kTestPolicyName1, IDS_POLICY_DLP_CLIPBOARD_BLOCKED_ON_COPY_VM,
       {base::UTF8ToUTF16("SomeSource"), base::UTF8ToUTF16("SomeDestination")});
-  EXPECT_EQ(
-      base::UTF8ToUTF16("abcd\n1234\n5678\nYour administrator has blocked "
-                        "sharing from SomeSource to SomeDestination"),
-      entry1->GetLocalizedErrors(lookup));
+  EXPECT_EQ(base::UTF8ToUTF16("1234\n5678\nYour administrator has blocked "
+                              "sharing from SomeSource to SomeDestination"),
+            entry1->GetLocalizedErrors(lookup));
   EXPECT_EQ(base::UTF8ToUTF16("1357\nThis policy is deprecated. You should use "
                               "the SomeNewPolicy policy instead."),
             entry2->GetLocalizedErrors(lookup));

components/policy/core/common/policy_migrator.cc

@@ -33,17 +33,16 @@ void PolicyMigrator::CopyPolicyIfUnset(PolicyMap& source,
               << "' has been copied to '" << migration.new_name << "'.";
       auto new_entry = entry->DeepCopy();
       migration.transform.Run(new_entry.value());
-      new_entry.AddError(
-          l10n_util::GetStringFUTF8(IDS_POLICY_MIGRATED_NEW_POLICY,
-                                    base::UTF8ToUTF16(migration.old_name)));
+      new_entry.AddError(IDS_POLICY_MIGRATED_NEW_POLICY,
+                         {base::UTF8ToUTF16(migration.old_name)});
       dest->Set(migration.new_name, std::move(new_entry));
     } else {
       VLOG(3) << "Legacy policy '" << migration.old_name
               << "' is ignored because '" << migration.new_name
               << "' is also set. ";
     }
-    entry->AddError(l10n_util::GetStringFUTF8(
-        IDS_POLICY_MIGRATED_OLD_POLICY, base::UTF8ToUTF16(migration.new_name)));
+    entry->AddError(IDS_POLICY_MIGRATED_OLD_POLICY,
+                    {base::UTF8ToUTF16(migration.new_name)});
   } else {
     VLOG(3) << "Legacy policy '" << migration.old_name << "' is not set.";
   }

components/policy/core/common/policy_proto_decoders.cc

@@ -10,11 +10,13 @@
 #include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "components/policy/core/common/cloud/cloud_external_data_manager.h"
 #include "components/policy/core/common/policy_map.h"
 #include "components/policy/policy_constants.h"
 #include "components/policy/proto/cloud_policy.pb.h"
+#include "components/strings/grit/components_strings.h"
 
 namespace policy {
 
@@ -140,7 +142,8 @@ void DecodeProtoFields(
     map->Set(access->policy_key, level, scope, source,
              DecodeIntegerProto(proto, &error), nullptr);
     if (!error.empty())
-      map->AddError(access->policy_key, error);
+      map->AddError(access->policy_key, IDS_POLICY_PROTO_PARSING_ERROR,
+                    {base::UTF8ToUTF16(error)});
   }
 
   for (const StringPolicyAccess* access = &kStringPolicyAccess[0];
@@ -166,7 +169,8 @@ void DecodeProtoFields(
     map->Set(access->policy_key, level, scope, source, std::move(value),
              std::move(external_data_fetcher));
     if (!error.empty())
-      map->AddError(access->policy_key, error);
+      map->AddError(access->policy_key, IDS_POLICY_PROTO_PARSING_ERROR,
+                    {base::UTF8ToUTF16(error)});
   }
 
   for (const StringListPolicyAccess* access = &kStringListPolicyAccess[0];

components/policy_strings.grdp

@@ -274,6 +274,9 @@ Additional details:
   <message name="IDS_POLICY_UNKNOWN" desc="Text displayed in the status column when a policy name is not recognized.">
     Unknown policy.
   </message>
+  <message name="IDS_POLICY_PROTO_PARSING_ERROR" desc="Text displayed when error is encountered during policy parsing/decoding.">
+    Policy parsing error: <ph name="ERROR">$1<ex>Invalid JSON string: Line: 1, column: 14</ex></ph>
+  </message>
 
   <!-- chrome://policy -->
   <message name="IDS_POLICY_TITLE" desc="Page title and the title of the section that lists policies.">

components/policy_strings_grdp/IDS_POLICY_PROTO_PARSING_ERROR.png.sha1

+1955b17099ef4b9e48c1ce020c548614d64087c0
\ No newline at end of file