[Password Manager] Fix bug in crowdsourcing of |ACCOUNT_CREATION_PASSWORD| vote

When Chrome uploads |ACCOUNT_CREATION_PASSWORD| vote, the uploaded form data should
belong to the saved credential(i.e. |PasswordFormManager::pending_credentials|), but
not to |submitted_form_| or |observed_form_|. This CL fixes it.

The CL also changes how |password_element| is set in |CreatePendingCredentials|. If
|pending_credentials_| is an already saved credential, then |password_element| should
be from the saved credential, but not from the submitted form.

Bug: 552420
Change-Id: I84dec939e097f0356d3ad7af8cdf3dc9ec97e2a5
Reviewed-on: https://chromium-review.googlesource.com/1014105Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551322}

components/password_manager/core/browser/password_form_manager.cc

@@ -152,7 +152,7 @@ void SetFieldLabelsOnUpdate(const autofill::ServerFieldType password_type,
 // Sets the autofill type of the password field stored in |submitted_form| to
 // |password_type| in |field_types| map.
 void SetFieldLabelsOnSave(const autofill::ServerFieldType password_type,
-                          const autofill::PasswordForm& submitted_form,
+                          const autofill::PasswordForm& form,
                           FieldTypeMap* field_types) {
   DCHECK(password_type == autofill::PASSWORD ||
          password_type == autofill::PROBABLY_ACCOUNT_CREATION_PASSWORD ||
@@ -160,11 +160,11 @@ void SetFieldLabelsOnSave(const autofill::ServerFieldType password_type,
          password_type == autofill::NOT_ACCOUNT_CREATION_PASSWORD)
       << password_type;
 
-  if (!submitted_form.new_password_element.empty()) {
-    (*field_types)[submitted_form.new_password_element] = password_type;
+  if (!form.new_password_element.empty()) {
+    (*field_types)[form.new_password_element] = password_type;
   } else {
-    DCHECK(!submitted_form.password_element.empty());
-    (*field_types)[submitted_form.password_element] = password_type;
+    DCHECK(!form.password_element.empty());
+    (*field_types)[form.password_element] = password_type;
   }
 }
 
@@ -914,18 +914,18 @@ bool PasswordFormManager::UploadPasswordVote(
       autofill::AutofillUploadContents::Field::NO_INFORMATION;
   if (autofill_type != autofill::USERNAME) {
     if (has_autofill_vote) {
-      DCHECK(submitted_form_);
       bool is_update = autofill_type == autofill::NEW_PASSWORD ||
                        autofill_type == autofill::PROBABLY_NEW_PASSWORD ||
                        autofill_type == autofill::NOT_NEW_PASSWORD;
+
       if (is_update) {
-        if (submitted_form_->new_password_element.empty())
+        if (form_to_upload.new_password_element.empty())
           return false;
-        SetFieldLabelsOnUpdate(autofill_type, *submitted_form_, &field_types);
+        SetFieldLabelsOnUpdate(autofill_type, form_to_upload, &field_types);
       } else {  // Saving.
-        SetFieldLabelsOnSave(autofill_type, *submitted_form_, &field_types);
+        SetFieldLabelsOnSave(autofill_type, form_to_upload, &field_types);
       }
-      field_types[submitted_form_->confirmation_password_element] =
+      field_types[form_to_upload.confirmation_password_element] =
           autofill::CONFIRMATION_PASSWORD;
     }
     if (autofill_type != autofill::ACCOUNT_CREATION_PASSWORD) {
@@ -1146,7 +1146,7 @@ void PasswordFormManager::CreatePendingCredentials() {
       // If a password was generated and we didn't find a match, we have to save
       // it in a separate entry since we have to store it but we don't know
       // where.
-      CreatePendingCredentialsForNewCredentials();
+      CreatePendingCredentialsForNewCredentials(password_to_save.second);
       is_new_login_ = true;
     } else {
       // We don't have a good candidate to choose as the default credential for
@@ -1159,7 +1159,7 @@ void PasswordFormManager::CreatePendingCredentials() {
     is_new_login_ = true;
     // No stored credentials can be matched to the submitted form. Offer to
     // save new credentials.
-    CreatePendingCredentialsForNewCredentials();
+    CreatePendingCredentialsForNewCredentials(password_to_save.second);
     // Generate username correction votes.
     bool username_correction_found = FindCorrectedUsernameElement(
         submitted_form_->username_value, submitted_form_->password_value);
@@ -1174,7 +1174,6 @@ void PasswordFormManager::CreatePendingCredentials() {
 
   if (!IsValidAndroidFacetURI(pending_credentials_.signon_realm)) {
     pending_credentials_.action = submitted_form_->action;
-    pending_credentials_.password_element = password_to_save.second;
     // If the user selected credentials we autofilled from a PasswordForm
     // that contained no action URL (IE6/7 imported passwords, for example),
     // bless it with the action URL from the observed form. See b/1107719.
@@ -1348,7 +1347,8 @@ const PasswordForm* PasswordFormManager::FindBestSavedMatch(
   return nullptr;
 }
 
-void PasswordFormManager::CreatePendingCredentialsForNewCredentials() {
+void PasswordFormManager::CreatePendingCredentialsForNewCredentials(
+    const base::string16& password_element) {
   // User typed in a new, unknown username.
   SetUserAction(UserAction::kOverrideUsernameAndPassword);
   pending_credentials_ = observed_form_;
@@ -1361,7 +1361,9 @@ void PasswordFormManager::CreatePendingCredentialsForNewCredentials() {
 
   // The password value will be filled in later, remove any garbage for now.
   pending_credentials_.password_value.clear();
-  pending_credentials_.password_element.clear();
+  // The password element should be determined earlier in |PasswordToSave|.
+  pending_credentials_.password_element = password_element;
+  // The new password's value and element name should be empty.
   pending_credentials_.new_password_value.clear();
   pending_credentials_.new_password_element.clear();
 }

components/password_manager/core/browser/password_form_manager.h

@@ -411,7 +411,8 @@ class PasswordFormManager : public FormFetcher::Consumer {
 
   // Create pending credentials from provisionally saved form when this form
   // represents credentials that were not previosly saved.
-  void CreatePendingCredentialsForNewCredentials();
+  void CreatePendingCredentialsForNewCredentials(
+      const base::string16& password_element);
 
   // If |best_matches_| contains only one entry, then return this entry.
   // Otherwise for empty |password| return nullptr and for non-empty |password|

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -478,6 +478,7 @@ class PasswordFormManagerTest : public testing::Test {
     PasswordForm form_to_save(form);
     form_to_save.preferred = true;
     form_to_save.username_element = ASCIIToUTF16("observed-username-field");
+    form_to_save.password_element = ASCIIToUTF16("observed-password-field");
     form_to_save.username_value = match.username_value;
     form_to_save.password_value = match.password_value;
 
@@ -1241,7 +1242,7 @@ TEST_F(PasswordFormManagerTest, TestUpdatePasswordFromNewPasswordElement) {
   // The password should be updated.
   EXPECT_EQ(credentials.new_password_value, new_credentials.password_value);
   EXPECT_EQ(saved_match()->username_element, new_credentials.username_element);
-  EXPECT_EQ(credentials.new_password_element, new_credentials.password_element);
+  EXPECT_EQ(saved_match()->password_element, new_credentials.password_element);
   EXPECT_EQ(saved_match()->submit_element, new_credentials.submit_element);
 }
 
@@ -2023,13 +2024,13 @@ TEST_F(PasswordFormManagerTest, UploadFormData_NewPassword_Blacklist) {
 TEST_F(PasswordFormManagerTest, UploadPasswordForm) {
   autofill::FormData observed_form_data;
   autofill::FormFieldData field;
-  field.label = ASCIIToUTF16("Email");
+  field.label = ASCIIToUTF16("Email:");
   field.name = ASCIIToUTF16("observed-username-field");
   field.form_control_type = "text";
   observed_form_data.fields.push_back(field);
 
-  field.label = ASCIIToUTF16("password");
-  field.name = ASCIIToUTF16("password");
+  field.label = ASCIIToUTF16("Password:");
+  field.name = ASCIIToUTF16("observed-password-field");
   field.form_control_type = "password";
   observed_form_data.fields.push_back(field);
 
@@ -2310,9 +2311,9 @@ TEST_F(PasswordFormManagerTest, TestUpdateNoUsernameTextfieldPresent) {
   EXPECT_EQ(saved_match()->username_value, new_credentials.username_value);
   EXPECT_EQ(credentials.new_password_value, new_credentials.password_value);
   EXPECT_EQ(saved_match()->username_element, new_credentials.username_element);
-  EXPECT_EQ(credentials.new_password_element, new_credentials.password_element);
-  EXPECT_EQ(base::string16(), new_credentials.new_password_value);
-  EXPECT_EQ(base::string16(), new_credentials.new_password_element);
+  EXPECT_EQ(saved_match()->password_element, new_credentials.password_element);
+  EXPECT_TRUE(new_credentials.new_password_value.empty());
+  EXPECT_TRUE(new_credentials.new_password_element.empty());
   EXPECT_EQ(saved_match()->submit_element, new_credentials.submit_element);
 }