[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 "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;
+
+typedef std::vector<SyncChange> SyncChangeList;

[Nicolas Zea, 2013/10/21 23:55:14]

put this and the rest of the file in an anonymous namespace?

[lipalani1, 2013/10/24 00:13:58]

Done.

+
+// 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 MockPasswordSyncableService : public PasswordSyncableService {

[Nicolas Zea, 2013/10/21 23:55:14]

It's generally cleaner to use fakes when overriding functionality than mocks.
Mocks are better for purely virtual interfaces. Perhaps just have this be a
TestPasswordSyncableService whose NotifyPasswordStore implementation counts the
number of notifications?

Similarly, you could abstract the other password store interaction into separate
methods, which the test version overrides and supplies test data for/counts
invokations, and avoid the need for using the MockPasswordStore below. This will
probably also make it easier for future refactorings that remove the need for
friending.

See
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/mocks/...
for more discussion on that

[lipalani1, 2013/10/24 00:13:58]

I tried this approach. it ended up with the boiler plate code in tests, to check
the number of times NotifyPasswordStore was
invoked(EXPECT_EQ(test_password_syncable_service.invocation_count(),1)).
EXPECT_CALL gives this functionality built in. 
If there is need to add custom methods for other password store methods it is
possible to add it in this class itself(those methods would be regular virtual
methods rather than mocks) Let me know what you think.
On 2013/10/21 23:55:14, Nicolas Zea wrote:

+ public:
+  explicit MockPasswordSyncableService(PasswordStore* password_store):
+      PasswordSyncableService(password_store) {}
+  virtual ~MockPasswordSyncableService() {}
+  MOCK_METHOD0(NotifyPasswordStore, void());
+};
+
+// Concrete implementation of SyncChangeprocessor. The methods will
+// verify that the |PasswordSyncableService| is calling the
+// |SyncChangeProcessor| with right arguments.
+class MockSyncChangeProcessor : public syncer::SyncChangeProcessor {

[Nicolas Zea, 2013/10/21 23:55:14]

naming nit: This isn't a mock, so maybe just call it TestSyncChangeProccesor?

[lipalani1, 2013/10/24 00:13:58]

Done.

+ public:
+  MockSyncChangeProcessor() {}
+  ~MockSyncChangeProcessor() {}
+  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:
+  SyncChangeList expected_changes_;
+};
+
+// 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;
+};
+
+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);
+
+  MockPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("def");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<MockSyncChangeProcessor> sync_change_processor(
+      new MockSyncChangeProcessor);
+  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;
+
+  MockPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("def");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<MockSyncChangeProcessor> sync_change_processor(
+      new MockSyncChangeProcessor);
+
+  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);
+
+  MockPasswordSyncableService service(password_store_.get());
+
+  SyncDataList list;
+
+  scoped_ptr<MockSyncChangeProcessor> sync_change_processor(
+      new MockSyncChangeProcessor);
+  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);
+
+  MockPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("abc");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<MockSyncChangeProcessor> sync_change_processor(
+      new MockSyncChangeProcessor);
+
+  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);
+
+  MockPasswordSyncableService service(password_store_.get());
+
+  SyncData sync_data = CreateSyncData("abc");
+  SyncDataList list;
+  list.push_back(sync_data);
+
+  scoped_ptr<MockSyncChangeProcessor> sync_change_processor(
+      new MockSyncChangeProcessor);
+  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());
+}
+