[dice] Avoid saving passwords when user is signing in to Chrome.

This CL avoids saving passwords when the user is signing in to Chrome via the Desktop Identity Consistency flow. It basically avoids saving password for URLs with origin "accounts.google.com" and that have the URL parameter "ssp=1". This CL also reverts https://chromium-review.googlesource.com/888623 which was a poor attempt to fix the same bug. To review the code: * Patch 1 is the revert of https://chromium-review.googlesource.com/888623 * Following patches contain the real fix. Bug: 805826 Change-Id: I947b63cea5e2a875260fc9d6066eed696198e35f Reviewed-on: https://chromium-review.googlesource.com/979932 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Reviewed-by: Colin Blundell <blundell@chromium.org> Commit-Queue: Mihai Sardarescu <msarda@chromium.org> Cr-Commit-Position: refs/heads/master@{#545789}
Denger-Network · Mar 26, 2018 · 83e59ac · 83e59ac
1 parent 058bf2f
commit 83e59ac
Show file tree

Hide file tree

Showing 17 changed files with 81 additions and 55 deletions.
diff --git a/components/BUILD.gn b/components/BUILD.gn
@@ -473,6 +473,7 @@ if (!is_ios) {
       "//content/test:test_support",
       "//device/bluetooth",
       "//device/geolocation/public/cpp:test_support",
+      "//google_apis",
       "//ipc:test_support",
       "//net:test_support",
       "//printing/features",

diff --git a/components/autofill/content/renderer/password_autofill_agent.cc b/components/autofill/content/renderer/password_autofill_agent.cc
@@ -1102,6 +1102,11 @@ void PasswordAutofillAgent::SendPasswordForms(bool only_visible) {
       // page will be ignored.
       return;
     }
+    if (IsGaiaWithSkipSavePasswordForm(form)) {
+      // Bail if this is a GAIA enable Chrome sync flow, so that page will be
+      // ignored.
+      return;
+    }
     if (only_visible) {
       bool is_form_visible = form_util::AreFormContentsVisible(form);
       if (logger) {

diff --git a/components/autofill/content/renderer/password_form_conversion_utils.cc b/components/autofill/content/renderer/password_form_conversion_utils.cc
@@ -27,6 +27,7 @@
 #include "components/autofill/core/common/password_form_field_prediction_map.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "google_apis/gaia/gaia_urls.h"
+#include "net/base/url_util.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/platform/WebVector.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
@@ -867,6 +868,18 @@ bool IsGaiaReauthenticationForm(const blink::WebFormElement& form) {
   return has_rart_field && has_continue_field;
 }
 
+bool IsGaiaWithSkipSavePasswordForm(const blink::WebFormElement& form) {
+  GURL url(form.GetDocument().Url());
+  if (url.GetOrigin() != GaiaUrls::GetInstance()->gaia_url().GetOrigin()) {
+    return false;
+  }
+
+  std::string should_skip_password;
+  if (!net::GetValueForKeyInQuery(url, "ssp", &should_skip_password))
+    return false;
+  return should_skip_password == "1";
+}
+
 std::unique_ptr<PasswordForm> CreatePasswordFormFromWebForm(
     const WebFormElement& web_form,
     const FieldValueAndPropertiesMaskMap* field_value_and_properties_map,

diff --git a/components/autofill/content/renderer/password_form_conversion_utils.h b/components/autofill/content/renderer/password_form_conversion_utils.h
@@ -48,6 +48,9 @@ re2::RE2* CreateMatcher(void* instance, const char* pattern);
 // Tests whether the given form is a GAIA reauthentication form.
 bool IsGaiaReauthenticationForm(const blink::WebFormElement& form);
 
+// Tests whether the given form is a GAIA form with a skip password argument.
+bool IsGaiaWithSkipSavePasswordForm(const blink::WebFormElement& form);
+
 typedef std::map<
     const blink::WebFormControlElement,
     std::pair<std::unique_ptr<base::string16>, FieldPropertiesMask>>

diff --git a/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc b/components/autofill/content/renderer/password_form_conversion_utils_browsertest.cc
@@ -20,6 +20,7 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "content/public/test/render_view_test.h"
+#include "google_apis/gaia/gaia_urls.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/platform/WebVector.h"
@@ -2228,6 +2229,40 @@ TEST_F(MAYBE_PasswordFormConversionUtilsTest, IsGaiaReauthFormIgnored) {
   }
 }
 
+TEST_F(MAYBE_PasswordFormConversionUtilsTest, IsGaiaWithSkipSavePasswordForm) {
+  struct TestCase {
+    const char* origin;
+    bool expected_form_has_skip_save_password;
+  } cases[] = {
+      // A common password form is parsed successfully.
+      {"https://example.com", false},
+      // A common GAIA sign-in page, with no skip save password argument.
+      {"https://accounts.google.com", false},
+      // A common GAIA sign-in page, with "0" skip save password argument.
+      {"https://accounts.google.com/?ssp=0", false},
+      // A common GAIA sign-in page, with skip save password argument.
+      {"https://accounts.google.com/?ssp=1", true},
+      // The Gaia page that is used to start a Chrome sign-in flow when Desktop
+      // Identity Consistency is enable.
+      {GaiaUrls::GetInstance()->signin_chrome_sync_dice().spec().c_str(), true},
+  };
+
+  for (TestCase& test_case : cases) {
+    SCOPED_TRACE(testing::Message("origin=")
+                 << test_case.origin
+                 << ", expected_form_has_skip_save_password="
+                 << test_case.expected_form_has_skip_save_password);
+    std::unique_ptr<PasswordFormBuilder> builder(new PasswordFormBuilder(""));
+    builder->AddTextField("username", "", nullptr);
+    builder->AddPasswordField("password", "", nullptr);
+    std::string html = builder->ProduceHTML();
+    WebFormElement form;
+    LoadWebFormFromHTML(html, &form, test_case.origin);
+    EXPECT_EQ(test_case.expected_form_has_skip_save_password,
+              IsGaiaWithSkipSavePasswordForm(form));
+  }
+}
+
 TEST_F(MAYBE_PasswordFormConversionUtilsTest,
        IdentifyingFieldsWithoutNameOrIdAttributes) {
   const char* kEmpty = nullptr;

diff --git a/components/password_manager/core/browser/credentials_filter.h b/components/password_manager/core/browser/credentials_filter.h
@@ -25,8 +25,7 @@ class CredentialsFilter {
       std::vector<std::unique_ptr<autofill::PasswordForm>> results) const = 0;
 
   // Should |form| be offered to be saved?
-  virtual bool ShouldSave(const autofill::PasswordForm& form,
-                          const GURL& main_frame_url) const = 0;
+  virtual bool ShouldSave(const autofill::PasswordForm& form) const = 0;
 
   // Call this if the form associated with |form_manager| was filled, and the
   // subsequent sign-in looked like a success.

diff --git a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc b/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
@@ -78,13 +78,12 @@ class NameFilter : public StubCredentialsFilter {
   std::vector<std::unique_ptr<PasswordForm>> FilterResults(
       std::vector<std::unique_ptr<PasswordForm>> results) const override {
     base::EraseIf(results, [this](const std::unique_ptr<PasswordForm>& form) {
-      return !ShouldSave(*form, form->origin);
+      return !ShouldSave(*form);
     });
     return results;
   }
 
-  bool ShouldSave(const PasswordForm& form,
-                  const GURL& main_frame_url) const override {
+  bool ShouldSave(const PasswordForm& form) const override {
     return form.username_value != name_;
   }
 

diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
@@ -856,8 +856,7 @@ void PasswordManager::OnLoginSuccessful() {
 
   DCHECK(provisional_save_manager_->submitted_form());
   if (!client_->GetStoreResultFilter()->ShouldSave(
-          *provisional_save_manager_->submitted_form(),
-          client_->GetMainFrameURL())) {
+          *provisional_save_manager_->submitted_form())) {
 #if defined(SYNC_PASSWORD_REUSE_DETECTION_ENABLED)
     // When |username_value| is empty, it's not clear whether the submitted
     // credentials are really sync credentials. Don't save sync password hash

diff --git a/components/password_manager/core/browser/password_manager_unittest.cc b/components/password_manager/core/browser/password_manager_unittest.cc
@@ -48,9 +48,7 @@ namespace {
 
 class MockStoreResultFilter : public StubCredentialsFilter {
  public:
-  MOCK_CONST_METHOD2(ShouldSave,
-                     bool(const autofill::PasswordForm& form,
-                          const GURL& main_frame_url));
+  MOCK_CONST_METHOD1(ShouldSave, bool(const autofill::PasswordForm& form));
   MOCK_CONST_METHOD1(ReportFormLoginSuccess,
                      void(const PasswordFormManager& form_manager));
 };
@@ -61,7 +59,7 @@ class MockPasswordManagerClient : public StubPasswordManagerClient {
     EXPECT_CALL(*this, GetStoreResultFilter())
         .Times(AnyNumber())
         .WillRepeatedly(Return(&filter_));
-    ON_CALL(filter_, ShouldSave(_, _)).WillByDefault(Return(true));
+    ON_CALL(filter_, ShouldSave(_)).WillByDefault(Return(true));
   }
 
   MOCK_CONST_METHOD0(IsSavingAndFillingEnabledForCurrentPage, bool());
@@ -100,7 +98,7 @@ class MockPasswordManagerClient : public StubPasswordManagerClient {
   }
 
   void FilterAllResultsForSaving() {
-    EXPECT_CALL(filter_, ShouldSave(_, _)).WillRepeatedly(Return(false));
+    EXPECT_CALL(filter_, ShouldSave(_)).WillRepeatedly(Return(false));
   }
 
  private:
@@ -809,7 +807,7 @@ TEST_F(PasswordManagerTest, ReportFormLoginSuccessAndShouldSaveCalled) {
 
   PasswordForm submitted_form = observed_form;
   submitted_form.preferred = true;
-  EXPECT_CALL(*client_.GetStoreResultFilter(), ShouldSave(submitted_form, _));
+  EXPECT_CALL(*client_.GetStoreResultFilter(), ShouldSave(submitted_form));
   EXPECT_CALL(*store_, UpdateLogin(_));
   observed.clear();
   manager()->OnPasswordFormsParsed(&driver_, observed);

diff --git a/components/password_manager/core/browser/stub_credentials_filter.cc b/components/password_manager/core/browser/stub_credentials_filter.cc
@@ -17,8 +17,8 @@ StubCredentialsFilter::FilterResults(
   return results;
 }
 
-bool StubCredentialsFilter::ShouldSave(const autofill::PasswordForm& form,
-                                       const GURL& main_frame_url) const {
+bool StubCredentialsFilter::ShouldSave(
+    const autofill::PasswordForm& form) const {
   return true;
 }
 

diff --git a/components/password_manager/core/browser/stub_credentials_filter.h b/components/password_manager/core/browser/stub_credentials_filter.h
@@ -22,8 +22,7 @@ class StubCredentialsFilter : public CredentialsFilter {
   std::vector<std::unique_ptr<autofill::PasswordForm>> FilterResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results)
       const override;
-  bool ShouldSave(const autofill::PasswordForm& form,
-                  const GURL& main_frame_url) const override;
+  bool ShouldSave(const autofill::PasswordForm& form) const override;
   void ReportFormLoginSuccess(
       const PasswordFormManager& form_manager) const override;
 

diff --git a/components/password_manager/sync/browser/sync_credentials_filter.cc b/components/password_manager/sync/browser/sync_credentials_filter.cc
@@ -14,7 +14,6 @@
 #include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/sync/browser/password_sync_util.h"
-#include "google_apis/gaia/gaia_auth_util.h"
 #include "google_apis/gaia/gaia_urls.h"
 #include "net/base/url_util.h"
 
@@ -65,7 +64,7 @@ std::vector<std::unique_ptr<PasswordForm>> SyncCredentialsFilter::FilterResults(
   auto begin_of_removed =
       std::partition(results.begin(), results.end(),
                      [this](const std::unique_ptr<PasswordForm>& form) {
-                       return ShouldSave(*form, form->origin);
+                       return ShouldSave(*form);
                      });
 
   UMA_HISTOGRAM_BOOLEAN("PasswordManager.SyncCredentialFiltered",
@@ -76,12 +75,11 @@ std::vector<std::unique_ptr<PasswordForm>> SyncCredentialsFilter::FilterResults(
   return results;
 }
 
-bool SyncCredentialsFilter::ShouldSave(const autofill::PasswordForm& form,
-                                       const GURL& main_frame_url) const {
-  return !gaia::ShouldSkipSavePasswordForGaiaURL(main_frame_url) &&
-         !sync_util::IsSyncAccountCredential(
-             form, sync_service_factory_function_.Run(),
-             signin_manager_factory_function_.Run(), client_->GetPrefs());
+bool SyncCredentialsFilter::ShouldSave(
+    const autofill::PasswordForm& form) const {
+  return !sync_util::IsSyncAccountCredential(
+      form, sync_service_factory_function_.Run(),
+      signin_manager_factory_function_.Run(), client_->GetPrefs());
 }
 
 void SyncCredentialsFilter::ReportFormLoginSuccess(

diff --git a/components/password_manager/sync/browser/sync_credentials_filter.h b/components/password_manager/sync/browser/sync_credentials_filter.h
@@ -43,8 +43,7 @@ class SyncCredentialsFilter : public CredentialsFilter {
   std::vector<std::unique_ptr<autofill::PasswordForm>> FilterResults(
       std::vector<std::unique_ptr<autofill::PasswordForm>> results)
       const override;
-  bool ShouldSave(const autofill::PasswordForm& form,
-                  const GURL& main_frame_url) const override;
+  bool ShouldSave(const autofill::PasswordForm& form) const override;
   void ReportFormLoginSuccess(
       const PasswordFormManager& form_manager) const override;
 

diff --git a/components/password_manager/sync/browser/sync_credentials_filter_unittest.cc b/components/password_manager/sync/browser/sync_credentials_filter_unittest.cc
@@ -28,7 +28,6 @@
 #include "components/password_manager/core/browser/stub_password_manager_driver.h"
 #include "components/password_manager/core/common/password_manager_features.h"
 #include "components/password_manager/sync/browser/sync_username_test_base.h"
-#include "google_apis/gaia/gaia_urls.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -340,29 +339,23 @@ TEST_F(CredentialsFilterTest, ShouldSave_NotSyncCredential) {
   ASSERT_NE("user@example.org",
             signin_manager()->GetAuthenticatedAccountInfo().email);
   SetSyncingPasswords(true);
-  EXPECT_TRUE(filter_.ShouldSave(form, GURL("https://example.org")));
+  EXPECT_TRUE(filter_.ShouldSave(form));
 }
 
 TEST_F(CredentialsFilterTest, ShouldSave_SyncCredential) {
   PasswordForm form = SimpleGaiaForm("user@example.org");
 
   FakeSigninAs("user@example.org");
   SetSyncingPasswords(true);
-  EXPECT_FALSE(filter_.ShouldSave(form, GURL("https://example.org")));
+  EXPECT_FALSE(filter_.ShouldSave(form));
 }
 
 TEST_F(CredentialsFilterTest, ShouldSave_SyncCredential_NotSyncingPasswords) {
   PasswordForm form = SimpleGaiaForm("user@example.org");
 
   FakeSigninAs("user@example.org");
   SetSyncingPasswords(false);
-  EXPECT_TRUE(filter_.ShouldSave(form, GURL("https://example.org")));
-}
-
-TEST_F(CredentialsFilterTest, ShouldSave_ChomeSigninURLForDice) {
-  PasswordForm form = SimpleGaiaForm("user@gmail.com");
-  EXPECT_FALSE(filter_.ShouldSave(
-      form, GaiaUrls::GetInstance()->signin_chrome_sync_dice()));
+  EXPECT_TRUE(filter_.ShouldSave(form));
 }
 
 TEST_F(CredentialsFilterTest, ShouldFilterOneForm) {

diff --git a/google_apis/gaia/gaia_auth_util.cc b/google_apis/gaia/gaia_auth_util.cc
@@ -15,7 +15,6 @@
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "google_apis/gaia/gaia_urls.h"
-#include "net/base/url_util.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request.h"
 #include "url/gurl.h"
@@ -195,14 +194,4 @@ void MarkURLFetcherAsGaia(net::URLFetcher* fetcher) {
                                  base::Bind(&GaiaURLRequestUserData::Create));
 }
 
-bool ShouldSkipSavePasswordForGaiaURL(const GURL& url) {
-  if (!IsGaiaSignonRealm(url.GetOrigin()))
-    return false;
-
-  std::string should_skip_password;
-  if (!net::GetValueForKeyInQuery(url, "ssp", &should_skip_password))
-    return false;
-  return should_skip_password == "1";
-}
-
 }  // namespace gaia
diff --git a/google_apis/gaia/gaia_auth_util.h b/google_apis/gaia/gaia_auth_util.h
@@ -52,13 +52,8 @@ bool AreEmailsSame(const std::string& email1, const std::string& email2);
 // Extract the domain part from the canonical form of the given email.
 std::string ExtractDomainName(const std::string& email);
 
-// Returns true if |url| matches the GAIA origin URL.
 bool IsGaiaSignonRealm(const GURL& url);
 
-// Returns true if the URL corresponds to a URL used during a Chrome sign-in
-// flow.
-bool ShouldSkipSavePasswordForGaiaURL(const GURL& url);
-
 // Parses JSON data returned by /ListAccounts call, returning a vector of
 // email/valid pairs.  An email addresses is considered valid if a passive
 // login would succeed (i.e. the user does not need to reauthenticate).

diff --git a/google_apis/gaia/gaia_urls.cc b/google_apis/gaia/gaia_urls.cc
@@ -22,8 +22,9 @@ const char kClientLoginUrlSuffix[] = "ClientLogin";
 const char kServiceLoginUrlSuffix[] = "ServiceLogin";
 const char kEmbeddedSetupChromeOsUrlSuffixV1[] = "embedded/setup/chromeos";
 const char kEmbeddedSetupChromeOsUrlSuffixV2[] = "embedded/setup/v2/chromeos";
-// Paramter "ssp=1" is used to skip showing the password bubble when a user
-// signs in to Chrome.
+// Parameter "ssp=1" is used to skip showing the password bubble when a user
+// signs in to Chrome. Note that Gaia will pass this client specified parameter
+// to all URLs that are loaded as part of thi sign-in flow.
 const char kSigninChromeSyncDice[] = "signin/chrome/sync?ssp=1";
 const char kServiceLoginAuthUrlSuffix[] = "ServiceLoginAuth";
 const char kServiceLogoutUrlSuffix[] = "Logout";