[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.

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

Ultra nit: Please omit the "(c)".

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

Done.

+// 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 {

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

nit: Please leave a blank line after this one.

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

Done.

+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
+
+// 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.

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

nit: Suggested rephrasing: "A testable implementation of the
|PasswordSyncableService| class that mocks out all interactions with the
password database."

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

Done.

+class TestPasswordSyncableService : public PasswordSyncableService {
+ public:
+  explicit TestPasswordSyncableService(PasswordStore* password_store)
+      : PasswordSyncableService(password_store) {}
+  virtual ~TestPasswordSyncableService() {}
+  MOCK_METHOD0(NotifyPasswordStoreOfLoginChanges, void());
+};

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

Rather than mocking out the method from the PasswordSyncableService, can we mock
out the method in MockPasswordStore?

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

It is calling private methods in password store.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+
+// 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) {

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

nit: OVERRIDE.

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

Done.

+    // Loop through the |change_list| and verify they are present in the

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

nit: Suggested rephrasing: "Verify that the |change_list| matches the
|expected_changes_|."

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

Done.

+    // |expected_changes_| list.
+    for (SyncChangeList::const_iterator it = change_list.begin();
+         it != change_list.end();
+         ++it) {
+      SyncChange data = *it;

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

nit: Can this be a const-ref?

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

Done.

+      const sync_pb::PasswordSpecificsData& password_specifics(
+          GetPasswordSpecifics(data.sync_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::PasswordSpecificsData& password_specifics(
+            GetPasswordSpecifics(expected_data.sync_data()));
+        std::string expected_tag = PasswordSyncableService::MakeTag(
+            password_specifics);
+        if (expected_tag == actual_tag) {
+          if (data.change_type() == expected_data.change_type()) {

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

It looks like this only verifies that the tags and the types match.  IMO it
would be better to verify that *all* the data matches.  For example, that would
catch a bug where the code attempted to merge the old entry rather than the
merged entry back up to the Sync server.

The simplest way to do that might be to serialize both data objects, and compare
the serialized bytes.

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

Done.

+            matched = true;
+          }
+          break;
+        }
+      }
+      EXPECT_TRUE(matched);
+    }
+    EXPECT_EQ(change_list.size(), expected_changes_.size());
+    return SyncError();
+  }
+
+  virtual SyncDataList GetAllSyncData(syncer::ModelType type) const {

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

nit: OVERRIDE.

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

Done.

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

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

What do you think of storing this as a map from tags to expected changes?  Seems
like that would simplify lines 78-94.

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

After using the maps in production code I am shying away from it because of poor
readability. As an example a statement here would be like:
iterator->second.sync_data();

Even people in sync team, who know the class names, got confused as to what
sync_data was referring to. But I understand your point. If it is all the same,
this is easy to understand(production code still uses maps as it is more
efficient).
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+  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) {

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

This is a strange API.  Why doesn't this function accept an
autofill::PasswordForm parameter instead?

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

The callers generally have the element in this form. So instead of callers
calling an additional function for transformation we let them pass what they
have.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+    const sync_pb::EntitySpecifics& specifics = data.GetSpecifics();

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

nit: Looks like this variable is unused.

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

Done.

+    const sync_pb::PasswordSpecificsData& password_specifics(
+        GetPasswordSpecifics(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. If
+  // found |password| would be removed from |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_);
+  }
+
+  size_t AddChangeCount() const {
+    return add_changes_.size();
+  }
+
+  size_t UpdateChangeCount() const {
+    return update_changes_.size();
+  }
+
+ private:
+  void VerifyChange(const autofill::PasswordForm& password,

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

nit: Docs, especially mentioning the side-effect (the erase() call).

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

That is an internal detail which the callers need not care about.

Other than that the name is clear on what it does. But in any case I have added
the comment.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

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

This is a private method, so the only callers are the ones that care about
internal details.  This is an observable side-effect outside of this method, and
a surprising one to at least one reader of this code (i.e. it surprised me). 
Please add docs to *this* method, i.e. VerifyChange().

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

+                    std::vector<autofill::PasswordForm>* password_list) {
+    bool matched = false;
+    for (std::vector<autofill::PasswordForm>::iterator it
+           = password_list->begin();

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

nit: The equals sign should be on the previous line.

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

Done.

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

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

nit: Please leave a blank line after this one.

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

nit: Consider adding an "expected_" prefix to these variable names.

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

Done.

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

Done.

+  DISALLOW_COPY_AND_ASSIGN(PasswordStoreDataVerifier);
+};

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

nit: These two classes ought to be in the anonymous namespace as well.

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

They need to be friended to PasswordStore and PasswordSyncableService.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+
+namespace {
+
+// Creates a sync data consisting of password specifics. The sign on real is

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

nit: real -> realm

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

Done.

+// set to |signon_realm|.

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

nit: Suggested rephrasing: Creates a SyncData instance with the given
|signon_realm|.

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

Done.

+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);
+}
+
+class PasswordSyncableServiceTest : public testing::Test {

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

nit: The test code itself should not be in the anonymous namespace, to avoid bad
interactions caused by C++ name mangling.

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

Done.

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

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

nit: IMO a better API would be to provide a wrapper around
MergeDataAndStartSyncing.

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

There will be test cases in future that pass in different parameter for error
factory and set expectations on the error factory. This allows us to do that
without breaking abstractions.
On 2013/11/19 22:49:04, Ilya Sherman wrote:

+
+  // 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)));
+    EXPECT_CALL(*(password_store_.get()), FillBlacklistLogins(_))
+        .WillOnce(Return(true));
+    // TODO(lipalani): Add tests that return data for calls to
+    // FillBlacklistLogins.

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

I'm not comfortable with checking in untested production code for blacklisted
logins.  If you don't think it's appropriate to add test coverage as part of
this CL, please also hold off on adding the production code, and log a bug to
help make sure this doesn't get lost.

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

Done.

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

Could you please annotate the corresponding production code with the tracking
bug to restore this functionality?

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

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

nit: Indentation is slightly off.

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

Done.

+
+  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>());
+
+  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);
+
+  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>());
+
+  EXPECT_EQ(0, verifier()->UpdateChangeCount());
+}
+
+}  // namespace

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

Please include test coverage to make sure that all of the PasswordSpecificsData
fields are copied correctly from Sync to the local datastore, and vice-versa.

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

It is verified when mergedataandstartsyncing is called.
On 2013/11/19 22:49:04, Ilya Sherman wrote: