[Sync] Implementation of model association for passwords using sync API

chrome/browser/password_manager/password_syncable_service.cc

Index: chrome/browser/password_manager/password_syncable_service.cc
diff --git a/chrome/browser/password_manager/password_syncable_service.cc b/chrome/browser/password_manager/password_syncable_service.cc
index 97e83414412fa89268967858c515ef6c78b39cf1..6ca2d1e04fc49eef5866829ed45cf15e437b1406 100644
--- a/chrome/browser/password_manager/password_syncable_service.cc
+++ b/chrome/browser/password_manager/password_syncable_service.cc
@@ -5,12 +5,53 @@
 #include "chrome/browser/password_manager/password_syncable_service.h"
 
 #include "base/location.h"
+#include "base/memory/scoped_vector.h"
+#include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_store.h"
 #include "components/autofill/core/common/password_form.h"
 #include "net/base/escape.h"
 #include "sync/api/sync_error_factory.h"
 
+bool PasswordSyncableService::MergeLocalAndSyncPasswords(
+    const sync_pb::PasswordSpecificsData& password_specifics,
+    const autofill::PasswordForm& password_form,
+    autofill::PasswordForm* new_password_form) {
+  if (password_specifics.scheme() == password_form.scheme &&
+      password_form.signon_realm == password_specifics.signon_realm() &&
+      password_form.origin.spec() == password_specifics.origin() &&
+      password_form.action.spec() == password_specifics.action() &&
+      UTF16ToUTF8(password_form.username_element) ==
+          password_specifics.username_element() &&
+      UTF16ToUTF8(password_form.password_element) ==
+          password_specifics.password_element() &&
+      UTF16ToUTF8(password_form.username_value) ==
+          password_specifics.username_value() &&
+      UTF16ToUTF8(password_form.password_value) ==
+          password_specifics.password_value() &&
+      password_specifics.ssl_valid() == password_form.ssl_valid &&
+      password_specifics.preferred() == password_form.preferred &&
+      password_specifics.date_created() ==
+          password_form.date_created.ToInternalValue() &&
+      password_specifics.blacklisted() ==
+          password_form.blacklisted_by_user) {
+    return false;
+  }
+
+  // If the passwords differ, take the one that was created more recently.
+  if (base::Time::FromInternalValue(password_specifics.date_created()) <=
+          password_form.date_created) {
+    *new_password_form = password_form;
+  } else {
+    PasswordSyncableService::ExtractPasswordFromSpecifics(
+        password_specifics,
+        new_password_form);
+  }
+
+  return true;
+}

[Ilya Sherman, 2013/11/19 22:49:04]

Can this method be tucked into an anonymous namespace in this implementation
file, rather than being exposed in the header file?

[lipalani1, 2013/11/26 23:31:35]

