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

chrome/browser/password_manager/password_syncable_service_unittest.cc

+// Copyright (c) 2013 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 <string>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "chrome/browser/password_manager/mock_password_store.h"
+#include "chrome/browser/password_manager/password_store_factory.h"
+#include "chrome/browser/password_manager/password_syncable_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/test/base/testing_profile.h"
+#include "sync/api/sync_change_processor.h"
+#include "sync/api/sync_error.h"
+#include "sync/api/sync_error_factory.h"
+#include "sync/protocol/password_specifics.pb.h"
+#include "sync/protocol/sync.pb.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using syncer::SyncChange;
+using syncer::SyncData;
+using syncer::SyncDataList;
+using syncer::SyncError;
+using testing::Invoke;
+using testing::Return;
+using testing::SetArgumentPointee;
+using testing::_;
+
+namespace {
+
+typedef std::vector<SyncChange> SyncChangeList;
+
+// This class will be instantiated by the tests rather than the
+// |PasswordSyncableService| as this class will mock away any calls
+// that touch the password db.
+class TestPasswordSyncableService : public PasswordSyncableService {
+ public:
+  explicit TestPasswordSyncableService(PasswordStore* password_store):

[Ilya Sherman, 2013/11/05 08:39:21]

nit: The colon belongs on the next line, formatted like so:

  explicit TestPasswordSyncableService(PasswordStore* password_store)
      : PasswordSyncableService(password_store) {}

[lipalani1, 2013/11/19 00:46:54]

Done.

+      PasswordSyncableService(password_store) {}
+  virtual ~TestPasswordSyncableService() {}
+  MOCK_METHOD0(NotifyPasswordStore, void());
+};
+
+// Concrete implementation of SyncChangeprocessor. The methods will

[Ilya Sherman, 2013/11/05 08:39:21]

nit: "SyncChangeprocessor" -> "SyncChangeProcessor"

[lipalani1, 2013/11/19 00:46:54]

Done.

+// verify that the |PasswordSyncableService| is calling the
+// |SyncChangeProcessor| with right arguments.
+class TestSyncChangeProcessor : public syncer::SyncChangeProcessor {
+ public:
+  TestSyncChangeProcessor() {}
+  virtual ~TestSyncChangeProcessor() {}
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) {
+    // Loop through the |change_list| and verify they are present in the
+    // |expected_changes_| list.
+    for (SyncChangeList::const_iterator it = change_list.begin();
+         it != change_list.end();
+         ++it) {
+      SyncChange data = *it;
+      const sync_pb::EntitySpecifics& specifics =
+          data.sync_data().GetSpecifics();
+      const sync_pb::PasswordSpecificsData& password_specifics(
+          specifics.password().client_only_encrypted_data());
+      std::string actual_tag = PasswordSyncableService::MakeTag(
+          password_specifics);

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Might be helpful to decompose lines 66-71 into a helper function.

[lipalani1, 2013/11/19 00:46:54]

Done.

+
+      bool matched = false;
+      for (SyncChangeList::iterator expected_it = expected_changes_.begin();
+           expected_it != expected_changes_.end();
+           ++expected_it) {
+        SyncChange expected_data = *expected_it;
+        const sync_pb::EntitySpecifics& specifics =
+            expected_data.sync_data().GetSpecifics();
+        const sync_pb::PasswordSpecificsData& password_specifics(
+            specifics.password().client_only_encrypted_data());
+        std::string expected_tag = PasswordSyncableService::MakeTag(
+            password_specifics);
+        if (expected_tag == actual_tag) {
+          if (data.change_type() == expected_data.change_type()) {
+            matched = true;
+          }
+          break;
+        }
+      }
+      EXPECT_TRUE(matched);
+    }
+    EXPECT_EQ(change_list.size(), expected_changes_.size());
+    return SyncError();
+  }
+
+  virtual SyncDataList GetAllSyncData(syncer::ModelType type) const {
+    return SyncDataList();
+  }
+
+  // Adds a password entry to the |expected_changes_| list.
+  void AddExpectedChange(const autofill::PasswordForm& password,
+                         SyncChange::SyncChangeType type) {
+    SyncData data = PasswordSyncableService::CreateSyncData(password);
+    SyncChange change(FROM_HERE, type, data);
+    expected_changes_.push_back(change);
+  }
+
+ private:
+  SyncChangeList expected_changes_;
+  DISALLOW_COPY_AND_ASSIGN(TestSyncChangeProcessor);
+};
+
+// Class to verify the arguments passed to |PasswordStore|.
+class PasswordStoreDataVerifier {
+ public:
+  PasswordStoreDataVerifier() {}
+  virtual ~PasswordStoreDataVerifier() {}
+
+  // Adds an expected add change.
+  void AddExpectedAddChange(const SyncData& data) {
+    const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();
+    const sync_pb::PasswordSpecificsData& password_specifics(
+        specifics.password().client_only_encrypted_data());
+
+    autofill::PasswordForm form;
+    PasswordSyncableService::ExtractPasswordFromSpecifics(password_specifics,
+                                                          &form);
+    add_changes_.push_back(form);
+  }
+
+  // Adds an expected update change.
+  void AddExpectedUpdateChange(const autofill::PasswordForm& form) {
+    update_changes_.push_back(form);
+  }
+
+  // Verifies that the |password| is present in the |add_changes_| list.

[Ilya Sherman, 2013/11/05 08:39:21]

Please document that this has a side-effect of removing the |password| from the
|add_changes_| list if it is found.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  void VerifyAdd(const autofill::PasswordForm& password) {
+    VerifyChange(password, &add_changes_);
+  }
+
+  // Verifies that the |password| is present in the |update_changes_| list.
+  void VerifyUpdate(const autofill::PasswordForm& password) {
+    VerifyChange(password, &update_changes_);
+  }
+
+  int AddChangeCount() const {

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Consider making the return value for this method have type size_t rather
than int.

[lipalani1, 2013/11/19 00:46:54]

Done.

+    return add_changes_.size();
+  }
+
+  int UpdateChangeCount() const {
+    return update_changes_.size();
+  }
+
+ private:
+  void VerifyChange(const autofill::PasswordForm& password,
+                    std::vector<autofill::PasswordForm>* password_list) {
+    bool matched = false;
+    for (std::vector<autofill::PasswordForm>::iterator it
+           = password_list->begin();
+         it != password_list->end();
+         ++it) {
+      if (password == *it) {
+        password_list->erase(it);
+        matched = true;
+        break;
+      }
+    }
+    EXPECT_TRUE(matched);
+  }

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Please leave a blank line after this one.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  std::vector<autofill::PasswordForm> add_changes_;
+  std::vector<autofill::PasswordForm> update_changes_;

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Consider naming these variables something more like "expected_new_entries_"
and "expected_updated_entries_" to match the names the production code uses.

[lipalani1, 2013/11/19 00:46:54]

'Add' and 'Update' terminologies are also widely used.
On 2013/11/05 08:39:21, Ilya Sherman wrote:

+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreDataVerifier);
+};
+
+SyncData CreateSyncData(std::string signon_realm) {

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Please pass by const-reference.

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Docs, please.

[lipalani1, 2013/11/19 00:46:54]

Done.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  sync_pb::EntitySpecifics password_data;
+  sync_pb::PasswordSpecificsData* password_specifics =
+      password_data.mutable_password()->mutable_client_only_encrypted_data();
+  password_specifics->set_signon_realm(signon_realm);
+
+  std::string tag = PasswordSyncableService::MakeTag(*password_specifics);
+  return syncer::SyncData::CreateLocalData(tag, tag, password_data);
+}

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Please tuck all of the code above this line into an anonymous namespace.

