Add Gstatic request to ChangePasswordUrlService

Bug: 1086141
Change-Id: Ied9a69a6cb27e0f6c966075d6cfe2c07f4f45c57
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2287471
Commit-Queue: Jonathan Mengedoht <mengedoht@google.com>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795430}

chrome/browser/password_manager/change_password_url_service_factory.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/password_manager/change_password_url_service_factory.h"
 
 #include "base/no_destructor.h"
+#include "chrome/browser/profiles/profile.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "components/password_manager/core/browser/change_password_url_service_impl.h"
 #include "content/public/browser/browser_context.h"
@@ -36,5 +37,6 @@ KeyedService* ChangePasswordUrlServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
   return new password_manager::ChangePasswordUrlServiceImpl(
       content::BrowserContext::GetDefaultStoragePartition(context)
-          ->GetURLLoaderFactoryForBrowserProcess());
+          ->GetURLLoaderFactoryForBrowserProcess(),
+      Profile::FromBrowserContext(context)->GetPrefs());
 }

components/password_manager/core/browser/change_password_url_service_impl.cc

@@ -4,19 +4,35 @@
 
 #include "components/password_manager/core/browser/change_password_url_service_impl.h"
 
+#include "base/bind.h"
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
 #include "base/json/json_reader.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_service.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
