[Sync] Gracefully handle writing to a node when the cryptographer is not ready.

Due to allowing users to configure without a passphrase if they don't have
passwords enabled, we can get into a situation where we require encryption of
datatypes, but don't have the cryptographer enabled. When in this situation
we need to ensure we don't corrupt any data. Once the user sets their
passphrase, we will reencrypt everything, which will overwrite unsynced entries
with their encrypted versions, allowing the sync cycle to finish.

BUG=93100
TEST=sync_unit_tests --gtest_filter="*GetCommitIdsFilterEntries*"
Also to manually test: set up sync on a machine with an explicit passphrase. Set up sync without passwords enabled on a separate machine and don't enter the passphrase. Enable encryption on the first client. Attempt to add a bookmark on the second client.

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

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

chrome/browser/sync/engine/get_commit_ids_command.cc

@@ -8,8 +8,11 @@
 #include <utility>
 #include <vector>
 
+#include "chrome/browser/sync/engine/nigori_util.h"
 #include "chrome/browser/sync/engine/syncer_util.h"
+#include "chrome/browser/sync/syncable/directory_manager.h"
 #include "chrome/browser/sync/syncable/syncable.h"
+#include "chrome/browser/sync/util/cryptographer.h"
 
 using std::set;
 using std::vector;
@@ -31,6 +34,15 @@ void GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
   syncable::Directory::UnsyncedMetaHandles all_unsynced_handles;
   SyncerUtil::GetUnsyncedEntries(session->write_transaction(),
                                  &all_unsynced_handles);
+
+  Cryptographer *cryptographer =
+      session->context()->directory_manager()->GetCryptographer(
+          session->write_transaction());
+  if (cryptographer) {
+    FilterEntriesNeedingEncryption(cryptographer->GetEncryptedTypes(),
+                                   session->write_transaction(),
+                                   &all_unsynced_handles);
+  }
   StatusController* status = session->status_controller();
   status->set_unsynced_handles(all_unsynced_handles);
 
@@ -46,6 +58,37 @@ void GetCommitIdsCommand::ExecuteImpl(SyncSession* session) {
   status->set_commit_set(*ordered_commit_set_.get());
 }
 
+// We create a new list of unsynced handles which omits all handles to entries
+// that require encryption but are written in plaintext. If any were found we
+// overwrite |unsynced_handles| with this new list, else no change is made.
+// Static.
+void GetCommitIdsCommand::FilterEntriesNeedingEncryption(
+    const syncable::ModelTypeSet& encrypted_types,
+    syncable::BaseTransaction* trans,
+    syncable::Directory::UnsyncedMetaHandles* unsynced_handles) {
+  bool removed_handles = false;
+  syncable::Directory::UnsyncedMetaHandles::iterator iter;
+  syncable::Directory::UnsyncedMetaHandles new_unsynced_handles;
+  new_unsynced_handles.reserve(unsynced_handles->size());
+  // TODO(zea): If this becomes a bottleneck, we should merge this loop into the
+  // AddCreatesAndMoves and AddDeletes loops.
+  for (iter = unsynced_handles->begin();
+       iter != unsynced_handles->end();
+       ++iter) {
+    syncable::Entry entry(trans, syncable::GET_BY_HANDLE, *iter);
+    if (syncable::EntryNeedsEncryption(encrypted_types, entry)) {
+      // This entry requires encryption but is not encrypted (possibly due to
+      // the cryptographer not being initialized). Don't add it to our new list
+      // of unsynced handles.
+      removed_handles = true;
+    } else {
+      new_unsynced_handles.push_back(*iter);
+    }
+  }
+  if (removed_handles)
+    *unsynced_handles = new_unsynced_handles;
+}
+
 void GetCommitIdsCommand::AddUncommittedParentsAndTheirPredecessors(
     syncable::BaseTransaction* trans,
     syncable::Id parent_id,

chrome/browser/sync/engine/get_commit_ids_command.h

-// Copyright (c) 2006-2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -29,6 +29,13 @@ class GetCommitIdsCommand : public SyncerCommand {
   // SyncerCommand implementation.
   virtual void ExecuteImpl(sessions::SyncSession* session);
 
+  // Filter |unsynced_handles| to exclude all handles to entries that require
+  // encryption but are in plaintext.
+  static void FilterEntriesNeedingEncryption(
+      const syncable::ModelTypeSet& encrypted_types,
+      syncable::BaseTransaction* trans,
+      syncable::Directory::UnsyncedMetaHandles* unsynced_handles);
+
   // Builds a vector of IDs that should be committed.
   void BuildCommitIds(const vector<int64>& unsynced_handles,
                       syncable::WriteTransaction* write_transaction,

chrome/browser/sync/engine/nigori_util.cc

@@ -63,19 +63,29 @@ bool VerifyUnsyncedChangesAreEncrypted(
       NOTREACHED();
       return false;
     }
-    const sync_pb::EntitySpecifics& entry_specifics = entry.Get(SPECIFICS);
-    ModelType type = entry.GetModelType();
-    if (type == PASSWORDS)
-      continue;
-    if (encrypted_types.count(type) > 0 &&
-        !entry_specifics.has_encrypted()) {
-      // This datatype requires encryption but this data is not encrypted.
+    if (EntryNeedsEncryption(encrypted_types, entry))
       return false;
-    }
   }
   return true;
 }
 
