[Sync] Ensure cryptographer ready before encrypting.

chrome/browser/sync/engine/syncapi.cc

Index: chrome/browser/sync/engine/syncapi.cc
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index 360b6947b8346a074646d9eb7374e070c9eacba7..667a991021356d09c912e3887426f91bc4f991c8 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -202,7 +202,7 @@ UserShare::~UserShare() {}
 ////////////////////////////////////
 // BaseNode member definitions.
 
-BaseNode::BaseNode() {}
+BaseNode::BaseNode() : password_data_(new sync_pb::PasswordSpecificsData) {}
 
 BaseNode::~BaseNode() {}
 
@@ -245,8 +245,10 @@ bool BaseNode::DecryptIfNecessary() {
     // Passwords have their own legacy encryption structure.
     scoped_ptr<sync_pb::PasswordSpecificsData> data(DecryptPasswordSpecifics(
         specifics, GetTransaction()->GetCryptographer()));
-    if (!data.get())
+    if (!data.get()) {
+      LOG(ERROR) << "Failed to decrypt password specifics.";
       return false;
+    }
     password_data_.swap(data);
     return true;
   }
@@ -259,9 +261,8 @@ bool BaseNode::DecryptIfNecessary() {
       specifics.encrypted();
   std::string plaintext_data = GetTransaction()->GetCryptographer()->
       DecryptToString(encrypted);
-  if (plaintext_data.length() == 0)
-    return false;
-  if (!unencrypted_data_.ParseFromString(plaintext_data)) {
+  if (plaintext_data.length() == 0 ||
+      !unencrypted_data_.ParseFromString(plaintext_data)) {
     LOG(ERROR) << "Failed to decrypt encrypted node of type " <<
       syncable::ModelTypeToString(GetModelType()) << ".";
     return false;
@@ -404,7 +405,6 @@ const sync_pb::NigoriSpecifics& BaseNode::GetNigoriSpecifics() const {
 
 const sync_pb::PasswordSpecificsData& BaseNode::GetPasswordSpecifics() const {
   DCHECK_EQ(syncable::PASSWORDS, GetModelType());
-  DCHECK(password_data_.get());
   return *password_data_;
 }
 
@@ -571,7 +571,9 @@ void WriteNode::SetPasswordSpecifics(
 
   sync_pb::PasswordSpecifics new_value;
   if (!cryptographer->Encrypt(data, new_value.mutable_encrypted())) {
-    NOTREACHED();
+    NOTREACHED() << "Failed to encrypt password, possibly due to sync node "
+                 << "corruption";
+    return;
   }
 
   sync_pb::EntitySpecifics entity_specifics;
@@ -1203,6 +1205,12 @@ class SyncManager::SyncInternal
   // Whether or not the Nigori node is encrypted using an explicit passphrase.
   bool IsUsingExplicitPassphrase();
 
+  // Update the Cryptographer from the current nigori node.
+  // Note: opens a transaction and can trigger an ON_PASSPHRASE_REQUIRED, so
+  // should only be called after syncapi is fully initialized.
+  // Returns true if cryptographer is ready, false otherwise.
+  bool UpdateCryptographerFromNigori();
+
   // Set the datatypes we want to encrypt and encrypt any nodes as necessary.
   void EncryptDataTypes(const syncable::ModelTypeSet& encrypted_types);
 
@@ -1795,50 +1803,42 @@ bool SyncManager::SyncInternal::Init(
 
 void SyncManager::SyncInternal::BootstrapEncryption(
     const std::string& restored_key_for_bootstrapping) {
+  // Cryptographer should only be accessed while holding a transaction.
+  ReadTransaction trans(GetUserShare());
+  Cryptographer* cryptographer = trans.GetCryptographer();
+
+  // Set the bootstrap token before bailing out if nigori node is not there.
+  // This could happen if server asked us to migrate nigri.
+  cryptographer->Bootstrap(restored_key_for_bootstrapping);
+}
+
+bool SyncManager::SyncInternal::UpdateCryptographerFromNigori() {
   syncable::ScopedDirLookup lookup(dir_manager(), username_for_share());
   if (!lookup.good()) {
-    VLOG(0) << "BootstrapEncryption: lookup not good so bailing out";
-    NOTREACHED();
-    return;
+    NOTREACHED() << "BootstrapEncryption: lookup not good so bailing out";
+    return false;
   }
+  if (!lookup->initial_sync_ended_for_type(syncable::NIGORI))
+    return false;  // Should only happen during first time sync.
 
-  sync_pb::NigoriSpecifics nigori;
-  syncable::ModelTypeSet encrypted_types;
-  {
-    // Cryptographer should only be accessed while holding a transaction.
-    ReadTransaction trans(GetUserShare());
-    Cryptographer* cryptographer = trans.GetCryptographer();
-
-    // Set the bootstrap token before bailing out if nigori node is not there.
-    // This could happen if server asked us to migrate nigri.
-    cryptographer->Bootstrap(restored_key_for_bootstrapping);
-
-    if (!lookup->initial_sync_ended_for_type(syncable::NIGORI))
-      return;
-
-    ReadNode node(&trans);
-    if (!node.InitByTagLookup(kNigoriTag)) {
-      NOTREACHED();
-      return;
-    }
-
-    nigori.CopyFrom(node.GetNigoriSpecifics());
-    Cryptographer::UpdateResult result = cryptographer->Update(nigori);
-    if (result == Cryptographer::NEEDS_PASSPHRASE) {
-      ObserverList<SyncManager::Observer> temp_obs_list;
-      CopyObservers(&temp_obs_list);
-      FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
-                        OnPassphraseRequired(sync_api::REASON_DECRYPTION));
-    }
+  ReadTransaction trans(GetUserShare());
+  Cryptographer* cryptographer = trans.GetCryptographer();
 
-    // Refresh list of encrypted datatypes.
-    encrypted_types = GetEncryptedTypes(&trans);
+  ReadNode node(&trans);
+  if (!node.InitByTagLookup(kNigoriTag)) {
+    NOTREACHED();
+    return false;
+  }
+  Cryptographer::UpdateResult result =
+      cryptographer->Update(node.GetNigoriSpecifics());
+  if (result == Cryptographer::NEEDS_PASSPHRASE) {
+    ObserverList<SyncManager::Observer> temp_obs_list;
+    CopyObservers(&temp_obs_list);
+    FOR_EACH_OBSERVER(SyncManager::Observer, temp_obs_list,
+                      OnPassphraseRequired(sync_api::REASON_DECRYPTION));
   }
 
-
-
-  // Ensure any datatypes that need encryption are encrypted.
-  EncryptDataTypes(encrypted_types);
+  return cryptographer->is_ready();
 }
 
 void SyncManager::SyncInternal::StartSyncingNormally() {
@@ -2085,14 +2085,19 @@ void SyncManager::SyncInternal::EncryptDataTypes(
   WriteTransaction trans(GetUserShare());
   WriteNode node(&trans);
   if (!node.InitByTagLookup(kNigoriTag)) {
-    LOG(ERROR) << "Unable to set encrypted datatypes because Nigori node not "
-               << "found.";
-    NOTREACHED();
+    NOTREACHED() << "Unable to set encrypted datatypes because Nigori node not "
+                 << "found.";
     return;
   }
 
   Cryptographer* cryptographer = trans.GetCryptographer();
 
+  if (!cryptographer->is_ready()) {
+    NOTREACHED() << "Attempting to encrypt datatypes when cryptographer not "
+                 << "ready.";
+    return;
+  }
+
   // Update the Nigori node set of encrypted datatypes so other machines notice.
   // Note, we merge the current encrypted types with those requested. Once a
   // datatypes is marked as needing encryption, it is never unmarked.
@@ -2990,12 +2995,17 @@ UserShare* SyncManager::GetUserShare() const {
   return data_->GetUserShare();
 }
 
+void SyncManager::ReloadNigori() {
+  DCHECK(data_->initialized());
+  if (data_->UpdateCryptographerFromNigori())

[tim (not reviewing), 2011/06/14 01:32:58]

is it possible for is_ready() to change from true to false after this call?

[Nicolas Zea, 2011/06/14 16:52:38]

You're right, once we release the transaction it's possible a new passphrase
will arrive. But, in EncryptDataTypes, as long as the cryptographer has been
initialized, we can still encrypt with the old passphrase. I've changed
EncryptDataTypes to just ensure the cryptographer is initialized.

+    data_->EncryptDataTypes(syncable::ModelTypeSet());
+}
+
 syncable::ModelTypeSet SyncManager::GetEncryptedDataTypes() const {
   sync_api::ReadTransaction trans(GetUserShare());
   return GetEncryptedTypes(&trans);
 }
 
-
 bool SyncManager::HasUnsyncedItems() const {
   sync_api::ReadTransaction trans(GetUserShare());
   return (trans.GetWrappedTrans()->directory()->unsynced_entity_count() != 0);