it calls into other private methods inside the class. All of them cannot be
anonymous as they are used by test cases.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+
 PasswordSyncableService::PasswordSyncableService(
     scoped_refptr<PasswordStore> password_store)
     : password_store_(password_store) {
@@ -18,6 +59,48 @@ PasswordSyncableService::PasswordSyncableService(
 
 PasswordSyncableService::~PasswordSyncableService() {}
 
+void PasswordSyncableService::CreateOrUpdateEntry(
+    const syncer::SyncData& data,
+    PasswordEntryMap* loaded_data,
+    ScopedVector<autofill::PasswordForm>* new_entries,
+    ScopedVector<autofill::PasswordForm>* updated_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());

[Ilya Sherman, 2013/11/19 22:49:04]

Could you explain to me what |client_only_encrypted_data| means?  In
MergeLocalAndSyncPasswords(), it looks like you compare PasswordSpecificsData to
unencrypted local data.  What does the "encrypted" part of the field name mean,
then?

[lipalani1, 2013/11/26 23:31:35]

This is the way things are stored in sync internally.

Passwords data are encrypted before they are shared with the server. When
clients receive this encrypted data from the server, it will be decrypted and
this decrypted data(stored in client_only_encrypted_data) is used for populating
the password store.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

[Ilya Sherman, 2013/12/05 07:01:54]

Ok.  That is a pretty confusing name for the field, since it stores *decrypted*
data on the client side.  Any chance of renaming it?  (I understand that this is
outside the scope of this CL.)

On 2013/11/26 23:31:35, lipalani1 wrote:

+  std::string tag = MakeTag(password_specifics);
+
+  // Find if the data from sync is already in password store.

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "Find if" -> "Check whether" or "Test whether" or something like that.

[lipalani1, 2013/11/26 23:31:35]

Done.

+  PasswordEntryMap::iterator loaded_data_index = loaded_data->find(tag);

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "index" -> "iter"

[lipalani1, 2013/11/26 23:31:35]

Done.

+  if (loaded_data_index == loaded_data->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.
+    scoped_ptr<autofill::PasswordForm> new_password(
+        new autofill::PasswordForm());
+    ExtractPasswordFromSpecifics(password_specifics, new_password.get());
+    new_entries->push_back(new_password.release());
+  } else {
+    // The entry is in password store. If the entries are not identical the

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "are not identical the entries" -> "are not identical, then the entries"

[lipalani1, 2013/11/26 23:31:35]

Done.

+    // entries need to be merged.
+    scoped_ptr<autofill::PasswordForm> new_password(
+        new autofill::PasswordForm());
+    if (MergeLocalAndSyncPasswords(password_specifics,
+                                   *(loaded_data_index->second),
+                                   new_password.get())) {

[Ilya Sherman, 2013/11/19 22:49:04]

Please add a comment within this if-stmt along the lines of: "It's possible that
both the Sync database and the passwords database need to be updated to reflect
the merged data.  Rather than testing whether each update is strictly needed,
simply always update both together.  The update is a no-op if the existing data
identically matches the merged data."  (Feel free to reword as much as you like
-- what I think is important here is to convey that while both updates might not
be strictly necessary, it's *possible* that they are.  This wasn't obvious to me
when first reading the code -- I expected one entry or the other to always be
more up-to-date.  In fact, looking at the code for MergeLocalAndSyncPasswords(),
it still seems to me like one is always more up-to-date, so I don't really
understand why we always issue two updates rather than just the one that's
needed.  Perhaps the better improvement here is to update the code rather than
to add documentation.)

[lipalani1, 2013/11/26 23:31:35]

Done.

+      updated_db_entries->push_back(
+          syncer::SyncChange(FROM_HERE,
+                             syncer::SyncChange::ACTION_UPDATE,
+                             CreateSyncData(*(new_password.get()))));
+
+      updated_entries->push_back(new_password.release());
+    }
+    // Remove the entry from the entry map to indicate a match has been found.
+    // Entries that remain in the map at the end of associating all sync entries
+    // will be treated as additions that needs to be propagated to sync.
+    loaded_data->erase(loaded_data_index);

[Ilya Sherman, 2013/11/19 22:49:04]

The comment in the header strongly implies that entries will only be removed
from |loaded_data| if an *identical* entry is found in the Sync DB.  Please
update that comment to clarify that entries will be removed even if the match is
not identical.

[lipalani1, 2013/11/26 23:31:35]

Done.

+  }
+}
+
 syncer::SyncMergeResult
 PasswordSyncableService::MergeDataAndStartSyncing(
     syncer::ModelType type,
@@ -28,9 +111,70 @@ PasswordSyncableService::MergeDataAndStartSyncing(
   sync_error_factory_ = sync_error_factory.Pass();
   sync_processor_ = sync_processor.Pass();
 
-  merge_result.set_error(sync_error_factory->CreateAndUploadError(
-      FROM_HERE,
-      "Password Syncable Service Not Implemented."));
+  ScopedVector<autofill::PasswordForm> password_entries;
+  if (!password_store_->FillAutofillableLogins(&(password_entries.get())) ||
+      !password_store_->FillBlacklistLogins(&(password_entries.get()))) {
+    // Password store often fails to load passwords. Track failures with UMA.
+    // (http://crbug.com/249000)
+    syncer::SyncError::LogDataLoadFailure(syncer::PASSWORDS);
+    merge_result.set_error(sync_error_factory->CreateAndUploadError(
+        FROM_HERE,
+        "Failed to get passwords from store."));
+    return merge_result;
+  }
+
+  PasswordEntryMap new_local_entries;
+  for (PasswordForms::iterator it = password_entries.begin();
+       it != password_entries.end();
+       ++it) {
+     autofill::PasswordForm* password_form = *it;
+     // We add all the db entries as |new_local_entries| initially. While model
+     // association entries that match a sync entry will be removed and this

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "While model association entries" -> "During model association, entries"

[lipalani1, 2013/11/26 23:31:35]

Done.

+     // list will only contain entries that are not in sync.
+     new_local_entries[MakeTag(*password_form)] = password_form;
+  }
+
+  merge_result.set_num_items_before_association(new_local_entries.size());
+
+  // List that contains the entries that are known only to sync.

[Ilya Sherman, 2013/11/19 22:49:04]

nit: Needing comments like these generally implies that the variable names are
not sufficiently clear.  Part of the challenge, I think, is that
"new_sync_entries" does not immediately clarify whether these are entries that
are new to the local db, coming from Sync; or whether these are entries that are
new to Sync, coming to the local db.

I'd suggest trying to find names that are clear enough so that comments are not
needed.  For example, what do you think of the following set of names?

initial_sync_data -> synced_passwords
password_entries -> local_passwords
new_local_passwords -> tentative_new_passwords_from_local_store
new_sync_passwords -> new_passwords_from_sync
updated_sync_entries -> updated_passwords_from_sync
db_changes -> updated_passwords_from_local_store

(Note: the variable name "tentative_new_passwords_from_local_store" is quite a
mouthful, but I think that's partly because the variable is doing a lot: it's
both the set of local passwords that's used to search for matches from Sync, and
the set of passwords that aren't found in Sync, and need to be synced.  That
said, it might still be that the prefix |tentative_| is more distracting than it
is helpful, i.e. I'm not sure whether new_passwords_from_local_db would be a
better name.)

[lipalani1, 2013/11/26 23:31:35]

These names will make sense when looking at it as the current state of
things(i.e. before merging). The names proposed are more confusing. 
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+  ScopedVector<autofill::PasswordForm> new_sync_entries;
+
+  // List that contains the entries that are known to both sync and db but
+  // have updates in sync. They need to be updated in the passwords db.
+  ScopedVector<autofill::PasswordForm> updated_sync_entries;
+
+  // Changes from password db that needs to be propagated to sync.

[Ilya Sherman, 2013/11/19 22:49:04]

nit: "needs" -> "need"

[lipalani1, 2013/11/26 23:31:35]

Done.

+  syncer::SyncChangeList db_changes;

[Ilya Sherman, 2013/11/19 22:49:04]

Optional nit: I'd suggest leaving a blank line after this one, to clarify that
the comment applies just to this variable, and not to the block.

[lipalani1, 2013/11/26 23:31:35]

Done.

+  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,
+                        &new_sync_entries,
+                        &updated_sync_entries,
+                        &db_changes);
+  }
+
+  WriteToPasswordStore(new_sync_entries.get(),
+                       updated_sync_entries.get());

