Spellcheck: Apply policy for languages that lack Hunspell support

Spellcheck languages that have Windows platform support but not
Hunspell support (Arabic, Finnish, ...) cannot be disabled or forced
enabled by enterprise policy. This is because of the use of
spellcheck::GetCorrespondingSpellCheckLanguage in
SpellcheckLanguageBlacklistPolicyHandler::SortBlacklistedLanguages and
SpellcheckLanguagePolicyHandler::SortForcedLanguages, a method that
only recognizes Hunspell languages.

The fix is to instead check if a language with policy being enforced is
on the list of supported accept languages, as these are what are used
to populate the language list on the language settings page. Note that
in the upcoming implementation of support for non-Hunspell languages,
we will only allow Windows platform spellchecking for these accept
languages (if they have a Windows spellcheck dictionary).

Instead of just searching for the accept language within the tag, the
algorithm first tries for an exact match (so "en-CA" would be found),
then searches for an exact match for the language without locale
info, that is, what's to the left of the hyphen in a language tag
(so "ar" would be found for "ar-SA").

Also adds unit test coverage for the spellcheck policy handlers, which
previously had none.

Note that there is an interplay between forced and blocked languages,
as well as the spellcheck-enabled policy:

  1) If a language is both forced and blocked, forced wins.
  2) If spellcheck is disabled by policy, the forced/blocked languages
     policy has no effect.

Test cases cover this interplay.

Bug: 1056850
Change-Id: Iec54a851d38f00a7aa7597449f7acc30f3dd2de3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2080450Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Bruce Long <brlong@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#757107}

chrome/browser/spellchecker/spellcheck_language_blacklist_policy_handler.cc

@@ -12,12 +12,13 @@
 #include "base/syslog_logging.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "chrome/browser/spellchecker/spellcheck_service.h"
 #include "chrome/common/pref_names.h"
 #include "components/policy/core/browser/policy_error_map.h"
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_value_map.h"
 #include "components/spellcheck/browser/pref_names.h"
-#include "components/spellcheck/common/spellcheck_common.h"
+#include "components/spellcheck/common/spellcheck_features.h"
 #include "components/strings/grit/components_strings.h"
 
 SpellcheckLanguageBlacklistPolicyHandler::
