Merge 120264 - [Sync] Fix idempotency check for passwords datatype.

We do the same logic as in UpdateEntryWithEncryption in SetPasswordSpecifics
now. Added unit tests to ensure this doesn't happen again.

BUG=112402
TEST=sync_unit_tests --gtest_filter="*UpdatePasswordsData*"

Review URL: https://chromiumcodereview.appspot.com/9315048

TBR=zea@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9310092

git-svn-id: svn://svn.chromium.org/chrome/branches/1025/src@120383 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 177 insertions, 7 deletions

diff --git a/chrome/browser/sync/internal_api/base_node.h b/chrome/browser/sync/internal_api/base_node.h
index de46166..f61d0f7 100644
--- a/chrome/browser/sync/internal_api/base_node.h
+++ b/chrome/browser/sync/internal_api/base_node.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -213,6 +213,11 @@ class BaseNode {
   friend class SyncManagerTest;
   FRIEND_TEST_ALL_PREFIXES(SyncApiTest, GenerateSyncableHash);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdateEntryWithEncryption);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest,
+                           UpdatePasswordSetEntitySpecificsNoChange);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdatePasswordSetPasswordSpecifics);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdatePasswordNewPassphrase);
+  FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdatePasswordReencryptEverything);
 
   void* operator new(size_t size);  // Node is meant for stack use only.
 
diff --git a/chrome/browser/sync/internal_api/syncapi_unittest.cc b/chrome/browser/sync/internal_api/syncapi_unittest.cc
index 0fb6e4a..fcbbf07 100644
--- a/chrome/browser/sync/internal_api/syncapi_unittest.cc
+++ b/chrome/browser/sync/internal_api/syncapi_unittest.cc
@@ -1792,4 +1792,156 @@ TEST_F(SyncManagerTest, UpdateEntryWithEncryption) {
   }
 }
 
