[Passwords] Fill/Preview password from correct store

The password dropdown keys credentials by username. For two suggestions
with the same username (i.e. a locally and a account-stored one), the
first one in the list would match the selection of either entry.

This CL fixes that issue by checking from which store the selected entry
originated from. Therefore, the credential that is second in list can be
previewed and filled as expected.

(See linked bug for screenshot.)

Bug: 1060552
Change-Id: Icb13d4af705fc0cac5609c0a359ac0dc0bbaf8ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2119526Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Friedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#754848}

chrome/android/features/keyboard_accessory/internal/java/src/org/chromium/chrome/browser/keyboard_accessory/bar_component/KeyboardAccessoryMediator.java

@@ -145,6 +145,8 @@ class KeyboardAccessoryMediator
             case PopupItemId.ITEM_ID_TITLE:
             case PopupItemId.ITEM_ID_USERNAME_ENTRY:
             case PopupItemId.ITEM_ID_CREATE_HINT:
+            case PopupItemId.ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY:
+            case PopupItemId.ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY:
                 return true;
         }
         return true; // If it's not a special id, show the regular suggestion!

chrome/browser/ui/autofill/autofill_popup_controller_impl.cc

@@ -437,6 +437,8 @@ bool AutofillPopupControllerImpl::HasSuggestions() {
   return id > 0 || id == POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY ||
          id == POPUP_ITEM_ID_PASSWORD_ENTRY ||
          id == POPUP_ITEM_ID_USERNAME_ENTRY ||
+         id == POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY ||
+         id == POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY ||
          id == POPUP_ITEM_ID_DATALIST_ENTRY ||
          id == POPUP_ITEM_ID_SCAN_CREDIT_CARD;
 }

chrome/browser/ui/views/autofill/autofill_popup_view_native_views.cc

