Save Android Autofill credentials in the right format.

components/password_manager/core/browser/password_syncable_service.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/password_syncable_service.h"
 
 #include <algorithm>
 #include <iterator>
+#include <numeric>
 #include <utility>
 
 #include "base/auto_reset.h"
 #include "base/location.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/optional.h"
+#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/time/default_clock.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/android_affiliation/affiliation_utils.h"
 #include "components/password_manager/core/browser/password_manager_metrics_util.h"
 #include "components/password_manager/core/browser/password_store_sync.h"
 #include "components/sync/model/sync_error_factory.h"
 #include "net/base/escape.h"
 
 namespace password_manager {
 
 // Converts the |password| into a SyncData object.
 syncer::SyncData SyncDataFromPassword(const autofill::PasswordForm& password);
 
 // Extracts the |PasswordForm| data from sync's protobuf format.
 autofill::PasswordForm PasswordFromSpecifics(
     const sync_pb::PasswordSpecificsData& password);
 
 // Returns the unique tag that will serve as the sync identifier for the
 // |password| entry.
 std::string MakePasswordSyncTag(const sync_pb::PasswordSpecificsData& password);
 std::string MakePasswordSyncTag(const autofill::PasswordForm& password);
 
 namespace {
 
 // Returns true iff |password_specifics| and |password_specifics| are equal
-// memberwise.
-bool AreLocalAndSyncPasswordsEqual(
+// in the fields which don't comprise the sync tag.
+bool AreLocalAndSyncPasswordsNonSyncTagEqual(
     const sync_pb::PasswordSpecificsData& password_specifics,
     const autofill::PasswordForm& password_form) {
   return (password_form.scheme == password_specifics.scheme() &&
-          password_form.signon_realm == password_specifics.signon_realm() &&
-          password_form.origin.spec() == password_specifics.origin() &&
           password_form.action.spec() == password_specifics.action() &&
-          base::UTF16ToUTF8(password_form.username_element) ==
-              password_specifics.username_element() &&
-          base::UTF16ToUTF8(password_form.password_element) ==
-              password_specifics.password_element() &&
-          base::UTF16ToUTF8(password_form.username_value) ==
-              password_specifics.username_value() &&
           base::UTF16ToUTF8(password_form.password_value) ==
               password_specifics.password_value() &&
           password_form.preferred == password_specifics.preferred() &&
           password_form.date_created.ToInternalValue() ==
               password_specifics.date_created() &&
           password_form.blacklisted_by_user ==
               password_specifics.blacklisted() &&
           password_form.type == password_specifics.type() &&
           password_form.times_used == password_specifics.times_used() &&
           base::UTF16ToUTF8(password_form.display_name) ==
               password_specifics.display_name() &&
           password_form.icon_url.spec() == password_specifics.avatar_url() &&
           url::Origin(GURL(password_specifics.federation_url())).Serialize() ==
               password_form.federation_origin.Serialize());
 }
 
+// Returns true iff |password_specifics| and |password_specifics| are equal
+// memberwise.
+bool AreLocalAndSyncPasswordsEqual(
+    const sync_pb::PasswordSpecificsData& password_specifics,
+    const autofill::PasswordForm& password_form) {
+  return (password_form.signon_realm == password_specifics.signon_realm() &&
+          password_form.origin.spec() == password_specifics.origin() &&
+          base::UTF16ToUTF8(password_form.username_element) ==
+              password_specifics.username_element() &&
+          base::UTF16ToUTF8(password_form.password_element) ==
+              password_specifics.password_element() &&
+          base::UTF16ToUTF8(password_form.username_value) ==
+              password_specifics.username_value() &&
+          AreLocalAndSyncPasswordsNonSyncTagEqual(password_specifics,
+                                                  password_form));
+}
+
+// Compares the fields which are not part of the sync tag.
+bool AreNonSyncTagFieldsEqual(const sync_pb::PasswordSpecificsData& left,
+                              const sync_pb::PasswordSpecificsData& right) {
+  return (left.scheme() == right.scheme() && left.action() == right.action() &&
+          left.password_value() == right.password_value() &&
+          left.preferred() == right.preferred() &&
+          left.date_created() == right.date_created() &&
+          left.blacklisted() == right.blacklisted() &&
+          left.type() == right.type() &&
+          left.times_used() == right.times_used() &&
+          left.display_name() == right.display_name() &&
+          left.avatar_url() == right.avatar_url() &&
+          left.federation_url() == right.federation_url());
+}
+
 syncer::SyncChange::SyncChangeType GetSyncChangeType(
     PasswordStoreChange::Type type) {
   switch (type) {
     case PasswordStoreChange::ADD:
       return syncer::SyncChange::ACTION_ADD;
     case PasswordStoreChange::UPDATE:
       return syncer::SyncChange::ACTION_UPDATE;
     case PasswordStoreChange::REMOVE:
       return syncer::SyncChange::ACTION_DELETE;
   }
   NOTREACHED();
   return syncer::SyncChange::ACTION_INVALID;
 }
 
 // Creates a PasswordForm from |specifics| and |sync_time|, appends it to
 // |entries|.
 void AppendPasswordFromSpecifics(
     const sync_pb::PasswordSpecificsData& specifics,
     base::Time sync_time,
     std::vector<std::unique_ptr<autofill::PasswordForm>>* entries) {
   entries->push_back(base::MakeUnique<autofill::PasswordForm>(
       PasswordFromSpecifics(specifics)));
   entries->back()->date_synced = sync_time;
 }
 
+// Android autofill saves credential in a different format without trailing '/'.
+std::string GetAndroidAutofillSignonRealm(std::string android_autofill_realm) {

[engedy, 2017/07/20 18:34:14]

For extra readability in the code below, how about calling these:

Incorrect-/CorrectAndroidSignonRealms,
Short-/LongAndroidSignonRealms

We can define what these mean here in a comment, but perhaps the rest of the
code does not need to know where the bad entries are coming from?

[vasilii, 2017/07/20 19:50:18]

Done.

+  if (base::EndsWith(android_autofill_realm, "/", base::CompareCase::SENSITIVE))
+    android_autofill_realm.erase(android_autofill_realm.size() - 1);
+  return android_autofill_realm;
+}
+
+// The correct Android signon_realm should have a trailing '/'.
+std::string GetCorrectAndroidSignonRealm(std::string android_realm) {

[engedy, 2017/07/20 18:34:14]

Do we have complete confidence in [1] that we will never have empty |origins|
here?

[1]:
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...

[vasilii, 2017/07/20 19:50:17]

I didn't get the question. This particular function won't crash if |origin| is
empty.
I don't know why [1] handles Android credentials specifically. It seems like
|origin| shouldn't be empty for all the credentials.

[engedy, 2017/07/20 20:12:14]

Correct, no crashes, I was just wondering if having weird credentials around
with '/' origins will mess anything up.

+  if (!base::EndsWith(android_realm, "/", base::CompareCase::SENSITIVE))
+    android_realm += '/';
+  return android_realm;
+}
+
+// Android autofill saves credential in a different format without trailing '/'.

[engedy, 2017/07/20 18:34:14]

typo: credentials

[vasilii, 2017/07/20 19:50:18]

Done.

+// Return a sync tag for the style used by Android Autofill in the GMS Core v12.

[engedy, 2017/07/20 18:34:14]

typo: s/in the GMS/in GMS/

[vasilii, 2017/07/20 19:50:18]

Done.

+std::string AndroidAutofillSyncTag(
+    const sync_pb::PasswordSpecificsData& password) {
+  // realm has the same value as the origin.
+  std::string origin = GetAndroidAutofillSignonRealm(password.origin());

[engedy, 2017/07/20 18:34:14]

Just to triple check:

 -- Do you have some concrete example credentials originating from Android
Autofill, where you could verify that the trailing '/' is missing on the
|origin| field too?

 -- As per the original design, the |origin| fields was supposed to be left
empty for Android credentials. In the end, that's not the case anywhere, right?
In particular, Autofill fills out the |origin| field, right?

[vasilii, 2017/07/20 19:50:17]

- Yes, if you open the email thread then you can find a mail from me (I just
added you there) where I copypasted the real origin and signon_realm from my
machine.
- Per documentation and all the personal/testing entries on my machine, Android
always sets |origin| and |signon_realm| to the same value.

+  std::string signon_realm =
+      GetAndroidAutofillSignonRealm(password.signon_realm());
+  return (net::EscapePath(origin) + "|" +
+          net::EscapePath(password.username_element()) + "|" +
+          net::EscapePath(password.username_value()) + "|" +
+          net::EscapePath(password.password_element()) + "|" +
+          net::EscapePath(signon_realm));
+}
+
+// Return a sync tag for the correct format used by Android.
+std::string AndroidCorrectSyncTag(
+    const sync_pb::PasswordSpecificsData& password) {
+  // realm has the same value as the origin.
+  std::string origin = GetCorrectAndroidSignonRealm(password.origin());
+  std::string signon_realm =
+      GetCorrectAndroidSignonRealm(password.signon_realm());
+  return (net::EscapePath(origin) + "|" +
+          net::EscapePath(password.username_element()) + "|" +
+          net::EscapePath(password.username_value()) + "|" +
+          net::EscapePath(password.password_element()) + "|" +
+          net::EscapePath(signon_realm));
+}
+
+void PasswordSpecificsFromPassword(
+    const autofill::PasswordForm& password_form,
+    sync_pb::PasswordSpecificsData* password_specifics) {
+#define CopyField(field) password_specifics->set_##field(password_form.field)
+#define CopyStringField(field) \
+  password_specifics->set_##field(base::UTF16ToUTF8(password_form.field))
+  CopyField(scheme);
+  CopyField(signon_realm);
+  password_specifics->set_origin(password_form.origin.spec());
+  password_specifics->set_action(password_form.action.spec());
+  CopyStringField(username_element);
+  CopyStringField(password_element);
+  CopyStringField(username_value);
+  CopyStringField(password_value);
+  CopyField(preferred);
+  password_specifics->set_date_created(
+      password_form.date_created.ToInternalValue());
+  password_specifics->set_blacklisted(password_form.blacklisted_by_user);
+  CopyField(type);
+  CopyField(times_used);
+  CopyStringField(display_name);
+  password_specifics->set_avatar_url(password_form.icon_url.spec());
+  password_specifics->set_federation_url(
+      password_form.federation_origin.unique()
+          ? std::string()
+          : password_form.federation_origin.Serialize());
+#undef CopyStringField
+#undef CopyField
+}
+
+struct AndroidMergeResult {
+  // New value for Android entry in the correct format.
+  base::Optional<syncer::SyncData> new_android_correct;
+  // New value for Android autofill entry.
+  base::Optional<syncer::SyncData> new_android_incorrect;
+  // New value for local entry in the correct format.
+  base::Optional<autofill::PasswordForm> new_local_correct;
+  // New value for local entry in the Android autofill format.
+  base::Optional<autofill::PasswordForm> new_local_incorrect;
+};
+
+// Perform deduplication of Android credentials saved in the wrong format. As
+// the result all the four entries should be created and have the same value.
+AndroidMergeResult Perform4WayMergeAndroidCredentials(
+    const sync_pb::PasswordSpecificsData* correct_android,
+    const sync_pb::PasswordSpecificsData* incorrect_android,
+    const autofill::PasswordForm* correct_local,
+    const autofill::PasswordForm* incorrect_local) {
+  AndroidMergeResult result;
+
+  base::Optional<sync_pb::PasswordSpecificsData> local_correct_ps;
+  if (correct_local) {
+    local_correct_ps = sync_pb::PasswordSpecificsData();
+    PasswordSpecificsFromPassword(*correct_local, &local_correct_ps.value());
+  }
+
+  base::Optional<sync_pb::PasswordSpecificsData> local_incorrect_ps;
+  if (incorrect_local) {
+    local_incorrect_ps = sync_pb::PasswordSpecificsData();
+    PasswordSpecificsFromPassword(*incorrect_local,
+                                  &local_incorrect_ps.value());
+  }
+
+  const sync_pb::PasswordSpecificsData* all_data[4] = {
+      correct_android, incorrect_android,
+      local_correct_ps ? &local_correct_ps.value() : nullptr,
+      local_incorrect_ps ? &local_incorrect_ps.value() : nullptr};
+
+  // |newest_data| will point to the newest entry out of all 4.
+  const sync_pb::PasswordSpecificsData* newest_data = nullptr;
+  newest_data =

[engedy, 2017/07/20 18:34:14]

nit: Isn't this actually longer than a good old range-based for loop?

[vasilii, 2017/07/20 19:50:17]

Done.

+      std::accumulate(all_data, all_data + 4, newest_data,
+                      [](const sync_pb::PasswordSpecificsData* newest,
+                         const sync_pb::PasswordSpecificsData* current) {
+                        if (newest && current) {
+                          if (current->date_created() > newest->date_created())
+                            newest = current;
+                        } else if (current) {
+                          newest = current;
+                        }
+                        return newest;
+                      });
+  DCHECK(newest_data);
+
+  const std::string correct_tag = AndroidCorrectSyncTag(*newest_data);

[engedy, 2017/07/20 18:34:14]

For brevity, have you considered precalculating the correct/incorrect origins
and signon-realms here as well?

That would also allow adding the following sanity check:

DCHECK_EQ(GURL(incorrect_origin).spec(), incorrect_origin);

Which is true today, AFAICT, but only because GURL is not smart enough to
canonicalize android:// URIs. It's weird to rely on "missing functionality".
Maybe this even warrant a tiny test case on its own? :-/

[vasilii, 2017/07/20 19:50:18]

Done.

+  const std::string incorrect_tag = AndroidAutofillSyncTag(*newest_data);
+  // Set the correct Sync entry if needed.
+  if (newest_data != correct_android &&
+      (!correct_android ||
+       !AreNonSyncTagFieldsEqual(*correct_android, *newest_data))) {
+    sync_pb::EntitySpecifics password_data;
+    sync_pb::PasswordSpecificsData* password_specifics =
+        password_data.mutable_password()->mutable_client_only_encrypted_data();

[engedy, 2017/07/20 18:34:14]

To double check: are you familiar with how non-encrypted fields are handled? I
suppose they are derived / updated automatically, and we don't have to worry
about them, right?

[vasilii, 2017/07/20 19:50:18]

Right, not our business.

+    *password_specifics = *newest_data;
+    password_specifics->set_origin(
+        GetCorrectAndroidSignonRealm(newest_data->origin()));
+    password_specifics->set_signon_realm(
+        GetCorrectAndroidSignonRealm(newest_data->signon_realm()));
+    result.new_android_correct = syncer::SyncData::CreateLocalData(
+        correct_tag, correct_tag, password_data);
+  }
+  // Set the Andoroid Autofill Sync entry if needed.

[engedy, 2017/07/20 18:34:14]

optional nit: Consider blank lines between blocks.

[vasilii, 2017/07/20 19:50:18]

Done.

+  if (newest_data != incorrect_android &&
+      (!incorrect_android ||
+       !AreNonSyncTagFieldsEqual(*incorrect_android, *newest_data))) {
+    sync_pb::EntitySpecifics password_data;
+    sync_pb::PasswordSpecificsData* password_specifics =
+        password_data.mutable_password()->mutable_client_only_encrypted_data();
+    *password_specifics = *newest_data;
+    password_specifics->set_origin(
+        GetAndroidAutofillSignonRealm(newest_data->origin()));
+    password_specifics->set_signon_realm(
+        GetAndroidAutofillSignonRealm(newest_data->signon_realm()));
+    result.new_android_incorrect = syncer::SyncData::CreateLocalData(
+        incorrect_tag, incorrect_tag, password_data);
+  }
+  // Set the correct local entry if needed.
+  if (!local_correct_ps ||
+      (newest_data != &local_correct_ps.value() &&
+       !AreNonSyncTagFieldsEqual(local_correct_ps.value(), *newest_data))) {
+    result.new_local_correct = PasswordFromSpecifics(*newest_data);
+    result.new_local_correct.value().origin =
+        GURL(GetCorrectAndroidSignonRealm(newest_data->origin()));
+    result.new_local_correct.value().signon_realm =
+        GetCorrectAndroidSignonRealm(newest_data->signon_realm());
+  }
+  // Set the incorrect local entry if needed.
+  if (!local_incorrect_ps ||
+      (newest_data != &local_incorrect_ps.value() &&
+       !AreNonSyncTagFieldsEqual(local_incorrect_ps.value(), *newest_data))) {
+    result.new_local_incorrect = PasswordFromSpecifics(*newest_data);
+    result.new_local_incorrect.value().origin =
+        GURL(GetAndroidAutofillSignonRealm(newest_data->origin()));
+    result.new_local_incorrect.value().signon_realm =
+        GetAndroidAutofillSignonRealm(newest_data->signon_realm());
+  }
+  return result;
+}
+
 }  // namespace
 
 struct PasswordSyncableService::SyncEntries {
   std::vector<std::unique_ptr<autofill::PasswordForm>>* EntriesForChangeType(
       syncer::SyncChange::SyncChangeType type) {
     switch (type) {
       case syncer::SyncChange::ACTION_ADD:
         return &new_entries;
       case syncer::SyncChange::ACTION_UPDATE:
         return &updated_entries;
@@ skipping 13 matching lines @@
   // database but have updates in sync. They need to be updated in the local
   // database.
   std::vector<std::unique_ptr<autofill::PasswordForm>> updated_entries;
 
   // The list of entries to be deleted from the local database.
   std::vector<std::unique_ptr<autofill::PasswordForm>> deleted_entries;
 };
 
 PasswordSyncableService::PasswordSyncableService(
     PasswordStoreSync* password_store)
-    : password_store_(password_store), is_processing_sync_changes_(false) {
-}
+    : password_store_(password_store),
+      clock_(new base::DefaultClock),
+      is_processing_sync_changes_(false) {}
 
 PasswordSyncableService::~PasswordSyncableService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
 syncer::SyncMergeResult PasswordSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     std::unique_ptr<syncer::SyncChangeProcessor> sync_processor,
     std::unique_ptr<syncer::SyncErrorFactory> sync_error_factory) {
@@ skipping 17 matching lines @@
   if (password_entries.size() != new_local_entries.size()) {
     merge_result.set_error(sync_error_factory->CreateAndUploadError(
         FROM_HERE,
         "There are passwords with identical sync tags in the database."));
     metrics_util::LogPasswordSyncState(
         metrics_util::NOT_SYNCING_DUPLICATE_TAGS);
     return merge_result;
   }
   merge_result.set_num_items_before_association(new_local_entries.size());
 
+  // Changes from Sync to be applied locally.
   SyncEntries sync_entries;
   // Changes from password db that need to be propagated to sync.
   syncer::SyncChangeList updated_db_entries;
-  for (syncer::SyncDataList::const_iterator sync_iter =
-           initial_sync_data.begin();
-       sync_iter != initial_sync_data.end(); ++sync_iter) {
-    CreateOrUpdateEntry(*sync_iter,
-                        &new_local_entries,
-                        &sync_entries,
-                        &updated_db_entries);
-  }
+  MergeSyncDataWithLocalData(initial_sync_data, &new_local_entries,
+                             &sync_entries, &updated_db_entries);
 
   for (PasswordEntryMap::iterator it = new_local_entries.begin();
        it != new_local_entries.end(); ++it) {
     updated_db_entries.push_back(
         syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD,
                            SyncDataFromPassword(*it->second)));
   }
 
   WriteToPasswordStore(sync_entries);
   merge_result.set_error(
@@ skipping 43 matching lines @@
                  });
   return sync_data;
 }
 
 syncer::SyncError PasswordSyncableService::ProcessSyncChanges(
     const tracked_objects::Location& from_here,
     const syncer::SyncChangeList& change_list) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   base::AutoReset<bool> processing_changes(&is_processing_sync_changes_, true);
   SyncEntries sync_entries;
-  base::Time time_now = base::Time::Now();
+  base::Time time_now = clock_->Now();
 
   for (syncer::SyncChangeList::const_iterator it = change_list.begin();
        it != change_list.end(); ++it) {
     const sync_pb::EntitySpecifics& specifics = it->sync_data().GetSpecifics();
     std::vector<std::unique_ptr<autofill::PasswordForm>>* entries =
         sync_entries.EntriesForChangeType(it->change_type());
     if (!entries) {
       return sync_error_factory_->CreateAndUploadError(
           FROM_HERE, "Failed to process sync changes for passwords datatype.");
     }
     AppendPasswordFromSpecifics(
         specifics.password().client_only_encrypted_data(), time_now, entries);
+    if (IsValidAndroidFacetURI(entries->back()->signon_realm)) {
+      // Fix the Android Autofill credentials if needed.
+      entries->back()->signon_realm =
+          GetCorrectAndroidSignonRealm(entries->back()->signon_realm);
+      entries->back()->origin =
+          GURL(GetCorrectAndroidSignonRealm(entries->back()->origin.spec()));
+    }
   }
 
   WriteToPasswordStore(sync_entries);
   return syncer::SyncError();
 }
 
 void PasswordSyncableService::ActOnPasswordStoreChanges(
     const PasswordStoreChangeList& local_changes) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