@@ -113,9 +114,11 @@ void SpellcheckLanguageBlacklistPolicyHandler::SortBlacklistedLanguages(
   // Separate the valid languages from the unknown / unsupported languages and
   // the languages that also appear in the SpellcheckLanguage policy.
   for (const base::Value& language : value->GetList()) {
+    std::string candidate_language =
+        base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL)
+            .as_string();
     std::string current_language =
-        spellcheck::GetCorrespondingSpellCheckLanguage(
-            base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL));
+        SpellcheckService::GetSupportedAcceptLanguageCode(candidate_language);
 
     if (current_language.empty()) {
       unknown->emplace_back(language.GetString());

chrome/browser/spellchecker/spellcheck_language_policy_handler.cc

@@ -11,12 +11,13 @@
 #include "base/syslog_logging.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "chrome/browser/spellchecker/spellcheck_service.h"
 #include "chrome/common/pref_names.h"
 #include "components/policy/core/browser/policy_error_map.h"
 #include "components/policy/policy_constants.h"
 #include "components/prefs/pref_value_map.h"
 #include "components/spellcheck/browser/pref_names.h"
-#include "components/spellcheck/common/spellcheck_common.h"
+#include "components/spellcheck/common/spellcheck_features.h"
 #include "components/strings/grit/components_strings.h"
 
 SpellcheckLanguagePolicyHandler::SpellcheckLanguagePolicyHandler()
@@ -86,9 +87,11 @@ void SpellcheckLanguagePolicyHandler::SortForcedLanguages(
 
   // Separate the valid languages from the unknown / unsupported languages.
   for (const base::Value& language : value->GetList()) {
+    std::string candidate_language =
+        base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL)
+            .as_string();
     std::string current_language =
-        spellcheck::GetCorrespondingSpellCheckLanguage(
-            base::TrimWhitespaceASCII(language.GetString(), base::TRIM_ALL));
+        SpellcheckService::GetSupportedAcceptLanguageCode(candidate_language);
 
     if (current_language.empty()) {
       unknown->emplace_back(language.GetString());

chrome/browser/spellchecker/spellcheck_language_policy_handlers_unittest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Chromium style is to have one unit test per one header file. However, the
+// applied blocked spellcheck language policy depends on the applied forced
+// language policy. If a language is both blocked and forced, forced wins. It is
+// only practical to test this interaction in a single unit test covering both
+// header files.
+#include "chrome/browser/spellchecker/spellcheck_language_blacklist_policy_handler.h"
+#include "chrome/browser/spellchecker/spellcheck_language_policy_handler.h"
+
+#include <ostream>
+#include <string>
+#include <vector>
+
+#include "base/strings/string_util.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/values.h"
+#include "chrome/common/pref_names.h"
+#include "components/policy/core/common/policy_map.h"
+#include "components/policy/policy_constants.h"
+#include "components/prefs/pref_value_map.h"
+#include "components/spellcheck/browser/pref_names.h"
+#include "components/spellcheck/common/spellcheck_features.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace policy {
+
+struct TestCase {
+  TestCase(const std::vector<std::string>& blocked_languages,
+           const std::vector<std::string>& forced_languages,
+           const std::vector<std::string>& expected_blocked_languages,
+           const std::vector<std::string>& expected_forced_languages,
+           bool spellcheck_enabled,
+           bool windows_spellchecker_enabled)
+      : blocked_languages(blocked_languages),
+        forced_languages(forced_languages),
+        expected_blocked_languages(expected_blocked_languages),
+        expected_forced_languages(expected_forced_languages),
+        spellcheck_enabled(spellcheck_enabled),
+        windows_spellchecker_enabled(windows_spellchecker_enabled) {}
+
+  std::vector<std::string> blocked_languages;
+  std::vector<std::string> forced_languages;
+  std::vector<std::string> expected_blocked_languages;
+  std::vector<std::string> expected_forced_languages;
+  bool spellcheck_enabled;
+  bool windows_spellchecker_enabled;
+};
+
+std::ostream& operator<<(std::ostream& out, const TestCase& test_case) {
+  out << "blocked_languages=["
+      << base::JoinString(test_case.blocked_languages, ",")
+      << "], forced_languages=["
+      << base::JoinString(test_case.forced_languages, ",")
+      << "], expected_blocked_languages=["
+      << base::JoinString(test_case.expected_blocked_languages, ",")
+      << "], expected_forced_languages=["
+      << base::JoinString(test_case.expected_forced_languages, ",")
+      << "], spellcheck_enabled=" << test_case.spellcheck_enabled
+      << "], windows_spellchecker_enabled="
+      << test_case.windows_spellchecker_enabled;
+  return out;
+}
+
+class SpellcheckLanguagePolicyHandlersTest
+    : public testing::TestWithParam<TestCase> {
+ public:
+  SpellcheckLanguagePolicyHandlersTest() = default;
+  ~SpellcheckLanguagePolicyHandlersTest() override = default;
+
+  void CheckPrefs(const PrefValueMap& prefs,
+                  const std::string& key,
+                  const std::vector<std::string>& expected) {
+    // Retrieve the spellcheck enabled pref (if it exists).
+    const base::Value* spellcheck_enabled_pref = nullptr;
+    const bool is_spellcheck_enabled_pref_set = prefs.GetValue(
+        spellcheck::prefs::kSpellCheckEnable, &spellcheck_enabled_pref);
+
+    const base::Value* languages_list = nullptr;
+    if (GetParam().spellcheck_enabled) {
+      EXPECT_TRUE(is_spellcheck_enabled_pref_set);
+      EXPECT_TRUE(spellcheck_enabled_pref->is_bool());
+      EXPECT_TRUE(spellcheck_enabled_pref->GetBool());
+
+      EXPECT_TRUE(prefs.GetValue(key, &languages_list));
+      EXPECT_TRUE(languages_list->is_list());
+      EXPECT_EQ(expected.size(), languages_list->GetList().size());
+
+      for (const auto& language : languages_list->GetList()) {
+        EXPECT_TRUE(language.is_string());
+        EXPECT_TRUE(std::find(expected.begin(), expected.end(),
+                              language.GetString()) != expected.end());
+      }
+    } else {
+      EXPECT_FALSE(is_spellcheck_enabled_pref_set);
+      // No language list should be added to prefs if spellchecking disabled.
+      EXPECT_FALSE(prefs.GetValue(key, &languages_list));
+    }
+  }
+};
+
+TEST_P(SpellcheckLanguagePolicyHandlersTest, ApplyPolicySettings) {
+#if BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
+  base::test::ScopedFeatureList feature_list;
+  if (GetParam().windows_spellchecker_enabled) {
+    // Force hybrid spellchecking to be enabled.
+    feature_list.InitWithFeatures(
+        /*enabled_features=*/{spellcheck::kWinUseBrowserSpellChecker,
+                              spellcheck::kWinUseHybridSpellChecker},
+        /*disabled_features=*/{});
+  } else {
+    // Hunspell-only spellcheck languages will be used.
+    feature_list.InitAndDisableFeature(spellcheck::kWinUseBrowserSpellChecker);
+  }
+#endif  // BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
+
+  PrefValueMap prefs;
+  policy::PolicyMap policy;
+
+  base::Value blocked_languages_list(base::Value::Type::LIST);
+  for (const auto& blocked_language : GetParam().blocked_languages) {
+    blocked_languages_list.Append(std::move(blocked_language));
+  }
+
+  base::Value forced_languages_list(base::Value::Type::LIST);
+  for (const auto& forced_language : GetParam().forced_languages) {
+    forced_languages_list.Append(std::move(forced_language));
+  }
+
+  policy.Set(policy::key::kSpellcheckLanguageBlacklist,
+             policy::POLICY_LEVEL_MANDATORY, policy::POLICY_SCOPE_USER,
+             policy::POLICY_SOURCE_ENTERPRISE_DEFAULT,
+             base::Value::ToUniquePtrValue(std::move(blocked_languages_list)),
+             nullptr);
+
+  policy.Set(
+      policy::key::kSpellcheckLanguage, policy::POLICY_LEVEL_MANDATORY,
+      policy::POLICY_SCOPE_USER, policy::POLICY_SOURCE_ENTERPRISE_DEFAULT,
+      base::Value::ToUniquePtrValue(std::move(forced_languages_list)), nullptr);
+
+  policy.Set(
+      policy::key::kSpellcheckEnabled, policy::POLICY_LEVEL_MANDATORY,
+      policy::POLICY_SCOPE_USER, policy::POLICY_SOURCE_ENTERPRISE_DEFAULT,
+      std::make_unique<base::Value>(GetParam().spellcheck_enabled), nullptr);
+
+  // Apply policy to the forced languages handler.
+  SpellcheckLanguagePolicyHandler forced_languages_handler;
+  forced_languages_handler.ApplyPolicySettings(policy, &prefs);
+
+  // Apply policy to the blocked languages handler.
+  SpellcheckLanguageBlacklistPolicyHandler blocked_languages_handler;
+  blocked_languages_handler.ApplyPolicySettings(policy, &prefs);
+
+  // Check if forced languages preferences are as expected.
+  CheckPrefs(prefs, spellcheck::prefs::kSpellCheckForcedDictionaries,
+             GetParam().expected_forced_languages);
+
+  // Check if blocked languages preferences are as expected.
+  CheckPrefs(prefs, spellcheck::prefs::kSpellCheckBlacklistedDictionaries,
+             GetParam().expected_blocked_languages);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    TestCases,
+    SpellcheckLanguagePolicyHandlersTest,
+    testing::Values(
+#if BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
+        // Test cases for Windows spellchecker (policy languages not restricted
+        // to Hunspell).
+        TestCase({"ar-SA", "es-MX", "fi", "fr",
+                  "not-a-language"} /* blocked languages */,
+                 {"fi", "fr", "not-another-language"} /* forced languages */,
+                 {"ar", "es-MX"} /* expected blocked languages */,
+                 {"fi", "fr"} /* expected forced languages */,
+                 true /* spellcheck enabled */,
+                 true /* windows spellchecker enabled */),
+        TestCase({"ar-SA", "es-MX", "fi", "fr "} /* blocked languages */,
+                 {"fi", "fr"} /* forced languages */,
+                 {""} /* expected blocked languages */,
+                 {""} /* expected forced languages */,
+                 false /* spellcheck enabled */,
+                 true /* windows spellchecker enabled */),
+#endif  // BUILDFLAG(USE_WIN_HYBRID_SPELLCHECKER)
+        // Test cases for Hunspell only spellchecker. ar-SA and fi are
+        // non-Hunspell languages so are ignored for policy enforcement. If they
+        // ever obtain Hunspell support, the first test case below will fail.
+        TestCase({"ar-SA", "es-MX", "fi", "fr",
+                  "not-a-language"} /* blocked languages */,
+                 {"fi", "fr", "not-another-language"} /* forced languages */,
+                 {"es-MX"} /* expected blocked languages */,
+                 {"fr"} /* expected forced languages */,
+                 true /* spellcheck enabled */,
+                 false /* windows spellchecker enabled */),
+        TestCase({"ar-SA", "es-MX", "fi", "fr",
+                  "not-a-language"} /* blocked languages */,
+                 {"fi", "fr", "not-another-language"} /* forced languages */,
+                 {""} /* expected blocked languages */,
+                 {""} /* expected forced languages */,
+                 false /* spellcheck enabled */,
+                 false /* windows spellchecker enabled */)));
+
+}  // namespace policy

chrome/browser/spellchecker/spellcheck_service.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/spellchecker/spellcheck_service.h"
 
 #include <algorithm>
+#include <iterator>
 #include <set>
 #include <utility>
 
@@ -38,6 +39,7 @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/storage_partition.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "ui/base/l10n/l10n_util.h"
 
 #if defined(OS_WIN) && BUILDFLAG(USE_BROWSER_SPELLCHECKER)
 #include "base/task/post_task.h"
@@ -200,6 +202,84 @@ bool SpellcheckService::SignalStatusEvent(
   return true;
 }
 
+// static
+std::string SpellcheckService::GetSupportedAcceptLanguageCode(
+    const std::string& supported_language_full_tag) {
+  // Default to accept language in hardcoded list of Hunspell dictionaries
+  // (kSupportedSpellCheckerLanguages).
+  std::string supported_accept_language =
+      spellcheck::GetCorrespondingSpellCheckLanguage(
+          supported_language_full_tag);
+
+#if defined(OS_WIN)
+  if (!spellcheck::UseWinHybridSpellChecker())
+    return supported_accept_language;
+
+  // Collect the hardcoded list of accept-languages supported by the browser,
+  // that is, languages that can be added as preferred languages in the
+  // languages settings page.
+  std::vector<std::string> accept_languages;
+  l10n_util::GetAcceptLanguages(&accept_languages);
+
+  // First try exact match. Per BCP47, tags are in ASCII and should be treated
+  // as case-insensitive (although there are conventions for the capitalization
+  // of subtags).
+  auto iter =
+      std::find_if(accept_languages.begin(), accept_languages.end(),
+                   [supported_language_full_tag](const auto& accept_language) {
+                     return base::EqualsCaseInsensitiveASCII(
+                         supported_language_full_tag, accept_language);
+                   });
+  if (iter != accept_languages.end())
+    return *iter;
+
+  // Then try matching just the language and (optional) script subtags, but
+  // not the region subtag. For example, Edge supports sr-Cyrl-RS as an accept
+  // language, but not sr-Cyrl-CS. Matching language + script subtags assures
+  // we get the correct script for spellchecking, and not use sr-Latn-RS if
+  // language packs for both scripts are installed on the system.
+  if (!base::Contains(supported_language_full_tag, "-"))
+    return "";
+
+  iter =
+      std::find_if(accept_languages.begin(), accept_languages.end(),
+                   [supported_language_full_tag](const auto& accept_language) {
+                     return base::EqualsCaseInsensitiveASCII(
+                         SpellcheckService::GetLanguageAndScriptTag(
+                             supported_language_full_tag,
+                             /* include_script_tag */ true),
+                         SpellcheckService::GetLanguageAndScriptTag(
+                             accept_language,
+                             /* include_script_tag */ true));
+                   });
+
+  if (iter != accept_languages.end())
+    return *iter;
+
+  // Then try just matching the leading language subtag. E.g. Edge supports
+  // kok as an accept language, but if the Konkani language pack is
+  // installed the Windows spellcheck API reports kok-Deva-IN for the
+  // dictionary name.
+  iter =
+      std::find_if(accept_languages.begin(), accept_languages.end(),
+                   [supported_language_full_tag](const auto& accept_language) {
+                     return base::EqualsCaseInsensitiveASCII(
+                         SpellcheckService::GetLanguageAndScriptTag(
+                             supported_language_full_tag,
+                             /* include_script_tag */ false),
+                         SpellcheckService::GetLanguageAndScriptTag(
+                             accept_language,
+                             /* include_script_tag */ false));
+                   });
+
+  if (iter != accept_languages.end())
+    return *iter;
+
+#endif  // defined(OS_WIN)
+
+  return supported_accept_language;
+}
+
 void SpellcheckService::StartRecordingMetrics(bool spellcheck_enabled) {
   metrics_ = std::make_unique<SpellCheckHostMetrics>();
   metrics_->RecordEnabledStats(spellcheck_enabled);
@@ -360,6 +440,33 @@ void SpellcheckService::OverrideBinderForTesting(SpellCheckerBinder binder) {
   GetSpellCheckerBinderOverride() = std::move(binder);
 }
 
+// static
+std::string SpellcheckService::GetLanguageAndScriptTag(
+    const std::string& full_tag,
+    bool include_script_tag) {
+  std::string language_and_script_tag;
+
+  std::vector<std::string> subtags = base::SplitString(
+      full_tag, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+  // Language subtag is required, all others optional.
+  DCHECK_GE(subtags.size(), 1ULL);
+  std::vector<std::string> subtag_tokens_to_pass;
+  subtag_tokens_to_pass.push_back(subtags.front());
+  subtags.erase(subtags.begin());
+
+  // The optional script subtag always follows the language subtag, and is 4
+  // characters in length.
+  if (include_script_tag) {
+    if (!subtags.empty() && subtags.front().length() == 4) {
+      subtag_tokens_to_pass.push_back(subtags.front());
+    }
+  }
+
+  language_and_script_tag = base::JoinString(subtag_tokens_to_pass, "-");
+
+  return language_and_script_tag;
+}
+
 // static
 void SpellcheckService::AttachStatusEvent(base::WaitableEvent* status_event) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);

chrome/browser/spellchecker/spellcheck_service.h

@@ -94,6 +94,13 @@ class SpellcheckService : public KeyedService,
   // when we do not set an event to |status_event_|.
   static bool SignalStatusEvent(EventType type);
 
+  // Get the best match of a supported accept language code for the provided
+  // language tag. Returns an empty string if no match is found. Method cannot
+  // be defined in spellcheck_common.h as it depends on l10n_util, and code
+  // under components cannot depend on ui/base.
+  static std::string GetSupportedAcceptLanguageCode(
+      const std::string& supported_language_full_tag);
+
   // Instantiates SpellCheckHostMetrics object and makes it ready for recording
   // metrics. This should be called only if the metrics recording is active.
   void StartRecordingMetrics(bool spellcheck_enabled);
@@ -161,6 +168,11 @@ class SpellcheckService : public KeyedService,
  private:
   FRIEND_TEST_ALL_PREFIXES(SpellcheckServiceBrowserTest, DeleteCorruptedBDICT);
 
+  // Parses a full BCP47 language tag to return just the language subtag,
+  // optionally with a hyphen and script subtag appended.
+  static std::string GetLanguageAndScriptTag(const std::string& full_tag,
+                                             bool include_script_tag);
+
   // Attaches an event so browser tests can listen the status events.
   static void AttachStatusEvent(base::WaitableEvent* status_event);
 

chrome/test/BUILD.gn

@@ -4473,6 +4473,7 @@ test("unit_tests") {
     sources += [
       "../browser/spellchecker/spell_check_host_chrome_impl_mac_unittest.cc",
       "../browser/spellchecker/spellcheck_custom_dictionary_unittest.cc",
+      "../browser/spellchecker/spellcheck_language_policy_handlers_unittest.cc",
       "../browser/spellchecker/spellcheck_service_unittest.cc",
       "../tools/convert_dict/convert_dict_unittest.cc",
     ]

ui/base/l10n/l10n_util.cc

@@ -888,6 +888,12 @@ void GetAcceptLanguagesForLocale(const std::string& display_locale,
   }
 }
 
+void GetAcceptLanguages(std::vector<std::string>* locale_codes) {
+  for (const char* accept_language : kAcceptLanguageList) {
+    locale_codes->push_back(accept_language);
+  }
+}
+
 bool IsLanguageAccepted(const std::string& display_locale,
                         const std::string& locale) {
   for (const char* accept_language : kAcceptLanguageList) {

ui/base/l10n/l10n_util.h

@@ -212,6 +212,9 @@ UI_BASE_EXPORT void GetAcceptLanguagesForLocale(
     const std::string& display_locale,
     std::vector<std::string>* locale_codes);
 
+// Returns a vector of untranslated locale codes usable for accept-languages.
+UI_BASE_EXPORT void GetAcceptLanguages(std::vector<std::string>* locale_codes);
+
 // Returns true if |locale| is in a predefined AcceptLanguageList and
 // a display name for the |locale| is available in the locale |display_locale|.
 UI_BASE_EXPORT bool IsLanguageAccepted(const std::string& display_locale,