[PasswordScriptsFetcher] Support min Chrome version in PasswordScriptsFetcher

Scripts should not be offered on Chrome clients with version less than
min_version provided in scripts.

Bug: 1132942
Change-Id: Iec209027b6f7c2fff0f637adbb0badeef66aa9d9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2435352
Commit-Queue: Milica Selakovic <selakovic@google.com>
Reviewed-by: Maxim Kolosovskiy  <kolos@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812150}

chrome/browser/password_check/android/password_check_manager.cc

@@ -22,6 +22,7 @@
 #include "components/strings/grit/components_strings.h"
 #include "components/sync/driver/profile_sync_service.h"
 #include "components/url_formatter/url_formatter.h"
+#include "components/version_info/version_info.h"
 #include "ui/base/l10n/l10n_util.h"
 
 namespace {
@@ -228,7 +229,8 @@ CompromisedCredentialForUI PasswordCheckManager::MakeUICredential(
   ui_credential.has_startable_script =
       !credential.username.empty() && ShouldFetchPasswordScripts() &&
       password_script_fetcher_->IsScriptAvailable(
-          url::Origin::Create(credential.url.GetOrigin()));
+          url::Origin::Create(credential.url.GetOrigin()),
+          version_info::GetVersion());
   ui_credential.has_auto_change_button =
       ui_credential.has_startable_script &&
       base::FeatureList::IsEnabled(

chrome/browser/password_check/android/password_check_manager_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
+#include "base/version.h"
 #include "chrome/browser/password_check/android/password_check_ui_status.h"
 #include "chrome/browser/password_manager/bulk_leak_check_service_factory.h"
 #include "chrome/browser/password_manager/password_manager_test_util.h"
@@ -91,10 +92,15 @@ class MockPasswordScriptsFetcher
 
   MOCK_METHOD(void,
               FetchScriptAvailability,
-              (const url::Origin&, base::OnceCallback<void(bool)>),
+              (const url::Origin&,
+               const base::Version&,
+               base::OnceCallback<void(bool)>),
               (override));
 
-  MOCK_METHOD(bool, IsScriptAvailable, (const url::Origin&), (const override));
+  MOCK_METHOD(bool,
+              IsScriptAvailable,
+              (const url::Origin&, const base::Version&),
+              (const override));
 };
 
 BulkLeakCheckService* CreateAndUseBulkLeakCheckService(

components/password_manager/core/browser/password_scripts_fetcher.h

@@ -12,6 +12,10 @@ namespace url {
 class Origin;
 }
 
+namespace base {
+class Version;
+}
+
 namespace password_manager {
 
 // Abstract interface to fetch the list of password scripts.
@@ -29,20 +33,23 @@ class PasswordScriptsFetcher : public KeyedService {
   // another.
   virtual void RefreshScriptsIfNecessary(
       base::OnceClosure fetch_finished_callback) = 0;
-  // Returns whether there is a password change script for |origin| via
-  // |callback|. If the cache was never set or is stale, it triggers a re-fetch.
-  // In case of a network error, the verdict will default to no script being
-  // available.
+  // Returns whether there is a password change script for |origin| and
+  // Chrome's |version| via |callback|. If the cache was never set or is stale,
+  // it triggers a re-fetch. In case of a network error, the verdict will
+  // default to no script being available.
   virtual void FetchScriptAvailability(const url::Origin& origin,
+                                       const base::Version& version,
                                        ResponseCallback callback) = 0;
-  // Immediately returns whether there is a password change script for |origin|.
-  // The method does NOT trigger any network requests if the cache is not ready
-  // or stale but reads the current state of the cache. In case of a network
-  // error while fetching the scripts, the result will always be false.
+  // Immediately returns whether there is a password change script for |origin|
+  // and Chrome's |version|. The method does NOT trigger any network requests if
+  // the cache is not ready or stale but reads the current state of the cache.
+  // In case of a network error while fetching the scripts, the result will
+  // always be false.
   // TODO(crbug.com/1086114): It is better to deprecate this method and always
   // use |FetchScriptAvailability| instead because |IsScriptAvailable| may
   // return stale data.
-  virtual bool IsScriptAvailable(const url::Origin& origin) const = 0;
+  virtual bool IsScriptAvailable(const url::Origin& origin,
+                                 const base::Version& version) const = 0;
 };
 
 }  // namespace password_manager

components/password_manager/core/browser/password_scripts_fetcher_impl.cc

@@ -10,6 +10,7 @@
 #include "base/metrics/field_trial_params.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/no_destructor.h"
+#include "base/version.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
@@ -25,14 +26,15 @@ constexpr int kMaxDownloadSizeInBytes = 10 * 1024;
 using ParsingResult =
     password_manager::PasswordScriptsFetcherImpl::ParsingResult;
 
-// Extracts the domains for which password changes are supported and adds them
-// to |supported_domains|.
+// Extracts the domains and min Chrome's version for which password changes are
+// supported and adds them to |supported_domains|.
 // |script_config| is the dictionary passed for a domain, representig the
-// configuration data of one password change script.
-// For example, the fetched JSON might look like this:
+// configuration data of one password change script. For example, the fetched
+// JSON might look like this:
 // {
 //   'example.com': {
-//     'domains': [ 'https://www.example.com', 'https://m.example.com' ]
+//     'domains': [ 'https://www.example.com', 'https://m.example.com' ],
+//     'min_version': '86'
 //   }
 // }
 // In this case |script_config| would represent the dictionary value
@@ -41,9 +43,9 @@ using ParsingResult =
 // The function tries to be lax about errors and prefers to skip them
 // with warnings rather than bail the parsing. This is for forward
 // compatibility.
-base::flat_set<ParsingResult> ExtractPasswordDomains(
+base::flat_set<ParsingResult> ParseDomainSpecificParamaters(
     const base::Value& script_config,
-    base::flat_set<url::Origin>& supported_domains) {
+    base::flat_map<url::Origin, base::Version>& supported_domains) {
   if (!script_config.is_dict())
     return {ParsingResult::kInvalidJson};
 
@@ -53,6 +55,21 @@ base::flat_set<ParsingResult> ExtractPasswordDomains(
     return {ParsingResult::kInvalidJson};
 
   base::flat_set<ParsingResult> warnings;
+
+  const std::string* min_version = script_config.FindStringKey("min_version");
+  base::Version version;
+  if (!min_version) {
+    warnings.insert(ParsingResult::kInvalidJson);
+    // TODO(crbug.com/1132942): remove this when server side change is in place
+    // and return error.
+    version = base::Version("0");
+  } else {
+    version = base::Version(*min_version);
+    if (!version.IsValid()) {
+      return {ParsingResult::kInvalidJson};
+    }
+  }
+
   for (const base::Value& domain : supported_domains_list->GetList()) {
     if (!domain.is_string()) {
       warnings.insert(ParsingResult::kInvalidJson);
@@ -64,7 +81,7 @@ base::flat_set<ParsingResult> ExtractPasswordDomains(
       warnings.insert(ParsingResult::kInvalidUrl);
       continue;
     }
-    supported_domains.insert(url::Origin::Create(url));
+    supported_domains.insert(std::make_pair(url::Origin::Create(url), version));
   }
 
   return warnings;
@@ -128,21 +145,26 @@ void PasswordScriptsFetcherImpl::RefreshScriptsIfNecessary(
 
 void PasswordScriptsFetcherImpl::FetchScriptAvailability(
     const url::Origin& origin,
+    const base::Version& version,
     ResponseCallback callback) {
   if (IsCacheStale()) {
     pending_callbacks_.emplace_back(
-        std::make_pair(origin, std::move(callback)));
+        std::make_pair(std::make_pair(origin, version), std::move(callback)));
     StartFetch();
     return;
   }
 
-  RunResponseCallback(origin, std::move(callback));
+  RunResponseCallback(origin, version, std::move(callback));
 }
 
 bool PasswordScriptsFetcherImpl::IsScriptAvailable(
-    const url::Origin& origin) const {
-  return password_change_domains_.find(origin) !=
-         password_change_domains_.end();
+    const url::Origin& origin,
+    const base::Version& version) const {
+  const auto& it = password_change_domains_.find(origin);
+  if (it == password_change_domains_.end()) {
+    return false;
+  }
+  return version >= it->second;
 }
 
 void PasswordScriptsFetcherImpl::StartFetch() {
@@ -213,7 +235,9 @@ void PasswordScriptsFetcherImpl::OnFetchComplete(
   for (auto& callback : std::exchange(fetch_finished_callbacks_, {}))
     std::move(callback).Run();
   for (auto& callback : std::exchange(pending_callbacks_, {}))
-    RunResponseCallback(std::move(callback.first), std::move(callback.second));
+    RunResponseCallback(std::move(callback.first.first),
+                        std::move(callback.first.second),
+                        std::move(callback.second));
 }
 
 base::flat_set<ParsingResult> PasswordScriptsFetcherImpl::ParseResponse(
@@ -235,10 +259,12 @@ base::flat_set<ParsingResult> PasswordScriptsFetcherImpl::ParseResponse(
 
   base::flat_set<ParsingResult> warnings;
   for (const auto& script_it : data.value->DictItems()) {
-    // |script_it.first| is an identifier that we don't care about.
-    // |script_it.second| provides domain-sepcific parameters.
+    // |script_it.first| is an identifier (normally, a domain name, e.g.
+    // example.com) that we don't care about.
+    // |script_it.second| provides domain-specific parameters.
     base::flat_set<ParsingResult> warnings_for_script =
-        ExtractPasswordDomains(script_it.second, password_change_domains_);
+        ParseDomainSpecificParamaters(script_it.second,
+                                      password_change_domains_);
     warnings.insert(warnings_for_script.begin(), warnings_for_script.end());
   }
   return warnings;
@@ -253,11 +279,11 @@ bool PasswordScriptsFetcherImpl::IsCacheStale() const {
 
 void PasswordScriptsFetcherImpl::RunResponseCallback(
     url::Origin origin,
+    base::Version version,
     ResponseCallback callback) {
   DCHECK(!url_loader_);     // Fetching is not running.
   DCHECK(!IsCacheStale());  // Cache is ready.
-  bool has_script =
-      password_change_domains_.find(origin) != password_change_domains_.end();
+  bool has_script = IsScriptAvailable(origin, version);
   std::move(callback).Run(has_script);
 }
 

components/password_manager/core/browser/password_scripts_fetcher_impl.h

@@ -11,6 +11,7 @@
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/time/time.h"
@@ -25,6 +26,10 @@ namespace network {
 class SharedURLLoaderFactory;
 }
 
+namespace base {
+class Version;
+}
+
 namespace password_manager {
 
 extern const char kDefaultChangePasswordScriptsListUrl[];
@@ -73,8 +78,10 @@ class PasswordScriptsFetcherImpl
   void RefreshScriptsIfNecessary(
       base::OnceClosure fetch_finished_callback) override;
   void FetchScriptAvailability(const url::Origin& origin,
+                               const base::Version& version,
                                ResponseCallback callback) override;
-  bool IsScriptAvailable(const url::Origin& origin) const override;
+  bool IsScriptAvailable(const url::Origin& origin,
+                         const base::Version& version) const override;
 
 #if defined(UNIT_TEST)
   void make_cache_stale_for_testing() {
@@ -99,19 +106,23 @@ class PasswordScriptsFetcherImpl
   // Returns whether a re-fetch is needed.
   bool IsCacheStale() const;
   // Runs |callback| immediately with the script availability for |origin|.
-  void RunResponseCallback(url::Origin origin, ResponseCallback callback);
+  void RunResponseCallback(url::Origin origin,
+                           base::Version version,
+                           ResponseCallback callback);
 
   // URL to fetch a list of scripts from.
   const std::string scripts_list_url_;
 
   // Parsed set of domains from gstatic.
-  base::flat_set<url::Origin> password_change_domains_;
+  base::flat_map<url::Origin, base::Version> password_change_domains_;
   // Timestamp of the last finished request.
   base::TimeTicks last_fetch_timestamp_;
   // Stores the callbacks that are waiting for the request to finish.
   std::vector<base::OnceClosure> fetch_finished_callbacks_;
   // Stores the per-origin callbacks that are waiting for the request to finish.
-  std::vector<std::pair<url::Origin, ResponseCallback>> pending_callbacks_;
+  std::vector<
+      std::pair<std::pair<url::Origin, base::Version>, ResponseCallback>>
+      pending_callbacks_;
   // URL loader object for the gstatic request. If |url_loader_| is not null, a
   // request is currently in flight.
   std::unique_ptr<network::SimpleURLLoader> url_loader_;

components/password_manager/core/browser/password_scripts_fetcher_impl_unittests.cc

@@ -6,6 +6,7 @@
 
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/task_environment.h"
+#include "base/version.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_url_loader_factory.h"
@@ -21,6 +22,7 @@ constexpr char kOriginWithScript1[] = "https://example.com";
 constexpr char kOriginWithScript2[] = "https://mobile.example.com";
 constexpr char kOriginWithScript3[] = "https://test.com";
 constexpr char kOriginWithoutScript[] = "https://no-script.com";
+constexpr char kVersion[] = "87";
 
 constexpr char kTestResponseContent[] =
     R"json(
@@ -29,10 +31,12 @@ constexpr char kTestResponseContent[] =
           "domains": [
             "https://example.com",
             "https://mobile.example.com"
-          ]
+          ],
+          "min_version": "86"
         },
         "test.com": {
-          "domains": ["https://test.com"]
+          "domains": ["https://test.com"],
+          "min_version": "87"
         }
       }
     )json";
@@ -52,6 +56,11 @@ url::Origin GetOriginWithScript3() {
 url::Origin GetOriginWithoutScript() {
   return url::Origin::Create(GURL(kOriginWithoutScript));
 }
+
+base::Version GetVersion() {
+  return base::Version(kVersion);
+}
+
 }  // namespace
 
 namespace password_manager {
@@ -109,7 +118,8 @@ class PasswordScriptsFetcherImplTest : public ::testing::Test {
 
  private:
   void RequestSingleScriptAvailability(const url::Origin& origin) {
-    fetcher_->FetchScriptAvailability(origin, GenerateResponseCallback(origin));
+    fetcher_->FetchScriptAvailability(origin, GetVersion(),
+                                      GenerateResponseCallback(origin));
   }
 
   void RecordResponse(url::Origin origin, bool has_script) {
@@ -235,8 +245,10 @@ TEST_F(PasswordScriptsFetcherImplTest, InvalidResponseBody) {
        PasswordScriptsFetcherImpl::ParsingResult::kInvalidJson},
       {R"({"no-domains-attribute.com" : {}})",
        PasswordScriptsFetcherImpl::ParsingResult::kInvalidJson},
-      {R"({"not-url.com" : {"domains": ["scheme-forgotten.com"]}})",
-       PasswordScriptsFetcherImpl::ParsingResult::kInvalidUrl}};
+      {R"({"not-url.com" : {"domains": ["scheme-forgotten.com"], "min_version": "2"}})",
+       PasswordScriptsFetcherImpl::ParsingResult::kInvalidUrl},
+      {R"({"not-url.com" : {"domains": ["https://no-min-version.com"]}})",
+       PasswordScriptsFetcherImpl::ParsingResult::kInvalidJson}};
   for (const auto& test_case : kTestCases) {
     SCOPED_TRACE(testing::Message() << "test_case=" << test_case.response);
     base::HistogramTester histogram_tester;
@@ -283,10 +295,14 @@ TEST_F(PasswordScriptsFetcherImplTest, ServerError) {
 TEST_F(PasswordScriptsFetcherImplTest, IsScriptAvailable) {
   // |IsScriptAvailable| does not trigger any network requests and returns the
   // default value (false).
-  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithScript1()));
-  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithScript2()));
-  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithScript3()));
-  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithoutScript()));
+  EXPECT_FALSE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript1(), GetVersion()));
+  EXPECT_FALSE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript2(), GetVersion()));
+  EXPECT_FALSE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript3(), GetVersion()));
+  EXPECT_FALSE(
+      fetcher()->IsScriptAvailable(GetOriginWithoutScript(), GetVersion()));
   EXPECT_EQ(0, GetNumberOfPendingRequests());
 
   StartBulkCheck();
