[Passwords] Fixing sending votes for dynamic password forms.

VotesUploader uploades password votes on form submission. These votes
are used to generate server predictions that are requested and received
on the pageload. Thus if the form is changed dynamically, it makes sense
to send votes for the form signature that is seen on the pageload.
This CL starts using initial form signatures instead of signatures
seen at the moment of submission.

Bug: 1157048
Change-Id: I13f51852c2b9447625ae6d20cd335e481dcab4e8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578900Reviewed-by: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Maria Kazinova <kazinova@google.com>
Cr-Commit-Position: refs/heads/master@{#835202}

components/autofill_assistant/browser/website_login_manager_impl.cc

@@ -180,7 +180,9 @@ class WebsiteLoginManagerImpl::UpdatePasswordRequest
                 client->IsCommittedMainFrameSecure(),
                 client->GetUkmSourceId(),
                 client->GetPrefs())),
-        votes_uploader_(client, true /* is_possible_change_password_form */) {
+        votes_uploader_(client,
+                        true /* is_possible_change_password_form */,
+                        CalculateFormSignature(form_data_)) {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
     password_manager::PasswordStore::FormDigest digest(

components/password_manager/core/browser/multi_store_password_save_manager_unittest.cc

@@ -77,7 +77,8 @@ class MultiStorePasswordSaveManagerTest : public testing::Test {
  public:
   MultiStorePasswordSaveManagerTest()
       : votes_uploader_(&client_,
-                        false /* is_possible_change_password_form */) {
+                        false /* is_possible_change_password_form */,
+                        autofill::FormSignature()) {
     GURL origin = GURL("https://accounts.google.com/a/ServiceLoginAuth");
     GURL action = GURL("https://accounts.google.com/a/ServiceLogin");
     GURL psl_origin = GURL("https://myaccounts.google.com/a/ServiceLoginAuth");
@@ -152,6 +153,9 @@ class MultiStorePasswordSaveManagerTest : public testing::Test {
         client_.IsCommittedMainFrameSecure(), client_.GetUkmSourceId(),
         /*pref_service=*/nullptr);
 
+    votes_uploader_.set_initial_form_signature(
+        CalculateFormSignature(observed_form_));
+
     auto mock_profile_form_saver = std::make_unique<NiceMock<MockFormSaver>>();
     mock_profile_form_saver_ = mock_profile_form_saver.get();
 

components/password_manager/core/browser/password_form_manager.cc

@@ -894,7 +894,10 @@ PasswordFormManager::PasswordFormManager(
       password_save_manager_(std::move(password_save_manager)),
       // TODO(https://crbug.com/831123): set correctly
       // |is_possible_change_password_form| in |votes_uploader_| constructor
-      votes_uploader_(client, false /* is_possible_change_password_form */) {
+      votes_uploader_(client,
+                      false /* is_possible_change_password_form */,
+                      observed_form() ? CalculateFormSignature(*observed_form())
+                                      : FormSignature()) {
   form_fetcher_->AddConsumer(this);
   if (!metrics_recorder_) {
     metrics_recorder_ = base::MakeRefCounted<PasswordFormMetricsRecorder>(

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -2847,6 +2847,30 @@ TEST_F(PasswordFormManagerTestWithMockedSaver,
   EXPECT_TRUE(parsed_submitted_form.username_value.empty());
 }
 
+// Tests that if the initially observed form differs from the submitted form,
+// voting data should contain the initial form signature.
+TEST_P(PasswordFormManagerTest, VotingOnDynamicallyChangedForm) {
+  PasswordForm initial_form;
+  initial_form.form_data = non_password_form_;
+  CreateFormManager(non_password_form_);
+  fetcher_->NotifyFetchCompleted();
+
+  // Simulate observing form change.
+  form_manager_->FillForm(observed_form_, {});
+
+  // Simulate form filling and submission.
+  EXPECT_TRUE(
+      form_manager_->ProvisionallySave(submitted_form_, &driver_, nullptr));
+
+  EXPECT_CALL(
+      mock_autofill_download_manager_,
+      StartUploadRequest(SignatureIsSameAs(initial_form), _, _, _, _, _));
+
+  EXPECT_TRUE(CalculateFormSignature(initial_form.form_data).value() !=
+              CalculateFormSignature(submitted_form_).value());
+  form_manager_->Save();
+}
+
 }  // namespace
 
 }  // namespace password_manager

components/password_manager/core/browser/password_save_manager_impl_unittest.cc

@@ -198,7 +198,9 @@ class PasswordSaveManagerImplTest : public testing::Test,
                                     public testing::WithParamInterface<bool> {
  public:
   PasswordSaveManagerImplTest()
-      : votes_uploader_(&client_, false /* is_possible_change_password_form */),
+      : votes_uploader_(&client_,
+                        false /* is_possible_change_password_form */,
+                        autofill::FormSignature()),
         task_runner_(new TestMockTimeTaskRunner) {
     GURL origin = GURL("https://accounts.google.com/a/ServiceLoginAuth");
     GURL action = GURL("https://accounts.google.com/a/ServiceLogin");
@@ -302,6 +304,9 @@ class PasswordSaveManagerImplTest : public testing::Test,
     auto mock_form_saver = std::make_unique<NiceMock<MockFormSaver>>();
     mock_form_saver_ = mock_form_saver.get();
 
+    votes_uploader_.set_initial_form_signature(
+        CalculateFormSignature(observed_form_));
+
     if (!GetParam()) {
       password_save_manager_impl_ =
           std::make_unique<PasswordSaveManagerImpl>(std::move(mock_form_saver));

components/password_manager/core/browser/votes_uploader.cc

@@ -189,9 +189,11 @@ SingleUsernameVoteData::SingleUsernameVoteData(SingleUsernameVoteData&& other) =
 SingleUsernameVoteData::~SingleUsernameVoteData() = default;
 
 VotesUploader::VotesUploader(PasswordManagerClient* client,
-                             bool is_possible_change_password_form)
+                             bool is_possible_change_password_form,
+                             autofill::FormSignature form_signature)
     : client_(client),
-      is_possible_change_password_form_(is_possible_change_password_form) {}
+      is_possible_change_password_form_(is_possible_change_password_form),
+      initial_observed_form_signature_(form_signature) {}
 
 VotesUploader::VotesUploader(const VotesUploader& other) = default;
 VotesUploader::~VotesUploader() = default;
@@ -298,6 +300,7 @@ bool VotesUploader::UploadPasswordVote(
   // re-uses credentials, a vote about the saved form is sent. If the user saves
   // credentials, the observed and pending forms are the same.
   FormStructure form_structure(form_to_upload.form_data);
+  form_structure.set_form_signature(initial_observed_form_signature_);
   form_structure.set_submission_event(submitted_form.submission_event);
 
   ServerFieldTypeSet available_field_types;
@@ -402,6 +405,7 @@ void VotesUploader::UploadFirstLoginVotes(
   }
 
   FormStructure form_structure(form_to_upload.form_data);
+  form_structure.set_form_signature(initial_observed_form_signature_);
   form_structure.set_submission_event(form_to_upload.submission_event);
 
   FieldTypeMap field_types = {

components/password_manager/core/browser/votes_uploader.h

@@ -54,7 +54,8 @@ struct SingleUsernameVoteData {
 class VotesUploader {
  public:
   VotesUploader(PasswordManagerClient* client,
-                bool is_possible_change_password_form);
+                bool is_possible_change_password_form,
+                autofill::FormSignature form_signature);
   VotesUploader(const VotesUploader& other);
   ~VotesUploader();
 
@@ -168,6 +169,11 @@ class VotesUploader {
     single_username_vote_data_.emplace(renderer_id, form_predictions);
   }
 
+  void set_initial_form_signature(
+      autofill::FormSignature observed_form_signature) {
+    initial_observed_form_signature_ = observed_form_signature;
+  }
+
  private:
   // The outcome of the form classifier.
   enum FormClassifierOutcome {
@@ -245,6 +251,10 @@ class VotesUploader {
   // observed form.
   std::map<autofill::FieldRendererId, base::string16> initial_values_;
 
+  // Form signature of the initially observed form. Used to send votes on
+  // dynamic password forms.
+  autofill::FormSignature initial_observed_form_signature_;
+
   base::Optional<SingleUsernameVoteData> single_username_vote_data_;
 };
 

components/password_manager/core/browser/votes_uploader_unittest.cc

@@ -30,6 +30,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using autofill::AutofillDownloadManager;
+using autofill::CalculateFormSignature;
 using autofill::CONFIRMATION_PASSWORD;
 using autofill::FieldSignature;
 using autofill::FormData;
@@ -128,6 +129,7 @@ class VotesUploaderTest : public testing::Test {
       form_to_upload_.form_data.fields.push_back(field);
       submitted_form_.form_data.fields.push_back(field);
     }
+    initial_form_signature_ = CalculateFormSignature(form_to_upload_.form_data);
   }
 
  protected:
@@ -144,10 +146,11 @@ class VotesUploaderTest : public testing::Test {
   PasswordForm submitted_form_;
 
   std::string login_form_signature_ = "123";
+  FormSignature initial_form_signature_;
 };
 
 TEST_F(VotesUploaderTest, UploadPasswordVoteUpdate) {
-  VotesUploader votes_uploader(&client_, true);
+  VotesUploader votes_uploader(&client_, true, initial_form_signature_);
   base::string16 new_password_element = GetFieldNameByIndex(3);
   base::string16 confirmation_element = GetFieldNameByIndex(11);
   form_to_upload_.new_password_element = new_password_element;
@@ -176,7 +179,7 @@ TEST_F(VotesUploaderTest, UploadPasswordVoteUpdate) {
 }
 
 TEST_F(VotesUploaderTest, UploadPasswordVoteSave) {
-  VotesUploader votes_uploader(&client_, false);
+  VotesUploader votes_uploader(&client_, false, initial_form_signature_);
   base::string16 password_element = GetFieldNameByIndex(5);
   base::string16 confirmation_element = GetFieldNameByIndex(12);
   form_to_upload_.password_element = password_element;
@@ -220,7 +223,8 @@ TEST_F(VotesUploaderTest, InitialValueDetection) {
 
   form_data.fields = {other_field, username_field};
 
-  VotesUploader votes_uploader(&client_, true);
+  VotesUploader votes_uploader(&client_, true,
+                               CalculateFormSignature(form_data));
   votes_uploader.StoreInitialFieldValues(form_data);
 
   form_data.fields.at(1).value = ASCIIToUTF16("user entered value");
@@ -246,7 +250,7 @@ TEST_F(VotesUploaderTest, InitialValueDetection) {
 }
 
 TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote) {
-  VotesUploader votes_uploader(&client_, true);
+  VotesUploader votes_uploader(&client_, true, initial_form_signature_);
   // Checks that randomization distorts information about present and missed
   // character classes, but a true value is still restorable with aggregation
   // of many distorted reports.
@@ -314,7 +318,7 @@ TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote) {
 }
 
 TEST_F(VotesUploaderTest, GeneratePasswordSpecialSymbolVote) {
-  VotesUploader votes_uploader(&client_, true);
+  VotesUploader votes_uploader(&client_, true, initial_form_signature_);
 
   const base::string16 password_value = ASCIIToUTF16("password-withsymbols!");
   const int kNumberOfRuns = 2000;
@@ -360,7 +364,7 @@ TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote_OneCharacterPassword) {
   // password has only one character.
   FormData form;
   FormStructure form_structure(form);
-  VotesUploader votes_uploader(&client_, true);
+  VotesUploader votes_uploader(&client_, true, CalculateFormSignature(form));
   votes_uploader.GeneratePasswordAttributesVote(ASCIIToUTF16("1"),
                                                 &form_structure);
   base::Optional<std::pair<PasswordAttribute, bool>> vote =
@@ -373,7 +377,7 @@ TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote_OneCharacterPassword) {
 TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote_AllAsciiCharacters) {
   FormData form;
   FormStructure form_structure(form);
-  VotesUploader votes_uploader(&client_, true);
+  VotesUploader votes_uploader(&client_, true, CalculateFormSignature(form));
   votes_uploader.GeneratePasswordAttributesVote(
       base::UTF8ToUTF16("0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqr"
                         "stuvwxyz!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"),
@@ -392,7 +396,7 @@ TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote_NonAsciiPassword) {
         "ລະຫັດຜ່ານ-l", "စကားဝှက်ကို3", "პაროლი", "पारण शब्द"}) {
     FormData form;
     FormStructure form_structure(form);
-    VotesUploader votes_uploader(&client_, true);
+    VotesUploader votes_uploader(&client_, true, CalculateFormSignature(form));
     votes_uploader.GeneratePasswordAttributesVote(base::UTF8ToUTF16(password),
                                                   &form_structure);
     base::Optional<std::pair<PasswordAttribute, bool>> vote =
@@ -403,7 +407,7 @@ TEST_F(VotesUploaderTest, GeneratePasswordAttributesVote_NonAsciiPassword) {
 }
 
 TEST_F(VotesUploaderTest, NoSingleUsernameDataNoUpload) {
-  VotesUploader votes_uploader(&client_, false);
+  VotesUploader votes_uploader(&client_, false, initial_form_signature_);
   EXPECT_CALL(mock_autofill_download_manager_,
               StartUploadRequest(_, _, _, _, _, _))
       .Times(0);
@@ -413,7 +417,6 @@ TEST_F(VotesUploaderTest, NoSingleUsernameDataNoUpload) {
 TEST_F(VotesUploaderTest, UploadSingleUsername) {
   for (bool credentials_saved : {false, true}) {
     SCOPED_TRACE(testing::Message("credentials_saved = ") << credentials_saved);
-    VotesUploader votes_uploader(&client_, false);
 
     MockFieldInfoManager mock_field_manager;
     ON_CALL(mock_field_manager, GetFieldType(_, _))
@@ -425,6 +428,8 @@ TEST_F(VotesUploaderTest, UploadSingleUsername) {
     constexpr FieldSignature kUsernameFieldSignature(1234);
     constexpr FormSignature kFormSignature(1000);
 
+    VotesUploader votes_uploader(&client_, false, kFormSignature);
+
     FormPredictions form_predictions;
     form_predictions.form_signature = kFormSignature;
     // Add a non-username field.
@@ -454,11 +459,12 @@ TEST_F(VotesUploaderTest, UploadSingleUsername) {
 }
 
 TEST_F(VotesUploaderTest, SaveSingleUsernameVote) {
-  VotesUploader votes_uploader(&client_, false);
   constexpr autofill::FieldRendererId kUsernameRendererId(101);
   constexpr autofill::FieldSignature kUsernameFieldSignature(1234);
   constexpr autofill::FormSignature kFormSignature(1000);
 
+  VotesUploader votes_uploader(&client_, false, kFormSignature);
+
   FormPredictions form_predictions;
   form_predictions.form_signature = kFormSignature;
 
@@ -493,11 +499,12 @@ TEST_F(VotesUploaderTest, SaveSingleUsernameVote) {
 }
 
 TEST_F(VotesUploaderTest, DontUploadSingleUsernameWhenAlreadyUploaded) {
-  VotesUploader votes_uploader(&client_, false);
   constexpr autofill::FieldRendererId kUsernameRendererId(101);
   constexpr autofill::FieldSignature kUsernameFieldSignature(1234);
   constexpr autofill::FormSignature kFormSignature(1000);
 
+  VotesUploader votes_uploader(&client_, false, kFormSignature);
+
   MockFieldInfoManager mock_field_manager;
   ON_CALL(client_, GetFieldInfoManager())
       .WillByDefault(Return(&mock_field_manager));
@@ -523,4 +530,32 @@ TEST_F(VotesUploaderTest, DontUploadSingleUsernameWhenAlreadyUploaded) {
   votes_uploader.MaybeSendSingleUsernameVote(true /*credentials_saved*/);
 }
 
+// Tests that if the initial form signature stored by the votes uploader
+// differs from the form signature of the submitted form, upload
+// data should contain the initial form signature.
+TEST_F(VotesUploaderTest, UploadPasswordVoteOnDynamicallyChangedForm) {
+  PasswordForm initial_form;
+  FormData initial_form_data;
+  initial_form_data.name = base::ASCIIToUTF16("some_form");
+  FormFieldData initial_field_data;
+  initial_field_data.name = base::ASCIIToUTF16("some_field");
+  initial_form_data.fields.push_back(initial_field_data);
+  initial_form.form_data = initial_form_data;
+
+  // Create votes uploader and store the initially observed signature.
+  FormSignature initial_signature = CalculateFormSignature(initial_form_data);
+  VotesUploader votes_uploader(&client_, false, initial_signature);
+
+  EXPECT_CALL(
+      mock_autofill_download_manager_,
+      StartUploadRequest(SignatureIsSameAs(initial_form), _, _, _, _, _));
+
+  EXPECT_TRUE(initial_signature.value() !=
+              CalculateFormSignature(form_to_upload_.form_data).value());
+
+  // Upload password votes on a changed form.
+  EXPECT_TRUE(votes_uploader.UploadPasswordVote(
+      form_to_upload_, submitted_form_, PASSWORD, login_form_signature_));
+}
+
 }  // namespace password_manager