[lipalani1, 2013/11/19 00:46:54]

Done.

+
+class PasswordSyncableServiceTest : public testing::Test {
+ public:
+  PasswordSyncableServiceTest() {}
+  ~PasswordSyncableServiceTest() {}
+
+  virtual void SetUp() OVERRIDE {
+    TestingProfile::Builder builder;
+    scoped_ptr<Profile> profile = builder.Build().Pass();
+    password_store_ = static_cast<MockPasswordStore*>(
+        PasswordStoreFactory::GetInstance()->SetTestingFactoryAndUse(
+            profile.get(), MockPasswordStore::Build).get());
+    service_.reset(new TestPasswordSyncableService(password_store_));
+    sync_change_processor_.reset(new TestSyncChangeProcessor);
+  }
+
+  TestPasswordSyncableService* service() {
+    return service_.get();
+  }
+
+  PasswordStoreDataVerifier* verifier() {
+    return &verifier_;
+  }
+
+  TestSyncChangeProcessor* sync_change_processor() {
+    return sync_change_processor_.get();
+  }
+
+  scoped_ptr<TestSyncChangeProcessor> ReleaseSyncChangeProcessor() {
+    return sync_change_processor_.Pass();
+  }

[Ilya Sherman, 2013/11/05 08:39:21]

These four accessors seem unnecessary given that their backing variables all
have protected visibility.

[lipalani1, 2013/11/19 00:46:54]

Done. However I have left the verififer() in place as it simplifies the calling
site.

+
+  void SetPasswordStoreData(
+      const std::vector<autofill::PasswordForm*>& forms) {

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Docs, please.

[lipalani1, 2013/11/19 00:46:54]

Done.

+    EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+        .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+    EXPECT_CALL(*(password_store_.get()), FillBlacklistLogins(_))
+        .WillOnce(Return(true));

[Ilya Sherman, 2013/11/05 08:39:21]

Please also add test coverage for FillBlacklistLogins returning data, and
FillAutofillableLogins *not* returning data.

[lipalani1, 2013/11/19 00:46:54]

Good point. this cl has gotten big already. I have left a todo and will fix it
in a subsequent patch.
On 2013/11/05 08:39:21, Ilya Sherman wrote:

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

I'm not comfortable with punting tests to follow-up patches.  If you'd like to
reduce the size of this CL, please split out functionality, not test coverage.

On 2013/11/19 00:46:54, lipalani1 wrote:

+  }
+
+ protected:
+  scoped_refptr<MockPasswordStore> password_store_;
+  scoped_ptr<TestPasswordSyncableService> service_;
+  PasswordStoreDataVerifier verifier_;
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor_;
+};
+
+// Both sync and password db have data that are not present in the other.
+TEST_F(PasswordSyncableServiceTest, AdditionsInBoth) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Asterisk belongs before the space.

