Integrate ChangePasswordUrlService

Integrate ChangePasswordUrlService in
WellKnownChangePasswordNavigationThrottle to provide change password
url overrides.
ChangePasswordUrlService::GetChangePasswordUrl is now sync.

Bug: 1086141
Change-Id: If843098ed0f384e1b118ccc91c346c7089ca226a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2317191
Commit-Queue: Jonathan Mengedoht <mengedoht@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798262}

chrome/browser/ui/passwords/well_known_change_password_navigation_throttle.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/ui/passwords/well_known_change_password_navigation_throttle.h"
 
 #include "base/logging.h"
+#include "chrome/browser/password_manager/change_password_url_service_factory.h"
+#include "components/password_manager/core/browser/change_password_url_service.h"
 #include "components/password_manager/core/browser/well_known_change_password_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "content/public/browser/browser_context.h"
@@ -18,6 +20,7 @@
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
+#include "url/origin.h"
 
 namespace {
 
@@ -77,7 +80,12 @@ WellKnownChangePasswordNavigationThrottle::MaybeCreateThrottleFor(
 
 WellKnownChangePasswordNavigationThrottle::
     WellKnownChangePasswordNavigationThrottle(NavigationHandle* handle)
-    : NavigationThrottle(handle) {}
+    : NavigationThrottle(handle),
+      change_password_url_service_(
+          ChangePasswordUrlServiceFactory::GetForBrowserContext(
+              handle->GetWebContents()->GetBrowserContext())) {
+  change_password_url_service_->PrefetchURLs();
+}
 
 WellKnownChangePasswordNavigationThrottle::
     ~WellKnownChangePasswordNavigationThrottle() = default;
@@ -168,6 +176,8 @@ void WellKnownChangePasswordNavigationThrottle::
     ThrottleAction action = ContinueProcessing();
     if (action == NavigationThrottle::PROCEED) {
       Resume();
+    } else if (action == NavigationThrottle::CANCEL) {
+      CancelDeferredNavigation(NavigationThrottle::CANCEL);
     }
   }
 }
@@ -178,8 +188,10 @@ WellKnownChangePasswordNavigationThrottle::ContinueProcessing() {
   if (SupportsChangePasswordUrl()) {
     return NavigationThrottle::PROCEED;
   } else {
-    // TODO(crbug.com/1086141): Integrate Service that provides URL overrides
-    Redirect(navigation_handle()->GetURL().GetOrigin());
+    // Redirect call creates PostTask
+    GURL url = navigation_handle()->GetURL();
+    GURL redirect_url = change_password_url_service_->GetChangePasswordUrl(url);
+    Redirect(redirect_url.is_valid() ? redirect_url : url.GetOrigin());
     return NavigationThrottle::CANCEL;
   }
 }

chrome/browser/ui/passwords/well_known_change_password_navigation_throttle.h

@@ -22,6 +22,10 @@ namespace network {
 class SimpleURLLoader;
 }  // namespace network
 
+namespace password_manager {
+class ChangePasswordUrlService;
+}  // namespace password_manager
+
 // This NavigationThrottle checks whether a site supports the
 // .well-known/change-password url. To check whether a site supports the
 // change-password url, we also request a .well-known path that is defined to
@@ -66,6 +70,7 @@ class WellKnownChangePasswordNavigationThrottle
   int non_existing_resource_response_code_ = 0;
   int change_password_response_code_ = 0;
   std::unique_ptr<network::SimpleURLLoader> url_loader_;
+  password_manager::ChangePasswordUrlService* change_password_url_service_;
 };
 
 #endif  // CHROME_BROWSER_UI_PASSWORDS_WELL_KNOWN_CHANGE_PASSWORD_NAVIGATION_THROTTLE_H_

chrome/browser/ui/passwords/well_known_change_password_navigation_throttle_browsertest.cc

@@ -9,10 +9,14 @@
 #include "base/run_loop.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
+#include "chrome/browser/password_manager/change_password_url_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/cert_verifier_browser_test.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
 #include "chrome/common/url_constants.h"
+#include "components/password_manager/core/browser/change_password_url_service_impl.h"
 #include "components/password_manager/core/browser/well_known_change_password_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "content/public/test/browser_test.h"
@@ -39,6 +43,8 @@ using net::test_server::HttpResponse;
 using password_manager::kWellKnownChangePasswordPath;
 using password_manager::kWellKnownNotExistingResourcePath;
 