+bool EntryNeedsEncryption(const ModelTypeSet& encrypted_types,
+                          const Entry& entry) {
+  if (!entry.Get(UNIQUE_SERVER_TAG).empty())
+    return false;  // We don't encrypt unique server nodes.
+  return SpecificsNeedsEncryption(encrypted_types, entry.Get(SPECIFICS));
+}
+
+bool SpecificsNeedsEncryption(const ModelTypeSet& encrypted_types,
+                              const sync_pb::EntitySpecifics& specifics) {
+  ModelType type = GetModelTypeFromSpecifics(specifics);
+  if (type == PASSWORDS || type == NIGORI)
+    return false;  // These types have their own encryption schemes.
+  if (encrypted_types.count(type) == 0)
+    return false;  // This type does not require encryption
+  return !specifics.has_encrypted();
+}
+
 // Mainly for testing.
 bool VerifyDataTypeEncryption(BaseTransaction* const trans,
                               browser_sync::Cryptographer* cryptographer,

chrome/browser/sync/engine/nigori_util.h

@@ -15,11 +15,16 @@ namespace browser_sync {
 class Cryptographer;
 }
 
+namespace sync_pb {
+class EntitySpecifics;
+}
+
 namespace syncable {
 
 const char kEncryptedString[] = "encrypted";
 
 class BaseTransaction;
+class Entry;
 class ReadTransaction;
 class WriteTransaction;
 
@@ -43,6 +48,16 @@ bool ProcessUnsyncedChangesForEncryption(
     WriteTransaction* const trans,
     browser_sync::Cryptographer* cryptographer);
 
+// Returns true if the entry requires encryption but is not encrypted, false
+// otherwise. Note: this does not check that already encrypted entries are
+// encrypted with the proper key.
+bool EntryNeedsEncryption(const ModelTypeSet& encrypted_types,
+                          const Entry& entry);
+
+// Same as EntryNeedsEncryption, but looks at specifics.
+bool SpecificsNeedsEncryption(const ModelTypeSet& encrypted_types,
+                              const sync_pb::EntitySpecifics& specifics);
+
 // Verifies all data of type |type| is encrypted appropriately.
 bool VerifyDataTypeEncryption(BaseTransaction* const trans,
                               browser_sync::Cryptographer* cryptographer,

chrome/browser/sync/engine/nigori_util_unittest.cc

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/sync/protocol/bookmark_specifics.pb.h"
 #include "chrome/browser/sync/util/cryptographer.h"
 #include "chrome/browser/sync/engine/nigori_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -37,6 +38,36 @@ TEST_F(NigoriUtilTest, NigoriEncryptionTypes) {
   EXPECT_EQ(encrypted_types, test_types);
 }
 
+TEST(NigoriUtilTest, SpecificsNeedsEncryption) {
+  ModelTypeSet encrypted_types;
+  encrypted_types.insert(BOOKMARKS);
+  encrypted_types.insert(PASSWORDS);
+
+  sync_pb::EntitySpecifics specifics;
+  EXPECT_FALSE(SpecificsNeedsEncryption(ModelTypeSet(), specifics));
+  EXPECT_FALSE(SpecificsNeedsEncryption(encrypted_types, specifics));
+
+  AddDefaultExtensionValue(PREFERENCES, &specifics);
+  EXPECT_FALSE(SpecificsNeedsEncryption(encrypted_types, specifics));
+
+  sync_pb::EntitySpecifics bookmark_specifics;
+  AddDefaultExtensionValue(BOOKMARKS, &bookmark_specifics);
+  EXPECT_TRUE(SpecificsNeedsEncryption(encrypted_types, bookmark_specifics));
+
+  bookmark_specifics.MutableExtension(sync_pb::bookmark)->set_title("title");
+  bookmark_specifics.MutableExtension(sync_pb::bookmark)->set_url("url");
+  EXPECT_TRUE(SpecificsNeedsEncryption(encrypted_types, bookmark_specifics));
+  EXPECT_FALSE(SpecificsNeedsEncryption(ModelTypeSet(), bookmark_specifics));
+
+  bookmark_specifics.mutable_encrypted();
+  EXPECT_FALSE(SpecificsNeedsEncryption(encrypted_types, bookmark_specifics));
+  EXPECT_FALSE(SpecificsNeedsEncryption(ModelTypeSet(), bookmark_specifics));
+
+  sync_pb::EntitySpecifics password_specifics;
+  AddDefaultExtensionValue(PASSWORDS, &password_specifics);
+  EXPECT_FALSE(SpecificsNeedsEncryption(encrypted_types, password_specifics));
+}
+
 // ProcessUnsyncedChangesForEncryption and other methods that rely on the syncer
 // are tested in apply_updates_command_unittest.cc
 

chrome/browser/sync/engine/syncer_unittest.cc

@@ -543,6 +543,55 @@ TEST_F(SyncerTest, GetCommitIdsCommandTruncates) {
   DoTruncationTest(dir, unsynced_handle_view, expected_order);
 }
 
+TEST_F(SyncerTest, GetCommitIdsFilterEntriesNeedingEncryption) {
+  ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());
+  ASSERT_TRUE(dir.good());
+  int64 handle_x = CreateUnsyncedDirectory("X", ids_.MakeLocal("x"));
+  int64 handle_b = CreateUnsyncedDirectory("B", ids_.MakeLocal("b"));
+  int64 handle_c = CreateUnsyncedDirectory("C", ids_.MakeLocal("c"));
+  int64 handle_e = CreateUnsyncedDirectory("E", ids_.MakeLocal("e"));
+  int64 handle_f = CreateUnsyncedDirectory("F", ids_.MakeLocal("f"));
+  sync_pb::EncryptedData encrypted;
+  sync_pb::EntitySpecifics encrypted_bookmark;
+  encrypted_bookmark.mutable_encrypted();
+  AddDefaultExtensionValue(syncable::BOOKMARKS, &encrypted_bookmark);
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
+    MutableEntry entry_x(&wtrans, GET_BY_HANDLE, handle_x);
+    MutableEntry entry_b(&wtrans, GET_BY_HANDLE, handle_b);
+    MutableEntry entry_c(&wtrans, GET_BY_HANDLE, handle_c);
+    MutableEntry entry_e(&wtrans, GET_BY_HANDLE, handle_e);
+    MutableEntry entry_f(&wtrans, GET_BY_HANDLE, handle_f);
+    entry_x.Put(SPECIFICS, encrypted_bookmark);
+    entry_b.Put(SPECIFICS, DefaultBookmarkSpecifics());
+    entry_c.Put(SPECIFICS, DefaultBookmarkSpecifics());
+    entry_e.Put(SPECIFICS, encrypted_bookmark);
+    entry_f.Put(SPECIFICS, DefaultPreferencesSpecifics());
+  }
+
+  syncable::ModelTypeSet encrypted_types;
+  encrypted_types.insert(syncable::BOOKMARKS);
+  syncable::Directory::UnsyncedMetaHandles unsynced_handles;
+  unsynced_handles.push_back(handle_x);
+  unsynced_handles.push_back(handle_b);
+  unsynced_handles.push_back(handle_c);
+  unsynced_handles.push_back(handle_e);
+  unsynced_handles.push_back(handle_f);
+  // The unencrypted bookmarks should be removed from the list.
+  syncable::Directory::UnsyncedMetaHandles expected_handles;
+  expected_handles.push_back(handle_x);  // Was encrypted.
+  expected_handles.push_back(handle_e);  // Was encrypted.
+  expected_handles.push_back(handle_f);  // Does not require encryption.
+  {
+    WriteTransaction wtrans(FROM_HERE, UNITTEST, dir);
+    GetCommitIdsCommand::FilterEntriesNeedingEncryption(
+        encrypted_types,
+        &wtrans,
+        &unsynced_handles);
+    EXPECT_EQ(expected_handles, unsynced_handles);
+  }
+}
+
 // TODO(chron): More corner case unit tests around validation.
 TEST_F(SyncerTest, TestCommitMetahandleIterator) {
   ScopedDirLookup dir(syncdb_.manager(), syncdb_.name());

chrome/browser/sync/internal_api/sync_manager.cc

@@ -843,6 +843,7 @@ bool SyncManager::SyncInternal::UpdateCryptographerFromNigori() {
 
   allstatus_.SetCryptographerReady(cryptographer->is_ready());
   allstatus_.SetCryptoHasPendingKeys(cryptographer->has_pending_keys());
+  allstatus_.SetEncryptedTypes(cryptographer->GetEncryptedTypes());
 
   return cryptographer->is_ready();
 }
@@ -1025,9 +1026,12 @@ void SyncManager::SyncInternal::SetPassphrase(
     cryptographer->GetKeys(specifics.mutable_encrypted());
     specifics.set_using_explicit_passphrase(is_explicit);
     node.SetNigoriSpecifics(specifics);
-    ReEncryptEverything(&trans);
   }
 
+  // Does nothing if everything is already encrypted or the cryptographer has
+  // pending keys.
+  ReEncryptEverything(&trans);
+
   VLOG(1) << "Passphrase accepted, bootstrapping encryption.";
   std::string bootstrap_token;
   cryptographer->GetBootstrapToken(&bootstrap_token);
@@ -1065,7 +1069,7 @@ void SyncManager::SyncInternal::EncryptDataTypes(
 
   Cryptographer* cryptographer = trans.GetCryptographer();
 
-  if (!cryptographer->is_initialized()) {
+  if (!cryptographer->is_ready()) {
     VLOG(1) << "Attempting to encrypt datatypes when cryptographer not "
             << "initialized, prompting for passphrase.";
     ObserverList<SyncManager::Observer> temp_obs_list;
@@ -1089,22 +1093,12 @@ void SyncManager::SyncInternal::EncryptDataTypes(
                  std::inserter(newly_encrypted_types,
                                newly_encrypted_types.begin()));
   allstatus_.SetEncryptedTypes(newly_encrypted_types);
-  if (newly_encrypted_types == current_encrypted_types) {
-    // Set of encrypted types has not changed, just notify and return.
-    ObserverList<SyncManager::Observer> temp_obs_list;
-    CopyObservers(&temp_obs_list);
-    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
-                      OnEncryptionComplete(current_encrypted_types));
-    return;
-  }
   syncable::FillNigoriEncryptedTypes(newly_encrypted_types, &nigori);
   node.SetNigoriSpecifics(nigori);
-
   cryptographer->SetEncryptedTypes(nigori);
 
-  // TODO(zea): only reencrypt this datatype? ReEncrypting everything is a
-  // safer approach, and should not impact anything that is already encrypted
-  // (redundant changes are ignored).
+  // We reencrypt everything regardless of whether the set of encrypted
+  // types changed to ensure that any stray unencrypted entries are overwritten.
   ReEncryptEverything(&trans);
   return;
 }
@@ -1112,8 +1106,10 @@ void SyncManager::SyncInternal::EncryptDataTypes(
 // TODO(zea): Add unit tests that ensure no sync changes are made when not
 // needed.
 void SyncManager::SyncInternal::ReEncryptEverything(WriteTransaction* trans) {
-  syncable::ModelTypeSet encrypted_types =
-      GetEncryptedTypes(trans);
+  Cryptographer* cryptographer = trans->GetCryptographer();
+  if (!cryptographer || !cryptographer->is_ready())
+    return;
+  syncable::ModelTypeSet encrypted_types = GetEncryptedTypes(trans);
   ModelSafeRoutingInfo routes;
   registrar_->GetModelSafeRoutingInfo(&routes);
   std::string tag;

chrome/browser/sync/internal_api/write_node.cc

@@ -51,13 +51,10 @@ bool WriteNode::UpdateEntryWithEncryption(
   syncable::ModelType type = syncable::GetModelTypeFromSpecifics(new_specifics);
   DCHECK_GE(type, syncable::FIRST_REAL_MODEL_TYPE);
   syncable::ModelTypeSet encrypted_types = cryptographer->GetEncryptedTypes();
-
   sync_pb::EntitySpecifics generated_specifics;
-  if (type == syncable::PASSWORDS ||        // Has own encryption scheme.
-      type == syncable::NIGORI ||           // Encrypted separately.
-      encrypted_types.count(type) == 0 ||
-      new_specifics.has_encrypted()) {
-    // No encryption required.
+  if (!SpecificsNeedsEncryption(encrypted_types, new_specifics) ||
+      !cryptographer->is_initialized()) {
+    // No encryption required or we are unable to encrypt.
     generated_specifics.CopyFrom(new_specifics);
   } else {
     // Encrypt new_specifics into generated_specifics.
@@ -70,8 +67,6 @@ bool WriteNode::UpdateEntryWithEncryption(
               << " with content: "
               << info;
     }
-    if (!cryptographer->is_initialized())
-      return false;
     syncable::AddDefaultExtensionValue(type, &generated_specifics);
     if (!cryptographer->Encrypt(new_specifics,
                                 generated_specifics.mutable_encrypted())) {