@@ -1190,6 +1190,8 @@ void AutofillPopupViewNativeViews::CreateChildViews() {
 
       case autofill::PopupItemId::POPUP_ITEM_ID_USERNAME_ENTRY:
       case autofill::PopupItemId::POPUP_ITEM_ID_PASSWORD_ENTRY:
+      case autofill::PopupItemId::POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY:
+      case autofill::PopupItemId::POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY:
         rows_.push_back(PasswordPopupSuggestionView::Create(this, line_number,
                                                             frontend_id));
         break;

components/autofill/core/browser/autofill_external_delegate.cc

@@ -235,7 +235,9 @@ void AutofillExternalDelegate::DidAcceptSuggestion(const base::string16& value,
     AutofillMetrics::LogAutofillFormCleared();
     driver_->RendererShouldClearFilledSection();
   } else if (identifier == POPUP_ITEM_ID_PASSWORD_ENTRY ||
-             identifier == POPUP_ITEM_ID_USERNAME_ENTRY) {
+             identifier == POPUP_ITEM_ID_USERNAME_ENTRY ||
+             identifier == POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY ||
+             identifier == POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY) {
     NOTREACHED();  // Should be handled elsewhere.
   } else if (identifier == POPUP_ITEM_ID_DATALIST_ENTRY) {
     driver_->RendererShouldAcceptDataListSuggestion(value);

components/autofill/core/browser/ui/popup_item_ids.h

@@ -32,6 +32,8 @@ enum PopupItemId {
   POPUP_ITEM_ID_USE_VIRTUAL_CARD = -18,
   POPUP_ITEM_ID_LOADING_SPINNER = -20,
   POPUP_ITEM_ID_PASSWORD_ACCOUNT_STORAGE_OPT_IN_AND_GENERATE = -21,
+  POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY = -22,
+  POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY = -23,
 };
 
 }  // namespace autofill

components/password_manager/core/browser/password_autofill_manager.cc

@@ -29,6 +29,7 @@
 #include "components/autofill/core/common/autofill_constants.h"
 #include "components/autofill/core/common/autofill_data_validation.h"
 #include "components/autofill/core/common/autofill_util.h"
+#include "components/autofill/core/common/password_form_fill_data.h"
 #include "components/favicon/core/favicon_util.h"
 #include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
 #include "components/password_manager/core/browser/password_feature_manager.h"
@@ -125,9 +126,16 @@ void AppendSuggestionIfMatching(
     suggestion.label = GetHumanReadableRealm(signon_realm);
     suggestion.additional_label =
         base::string16(password_length, kPasswordReplacementChar);
-    suggestion.frontend_id = is_password_field
-                                 ? autofill::POPUP_ITEM_ID_PASSWORD_ENTRY
-                                 : autofill::POPUP_ITEM_ID_USERNAME_ENTRY;
+    if (from_account_store) {
+      suggestion.frontend_id =
+          is_password_field
+              ? autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY
+              : autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY;
+    } else {
+      suggestion.frontend_id = is_password_field
+                                   ? autofill::POPUP_ITEM_ID_PASSWORD_ENTRY
+                                   : autofill::POPUP_ITEM_ID_USERNAME_ENTRY;
+    }
     suggestion.match =
         show_all || base::StartsWith(lower_suggestion, lower_contents,
                                      base::CompareCase::SENSITIVE)
@@ -314,7 +322,8 @@ void PasswordAutofillManager::DidSelectSuggestion(const base::string16& value,
       identifier ==
           autofill::POPUP_ITEM_ID_PASSWORD_ACCOUNT_STORAGE_OPT_IN_AND_GENERATE)
     return;
-  bool success = PreviewSuggestion(GetUsernameFromSuggestion(value));
+  bool success =
+      PreviewSuggestion(GetUsernameFromSuggestion(value), identifier);
   DCHECK(success);
 }
 
@@ -378,7 +387,7 @@ void PasswordAutofillManager::DidAcceptSuggestion(const base::string16& value,
     metrics_util::LogPasswordDropdownItemSelected(
         PasswordDropdownSelectedOption::kPassword,
         password_client_->IsIncognito());
-    bool success = FillSuggestion(GetUsernameFromSuggestion(value));
+    bool success = FillSuggestion(GetUsernameFromSuggestion(value), identifier);
     DCHECK(success);
   }
 
@@ -493,12 +502,12 @@ void PasswordAutofillManager::DidNavigateMainFrame() {
 
 bool PasswordAutofillManager::FillSuggestionForTest(
     const base::string16& username) {
-  return FillSuggestion(username);
+  return FillSuggestion(username, autofill::POPUP_ITEM_ID_PASSWORD_ENTRY);
 }
 
 bool PasswordAutofillManager::PreviewSuggestionForTest(
     const base::string16& username) {
-  return PreviewSuggestion(username);
+  return PreviewSuggestion(username, autofill::POPUP_ITEM_ID_PASSWORD_ENTRY);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -598,10 +607,12 @@ void PasswordAutofillManager::UpdatePopup(
   autofill_client_->UpdatePopup(suggestions, autofill::PopupType::kPasswords);
 }
 
-bool PasswordAutofillManager::FillSuggestion(const base::string16& username) {
+bool PasswordAutofillManager::FillSuggestion(const base::string16& username,
+                                             int item_id) {
   autofill::PasswordAndMetadata password_and_meta_data;
-  if (fill_data_ && GetPasswordAndMetadataForUsername(
-                        username, *fill_data_, &password_and_meta_data)) {
+  if (fill_data_ &&
+      GetPasswordAndMetadataForUsername(username, item_id, *fill_data_,
+                                        &password_and_meta_data)) {
     bool is_android_credential =
         FacetURI::FromPotentiallyInvalidSpec(password_and_meta_data.realm)
             .IsValidAndroidFacetURI();
@@ -613,11 +624,12 @@ bool PasswordAutofillManager::FillSuggestion(const base::string16& username) {
   return false;
 }
 
-bool PasswordAutofillManager::PreviewSuggestion(
-    const base::string16& username) {
+bool PasswordAutofillManager::PreviewSuggestion(const base::string16& username,
+                                                int item_id) {
   autofill::PasswordAndMetadata password_and_meta_data;
-  if (fill_data_ && GetPasswordAndMetadataForUsername(
-                        username, *fill_data_, &password_and_meta_data)) {
+  if (fill_data_ &&
+      GetPasswordAndMetadataForUsername(username, item_id, *fill_data_,
+                                        &password_and_meta_data)) {
     password_manager_driver_->PreviewSuggestion(
         username, password_and_meta_data.password);
     return true;
@@ -627,14 +639,20 @@ bool PasswordAutofillManager::PreviewSuggestion(
 
 bool PasswordAutofillManager::GetPasswordAndMetadataForUsername(
     const base::string16& current_username,
+    int item_id,
     const autofill::PasswordFormFillData& fill_data,
     autofill::PasswordAndMetadata* password_and_meta_data) {
   // TODO(dubroy): When password access requires some kind of authentication
   // (e.g. Keychain access on Mac OS), use |password_manager_client_| here to
   // fetch the actual password. See crbug.com/178358 for more context.
 
+  bool item_uses_account_store =
+      item_id == autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY ||
+      item_id == autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY;
+
   // Look for any suitable matches to current field text.
-  if (fill_data.username_field.value == current_username) {
+  if (fill_data.username_field.value == current_username &&
+      fill_data.uses_account_store == item_uses_account_store) {
     password_and_meta_data->password = fill_data.password_field.value;
     password_and_meta_data->realm = fill_data.preferred_realm;
     password_and_meta_data->uses_account_store = fill_data.uses_account_store;
@@ -645,7 +663,8 @@ bool PasswordAutofillManager::GetPasswordAndMetadataForUsername(
   auto iter = fill_data.additional_logins.find(current_username);
   if (iter != fill_data.additional_logins.end()) {
     *password_and_meta_data = iter->second;
-    return true;
+    return password_and_meta_data->uses_account_store ==
+           item_uses_account_store;
   }
 
   return false;

components/password_manager/core/browser/password_autofill_manager.h

@@ -133,22 +133,26 @@ class PasswordAutofillManager : public autofill::AutofillPopupDelegate {
   // Validates and forwards the given objects to the autofill client.
   void UpdatePopup(const std::vector<autofill::Suggestion>& suggestions);
 
-  // Attempts to fill the password associated with user name |username|, and
-  // returns true if it was successful.
-  bool FillSuggestion(const base::string16& username);
-
-  // Attempts to preview the password associated with user name |username|, and
-  // returns true if it was successful.
-  bool PreviewSuggestion(const base::string16& username);
-
-  // If |current_username| matches a username for one of the login mappings in
-  // |fill_data|, returns true and assigns the password and the original signon
+  // Attempts to find and fill the suggestions with the user name |username| and
+  // the |item_id| indicating the store (account-stored or local). Returns true
+  // if it was successful.
+  bool FillSuggestion(const base::string16& username, int item_id);
+
+  // Attempts to find and preview the suggestions with the user name |username|
+  // and the |item_id| indicating the store (account-stored or local). Returns
+  // true if it was successful.
+  bool PreviewSuggestion(const base::string16& username, int item_id);
+
+  // If one of the login mappings in |fill_data| matches |current_username| and
+  // |item_id| (indicating whether a credential is stored in account or
+  // locally), return true and assign the password and the original signon
   // realm to |password_and_meta_data|. Note that if the credential comes from
   // the same realm as the one we're filling to, the |realm| field will be left
   // empty, as this is the behavior of |PasswordFormFillData|.
   // Otherwise, returns false and leaves |password_and_meta_data| untouched.
   bool GetPasswordAndMetadataForUsername(
       const base::string16& current_username,
+      int item_id,
       const autofill::PasswordFormFillData& fill_data,
       autofill::PasswordAndMetadata* password_and_meta_data);
 

components/password_manager/core/browser/password_autofill_manager_unittest.cc

@@ -41,6 +41,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/geometry/rect_f.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/image/image_unittest_util.h"
 
 #if defined(OS_ANDROID)
@@ -54,6 +55,7 @@ const char kPasswordName[] = "password";
 
 const char kAliceUsername[] = "alice";
 const char kAlicePassword[] = "password";
+const char kAliceAccountStoredPassword[] = "account-stored-password";
 const char kAliceEmail[] = "alice@gmail.com";
 
 using autofill::PopupType;
@@ -63,6 +65,7 @@ using autofill::SuggestionVectorIdsAre;
 using autofill::SuggestionVectorLabelsAre;
 using autofill::SuggestionVectorValuesAre;
 using favicon_base::FaviconImageCallback;
+using gfx::test::AreImagesEqual;
 using testing::_;
 using testing::AllOf;
 using testing::ElementsAreArray;
@@ -346,6 +349,7 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
 
     // Load filling and favicon data.
     autofill::PasswordFormFillData data = CreateTestFormFillData();
+    data.uses_account_store = false;
     favicon::MockFaviconService favicon_service;
     EXPECT_CALL(client, GetFaviconService()).WillOnce(Return(&favicon_service));
     EXPECT_CALL(favicon_service, GetFaviconImageForPageURL(data.origin, _, _))
@@ -372,8 +376,7 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
         base::i18n::RIGHT_TO_LEFT, base::string16(), show_suggestion_options,
         gfx::RectF());
     ASSERT_GE(suggestions.size(), 1u);
-    EXPECT_TRUE(
-        gfx::test::AreImagesEqual(suggestions[0].custom_icon, kTestFavicon));
+    EXPECT_TRUE(AreImagesEqual(suggestions[0].custom_icon, kTestFavicon));
 
     EXPECT_CALL(*client.mock_driver(),
                 FillSuggestion(test_username_, test_password_));
@@ -394,7 +397,71 @@ TEST_F(PasswordAutofillManagerTest, ExternalDelegatePasswordSuggestions) {
   }
 }
 
-// Test that the popup is updated once remote suggestions are unlocked.
+// Test that suggestions are filled correctly if account-stored and local
+// suggestion have duplicates.
+TEST_F(PasswordAutofillManagerTest,
+       ExternalDelegateAccountStorePasswordSuggestions) {
+  for (bool is_suggestion_on_password_field : {false, true}) {
+    SCOPED_TRACE(testing::Message() << "is_suggestion_on_password_field = "
+                                    << is_suggestion_on_password_field);
+    TestPasswordManagerClient client;
+    NiceMock<MockAutofillClient> autofill_client;
+    InitializePasswordAutofillManager(&client, &autofill_client);
+
+    // Load filling data and account-stored duplicate with a different password.
+    autofill::PasswordFormFillData data = CreateTestFormFillData();
+    autofill::PasswordAndMetadata duplicate;
+    duplicate.password = base::ASCIIToUTF16(kAliceAccountStoredPassword);
+    duplicate.realm = data.preferred_realm;
+    duplicate.uses_account_store = true;
+    data.additional_logins[data.username_field.value] = duplicate;
+    favicon::MockFaviconService favicon_service;
+    EXPECT_CALL(client, GetFaviconService()).WillOnce(Return(&favicon_service));
+    EXPECT_CALL(favicon_service, GetFaviconImageForPageURL(data.origin, _, _))
+        .WillOnce(Invoke(RespondWithTestIcon));
+    password_autofill_manager_->OnAddPasswordFillData(data);
+
+    // Show the popup and verify local and account-stored suggestion coexist.
+    std::vector<Suggestion> suggestions;
+    EXPECT_CALL(
+        autofill_client,
+        ShowAutofillPopup(
+            _, _,
+            SuggestionVectorIdsAre(ElementsAreArray(RemoveShowAllBeforeLollipop(
+                {is_suggestion_on_password_field
+                     ? autofill::POPUP_ITEM_ID_PASSWORD_ENTRY
+                     : autofill::POPUP_ITEM_ID_USERNAME_ENTRY,
+                 is_suggestion_on_password_field
+                     ? autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY
+                     : autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY,
+                 autofill::POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY}))),
+            /*autoselect_first_suggestion=*/false, PopupType::kPasswords, _))
+        .WillOnce(testing::SaveArg<2>(&suggestions));
+    password_autofill_manager_->OnShowPasswordSuggestions(
+        base::i18n::RIGHT_TO_LEFT, base::string16(),
+        is_suggestion_on_password_field ? autofill::IS_PASSWORD_FIELD : 0,
+        gfx::RectF());
+    ASSERT_GE(suggestions.size(), 2u);
+    EXPECT_TRUE(AreImagesEqual(suggestions[0].custom_icon, kTestFavicon));
+    EXPECT_TRUE(AreImagesEqual(suggestions[1].custom_icon, kTestFavicon));
+
+    // When selecting the account-stored credential, make sure the filled
+    // password belongs to the selected credential (and not to the first match).
+    EXPECT_CALL(*client.mock_driver(),
+                FillSuggestion(test_username_, duplicate.password));
+    EXPECT_CALL(
+        autofill_client,
+        HideAutofillPopup(autofill::PopupHidingReason::kAcceptSuggestion));
+    password_autofill_manager_->DidAcceptSuggestion(
+        test_username_,
+        is_suggestion_on_password_field
+            ? autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY
+            : autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_USERNAME_ENTRY,
+        /*position=*/1);
+  }
+}
+
+// Test that the popup is updated once account-stored suggestions are unlocked.
 TEST_F(PasswordAutofillManagerTest, ShowOptInAndFillButton) {
   TestPasswordManagerClient client;
   NiceMock<MockAutofillClient> autofill_client;
@@ -657,7 +724,7 @@ TEST_F(PasswordAutofillManagerTest,
       autofill_client,
       UpdatePopup(
           SuggestionVectorIdsAre(ElementsAreArray(RemoveShowAllBeforeLollipop(
-              {autofill::POPUP_ITEM_ID_PASSWORD_ENTRY,
+              {autofill::POPUP_ITEM_ID_ACCOUNT_STORAGE_PASSWORD_ENTRY,
                autofill::POPUP_ITEM_ID_PASSWORD_ENTRY,
                autofill::POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY}))),
           PopupType::kPasswords));