+constexpr char kMockChangePasswordPath[] = "/change-password-override";
+
 // ServerResponse describes how a server should respond to a given path.
 struct ServerResponse {
   net::HttpStatusCode status_code;
@@ -56,6 +62,26 @@ struct ResponseDelayParams {
 
 }  // namespace
 
+class TestChangePasswordUrlService
+    : public password_manager::ChangePasswordUrlService {
+ public:
+  void PrefetchURLs() override {}
+
+  GURL GetChangePasswordUrl(const GURL& url) override {
+    if (override_available_) {
+      GURL::Replacements replacement;
+      replacement.SetPathStr(kMockChangePasswordPath);
+      return url.ReplaceComponents(replacement);
+    }
+    return GURL();
+  }
+
+  void SetOverrideAvailable(bool available) { override_available_ = available; }
+
+ private:
+  bool override_available_ = false;
+};
+
 class WellKnownChangePasswordNavigationThrottleBrowserTest
     : public CertVerifierBrowserTest,
       public testing::WithParamInterface<ResponseDelayParams> {
@@ -71,6 +97,13 @@ class WellKnownChangePasswordNavigationThrottleBrowserTest
   void SetUpOnMainThread() override {
     ASSERT_TRUE(test_server_->InitializeAndListen());
     test_server_->StartAcceptingConnections();
+    url_service_ =
+        ChangePasswordUrlServiceFactory::GetInstance()
+            ->SetTestingSubclassFactoryAndUse(
+                browser()->profile(),
+                base::BindRepeating([](content::BrowserContext* context) {
+                  return std::make_unique<TestChangePasswordUrlService>();
+                }));
   }
 
  protected:
@@ -91,6 +124,7 @@ class WellKnownChangePasswordNavigationThrottleBrowserTest
   base::flat_map<std::string, ServerResponse> path_response_map_;
   std::unique_ptr<EmbeddedTestServer> test_server_ =
       std::make_unique<EmbeddedTestServer>(EmbeddedTestServer::TYPE_HTTPS);
+  TestChangePasswordUrlService* url_service_ = nullptr;
 
  private:
   // Returns a response for the given request. Uses |path_response_map_| to
@@ -196,11 +230,22 @@ IN_PROC_BROWSER_TEST_P(WellKnownChangePasswordNavigationThrottleBrowserTest,
       net::HTTP_NOT_FOUND, {}, response_delays.change_password_delay};
   path_response_map_[kWellKnownNotExistingResourcePath] = {
       net::HTTP_NOT_FOUND, {}, response_delays.not_exist_delay};
-  path_response_map_["/"] = {net::HTTP_OK, {}, 0};
 
   TestNavigationThrottle(/*expected_path=*/"/");
 }
 
+IN_PROC_BROWSER_TEST_P(WellKnownChangePasswordNavigationThrottleBrowserTest,
+                       NoSupportForChangePassword_WithUrlOverride) {
+  url_service_->SetOverrideAvailable(true);
+  auto response_delays = GetParam();
+  path_response_map_[kWellKnownChangePasswordPath] = {
+      net::HTTP_NOT_FOUND, {}, response_delays.change_password_delay};
+  path_response_map_[kWellKnownNotExistingResourcePath] = {
+      net::HTTP_NOT_FOUND, {}, response_delays.not_exist_delay};
+
+  TestNavigationThrottle(/*expected_path=*/kMockChangePasswordPath);
+}
+
 // Single page applications often return 200 for all paths
 IN_PROC_BROWSER_TEST_P(WellKnownChangePasswordNavigationThrottleBrowserTest,
                        NoSupportForChangePassword_Ok) {
@@ -209,7 +254,6 @@ IN_PROC_BROWSER_TEST_P(WellKnownChangePasswordNavigationThrottleBrowserTest,
       net::HTTP_OK, {}, response_delays.change_password_delay};
   path_response_map_[kWellKnownNotExistingResourcePath] = {
       net::HTTP_OK, {}, response_delays.not_exist_delay};
-  path_response_map_["/"] = {net::HTTP_OK, {}, 0};
 
   TestNavigationThrottle(/*expected_path=*/"/");
 }
@@ -225,7 +269,6 @@ IN_PROC_BROWSER_TEST_P(WellKnownChangePasswordNavigationThrottleBrowserTest,
       net::HTTP_PERMANENT_REDIRECT,
       {std::make_pair("Location", "/not-found")},
       response_delays.not_exist_delay};
-  path_response_map_["/"] = {net::HTTP_OK, {}, 0};
   path_response_map_["/not-found"] = {net::HTTP_NOT_FOUND, {}, 0};
 
   TestNavigationThrottle(/*expected_path=*/"/");

components/password_manager/core/browser/change_password_url_service.h

@@ -5,25 +5,21 @@
 #ifndef COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CHANGE_PASSWORD_URL_SERVICE_H_
 #define COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_CHANGE_PASSWORD_URL_SERVICE_H_
 
-#include "base/callback_forward.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 class GURL;
 
-namespace url {
-class Origin;
-}
-
 namespace password_manager {
 
 class ChangePasswordUrlService : public KeyedService {
  public:
-  using UrlCallback = base::OnceCallback<void(GURL)>;
-  // Initializes the service.
-  virtual void Initialize() = 0;
-  // Returns the change password URL for `origin` via `callback`.
-  virtual void GetChangePasswordUrl(const url::Origin& origin,
-                                    UrlCallback callback) = 0;
+  // Prefetch the change password URLs that point to the password change form
+  // from gstatic.
+  virtual void PrefetchURLs() = 0;
+  // Returns a change password url for a given |origin| using eTLD+1. If no
+  // override is available or the fetch is not completed yet an empty GURL is
+  // returned.
+  virtual GURL GetChangePasswordUrl(const GURL& url) = 0;
 };
 
 }  // namespace password_manager

components/password_manager/core/browser/change_password_url_service_impl.cc

@@ -8,20 +8,19 @@
 #include "base/callback.h"
 #include "base/containers/flat_map.h"
 #include "base/json/json_reader.h"
+#include "base/timer/elapsed_timer.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 "net/base/url_util.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 {
@@ -36,7 +35,7 @@ ChangePasswordUrlServiceImpl::ChangePasswordUrlServiceImpl(
 
 ChangePasswordUrlServiceImpl::~ChangePasswordUrlServiceImpl() = default;
 
-void ChangePasswordUrlServiceImpl::Initialize() {
+void ChangePasswordUrlServiceImpl::PrefetchURLs() {
   if (started_fetching_) {
     return;
   }
@@ -92,11 +91,7 @@ void ChangePasswordUrlServiceImpl::Initialize() {
         })");
   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.
@@ -107,15 +102,17 @@ void ChangePasswordUrlServiceImpl::Initialize() {
       kMaxDownloadSize);
 }
 
-void ChangePasswordUrlServiceImpl::GetChangePasswordUrl(
-    const url::Origin& origin,
-    UrlCallback callback) {
-  DCHECK(started_fetching_) << "Call Initialize() before.";
-  if (fetch_complete_) {
-    std::move(callback).Run(ChangePasswordUrlFor(origin));
-  } else {
-    url_callbacks_.emplace_back(origin, std::move(callback));
+GURL ChangePasswordUrlServiceImpl::GetChangePasswordUrl(const GURL& url) {
+  DCHECK(started_fetching_) << "Call PrefetchURLs() before.";
+  std::string domain_and_registry =
+      net::registry_controlled_domains::GetDomainAndRegistry(
+          url, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+  auto it = change_password_url_map_.find(domain_and_registry);
+  if (it != change_password_url_map_.end()) {
+    return it->second;
   }
+  // Fallback if no valid change-password url available no response available.
+  return GURL();
 }
 
 void ChangePasswordUrlServiceImpl::OnFetchComplete(
@@ -136,24 +133,6 @@ void ChangePasswordUrlServiceImpl::OnFetchComplete(
       }
     }
   }
-
-  for (auto& url_callback : std::exchange(url_callbacks_, {})) {
-    GURL url = ChangePasswordUrlFor(url_callback.first);
-    std::move(url_callback.second).Run(std::move(url));
-  }
-}
-
-GURL ChangePasswordUrlServiceImpl::ChangePasswordUrlFor(
-    const url::Origin& origin) {
-  std::string domain_and_registry =
-      net::registry_controlled_domains::GetDomainAndRegistry(
-          origin, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
-  auto it = change_password_url_map_.find(domain_and_registry);
-  if (it != change_password_url_map_.end()) {
-    return it->second;
-  }
-  // Fallback if no valid change-password url available or request failed.
-  return origin.GetURL();
 }
 
 }  // namespace password_manager

components/password_manager/core/browser/change_password_url_service_impl.h

@@ -19,10 +19,6 @@
 class GURL;
 class PrefService;
 
-namespace url {
-class Origin;
-}
-
 namespace network {
 class SharedURLLoaderFactory;
 }
@@ -37,11 +33,9 @@ class ChangePasswordUrlServiceImpl
       PrefService* pref_service);
   ~ChangePasswordUrlServiceImpl() override;
 
-  void Initialize() override;
-  // When the gstatic response arrives the callback is called with the override
-  // url for the given |url|. If no override is there the origin is returned.
-  void GetChangePasswordUrl(const url::Origin& origin,
-                            UrlCallback callback) override;
+  // ChangePasswordUrlService:
+  void PrefetchURLs() override;
+  GURL GetChangePasswordUrl(const GURL& url) override;
 
   static constexpr char kChangePasswordUrlOverrideUrl[] =
       "https://www.gstatic.com/chrome/password-manager/"
@@ -50,9 +44,6 @@ class ChangePasswordUrlServiceImpl
  private:
   // Callback for the the request to gstatic.
   void OnFetchComplete(std::unique_ptr<std::string> response_body);
-  // Retrieves the url override from the |change_password_url_map_| for a given
-  // origin using eTLD+1. The origin is returned when no override is available.
-  GURL ChangePasswordUrlFor(const url::Origin& origin);
 
   // Stores if the request is already started to only fetch once.
   bool started_fetching_ = false;
@@ -60,9 +51,6 @@ class ChangePasswordUrlServiceImpl
   bool fetch_complete_ = false;
   // Stores the JSON result for the url overrides.
   base::flat_map<std::string, GURL> change_password_url_map_;
-  // 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

components/password_manager/core/browser/change_password_url_service_impl_unittest.cc

@@ -36,15 +36,18 @@ class ChangePasswordUrlServiceTest : public testing::Test {
         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);
+  // Fetches the url overrides and waits until the response arrived.
+  void PrefetchAndWaitUntilDone();
 
   void DisablePasswordManagerEnabledPolicy() {
     test_pref_service_.SetBoolean(
         password_manager::prefs::kCredentialsEnableService, false);
   }
 
+  GURL GetChangePasswordUrl(const GURL& url) {
+    return change_password_url_service_.GetChangePasswordUrl(url);
+  }
+
   void ClearMockResponses() { test_url_loader_factory_.ClearResponses(); }
 
  private:
@@ -59,50 +62,45 @@ class ChangePasswordUrlServiceTest : public testing::Test {
       test_shared_loader_factory_, &test_pref_service_};
 };
 
-void ChangePasswordUrlServiceTest::TestOverride(GURL url, GURL expected_url) {
-  change_password_url_service_.Initialize();
-
-  base::MockCallback<password_manager::ChangePasswordUrlService::UrlCallback>
-      callback;
-  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));
+void ChangePasswordUrlServiceTest::PrefetchAndWaitUntilDone() {
+  change_password_url_service_.PrefetchURLs();
   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"));
+  PrefetchAndWaitUntilDone();
+
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://google.com/foo")),
+            GURL("https://google.com/change-password"));
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://a.google.com/foo")),
+            GURL("https://google.com/change-password"));
 
-  TestOverride(GURL("https://web.app"), GURL("https://web.app"));
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://web.app")), GURL());
 
-  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"));
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://netlify.com")), GURL());
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://a.netlify.com")),
+            GURL("https://a.netlify.com/change-password"));
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://b.netlify.com")), GURL());
 
-  TestOverride(GURL("https://notlisted.com/foo"),
-               GURL("https://notlisted.com"));
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://notlisted.com/foo")), GURL());
 }
 
 TEST_F(ChangePasswordUrlServiceTest, PassworManagerPolicyDisabled) {
   DisablePasswordManagerEnabledPolicy();
 
-  TestOverride(GURL("https://google.com/foo"), GURL("https://google.com/"));
+  PrefetchAndWaitUntilDone();
+
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://google.com/foo")), GURL());
 }
 
 TEST_F(ChangePasswordUrlServiceTest, NetworkRequestFails) {
   ClearMockResponses();
 
-  TestOverride(GURL("https://google.com/foo"), GURL("https://google.com/"));
+  PrefetchAndWaitUntilDone();
+
+  EXPECT_EQ(GetChangePasswordUrl(GURL("https://google.com/foo")), GURL());
 }
 
 }  // namespace password_manager