[Ilya Sherman, 2013/11/05 08:39:21]

Please use a scoped_ptr to own the memory for this variable until it's passed to
its next owner.

[lipalani1, 2013/11/19 00:46:54]

Done.

[lipalani1, 2013/11/19 00:46:54]

Done.

+  form1->signon_realm = "abc";
+
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+
+  SyncData sync_data = CreateSyncData("def");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  sync_change_processor()->AddExpectedChange(*form1,
+                                             SyncChange::ACTION_ADD);
+
+  verifier()->AddExpectedAddChange(sync_data);
+
+  SetPasswordStoreData(forms);
+  EXPECT_CALL(*password_store_, AddLoginImpl(_))
+      .WillRepeatedly(Invoke(
+           verifier(), &PasswordStoreDataVerifier::VerifyAdd));
+
+  EXPECT_CALL(*service(), NotifyPasswordStore());
+
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
+
+  EXPECT_EQ(0, verifier()->AddChangeCount());
+}
+
+// Sync has data that is not present in the password db.
+TEST_F(PasswordSyncableServiceTest, AdditionOnlyInSync) {
+  std::vector<autofill::PasswordForm*> forms;
+
+  SyncData sync_data = CreateSyncData("def");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  verifier()->AddExpectedAddChange(sync_data);
+
+  SetPasswordStoreData(forms);
+
+  EXPECT_CALL(*password_store_, AddLoginImpl(_))
+      .WillRepeatedly(Invoke(
+           verifier(), &PasswordStoreDataVerifier::VerifyAdd));
+
+  EXPECT_CALL(*service(), NotifyPasswordStore());
+
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
+
+  EXPECT_EQ(0, verifier()->AddChangeCount());
+}
+
+// Passwords db has data that is not present in sync.
+TEST_F(PasswordSyncableServiceTest, AdditionOnlyInPasswordStore) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;
+  form1->signon_realm = "abc";
+
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+
+  SyncDataList list;
+
+  sync_change_processor()->AddExpectedChange(*form1,
+                                             SyncChange::ACTION_ADD);
+
+  SetPasswordStoreData(forms);
+
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
+
+  EXPECT_EQ(0, verifier()->AddChangeCount());
+}
+
+// Both passwords db and sync contain the same data.
+TEST_F(PasswordSyncableServiceTest, BothInSync) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;
+  form1->signon_realm = "abc";
+
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+
+  SyncData sync_data = CreateSyncData("abc");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  SetPasswordStoreData(forms);
+
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
+  EXPECT_EQ(0, verifier()->AddChangeCount());
+  EXPECT_EQ(0, verifier()->UpdateChangeCount());
+}
+
+// Both passwords db and sync have the same data but they need to be merged
+// as some fields of the data differ.
+TEST_F(PasswordSyncableServiceTest, Merge) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;
+  form1->signon_realm = "abc";
+  form1->action = GURL("http://pie.com");
+
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+
+  SyncData sync_data = CreateSyncData("abc");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  sync_change_processor()->AddExpectedChange(*form1,
+                                             SyncChange::ACTION_UPDATE);
+
+  verifier()->AddExpectedUpdateChange(*form1);

[Ilya Sherman, 2013/11/05 08:39:21]

Why are both the local and the sync value expected to be updated?  What is the
expected resulting value?

[lipalani1, 2013/11/19 00:46:54]

In this case form1 wins. But in our production code we update both stores in
these types of cases. 
On 2013/11/05 08:39:21, Ilya Sherman wrote:

+
+  SetPasswordStoreData(forms);
+
+  EXPECT_CALL(*password_store_, UpdateLoginImpl(_))
+      .WillRepeatedly(Invoke(verifier(),
+                             &PasswordStoreDataVerifier::VerifyUpdate));
+
+  EXPECT_CALL(*service(), NotifyPasswordStore());
+
+  service()->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());
+
+  EXPECT_EQ(0, verifier()->UpdateChangeCount());
+}

[Ilya Sherman, 2013/11/05 08:39:21]

Please also add test coverage to verify that *all* fields are copied correctly,
both when there are new entries either on the Sync or local side, and when the
data is being merged.  The best way to do this might be to expand the data
that's passed into AddExpectedChange() and AddExpectedUpdateChange().

[lipalani1, 2013/11/19 00:46:54]

When verifying add/update we compare all fields.
On 2013/11/05 08:39:21, Ilya Sherman wrote:

+
+}  // namespace
+

[Ilya Sherman, 2013/11/05 08:39:21]

nit: Spurious newline.

[lipalani1, 2013/11/19 00:46:54]

Done.