[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/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):
+      PasswordSyncableService(password_store) {}
+  virtual ~TestPasswordSyncableService() {}
+  MOCK_METHOD0(NotifyPasswordStore, void());
+};
+
+// Concrete implementation of SyncChangeprocessor. The methods will
+// verify that the |PasswordSyncableService| is calling the
+// |SyncChangeProcessor| with right arguments.
+class TestSyncChangeProcessor : public syncer::SyncChangeProcessor {
+ public:
+  TestSyncChangeProcessor() {}
+  ~TestSyncChangeProcessor() {}

[Nicolas Zea, 2013/10/25 20:20:13]

virtual destructor, newline after

[lipalani1, 2013/10/29 20:16:36]

Done.

+  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);
+
+      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:

[Nicolas Zea, 2013/10/25 20:20:13]

newline before

[lipalani1, 2013/10/29 20:16:36]

Done.

+  SyncChangeList expected_changes_;

[Nicolas Zea, 2013/10/25 20:20:13]

disallow copy and assign?

[lipalani1, 2013/10/29 20:16:36]

Done.

+};
+
+// Class to verify the arguments passed to |PasswordStore|.
+class PasswordStoreDataVerifier {
+ public:
+  // 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.
+  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 {
+    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);
+  }
+  std::vector<autofill::PasswordForm> add_changes;
+  std::vector<autofill::PasswordForm> update_changes;

[Nicolas Zea, 2013/10/25 20:20:13]

disallow copy and assign

[lipalani1, 2013/10/29 20:16:36]

Done.

+};
+
+SyncData CreateSyncData(std::string signon_realm) {
+  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);
+}
+
+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());
+  }
+
+ protected:
+  scoped_refptr<MockPasswordStore> password_store_;
+};
+
+// Both sync and password db have data that are not present in the other.
+TEST_F(PasswordSyncableServiceTest, AdditionsInBoth) {
+  autofill::PasswordForm *form1 = new autofill::PasswordForm;
+  form1->signon_realm = "abc";
+
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1);
+
+  TestPasswordSyncableService service(password_store_.get());

[Nicolas Zea, 2013/10/25 20:20:13]

Looks like all tests define this. Maybe just have it be a private member in the
PasswordSyncableServiceTest class, and have a public service() getter?

Similarly, the behavior below where you set the local data, and the expectations
of the changes, seems to be pretty similar across the various tests. Perhaps
have helper methods that do this for you?

For example, the final code for this test might look like:
sync_change_processor()->AddExpectedChange(form1, ACTION_ADD);
verifier()->AddExpectedChange(sync_data);
SetPasswordStoreData(forms);
service()->MergeDataAndStartSyncing(..)
EXPECT_EQ(0, verifier.AddChangeCount())

And hopefully the other tests might be similar.

[lipalani1, 2013/10/29 20:16:36]

I have done some refactoring and it is better now.

The problem is most of the tests don't need one aspect of the bundle so I
bundled few common denominators to functions of each.
On 2013/10/25 20:20:13, Nicolas Zea wrote:

+
+  SyncData sync_data = CreateSyncData("def");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor(
+      new TestSyncChangeProcessor);
+  sync_change_processor->AddExpectedChange(*form1,
+                                           SyncChange::ACTION_ADD);
+
+  PasswordStoreDataVerifier verifier;
+  verifier.AddExpectedAddChange(sync_data);
+
+  EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+  EXPECT_CALL(*(password_store_.get()), FillBlacklistLogins(_))
+      .WillOnce(Return(true));
+
+  EXPECT_CALL(*(password_store_.get()), AddLoginImpl(_))
+      .WillRepeatedly(Invoke(
+           &verifier, &PasswordStoreDataVerifier::VerifyAdd));
+
+  EXPECT_CALL(service, NotifyPasswordStore());
+
+  service.MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                   list,
+                                   sync_change_processor.Pass(),
+                                   scoped_ptr<syncer::SyncErrorFactory>()
+                                       .Pass());
+
+  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;
+
+  TestPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("def");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor(
+      new TestSyncChangeProcessor);
+
+  PasswordStoreDataVerifier verifier;
+  verifier.AddExpectedAddChange(sync_data);
+
+  EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+  EXPECT_CALL(*password_store_.get(), FillBlacklistLogins(_))
+      .WillOnce(Return(true));
+
+  EXPECT_CALL(*(password_store_.get()), AddLoginImpl(_))
+      .WillRepeatedly(Invoke(
+           &verifier, &PasswordStoreDataVerifier::VerifyAdd));
+
+  EXPECT_CALL(service, NotifyPasswordStore());
+
+  service.MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                   list,
+                                   sync_change_processor.Pass(),
+                                   scoped_ptr<syncer::SyncErrorFactory>()
+                                       .Pass());
+
+  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);
+
+  TestPasswordSyncableService service(password_store_.get());
+
+  SyncDataList list;
+
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor(
+      new TestSyncChangeProcessor);
+  sync_change_processor->AddExpectedChange(*form1,
+                                           SyncChange::ACTION_ADD);
+
+  PasswordStoreDataVerifier verifier;
+
+  EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+  EXPECT_CALL(*password_store_.get(), FillBlacklistLogins(_))
+      .WillOnce(Return(true));
+
+  service.MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                   list,
+                                   sync_change_processor.Pass(),
+                                   scoped_ptr<syncer::SyncErrorFactory>()
+                                       .Pass());
+
+  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);
+
+  TestPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("abc");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor(
+      new TestSyncChangeProcessor);
+
+  EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+  EXPECT_CALL(*password_store_.get(), FillBlacklistLogins(_))
+      .WillOnce(Return(true));
+
+  service.MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                   list,
+                                   sync_change_processor.Pass(),
+                                   scoped_ptr<syncer::SyncErrorFactory>()
+                                       .Pass());
+}
+
+// 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);
+
+  TestPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("abc");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor(
+      new TestSyncChangeProcessor);
+  sync_change_processor->AddExpectedChange(*form1,
+                                           SyncChange::ACTION_UPDATE);
+
+  PasswordStoreDataVerifier verifier;
+  verifier.AddExpectedUpdateChange(*form1);
+
+  EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+      .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+  EXPECT_CALL(*password_store_.get(), FillBlacklistLogins(_))
+      .WillOnce(Return(true));
+
+  EXPECT_CALL(*(password_store_.get()), UpdateLoginImpl(_))
+      .WillRepeatedly(Invoke(&verifier,
+                             &PasswordStoreDataVerifier::VerifyUpdate));
+
+  EXPECT_CALL(service, NotifyPasswordStore());
+
+  service.MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                   list,
+                                   sync_change_processor.Pass(),
+                                   scoped_ptr<syncer::SyncErrorFactory>()
+                                       .Pass());
+
+  EXPECT_EQ(0, verifier.UpdateChangeCount());
+}
+}  // namespace
+