[Sync] Dont re-encrypt data when new nigori arrives.

The logic was previously meant to handle newly encrypted types. But, it would
also attempt to re-encrypt data when any new nigori arrived. We now skip
data that is already encrypted (even if it's not with the most recent
encryption keys).

BUG=115309
TEST=sync_unit_tests. Receiving a nigori node update that leaves the
cryptographer ready for an account with enrypt everything should no longer
crash.


Review URL: http://codereview.chromium.org/9550001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@124354 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 78 insertions, 17 deletions

diff --git a/chrome/browser/sync/engine/apply_updates_command_unittest.cc b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
index e6ccb1d..4af85cc 100644
--- a/chrome/browser/sync/engine/apply_updates_command_unittest.cc
+++ b/chrome/browser/sync/engine/apply_updates_command_unittest.cc
@@ -764,6 +764,11 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) {
           .Equals(syncable::ModelTypeSet::All()));
 }
 
+// Create some local unsynced and unencrypted data. Apply a nigori update that
+// turns on encryption for the unsynced data. Ensure we properly encrypt the
+// data as part of the nigori update. Apply another nigori update with no
+// changes. Ensure we ignore already-encrypted unsynced data and that nothing
+// breaks.
 TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
   // Storing the cryptographer separately is bad, but for this test we
   // know it's safe.
@@ -830,18 +835,20 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
   ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE);
   apply_updates_command_.ExecuteImpl(session());
 
-  sessions::StatusController* status = session()->mutable_status_controller();
-  sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
-  ASSERT_TRUE(status->update_progress());
-  EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
-      << "All updates should have been attempted";
-  ASSERT_TRUE(status->conflict_progress());
-  EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
-      << "No updates should be in conflict";
-  EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize())
-      << "No updates should be in conflict";
-  EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
-      << "The nigori update should be applied";
+  {
+    sessions::StatusController* status = session()->mutable_status_controller();
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
+    ASSERT_TRUE(status->update_progress());
+    EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize())
+        << "All updates should have been attempted";
+    ASSERT_TRUE(status->conflict_progress());
+    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
+        << "No updates should be in conflict";
+    EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize())
+        << "No updates should be in conflict";
+    EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount())
+        << "The nigori update should be applied";
+  }
   EXPECT_FALSE(cryptographer->has_pending_keys());
   EXPECT_TRUE(cryptographer->is_ready());
   {
@@ -857,6 +864,46 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) {
     SyncerUtil::GetUnsyncedEntries(&trans, &handles);
     EXPECT_EQ(2*batch_s+1, handles.size());
   }
+
+  // Simulate another nigori update that doesn't change anything.
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, directory());
+    MutableEntry entry(&trans, syncable::GET_BY_SERVER_TAG,
+                       syncable::ModelTypeToRootTag(syncable::NIGORI));
+    ASSERT_TRUE(entry.good());
+    entry.Put(syncable::SERVER_VERSION, GetNextRevision());
+    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+  }
+  ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE);
+  apply_updates_command_.ExecuteImpl(session());
+  {
+    sessions::StatusController* status = session()->mutable_status_controller();
+    sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE);
+    ASSERT_TRUE(status->update_progress());
+    EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize())
+        << "All updates should have been attempted";
+    ASSERT_TRUE(status->conflict_progress());
+    EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize())
+        << "No updates should be in conflict";
+    EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize())
+        << "No updates should be in conflict";
+    EXPECT_EQ(2, status->update_progress()->SuccessfullyAppliedUpdateCount())
+        << "The nigori update should be applied";
+  }
+  EXPECT_FALSE(cryptographer->has_pending_keys());
+  EXPECT_TRUE(cryptographer->is_ready());
+  {
+    ReadTransaction trans(FROM_HERE, directory());
+
+    // All our changes should still be encrypted.
+    EXPECT_TRUE(syncable::ModelTypeSet::All().Equals(
+        cryptographer->GetEncryptedTypes()));
+    EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types));
+
+    Syncer::UnsyncedMetaHandles handles;
+    SyncerUtil::GetUnsyncedEntries(&trans, &handles);
+    EXPECT_EQ(2*batch_s+1, handles.size());
+  }
 }
 
 TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) {
diff --git a/chrome/browser/sync/engine/nigori_util.cc b/chrome/browser/sync/engine/nigori_util.cc
index f8a26a9..b9791ab 100644
--- a/chrome/browser/sync/engine/nigori_util.cc
+++ b/chrome/browser/sync/engine/nigori_util.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.
 
@@ -21,13 +21,23 @@ bool ProcessUnsyncedChangesForEncryption(
   DCHECK(cryptographer->is_ready());
   // Get list of all datatypes with unsynced changes. It's possible that our
   // local changes need to be encrypted if encryption for that datatype was
-  // just turned on (and vice versa). This should never affect passwords.
+  // just turned on (and vice versa).
+  // Note: we do not attempt to re-encrypt data with a new key here as key
+  // changes in this code path are likely due to consistency issues (we have
+  // to be updated to a key we already have, e.g. an old key).
   std::vector<int64> handles;
   browser_sync::SyncerUtil::GetUnsyncedEntries(trans, &handles);
   for (size_t i = 0; i < handles.size(); ++i) {
     MutableEntry entry(trans, GET_BY_HANDLE, handles[i]);
+    const sync_pb::EntitySpecifics& specifics = entry.Get(SPECIFICS);
+    // Ignore types that don't need encryption or entries that are already
+    // encrypted.
+    if (!SpecificsNeedsEncryption(cryptographer->GetEncryptedTypes(),
+                                  specifics)) {
+      continue;
+    }
     if (!sync_api::WriteNode::UpdateEntryWithEncryption(cryptographer,
-                                                        entry.Get(SPECIFICS),
+                                                        specifics,
                                                         &entry)) {
       NOTREACHED();
       return false;
diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index 9867b03..2efe5d0 100644
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -300,8 +300,8 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
 
       // If this fails, something is wrong with the cryptographer, but there's
       // nothing we can do about it here.
-      syncable::ProcessUnsyncedChangesForEncryption(trans,
-                                                    cryptographer);
+      DVLOG(1) << "Received new nigori, encrypting unsynced changes.";
+      syncable::ProcessUnsyncedChangesForEncryption(trans, cryptographer);
     }
   }
 
diff --git a/chrome/browser/sync/internal_api/write_node.cc b/chrome/browser/sync/internal_api/write_node.cc
index 00f9550..8fdbfe9 100644
--- a/chrome/browser/sync/internal_api/write_node.cc
+++ b/chrome/browser/sync/internal_api/write_node.cc
@@ -45,6 +45,10 @@ bool WriteNode::UpdateEntryWithEncryption(
   // specifics are already encrypted, we want to ensure we continue encrypting.
   bool was_encrypted = old_specifics.has_encrypted();
   sync_pb::EntitySpecifics generated_specifics;
+  if (new_specifics.has_encrypted()) {
+    NOTREACHED() << "New specifics already has an encrypted blob.";
+    return false;
+  }
   if ((!SpecificsNeedsEncryption(encrypted_types, new_specifics) &&
        !was_encrypted) ||
       !cryptographer->is_initialized()) {