[Ilya Sherman, 2013/11/19 22:49:04]

Does this line compile?  It looks like you're passing pointers to a method that
expects const-references.

[lipalani1, 2013/11/26 23:31:35]

Done.

+
+  merge_result.set_num_items_after_association(
+      merge_result.num_items_before_association() + new_sync_entries.size());
+
+  merge_result.set_num_items_added(new_sync_entries.size());
+
+  merge_result.set_num_items_modified(updated_sync_entries.size());
+
+  for (PasswordEntryMap::iterator it = new_local_entries.begin();
+       it != new_local_entries.end();
+       ++it) {
+    db_changes.push_back(syncer::SyncChange(FROM_HERE,
+                              syncer::SyncChange::ACTION_ADD,
+                              CreateSyncData(*(it->second))));
+  }
+
+  merge_result.set_error(
+      sync_processor_->ProcessSyncChanges(FROM_HERE, db_changes));
   return merge_result;
 }
 
@@ -54,29 +198,33 @@ syncer::SyncError PasswordSyncableService::ProcessSyncChanges(
 }
 
 void PasswordSyncableService::WriteToPasswordStore(
-    PasswordForms* new_entries,
-    PasswordForms* updated_entries) {
+    const PasswordForms& new_entries,
+    const PasswordForms& updated_entries) {
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           new_entries->begin();
-       it != new_entries->end();
+           new_entries.begin();
+       it != new_entries.end();
        ++it) {
     password_store_->AddLoginImpl(**it);
   }
 
   for (std::vector<autofill::PasswordForm*>::const_iterator it =
-           updated_entries->begin();
-       it != updated_entries->end();
+           updated_entries.begin();
+       it != updated_entries.end();
        ++it) {
     password_store_->UpdateLoginImpl(**it);
   }
 
-  if (!new_entries->empty() || !updated_entries->empty()) {
+  if (!new_entries.empty() || !updated_entries.empty()) {
     // 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_->PostNotifyLoginsChanged();
+    NotifyPasswordStoreOfLoginChanges();
   }
 }
 
+void PasswordSyncableService::NotifyPasswordStoreOfLoginChanges() {
+  password_store_->PostNotifyLoginsChanged();
+}
+
 syncer::SyncData PasswordSyncableService::CreateSyncData(
     const autofill::PasswordForm& password_form) {
   sync_pb::EntitySpecifics password_data;
@@ -138,3 +286,27 @@ std::string PasswordSyncableService::MakeTag(
                  password.signon_realm());
 }
 
+// static
+void PasswordSyncableService::ExtractPasswordFromSpecifics(
+     const sync_pb::PasswordSpecificsData& password,
+     autofill::PasswordForm* new_password) {
+  new_password->scheme =
+      static_cast<autofill::PasswordForm::Scheme>(password.scheme());
+  new_password->signon_realm = password.signon_realm();
+  new_password->origin = GURL(password.origin());
+  new_password->action = GURL(password.action());
+  new_password->username_element =
+      UTF8ToUTF16(password.username_element());
+  new_password->password_element =
+      UTF8ToUTF16(password.password_element());
+  new_password->username_value =
+      UTF8ToUTF16(password.username_value());
+  new_password->password_value =
+      UTF8ToUTF16(password.password_value());
+  new_password->ssl_valid = password.ssl_valid();
+  new_password->preferred = password.preferred();
+  new_password->date_created =
+      base::Time::FromInternalValue(password.date_created());
+  new_password->blacklisted_by_user =
+      password.blacklisted();
+}

[Ilya Sherman, 2013/11/19 22:49:04]

Can this be tucked into the anonymous namespace, rather than being exported in
the header file?

[lipalani1, 2013/11/26 23:31:35]

Are used in test cases.
On 2013/11/19 22:49:04, Ilya Sherman wrote: