Updating of all entries in PasswordManager of the same credentials on password update

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 1908ec566c1ae4fc494ba4d107980eaa77dbdd80..5665bd1c11719ae690e222169adc4356e3ab8056 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -234,7 +234,7 @@ bool PasswordFormManager::IsNewLogin() const {
 }
 
 bool PasswordFormManager::IsPendingCredentialsPublicSuffixMatch() const {
-  return pending_credentials_.IsPublicSuffixMatch();
+  return pending_credentials_.is_public_suffix_match;
 }
 
 void PasswordFormManager::ProvisionallySave(
@@ -442,27 +442,22 @@ void PasswordFormManager::OnRequestDone(
 
       if (is_credential_protected)
         protected_credentials.push_back(login.Pass());
+      else
+        not_best_matches_.push_back(login.Pass());
       continue;
     }
 
-    // If there is another best-score match for the same username, replace it.
-    // TODO(vabr): Spare the replacing and keep the first instead of the last
-    // candidate.
+    // If there is another best-score match for the same username then leave it
+    // and add the current form to |not_best_matches_|.
     const base::string16& username = login->username_value;
     auto best_match_username = best_matches_.find(username);
-    if (best_match_username != best_matches_.end() &&
-        best_match_username->second == preferred_match_) {
-      preferred_match_ = nullptr;
+    if (best_match_username == best_matches_.end()) {
+      if (login->preferred)
+        preferred_match_ = login.get();
+      best_matches_.insert(username, login.Pass());
+    } else {
+      not_best_matches_.push_back(login.Pass());
     }
-    // Transfer ownership into the map.
-    const PasswordForm* best_match = login.get();
-    // TODO(mgiuca): Directly assign to |best_match_username|, instead of doing
-    // a second map traversal. This will only be possible once we have C++11
-    // library support (then |best_matches_| can be a map of scoped_ptrs instead
-    // of a ScopedPtrMap).
-    best_matches_.set(username, login.Pass());
-    if (best_match->preferred)
-      preferred_match_ = best_match;
   }
 
   // Add the protected results if we don't already have a result with the same
@@ -473,7 +468,10 @@ void PasswordFormManager::OnRequestDone(
     scoped_ptr<PasswordForm> protege(*it);
     *it = nullptr;
     const base::string16& username = protege->username_value;
-    best_matches_.insert(username, protege.Pass());
+    if (best_matches_.find(username) == best_matches_.end())
+      best_matches_.insert(username, protege.Pass());
+    else
+      not_best_matches_.push_back(protege.Pass());
   }
 
   UMA_HISTOGRAM_COUNTS("PasswordManager.NumPasswordsNotShown",
@@ -528,7 +526,7 @@ void PasswordFormManager::ProcessFrameInternal(
       (!IsValidAndroidFacetURI(preferred_match_->original_signon_realm) &&

[engedy, 2015/09/29 19:11:04]

I think this should be just |signon_realm|, and is causing the trybot failures.

[dvadym, 2015/10/12 09:30:44]

Done.

        (observed_form_.action.GetWithEmptyPath() !=
             preferred_match_->action.GetWithEmptyPath() ||
-        preferred_match_->IsPublicSuffixMatch() ||
+        preferred_match_->is_public_suffix_match ||
         observed_form_.IsPossibleChangePasswordForm()));
   if (wait_for_username)
     manager_action_ = kManagerActionNone;
@@ -664,6 +662,7 @@ void PasswordFormManager::UpdateLogin() {
 
   UpdatePreferredLoginState(password_store);
 
+  bool password_was_updated = false;
   // Update the new preferred login.
   if (!selected_username_.empty()) {
     // Username has changed. We set this selected username as the real
@@ -689,8 +688,28 @@ void PasswordFormManager::UpdateLogin() {
     pending_credentials_.submit_element = observed_form_.submit_element;
     password_store->UpdateLoginWithPrimaryKey(pending_credentials_,
                                               old_primary_key);
+    password_was_updated = true;
   } else {
     password_store->UpdateLogin(pending_credentials_);
+    password_was_updated = true;
+  }
+  // If this was password update then update all non-best matches entries with
+  // the same username and the same old password.
+  if (password_was_updated) {
+    PasswordFormMap::const_iterator updated_password_it =
+        best_matches_.find(pending_credentials_.username_value);
+    DCHECK(best_matches_.end() != updated_password_it);
+    const base::string16& old_password =
+        updated_password_it->second->password_value;
+    for (size_t i = 0; i < not_best_matches_.size(); ++i) {
+      if (not_best_matches_[i]->username_value ==
+              pending_credentials_.username_value &&
+          not_best_matches_[i]->password_value == old_password) {
+        not_best_matches_[i]->password_value =
+            pending_credentials_.password_value;
+        password_store->UpdateLogin(*not_best_matches_[i]);
+      }
+    }
   }
 }
 
@@ -793,10 +812,11 @@ void PasswordFormManager::CreatePendingCredentials() {
     pending_credentials_ = *it->second;
     password_overridden_ =
         pending_credentials_.password_value != password_to_save;
-    if (IsPendingCredentialsPublicSuffixMatch()) {
-      // If the autofilled credentials were only a PSL match, store a copy with
-      // the current origin and signon realm. This ensures that on the next
-      // visit, a precise match is found.
+    if (IsPendingCredentialsPublicSuffixMatch() ||
+        IsValidAndroidFacetURI(preferred_match_->signon_realm)) {
+      // If the autofilled credentials were a PSL match or Android federated

[engedy, 2015/09/29 19:11:04]

phrasing suggestion:

// ... or credentials saved from Android apps, store ... 


[Note: These are traditional username/password credentials saved through Android
apps. Federated credentials (be from apps or websites) should never reach this
point. (They currently do, but I am working on fixing that. :-))]

[dvadym, 2015/10/12 09:30:44]

Done.

+      // credentials, store a copy with the current origin and signon realm.
+      // This ensures that on the next visit, a precise match is found.
       is_new_login_ = true;
       user_action_ = password_overridden_ ? kUserActionOverridePassword
                                           : kUserActionChoosePslMatch;
@@ -816,6 +836,11 @@ void PasswordFormManager::CreatePendingCredentials() {
       // passwords. This is a much larger change.
       UpdateMetadataForUsage(pending_credentials_);
 
+      // Update |pending_credentials_| in order to be able correctly save it.
+      pending_credentials_.origin = provisionally_saved_form_->origin;
+      pending_credentials_.signon_realm =
+          provisionally_saved_form_->signon_realm;
+
       // Normally, the copy of the PSL matched credentials, adapted for the
       // current domain, is saved automatically without asking the user, because
       // the copy likely represents the same account, i.e., the one for which
@@ -825,9 +850,9 @@ void PasswordFormManager::CreatePendingCredentials() {
       // that the autofilled credentials and |provisionally_saved_form_|
       // actually correspond  to two different accounts (see
       // http://crbug.com/385619). In that case the user should be asked again
-      // before saving the password. This is ensured by clearing
-      // |original_signon_realm| on |pending_credentials_|, which unmarks it as
-      // a PSL match.
+      // before saving the password. This is ensured by setting
+      // |password_overriden_| on |pending_credentials_| to false and setting
+      // |origin| and |signon_realm| to correct values.
       //
       // There is still the edge case when the autofilled credentials represent
       // the same account as |provisionally_saved_form_| but the stored password
@@ -839,8 +864,8 @@ void PasswordFormManager::CreatePendingCredentials() {
       // user by asking them is not significant, so we are fine with asking
       // here again.
       if (password_overridden_) {
-        pending_credentials_.original_signon_realm.clear();
-        DCHECK(!IsPendingCredentialsPublicSuffixMatch());
+        pending_credentials_.is_public_suffix_match = false;
+        password_overridden_ = false;
       }
     } else {  // Not a PSL match.
       is_new_login_ = false;
@@ -896,6 +921,7 @@ void PasswordFormManager::CreatePendingCredentials() {
   }
 
   pending_credentials_.action = provisionally_saved_form_->action;
+
   // 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 bug 1107719.
@@ -943,7 +969,7 @@ uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
       std::min(form_path_segments_.size(), kSegmentCountCap);
 
   uint32_t score = 0u;
-  if (!candidate.IsPublicSuffixMatch()) {
+  if (!candidate.is_public_suffix_match) {
     score += 1u << 7;
   }
   if (candidate.origin == observed_form_.origin) {
@@ -986,7 +1012,7 @@ bool PasswordFormManager::IsBlacklistMatch(
     const autofill::PasswordForm& blacklisted_form) const {
   DCHECK(blacklisted_form.blacklisted_by_user);
 
-  if (blacklisted_form.IsPublicSuffixMatch())
+  if (blacklisted_form.is_public_suffix_match)
     return false;
   if (blacklisted_form.origin.GetOrigin() != observed_form_.origin.GetOrigin())
     return false;
@@ -1014,7 +1040,7 @@ void PasswordFormManager::DeleteEmptyUsernameCredentials() {
   }
   for (auto iter = best_matches_.begin(); iter != best_matches_.end(); ++iter) {
     PasswordForm* form = iter->second;
-    if (!form->IsPublicSuffixMatch() && form->username_value.empty() &&
+    if (!form->is_public_suffix_match && form->username_value.empty() &&
         form->password_value == pending_credentials_.password_value)
       password_store->RemoveLogin(*form);
   }