Disable requirements fetching if browsing history is not synced

This CL disables password requirements fetching from gstatic unless the user
syncs browsing history without a custom passphrase.

NOTRY=true

Bug: 846694
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: If500c349d4fcbc94c3fe8b6c74aec1f9a2403420
Reviewed-on: https://chromium-review.googlesource.com/1097411
Commit-Queue: Dominic Battré <battre@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566756}

chrome/browser/password_manager/chrome_password_manager_client.cc

@@ -593,6 +593,13 @@ password_manager::SyncState ChromePasswordManagerClient::GetPasswordSyncState()
   return password_manager_util::GetPasswordSyncState(sync_service);
 }
 
+password_manager::SyncState ChromePasswordManagerClient::GetHistorySyncState()
+    const {
+  const browser_sync::ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  return password_manager_util::GetHistorySyncState(sync_service);
+}
+
 bool ChromePasswordManagerClient::WasLastNavigationHTTPError() const {
   DCHECK(web_contents());
 

chrome/browser/password_manager/chrome_password_manager_client.h

@@ -96,6 +96,7 @@ class ChromePasswordManagerClient
   PrefService* GetPrefs() const override;
   password_manager::PasswordStore* GetPasswordStore() const override;
   password_manager::SyncState GetPasswordSyncState() const override;
+  password_manager::SyncState GetHistorySyncState() const override;
   bool WasLastNavigationHTTPError() const override;
   net::CertStatus GetMainFrameCertStatus() const override;
   bool IsIncognito() const override;

components/autofill/core/browser/password_requirements_spec_fetcher_impl.cc

@@ -108,6 +108,12 @@ void PasswordRequirementsSpecFetcherImpl::Fetch(
     return;
   }
 
+  if (!url_loader_factory_) {
+    TriggerCallback(std::move(callback), ResultCode::kErrorNoUrlLoader,
+                    PasswordRequirementsSpec());
+    return;
+  }
+
   if (!origin.is_valid() || origin.HostIsIPAddress() ||
       !origin.SchemeIsHTTPOrHTTPS()) {
     VLOG(1) << "No valid origin";

components/password_manager/core/browser/password_generation_manager.cc

@@ -54,8 +54,10 @@ void PasswordGenerationManager::ProcessPasswordRequirements(
     return;
 
   // Fetch password requirements for the domain.
-  password_requirements_service->PrefetchSpec(
-      client_->GetLastCommittedEntryURL().GetOrigin());
+  if (IsRequirementsFetchingEnabled()) {
+    password_requirements_service->PrefetchSpec(
+        client_->GetLastCommittedEntryURL().GetOrigin());
+  }
 
   // Store password requirements from the autofill server.
   for (const autofill::FormStructure* form : forms) {
@@ -128,6 +130,10 @@ bool PasswordGenerationManager::IsGenerationEnabled(bool log_debug_data) const {
   return false;
 }
 
+bool PasswordGenerationManager::IsRequirementsFetchingEnabled() const {
+  return client_->GetHistorySyncState() == SYNCING_NORMAL_ENCRYPTION;
+}
+
 void PasswordGenerationManager::CheckIfFormClassifierShouldRun() {
   if (autofill::FormStructure::IsAutofillFieldMetadataEnabled())
     driver_->AllowToRunFormClassifier();

components/password_manager/core/browser/password_generation_manager.h

@@ -56,6 +56,10 @@ class PasswordGenerationManager {
   // autofill::SavePasswordProgressLogger.
   bool IsGenerationEnabled(bool log_debug_data) const;
 
+  // Determines whether the PasswordGeneraitonManager has the permission to
+  // fetch domain wide password requirements from gstatic.com.
+  bool IsRequirementsFetchingEnabled() const;
+
   // Determine if the form classifier should run. If yes, sends a message to the
   // renderer.
   // TODO(crbug.com/621442): Remove client-side form classifier when server-side

components/password_manager/core/browser/password_generation_manager_unittest.cc

@@ -10,6 +10,7 @@
 
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/field_trial.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/autofill_field.h"
 #include "components/autofill/core/browser/autofill_metrics.h"
@@ -44,11 +45,11 @@ namespace password_manager {
 namespace {
 
 // Magic constants.
-// Origin of a server for which no domain wide requirements are given.
-constexpr char kNoServerResponse[] = "https://www.no_server_response.com/";
-// Origin of a server for which the autofill requirements are overriden via a
-// domain wide spec.
-constexpr char kHasServerResponse[] = "https://www.has_server_response.com/";
+// Host (suffix) of a server for which no domain wide requirements are given.
+constexpr char kNoServerResponse[] = "www.no_server_response.com";
+// Host (suffix) of a server for which the autofill requirements are overriden
+// via a domain wide spec.
+constexpr char kHasServerResponse[] = "www.has_server_response.com";
 
 class TestPasswordManagerDriver : public StubPasswordManagerDriver {
  public:
@@ -113,9 +114,11 @@ class FakePasswordRequirementsSpecFetcher
   ~FakePasswordRequirementsSpecFetcher() override = default;
 
   void Fetch(GURL origin, FetchCallback callback) override {
-    if (origin.GetOrigin() == GURL(kNoServerResponse)) {
+    if (origin.GetOrigin().host_piece().find(kNoServerResponse) !=
+        std::string::npos) {
       std::move(callback).Run(autofill::PasswordRequirementsSpec());
-    } else if (origin.GetOrigin() == GURL(kHasServerResponse)) {
+    } else if (origin.GetOrigin().host_piece().find(kHasServerResponse) !=
+               std::string::npos) {
       std::move(callback).Run(GetDomainWideRequirements());
     } else {
       NOTREACHED();
@@ -126,6 +129,7 @@ class FakePasswordRequirementsSpecFetcher
 class MockPasswordManagerClient : public StubPasswordManagerClient {
  public:
   MOCK_CONST_METHOD0(GetPasswordSyncState, SyncState());
+  MOCK_CONST_METHOD0(GetHistorySyncState, SyncState());
   MOCK_CONST_METHOD0(IsSavingAndFillingEnabledForCurrentPage, bool());
   MOCK_CONST_METHOD0(IsIncognito, bool());
 
@@ -233,11 +237,11 @@ TEST_F(PasswordGenerationManagerTest, ProcessPasswordRequirements) {
       .WillRepeatedly(testing::Return(true));
   EXPECT_CALL(*client_, GetPasswordSyncState())
       .WillRepeatedly(testing::Return(SYNCING_NORMAL_ENCRYPTION));
-
   struct {
     const char* name;
     bool has_domain_wide_requirements = false;
     bool has_field_requirements = false;
+    bool allowed_to_fetch_specs = true;
     autofill::PasswordRequirementsSpec expected_spec;
   } kTests[] = {
       {
@@ -260,10 +264,24 @@ TEST_F(PasswordGenerationManagerTest, ProcessPasswordRequirements) {
           .has_field_requirements = true,
           .expected_spec = GetDomainWideRequirements(),
       },
+      {
+          .name = "Don't fetch spec if not allowed",
+          .has_domain_wide_requirements = true,
+          .allowed_to_fetch_specs = false,
+          // Default value is expected even though .has_domain_wide_requirements
+          // is true.
+          .expected_spec = autofill::PasswordRequirementsSpec(),
+      },
   };
 
+  // The purpose of this counter is to generate unique URLs and field signatures
+  // so that no caching can happen between test runs. If this causes issues in
+  // the future, the test should be converted into a parameterized test that
+  // creates unqieue PasswordRequirementsService instances for each run.
+  int test_counter = 0;
   for (const auto& test : kTests) {
     SCOPED_TRACE(test.name);
+    ++test_counter;
 
     autofill::FormFieldData username;
     username.label = ASCIIToUTF16("username");
@@ -272,7 +290,8 @@ TEST_F(PasswordGenerationManagerTest, ProcessPasswordRequirements) {
 
     autofill::FormFieldData password;
     password.label = ASCIIToUTF16("password");
-    password.name = ASCIIToUTF16("password");
+    password.name =
+        ASCIIToUTF16(base::StringPrintf("password%d", test_counter));
     password.form_control_type = "password";
 
     autofill::FormData account_creation_form;
@@ -299,14 +318,21 @@ TEST_F(PasswordGenerationManagerTest, ProcessPasswordRequirements) {
     // Configure the last committed entry URL with some magic constants for
     // which the FakePasswordRequirementsFetcher is configured to respond
     // with a filled or empty response.
-    GURL origin = test.has_domain_wide_requirements ? GURL(kHasServerResponse)
-                                                    : GURL(kNoServerResponse);
+    GURL origin(base::StringPrintf("https://%d-%s/", test_counter,
+                                   test.has_domain_wide_requirements
+                                       ? kHasServerResponse
+                                       : kNoServerResponse));
     client_->SetLastCommittedEntryUrl(origin);
 
     std::string response_string;
     ASSERT_TRUE(response.SerializeToString(&response_string));
     autofill::FormStructure::ParseQueryResponse(response_string, forms);
 
+    EXPECT_CALL(*client_, GetHistorySyncState())
+        .WillRepeatedly(testing::Return(test.allowed_to_fetch_specs
+                                            ? SYNCING_NORMAL_ENCRYPTION
+                                            : NOT_SYNCING));
+
     // Processs the password requirements with expected side effects of
     // either storing the requirements from the AutofillQueryResponseContents)
     // in the PasswordRequirementsService or triggering a lookup for the

components/password_manager/core/browser/password_manager_client.cc

@@ -44,6 +44,10 @@ SyncState PasswordManagerClient::GetPasswordSyncState() const {
   return NOT_SYNCING;
 }
 
+SyncState PasswordManagerClient::GetHistorySyncState() const {
+  return NOT_SYNCING;
+}
+
 bool PasswordManagerClient::WasLastNavigationHTTPError() const {
   return false;
 }

components/password_manager/core/browser/password_manager_client.h

@@ -178,9 +178,14 @@ class PasswordManagerClient {
 
   // Reports whether and how passwords are synced in the embedder. The default
   // implementation always returns NOT_SYNCING.
-  // TODO(vabr): Factor this out of the client to the sync layer.
+  // TODO(crbug.com/515108): Factor this out of the client to the sync layer.
   virtual SyncState GetPasswordSyncState() const;
 
+  // Reports whether and how browsing history is synced in the embedder. The
+  // default implementation always returns NOT_SYNCING.
+  // TODO(crbug.com/515108): Factor this out of the client to the sync layer.
+  virtual SyncState GetHistorySyncState() const;
+
   // Returns true if last navigation page had HTTP error i.e 5XX or 4XX
   virtual bool WasLastNavigationHTTPError() const;
 

components/password_manager/core/browser/password_manager_util.cc

@@ -97,6 +97,20 @@ password_manager::SyncState GetPasswordSyncState(
   return password_manager::NOT_SYNCING;
 }
 
+password_manager::SyncState GetHistorySyncState(
+    const syncer::SyncService* sync_service) {
+  if (sync_service && sync_service->IsFirstSetupComplete() &&
+      sync_service->IsSyncActive() &&
+      (sync_service->GetActiveDataTypes().Has(
+           syncer::HISTORY_DELETE_DIRECTIVES) ||
+       sync_service->GetActiveDataTypes().Has(syncer::PROXY_TABS))) {
+    return sync_service->IsUsingSecondaryPassphrase()
+               ? password_manager::SYNCING_WITH_CUSTOM_PASSPHRASE
+               : password_manager::SYNCING_NORMAL_ENCRYPTION;
+  }
+  return password_manager::NOT_SYNCING;
+}
+
 void FindDuplicates(
     std::vector<std::unique_ptr<autofill::PasswordForm>>* forms,
     std::vector<std::unique_ptr<autofill::PasswordForm>>* duplicates,

components/password_manager/core/browser/password_manager_util.h

@@ -33,10 +33,15 @@ class PrefService;
 namespace password_manager_util {
 
 // Reports whether and how passwords are currently synced. In particular, for a
-// null |sync_service| returns NOT_SYNCING_PASSWORDS.
+// null |sync_service| returns NOT_SYNCING.
 password_manager::SyncState GetPasswordSyncState(
     const syncer::SyncService* sync_service);
 
+// Reports whether and how browsing history is currently synced. In particular,
+// for a null |sync_service| returns NOT_SYNCING.
+password_manager::SyncState GetHistorySyncState(
+    const syncer::SyncService* sync_service);
+
 // Finds the forms with a duplicate sync tags in |forms|. The first one of
 // the duplicated entries stays in |forms|, the others are moved to
 // |duplicates|.

ios/chrome/browser/passwords/ios_chrome_password_manager_client.h

@@ -55,6 +55,7 @@ class IOSChromePasswordManagerClient
 
   // password_manager::PasswordManagerClient implementation.
   password_manager::SyncState GetPasswordSyncState() const override;
+  password_manager::SyncState GetHistorySyncState() const override;
   bool PromptUserToSaveOrUpdatePassword(
       std::unique_ptr<password_manager::PasswordFormManagerForUI> form_to_save,
       bool update_password) override;

ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm

@@ -76,6 +76,12 @@ SyncState IOSChromePasswordManagerClient::GetPasswordSyncState() const {
   return password_manager_util::GetPasswordSyncState(sync_service);
 }
 
+SyncState IOSChromePasswordManagerClient::GetHistorySyncState() const {
+  browser_sync::ProfileSyncService* sync_service =
+      ProfileSyncServiceFactory::GetForBrowserState(delegate_.browserState);
+  return password_manager_util::GetHistorySyncState(sync_service);
+}
+
 bool IOSChromePasswordManagerClient::PromptUserToChooseCredentials(
     std::vector<std::unique_ptr<autofill::PasswordForm>> local_forms,
     const GURL& origin,