@@ -298,17 +314,25 @@ TEST_F(PasswordScriptsFetcherImplTest, IsScriptAvailable) {
   SimulateResponse();
   base::RunLoop().RunUntilIdle();
   // Cache is ready.
-  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript1()));
-  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript2()));
-  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript3()));
-  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithoutScript()));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript1(), GetVersion()));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript2(), GetVersion()));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript3(), GetVersion()));
+  EXPECT_FALSE(
+      fetcher()->IsScriptAvailable(GetOriginWithoutScript(), GetVersion()));
 
   // |IsScriptAvailable| does not trigger refetching and returns stale values.
   fetcher()->make_cache_stale_for_testing();
-  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript1()));
-  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript2()));
-  EXPECT_TRUE(fetcher()->IsScriptAvailable(GetOriginWithScript3()));
-  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithoutScript()));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript1(), GetVersion()));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript2(), GetVersion()));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript3(), GetVersion()));
+  EXPECT_FALSE(
+      fetcher()->IsScriptAvailable(GetOriginWithoutScript(), GetVersion()));
   EXPECT_EQ(0, GetNumberOfPendingRequests());
 }
 
@@ -328,7 +352,8 @@ TEST_F(PasswordScriptsFetcherImplTest, AnotherScriptsListUrl) {
       R"json(
       {
         "experiment.com": {
-          "domains": ["https://experiment.com"]
+          "domains": ["https://experiment.com"],
+          "min_version" : "86"
         }
       }
     )json";