+namespace {
+
+using network::SimpleURLLoader;
+
+constexpr size_t kMaxDownloadSize = 50 * 1024;
+constexpr int kMaxRetries = 3;
+constexpr base::TimeDelta kFetchTimeout = base::TimeDelta::FromSeconds(3);
+}  // namespace
+
 namespace password_manager {
 
+constexpr char ChangePasswordUrlServiceImpl::kChangePasswordUrlOverrideUrl[];
+
 ChangePasswordUrlServiceImpl::ChangePasswordUrlServiceImpl(
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
-    : url_loader_factory_(std::move(url_loader_factory)) {}
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+    PrefService* pref_service)
+    : url_loader_factory_(std::move(url_loader_factory)),
+      pref_service_(pref_service) {}
 
 ChangePasswordUrlServiceImpl::~ChangePasswordUrlServiceImpl() = default;
 
@@ -25,8 +41,70 @@ void ChangePasswordUrlServiceImpl::Initialize() {
     return;
   }
   started_fetching_ = true;
-  // TODO(crbug.com/1086141): make request to gstatic.
-  OnFetchComplete(std::make_unique<std::string>("{}"));
+
+  // Don't fetch the gstatic file when PasswordManager policy is disabled.
+  if (!pref_service_->GetBoolean(
+          password_manager::prefs::kCredentialsEnableService)) {
+    fetch_complete_ = true;
+    return;
+  }
+
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = GURL(kChangePasswordUrlOverrideUrl);
+  resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;
+  net::NetworkTrafficAnnotationTag traffic_annotation =
+      net::DefineNetworkTrafficAnnotation(
+          "gstatic_change_password_override_urls",
+          R"(
+        semantics {
+          sender: "Password Manager"
+          description:
+            "Downloads a JSON file hosted by gstatic containing a map from "
+            "host to change-password url. These urls are used by password "
+            "checkup to link to user directly to a password change form when "
+            "the password is compromised."
+            "Background: when a user has compromised credentials, we want to "
+            "link directly to a password change form. Some websites implement "
+            "the .well-known/change-password path that points to the site's "
+            "password change form. For popular sites we manually looked up the "
+            "url and saved them in this JSON file to provide a fallback when "
+            ".well-known/change-password is not supported."
+            "Spec: https://wicg.github.io/change-password-url/"
+          trigger:
+            "When the user visits chrome://settings/passwords/check or "
+            "[ORIGIN]/.well-known/change-password special URL, Chrome makes "
+            "this additional request. This can also be made when a "
+            "compromised password dialog appears e.g. after a sign in."
+          data:
+            "The request body is empty. No user data is included."
+          destination: GOOGLE_OWNED_SERVICE
+        }
+        policy {
+          cookies_allowed: NO
+          setting: "Disabled when the password manager is disabled."
+            "disabled."
+          chrome_policy {
+            PasswordManagerEnabled {
+              policy_options {mode: MANDATORY}
+              PasswordManagerEnabled: false
+            }
+          }
+        })");
+  url_loader_ =
+      SimpleURLLoader::Create(std::move(resource_request), traffic_annotation);
+  url_loader_->SetRetryOptions(kMaxRetries,
+                               SimpleURLLoader::RETRY_ON_5XX |
+                                   SimpleURLLoader::RETRY_ON_NETWORK_CHANGE |
+                                   SimpleURLLoader::RETRY_ON_NAME_NOT_RESOLVED);
+  url_loader_->SetTimeoutDuration(kFetchTimeout);
+  // Binding the callback to |this| is safe, because the navigationthrottle
+  // defers if the request is not received yet. Thereby the throttle still exist
+  // when the response arrives.
+  url_loader_->DownloadToString(
+      url_loader_factory_.get(),
+      base::BindOnce(&ChangePasswordUrlServiceImpl::OnFetchComplete,
+                     base::Unretained(this)),
+      kMaxDownloadSize);
 }
 
 void ChangePasswordUrlServiceImpl::GetChangePasswordUrl(

components/password_manager/core/browser/change_password_url_service_impl.h

@@ -14,8 +14,10 @@
 #include "base/containers/flat_map.h"
 #include "base/memory/scoped_refptr.h"
 #include "components/password_manager/core/browser/change_password_url_service.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 
 class GURL;
+class PrefService;
 
 namespace url {
 class Origin;
@@ -31,7 +33,8 @@ class ChangePasswordUrlServiceImpl
     : public password_manager::ChangePasswordUrlService {
  public:
   explicit ChangePasswordUrlServiceImpl(
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      PrefService* pref_service);
   ~ChangePasswordUrlServiceImpl() override;
 
   void Initialize() override;
@@ -40,6 +43,10 @@ class ChangePasswordUrlServiceImpl
   void GetChangePasswordUrl(const url::Origin& origin,
                             UrlCallback callback) override;
 
+  static constexpr char kChangePasswordUrlOverrideUrl[] =
+      "https://www.gstatic.com/chrome/password-manager/"
+      "change_password_urls.json";
+
  private:
   // Callback for the the request to gstatic.
   void OnFetchComplete(std::unique_ptr<std::string> response_body);
@@ -56,9 +63,14 @@ class ChangePasswordUrlServiceImpl
   // Stores the callbacks that are waiting for the request to finish.
   std::vector<std::pair<url::Origin, base::OnceCallback<void(GURL)>>>
       url_callbacks_;
+  // URL loader object for the gstatic request.
+  std::unique_ptr<network::SimpleURLLoader> url_loader_;
   // SharedURLLoaderFactory for the gstatic request, argument in the
   // constructor.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  // We are only fetching the gstatic file if PasswordManager is enabled.
+  // We use the PrefService to check if the PasswordManager is enabled.
+  PrefService* pref_service_;
 };
 
 }  // namespace password_manager

components/password_manager/core/browser/change_password_url_service_impl_unittest.cc

@@ -6,28 +6,57 @@
 #include "base/logging.h"
 #include "base/test/mock_callback.h"
 #include "base/test/task_environment.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_registry_simple.h"
+#include "components/prefs/testing_pref_service.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_shared_url_loader_factory.h"
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+constexpr char kMockResponse[] = R"({
+  "google.com": "https://google.com/change-password",
+  "a.netlify.com": "https://a.netlify.com/change-password",
+  "web.app": "https://web.app/change-password"
+})";
+}  // namespace
+
 namespace password_manager {
 
 class ChangePasswordUrlServiceTest : public testing::Test {
  public:
-  ChangePasswordUrlServiceTest() = default;
+  ChangePasswordUrlServiceTest() {
+    test_url_loader_factory_.AddResponse(
+        password_manager::ChangePasswordUrlServiceImpl::
+            kChangePasswordUrlOverrideUrl,
+        kMockResponse);
+    // Password Manager is enabled by default.
+    test_pref_service_.registry()->RegisterBooleanPref(
+        password_manager::prefs::kCredentialsEnableService, true);
+  }
 
   // Test the bevahiour for a given |url| and compares the result to the given
   // |expected_url| in the callback.
   void TestOverride(GURL url, GURL expected_url);
 
+  void DisablePasswordManagerEnabledPolicy() {
+    test_pref_service_.SetBoolean(
+        password_manager::prefs::kCredentialsEnableService, false);
+  }
+
+  void ClearMockResponses() { test_url_loader_factory_.ClearResponses(); }
+
  private:
+  base::test::SingleThreadTaskEnvironment task_environment_{
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME};
   network::TestURLLoaderFactory test_url_loader_factory_;
   scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_ =
       base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
           &test_url_loader_factory_);
+  TestingPrefServiceSimple test_pref_service_;
   ChangePasswordUrlServiceImpl change_password_url_service_{
-      test_shared_loader_factory_};
+      test_shared_loader_factory_, &test_pref_service_};
 };
 
 void ChangePasswordUrlServiceTest::TestOverride(GURL url, GURL expected_url) {
@@ -38,11 +67,41 @@ void ChangePasswordUrlServiceTest::TestOverride(GURL url, GURL expected_url) {
   EXPECT_CALL(callback, Run(expected_url));
   change_password_url_service_.GetChangePasswordUrl(url::Origin::Create(url),
                                                     callback.Get());
+  // Retry option is set to 3 times with timeout of 3s -> 9s. One added second
+  // is no problem because the |task_environment_| is still executing in the
+  // correct order and does not skip tasks.
+  task_environment_.FastForwardBy(base::TimeDelta::FromSeconds(10));
+  task_environment_.RunUntilIdle();
 }
 
 TEST_F(ChangePasswordUrlServiceTest, eTLDLookup) {
   // TODO(crbug.com/1086141): If possible mock eTLD registry to ensure sites are
   // listed.
+  TestOverride(GURL("https://google.com/foo"),
+               GURL("https://google.com/change-password"));
+  TestOverride(GURL("https://a.google.com/foo"),
+               GURL("https://google.com/change-password"));
+
+  TestOverride(GURL("https://web.app"), GURL("https://web.app"));
+
+  TestOverride(GURL("https://netlify.com"), GURL("https://netlify.com"));
+  TestOverride(GURL("https://a.netlify.com"),
+               GURL("https://a.netlify.com/change-password"));
+  TestOverride(GURL("https://b.netlify.com"), GURL("https://b.netlify.com"));
+
+  TestOverride(GURL("https://notlisted.com/foo"),
+               GURL("https://notlisted.com"));
+}
+
+TEST_F(ChangePasswordUrlServiceTest, PassworManagerPolicyDisabled) {
+  DisablePasswordManagerEnabledPolicy();
+
+  TestOverride(GURL("https://google.com/foo"), GURL("https://google.com/"));
+}
+
+TEST_F(ChangePasswordUrlServiceTest, NetworkRequestFails) {
+  ClearMockResponses();
+
   TestOverride(GURL("https://google.com/foo"), GURL("https://google.com/"));
 }
 

tools/traffic_annotation/summary/annotations.xml

@@ -139,6 +139,7 @@ Refer to README.md for content description and update process.
  <item id="gcm_unregistration" hash_code="119542033" type="0" content_hash_code="30144127" os_list="linux,windows" file_path="google_apis/gcm/engine/unregistration_request.cc"/>
  <item id="geo_language_provider" hash_code="52821843" type="1" second_id="96590038" content_hash_code="65327456" os_list="linux,windows" semantics_fields="1" policy_fields="3,4" file_path="components/language/content/browser/geo_language_provider.cc"/>
  <item id="google_url_tracker" hash_code="5492492" type="0" deprecated="2019-08-01" content_hash_code="54474899" file_path=""/>
+ <item id="gstatic_change_password_override_urls" hash_code="135799714" type="0" content_hash_code="133151871" os_list="linux,windows" file_path="components/password_manager/core/browser/change_password_url_service_impl.cc"/>
  <item id="headless_url_request" hash_code="29865866" type="0" deprecated="2018-07-10" content_hash_code="76700151" file_path=""/>
  <item id="heartbeat_sender" hash_code="4883150" type="0" content_hash_code="131927805" os_list="linux,windows" file_path="remoting/host/heartbeat_sender.cc"/>
  <item id="hintsfetcher_gethintsrequest" hash_code="34557599" type="0" content_hash_code="57003380" os_list="linux,windows" file_path="components/optimization_guide/hints_fetcher.cc"/>

tools/traffic_annotation/summary/grouping.xml

@@ -131,6 +131,7 @@ hidden="true" so that these annotations don't show up in the document.
       <traffic_annotation unique_id="services_http_server_error_response"/>
       <traffic_annotation unique_id="webrtc_peer_connection"/>
       <traffic_annotation unique_id="floc_id_provider_impl"/>
+      <traffic_annotation unique_id="gstatic_change_password_override_urls"/>
     </sender>
   </group>
   <group name="Admin Features">