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

chrome/browser/password_manager/password_syncable_service_unittest.cc

Index: chrome/browser/password_manager/password_syncable_service_unittest.cc
diff --git a/chrome/browser/password_manager/password_syncable_service_unittest.cc b/chrome/browser/password_manager/password_syncable_service_unittest.cc
new file mode 100755
index 0000000000000000000000000000000000000000..4706b559363f25fb0ecd576784f77ad73717212b
--- /dev/null
+++ b/chrome/browser/password_manager/password_syncable_service_unittest.cc
@@ -0,0 +1,396 @@
+// Copyright 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"

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

nit: This include should be at the very top -- see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

+#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;
+
+const sync_pb::PasswordSpecificsData& GetPasswordSpecifics(
+    const syncer::SyncData& sync_change) {
+  const sync_pb::EntitySpecifics& specifics = sync_change.GetSpecifics();
+  return specifics.password().client_only_encrypted_data();
+}
+
+}  // namespace
+
+// A testable implementation of the |PasswordSyncableService| that mocks
+// out all interaction with the password database.
+class TestPasswordSyncableService : public PasswordSyncableService {

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

nit: TestPasswordSyncableService -> MockPasswordSyncableService

+ public:
+  explicit TestPasswordSyncableService(PasswordStore* password_store)
+      : PasswordSyncableService(password_store) {}
+  virtual ~TestPasswordSyncableService() {}
+  MOCK_METHOD0(NotifyPasswordStoreOfLoginChanges, void());
+};
+
+bool PasswordsEqual(const sync_pb::PasswordSpecificsData& actual_password,
+                    const sync_pb::PasswordSpecificsData& expected_password) {
+  return (actual_password.scheme() == expected_password.scheme() &&
+          actual_password.signon_realm() == expected_password.signon_realm() &&
+          actual_password.origin() == expected_password.origin() &&
+          actual_password.action() == expected_password.action() &&
+          actual_password.username_element() ==
+              expected_password.username_element() &&
+          actual_password.password_element() ==
+              expected_password.password_element() &&
+          actual_password.username_value() ==
+              expected_password.username_value() &&
+          actual_password.password_value() ==
+              expected_password.password_value() &&
+          actual_password.ssl_valid() == expected_password.ssl_valid() &&
+          actual_password.preferred() == expected_password.preferred() &&
+          actual_password.date_created() ==
+              expected_password.date_created() &&
+          actual_password.blacklisted() ==
+              expected_password.blacklisted());
+}
+
+// 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() {}
+  virtual ~TestSyncChangeProcessor() {}
+  virtual SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const SyncChangeList& change_list) OVERRIDE {
+    // Verifies the |change_list| matches the expected changes.
+    for (SyncChangeList::const_iterator it = change_list.begin();
+         it != change_list.end();
+         ++it) {
+      const SyncChange& data = *it;
+      const sync_pb::PasswordSpecificsData& actual_password(
+          GetPasswordSpecifics(data.sync_data()));
+      std::string actual_tag = PasswordSyncableService::MakeTag(
+          actual_password);
+
+      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::PasswordSpecificsData& expected_password(
+            GetPasswordSpecifics(expected_data.sync_data()));
+        std::string expected_tag = PasswordSyncableService::MakeTag(
+            expected_password);
+        if (expected_tag == actual_tag &&
+            PasswordsEqual(actual_password, expected_password)) {
+          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 OVERRIDE{
+    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::PasswordSpecificsData& password_specifics(
+        GetPasswordSpecifics(data));
+
+    autofill::PasswordForm form;
+    PasswordSyncableService::ExtractPasswordFromSpecifics(password_specifics,
+                                                          &form);
+    expected_add_changes_.push_back(form);
+  }
+
+  // Adds an expected update change.
+  void AddExpectedUpdateChange(const autofill::PasswordForm& form) {
+    expected_update_changes_.push_back(form);
+  }
+
+  // Verifies that the |password| is present in the |expected_add_changes_|
+  // list. If found |password| would be removed from
+  // |expected_add_changes_| list.
+  void VerifyAdd(const autofill::PasswordForm& password) {
+    VerifyChange(password, &expected_add_changes_);
+  }
+
+  // Verifies that the |password| is present in the |expected_update_changes_|
+  // list.

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

nit: Please document here as well that the entry will be removed if found.  This
is an externally observable side-effect, since a client calling
UpdateChangeCount() before and after this method would see different results.

+  void VerifyUpdate(const autofill::PasswordForm& password) {
+    VerifyChange(password, &expected_update_changes_);
+  }
+
+  size_t AddChangeCount() const {
+    return expected_add_changes_.size();
+  }
+
+  size_t UpdateChangeCount() const {
+    return expected_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> expected_add_changes_;
+  std::vector<autofill::PasswordForm> expected_update_changes_;
+
+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreDataVerifier);
+};
+
+namespace {
+
+// Creates a sync data consisting of password specifics. The sign on realm is
+// set to |signon_realm|.
+SyncData CreateSyncData(const 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);
+}

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

nit: The anonymous namespace should end here.  The actual test code should not
be in the anonymous namespace, as otherwise the ensuing name-mangling can cause
problems for our test runners.

+
+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);
+  }
+
+  PasswordStoreDataVerifier* verifier() {
+    return &verifier_;
+  }
+
+  scoped_ptr<TestSyncChangeProcessor> ReleaseSyncChangeProcessor() {
+    return sync_change_processor_.Pass();
+  }
+
+  // Sets the data that will be returned to the caller accessing password store.
+  void SetPasswordStoreData(
+      const std::vector<autofill::PasswordForm*>& forms) {
+    EXPECT_CALL(*(password_store_.get()), FillAutofillableLogins(_))
+        .WillOnce(DoAll(SetArgumentPointee<0>(forms), Return(true)));
+  }
+
+ protected:
+  scoped_refptr<MockPasswordStore> password_store_;
+  scoped_ptr<TestPasswordSyncableService> service_;
+  PasswordStoreDataVerifier verifier_;
+  scoped_ptr<TestSyncChangeProcessor> sync_change_processor_;
+};
+
+}  // namespace
+
+// Both sync and password db have data that are not present in the other.
+TEST_F(PasswordSyncableServiceTest, AdditionsInBoth) {
+  scoped_ptr<autofill::PasswordForm> form1(new autofill::PasswordForm);
+  form1->signon_realm = "abc";
+
+  sync_change_processor_->AddExpectedChange(*form1,
+                                             SyncChange::ACTION_ADD);
+
+  std::vector<autofill::PasswordForm*> forms;
+  forms.push_back(form1.release());
+
+  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_, NotifyPasswordStoreOfLoginChanges());
+
+  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_, NotifyPasswordStoreOfLoginChanges());
+
+  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());

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

nit: Please align params.

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

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

nit: "autofill::PasswordForm *form1" -> "scoped_ptr<autofill::PasswordForm>
form1"  (applies throughout)

+  form1->signon_realm = "abc";
+
+  std::vector<autofill::PasswordForm*> forms;

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

nit: ScopedVector (applies throughout)

+  forms.push_back(form1);
+
+  SyncDataList list;

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

nit: You could just pass SyncDataList() on line 333.

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

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

nit: Align args.

+  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);
+
+  SetPasswordStoreData(forms);
+
+  EXPECT_CALL(*password_store_, UpdateLoginImpl(_))
+      .WillRepeatedly(Invoke(verifier(),
+                             &PasswordStoreDataVerifier::VerifyUpdate));
+
+  EXPECT_CALL(*service_, NotifyPasswordStoreOfLoginChanges());
+
+  service_->MergeDataAndStartSyncing(syncer::PASSWORDS,
+                                      list,
+                                      ReleaseSyncChangeProcessor(),
+                                      scoped_ptr<syncer::SyncErrorFactory>());

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

nit: Align args.

+
+  EXPECT_EQ(0, verifier()->UpdateChangeCount());
+}
+