@@ -339,8 +364,21 @@ TEST_F(PasswordScriptsFetcherImplTest, AnotherScriptsListUrl) {
   // Use |IsScriptAvailable(origin)| instead of |FetchScriptAvailability(origin,
   // callback)| to simplify the test.
   EXPECT_TRUE(fetcher.IsScriptAvailable(
-      url::Origin::Create(GURL(kExperimentalDomain))));
-  EXPECT_FALSE(fetcher.IsScriptAvailable(GetOriginWithScript3()));
+      url::Origin::Create(GURL(kExperimentalDomain)), GetVersion()));
+  EXPECT_FALSE(fetcher.IsScriptAvailable(GetOriginWithScript3(), GetVersion()));
+}
+
+TEST_F(PasswordScriptsFetcherImplTest, DifferentVersions) {
+  StartBulkCheck();
+  EXPECT_EQ(1, GetNumberOfPendingRequests());
+  SimulateResponse();
+  base::RunLoop().RunUntilIdle();
+
+  const char kOlderVersion[] = "86";
+  EXPECT_FALSE(fetcher()->IsScriptAvailable(GetOriginWithScript3(),
+                                            base::Version(kOlderVersion)));
+  EXPECT_TRUE(
+      fetcher()->IsScriptAvailable(GetOriginWithScript3(), GetVersion()));
 }
 
 }  // namespace password_manager