+// Passwords have their own handling for encryption. Verify it does not result
+// in unnecessary writes via SetEntitySpecifics.
+TEST_F(SyncManagerTest, UpdatePasswordSetEntitySpecificsNoChange) {
+  std::string client_tag = "title";
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION));
+  sync_pb::EntitySpecifics entity_specifics;
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    sync_pb::PasswordSpecificsData data;
+    data.set_password_value("secret");
+    cryptographer->Encrypt(
+        data,
+        entity_specifics.MutableExtension(sync_pb::password)->
+            mutable_encrypted());
+  }
+  MakeServerNode(sync_manager_.GetUserShare(), syncable::PASSWORDS, client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::PASSWORDS,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+
+  // Manually change to the same data via SetEntitySpecifics. Should not set
+  // is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PASSWORDS, client_tag));
+    node.SetEntitySpecifics(entity_specifics);
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+}
+
+// Passwords have their own handling for encryption. Verify it does not result
+// in unnecessary writes via SetPasswordSpecifics.
+TEST_F(SyncManagerTest, UpdatePasswordSetPasswordSpecifics) {
+  std::string client_tag = "title";
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION));
+  sync_pb::EntitySpecifics entity_specifics;
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    sync_pb::PasswordSpecificsData data;
+    data.set_password_value("secret");
+    cryptographer->Encrypt(
+        data,
+        entity_specifics.MutableExtension(sync_pb::password)->
+            mutable_encrypted());
+  }
+  MakeServerNode(sync_manager_.GetUserShare(), syncable::PASSWORDS, client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::PASSWORDS,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+
+  // Manually change to the same data via SetPasswordSpecifics. Should not set
+  // is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PASSWORDS, client_tag));
+    node.SetPasswordSpecifics(node.GetPasswordSpecifics());
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+
+  // Manually change to different data. Should set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_TRUE(node.InitByClientTagLookup(syncable::PASSWORDS, client_tag));
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    sync_pb::PasswordSpecificsData data;
+    data.set_password_value("secret2");
+    cryptographer->Encrypt(
+        data,
+        entity_specifics.MutableExtension(sync_pb::password)->
+            mutable_encrypted());
+    node.SetPasswordSpecifics(data);
+    const syncable::Entry* node_entry = node.GetEntry();
+    EXPECT_TRUE(node_entry->Get(IS_UNSYNCED));
+  }
+}
+
+// Passwords have their own handling for encryption. Verify setting a new
+// passphrase updates the data.
+TEST_F(SyncManagerTest, UpdatePasswordNewPassphrase) {
+  std::string client_tag = "title";
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION));
+  sync_pb::EntitySpecifics entity_specifics;
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    sync_pb::PasswordSpecificsData data;
+    data.set_password_value("secret");
+    cryptographer->Encrypt(
+        data,
+        entity_specifics.MutableExtension(sync_pb::password)->
+            mutable_encrypted());
+  }
+  MakeServerNode(sync_manager_.GetUserShare(), syncable::PASSWORDS, client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::PASSWORDS,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+
+  // Set a new passphrase. Should set is_unsynced.
+  testing::Mock::VerifyAndClearExpectations(&observer_);
+  EXPECT_CALL(observer_, OnPassphraseAccepted(_));
+  EXPECT_CALL(observer_, OnEncryptionComplete());
+  sync_manager_.SetPassphrase("new_passphrase", true);
+  EXPECT_TRUE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+}
+
+// Passwords have their own handling for encryption. Verify it does not result
+// in unnecessary writes via ReencryptEverything.
+TEST_F(SyncManagerTest, UpdatePasswordReencryptEverything) {
+  std::string client_tag = "title";
+  EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION));
+  sync_pb::EntitySpecifics entity_specifics;
+  {
+    ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    Cryptographer* cryptographer = trans.GetCryptographer();
+    sync_pb::PasswordSpecificsData data;
+    data.set_password_value("secret");
+    cryptographer->Encrypt(
+        data,
+        entity_specifics.MutableExtension(sync_pb::password)->
+            mutable_encrypted());
+  }
+  MakeServerNode(sync_manager_.GetUserShare(), syncable::PASSWORDS, client_tag,
+                 BaseNode::GenerateSyncableHash(syncable::PASSWORDS,
+                                                client_tag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+
+  // Force a re-encrypt everything. Should not set is_unsynced.
+  testing::Mock::VerifyAndClearExpectations(&observer_);
+  EXPECT_CALL(observer_, OnEncryptionComplete());
+  sync_manager_.RefreshNigori(base::Bind(&SyncManagerTest::EmptyClosure,
+                                         base::Unretained(this)));
+  scoped_refptr<base::ThreadTestHelper> helper(
+      new base::ThreadTestHelper(
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)));
+  ASSERT_TRUE(helper->Run());
+  PumpLoop();
+  EXPECT_FALSE(ResetUnsyncedEntry(syncable::PASSWORDS, client_tag));
+}
+
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/internal_api/write_node.cc b/chrome/browser/sync/internal_api/write_node.cc
index 7f58690..58bf04a 100644
--- a/chrome/browser/sync/internal_api/write_node.cc
+++ b/chrome/browser/sync/internal_api/write_node.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -195,15 +195,28 @@ void WriteNode::SetPasswordSpecifics(
 
   Cryptographer* cryptographer = GetTransaction()->GetCryptographer();
 
-  sync_pb::PasswordSpecifics new_value;
-  if (!cryptographer->Encrypt(data, new_value.mutable_encrypted())) {
+  // We have to do the idempotency check here (vs in UpdateEntryWithEncryption)
+  // because Passwords have their encrypted data within the PasswordSpecifics,
+  // vs within the EntitySpecifics like all the other types.
+  const sync_pb::EntitySpecifics& old_specifics = GetEntry()->Get(SPECIFICS);
+  sync_pb::EntitySpecifics entity_specifics;
+  // Copy over the old specifics if they exist.
+  if (syncable::GetModelTypeFromSpecifics(old_specifics) ==
+          syncable::PASSWORDS) {
+    entity_specifics.CopyFrom(old_specifics);
+  } else {
+    syncable::AddDefaultExtensionValue(syncable::PASSWORDS,
+                                       &entity_specifics);
+  }
+  sync_pb::PasswordSpecifics* password_specifics =
+      entity_specifics.MutableExtension(sync_pb::password);
+  // This will only update password_specifics if the underlying unencrypted blob
+  // was different from |data| or was not encrypted with the proper passphrase.
+  if (!cryptographer->Encrypt(data, password_specifics->mutable_encrypted())) {
     NOTREACHED() << "Failed to encrypt password, possibly due to sync node "
                  << "corruption";
     return;
   }
-
-  sync_pb::EntitySpecifics entity_specifics;
-  entity_specifics.MutableExtension(sync_pb::password)->CopyFrom(new_value);
   SetEntitySpecifics(entity_specifics);
 }