@@ skipping 69 matching lines @@
   WriteEntriesToDatabase(&PasswordStoreSync::UpdateLoginSync,
                          entries.updated_entries, &changes);
   WriteEntriesToDatabase(&PasswordStoreSync::RemoveLoginSync,
                          entries.deleted_entries, &changes);
 
   // We have to notify password store observers of the change by hand since
   // we use internal password store interfaces to make changes synchronously.
   password_store_->NotifyLoginsChanged(changes);
 }
 
-// static
-void PasswordSyncableService::CreateOrUpdateEntry(
-    const syncer::SyncData& data,
+void PasswordSyncableService::MergeSyncDataWithLocalData(
+    const syncer::SyncDataList& sync_data,
     PasswordEntryMap* unmatched_data_from_password_db,
     SyncEntries* sync_entries,
     syncer::SyncChangeList* updated_db_entries) {
-  const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
-  const sync_pb::PasswordSpecificsData& password_specifics(
-      specifics.password().client_only_encrypted_data());
+  std::map<std::string, const sync_pb::PasswordSpecificsData*> sync_data_map;
+  for (const auto& data : sync_data) {
+    const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
+    const sync_pb::PasswordSpecificsData* password_specifics =
+        &specifics.password().client_only_encrypted_data();
+    sync_data_map[MakePasswordSyncTag(*password_specifics)] =

[engedy, 2017/07/20 18:34:14]

Should we add a DCHECK() that there was no such key yet, or
DCHECK_EQ(sync_data_map.size(), sync_data.size()) after the loop?

[vasilii, 2017/07/20 19:50:18]

Done.

+        password_specifics;
+  }
+
+  for (auto it = sync_data_map.begin(); it != sync_data_map.end();) {
+    if (IsValidAndroidFacetURI(it->second->signon_realm())) {
+      // Perform deduplication of Android credentials saved in the wrong format.
+      // An incorrect entry is copied in the correct format so Chrome can make

[engedy, 2017/07/20 18:34:14]

phrasing nit: For each incorrect entry, a duplicate of is created in the correct
format, so...

[vasilii, 2017/07/20 19:50:17]

Done.

+      // use of it. The wrong sync entries are not deleted for now.

[engedy, 2017/07/20 18:34:14]

phrasing nit: s/wrong/incorrect/

[vasilii, 2017/07/20 19:50:18]

Done.

+      std::string incorrect_tag = AndroidAutofillSyncTag(*it->second);
+      std::string correct_tag = AndroidCorrectSyncTag(*it->second);
+      auto it_android_incorrect = sync_data_map.find(incorrect_tag);

[engedy, 2017/07/20 18:34:14]

For extra readability, have you considered s/android/remote/g or
s/android/sync/g? It's a bit misleading next to all the Android autofill stuff.

[vasilii, 2017/07/20 19:50:17]

Done.

+      auto it_android_correct = sync_data_map.find(correct_tag);
+      auto it_local_data_correct =
+          unmatched_data_from_password_db->find(correct_tag);
+      auto it_local_data_incorrect =
+          unmatched_data_from_password_db->find(incorrect_tag);

[engedy, 2017/07/20 18:34:14]

Should we fall back to default handling if (it != it_android_incorrect && it !=
it_android_correct)?

E.g., it can happen if the trailing slash is there for |origin|, but missing for
|signon_realm|, which should not happen...

Also, did we verify that the Android Autofill folks actually added the trailing
slash to both |signon_realm| and |origin|?

[vasilii, 2017/07/20 19:50:18]

Good point.

They claim that they use the same library as YOLO. I can follow up.

+      if (it_android_incorrect == sync_data_map.end() &&
+          it_local_data_incorrect == unmatched_data_from_password_db->end()) {
+        // Wrong credential don't exist. Just do what Sync would normally do.
+        CreateOrUpdateEntry(*it->second, unmatched_data_from_password_db,
+                            sync_entries, updated_db_entries);
+        ++it;
+      } else {
+        AndroidMergeResult result = Perform4WayMergeAndroidCredentials(
+            it_android_correct == sync_data_map.end()
+                ? nullptr
+                : it_android_correct->second,
+            it_android_incorrect == sync_data_map.end()
+                ? nullptr
+                : it_android_incorrect->second,
+            it_local_data_correct == unmatched_data_from_password_db->end()
+                ? nullptr
+                : it_local_data_correct->second,
+            it_local_data_incorrect == unmatched_data_from_password_db->end()
+                ? nullptr
+                : it_local_data_incorrect->second);
+        // Add or update the correct local entry.
+        if (result.new_local_correct) {
+          auto* local_changes = sync_entries->EntriesForChangeType(
+              it_local_data_correct == unmatched_data_from_password_db->end()
+                  ? syncer::SyncChange::ACTION_ADD
+                  : syncer::SyncChange::ACTION_UPDATE);
+          local_changes->push_back(base::MakeUnique<autofill::PasswordForm>(
+              result.new_local_correct.value()));
+          local_changes->back()->date_synced = clock_->Now();
+        }
+        // Add or update the incorrect local entry.
+        if (result.new_local_incorrect) {
+          auto* local_changes = sync_entries->EntriesForChangeType(
+              it_local_data_incorrect == unmatched_data_from_password_db->end()
+                  ? syncer::SyncChange::ACTION_ADD
+                  : syncer::SyncChange::ACTION_UPDATE);
+          local_changes->push_back(base::MakeUnique<autofill::PasswordForm>(
+              result.new_local_incorrect.value()));
+          local_changes->back()->date_synced = clock_->Now();
+        }
+        if (it_local_data_correct != unmatched_data_from_password_db->end())
+          unmatched_data_from_password_db->erase(it_local_data_correct);
+        if (it_local_data_incorrect != unmatched_data_from_password_db->end())
+          unmatched_data_from_password_db->erase(it_local_data_incorrect);
+        // Add or update the correct sync entry.
+        if (result.new_android_correct) {
+          updated_db_entries->push_back(
+              syncer::SyncChange(FROM_HERE,
+                                 it_android_correct == sync_data_map.end()
+                                     ? syncer::SyncChange::ACTION_ADD
+                                     : syncer::SyncChange::ACTION_UPDATE,
+                                 result.new_android_correct.value()));
+        }
+        // Add or update the Android Autofill sync entry.
+        if (result.new_android_incorrect) {
+          updated_db_entries->push_back(
+              syncer::SyncChange(FROM_HERE,
+                                 it_android_incorrect == sync_data_map.end()
+                                     ? syncer::SyncChange::ACTION_ADD
+                                     : syncer::SyncChange::ACTION_UPDATE,
+                                 result.new_android_incorrect.value()));
+        }
+        bool increment = true;
+        for (auto sync_data_it : {it_android_incorrect, it_android_correct}) {
+          if (sync_data_it != sync_data_map.end()) {
+            if (sync_data_it == it) {
+              it = sync_data_map.erase(it);
+              increment = false;
+            } else {
+              sync_data_map.erase(sync_data_it);
+            }
+          }
+        }
+        if (increment)
+          ++it;
+      }
+    } else {
+      // Not Android.
+      CreateOrUpdateEntry(*it->second, unmatched_data_from_password_db,
+                          sync_entries, updated_db_entries);
+      ++it;
+    }
+  }
+}
+
+void PasswordSyncableService::CreateOrUpdateEntry(
+    const sync_pb::PasswordSpecificsData& password_specifics,
+    PasswordEntryMap* unmatched_data_from_password_db,
+    SyncEntries* sync_entries,
+    syncer::SyncChangeList* updated_db_entries) {
   std::string tag = MakePasswordSyncTag(password_specifics);
 
   // Check whether the data from sync is already in the password store.
   PasswordEntryMap::iterator existing_local_entry_iter =
       unmatched_data_from_password_db->find(tag);
-  base::Time time_now = base::Time::Now();
+  base::Time time_now = clock_->Now();
   if (existing_local_entry_iter == unmatched_data_from_password_db->end()) {
       // The sync data is not in the password store, so we need to create it in
       // the password store. Add the entry to the new_entries list.
       AppendPasswordFromSpecifics(password_specifics, time_now,
                                   &sync_entries->new_entries);
   } else {
     // The entry is in password store. If the entries are not identical, then
     // the entries need to be merged.
     // If the passwords differ, take the one that was created more recently.
     const autofill::PasswordForm& password_form =
@@ skipping 27 matching lines @@
                         new_changes.begin(),
                         new_changes.end());
   }
 }
 
 syncer::SyncData SyncDataFromPassword(
     const autofill::PasswordForm& password_form) {
   sync_pb::EntitySpecifics password_data;
   sync_pb::PasswordSpecificsData* password_specifics =
       password_data.mutable_password()->mutable_client_only_encrypted_data();
-#define CopyField(field) password_specifics->set_##field(password_form.field)
-#define CopyStringField(field) \
-  password_specifics->set_##field(base::UTF16ToUTF8(password_form.field))
-  CopyField(scheme);
-  CopyField(signon_realm);
-  password_specifics->set_origin(password_form.origin.spec());
-  password_specifics->set_action(password_form.action.spec());
-  CopyStringField(username_element);
-  CopyStringField(password_element);
-  CopyStringField(username_value);
-  CopyStringField(password_value);
-  CopyField(preferred);
-  password_specifics->set_date_created(
-      password_form.date_created.ToInternalValue());
-  password_specifics->set_blacklisted(password_form.blacklisted_by_user);
-  CopyField(type);
-  CopyField(times_used);
-  CopyStringField(display_name);
-  password_specifics->set_avatar_url(password_form.icon_url.spec());
-  password_specifics->set_federation_url(
-      password_form.federation_origin.unique()
-          ? std::string()
-          : password_form.federation_origin.Serialize());
-#undef CopyStringField
-#undef CopyField
+  PasswordSpecificsFromPassword(password_form, password_specifics);
 
   std::string tag = MakePasswordSyncTag(*password_specifics);
   return syncer::SyncData::CreateLocalData(tag, tag, password_data);
 }
 
 autofill::PasswordForm PasswordFromSpecifics(
     const sync_pb::PasswordSpecificsData& password) {
   autofill::PasswordForm new_password;
   new_password.scheme =
       static_cast<autofill::PasswordForm::Scheme>(password.scheme());
@@ skipping 26 matching lines @@
 
 std::string MakePasswordSyncTag(const autofill::PasswordForm& password) {
   return (net::EscapePath(password.origin.spec()) + "|" +
           net::EscapePath(base::UTF16ToUTF8(password.username_element)) + "|" +
           net::EscapePath(base::UTF16ToUTF8(password.username_value)) + "|" +
           net::EscapePath(base::UTF16ToUTF8(password.password_element)) + "|" +
           net::EscapePath(password.signon_realm));
 }
 
 }  // namespace password_manager