[Sync] Add keystore migration conflict support.

sync/engine/apply_control_data_updates.cc

Index: sync/engine/apply_control_data_updates.cc
diff --git a/sync/engine/apply_control_data_updates.cc b/sync/engine/apply_control_data_updates.cc
index 7a52e8c36c58f5f6ec522e06aad5396a8b794b84..7cb9cb18b8c53f6709f529338d1bbf4bfb179fb0 100644
--- a/sync/engine/apply_control_data_updates.cc
+++ b/sync/engine/apply_control_data_updates.cc
@@ -57,6 +57,8 @@ bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
     return true;
   }
 
+  // We apply the nigori update regardless of whether there's a conflict or
+  // not in order to preserve any new encrypted types or encryption keys.
   const sync_pb::NigoriSpecifics& nigori =
       nigori_node.Get(SERVER_SPECIFICS).nigori();
   trans->directory()->GetNigoriHandler()->ApplyNigoriUpdate(nigori, trans);
@@ -83,45 +85,73 @@ bool ApplyNigoriUpdates(syncable::WriteTransaction* trans,
   if (!nigori_node.Get(IS_UNSYNCED)) {  // Update only.
     UpdateLocalDataFromServerData(trans, &nigori_node);
   } else {  // Conflict.
-    // Create a new set of specifics based on the server specifics (which
-    // preserves their encryption keys).
-    sync_pb::EntitySpecifics specifics = nigori_node.Get(SERVER_SPECIFICS);
-    sync_pb::NigoriSpecifics* server_nigori = specifics.mutable_nigori();
-    // Store the merged set of encrypted types.
-    // (NigoriHandler::ApplyNigoriUpdate(..) will have merged the local types
-    // already).
+    const sync_pb::EntitySpecifics& server_specifics =
+        nigori_node.Get(SERVER_SPECIFICS);
+    const sync_pb::NigoriSpecifics& server_nigori = server_specifics.nigori();
+    const sync_pb::EntitySpecifics& local_specifics =
+        nigori_node.Get(SPECIFICS);
+    const sync_pb::NigoriSpecifics& local_nigori = local_specifics.nigori();
+
+    // We initialize the new nigori with the server state, and will override
+    // it as necessary below.
+    sync_pb::EntitySpecifics new_specifics = nigori_node.Get(SERVER_SPECIFICS);
+    sync_pb::NigoriSpecifics* new_nigori = new_specifics.mutable_nigori();
+
+    // Always update to the safest set of encrypted types.
     trans->directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes(
-        server_nigori,
+        new_nigori,
         trans);
-    // The cryptographer has the both the local and remote encryption keys
-    // (added at NigoriHandler::ApplyNigoriUpdate(..) time).
-    // If the cryptographer is ready, then it already merged both sets of keys
-    // and we can store them back in. In that case, the remote key was already
-    // part of the local keybag, so we preserve the local key as the default
-    // (including whether it's an explicit key).
-    // If the cryptographer is not ready, then the user will have to provide
-    // the passphrase to decrypt the pending keys. When they do so, the
-    // SetDecryptionPassphrase code will act based on whether the server
-    // update has an explicit passphrase or not.
-    // - If the server had an explicit passphrase, that explicit passphrase
-    //   will be preserved as the default encryption key.
-    // - If the server did not have an explicit passphrase, we assume the
-    //   local passphrase is the most up to date and preserve the local
-    //   default encryption key marked as an implicit passphrase.
-    // This works fine except for the case where we had locally set an
-    // explicit passphrase. In that case the nigori node will have the default
-    // key based on the local explicit passphassphrase, but will not have it
-    // marked as explicit. To fix this we'd have to track whether we have a
-    // explicit passphrase or not separate from the nigori, which would
-    // introduce even more complexity, so we leave it up to the user to reset
-    // that passphrase as an explicit one via settings. The goal here is to
-    // ensure both sets of encryption keys are preserved.
+
+    // If the cryptographer is not ready, another client set a new encryption
+    // passphrase. If we had migrated locally, we will re-migrate when the
+    // pending keys are provided. If we had set a new custom passphrase locally
+    // the user will have another chance to set a custom passphrase later
+    // (assuming they hadn't set a custom passphrase on the other client).
+    // Therefore, we only attempt to merge the nigori nodes if the cryptographer
+    // is ready.
+    // Note: we only update the encryption keybag if we're sure that we aren't
+    // invalidating the keystore_decryptor_token (i.e. we're either
+    // not migrated or we copying over all local state).
     if (cryptographer->is_ready()) {
-      cryptographer->GetKeys(server_nigori->mutable_encryption_keybag());
-      server_nigori->set_keybag_is_frozen(
-          nigori_node.Get(SPECIFICS).nigori().keybag_is_frozen());
+      if (local_nigori.has_passphrase_type() &&
+          server_nigori.has_passphrase_type()) {
+        // They're both migrated, preserve the local passphrase type if it
+        // is more conservative. We don't have to worry about copying over the
+        // keystore decryptor since this only happens if local has an explicit

[rlarocque, 2012/09/13 22:10:51]

DCHECK that assumption?

[Nicolas Zea, 2012/09/13 22:50:14]

Done.

+        // passphrase type.
+        if (server_nigori.passphrase_type() ==
+                sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE &&
+            local_nigori.passphrase_type() !=
+                sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE) {
+          new_nigori->set_passphrase_type(local_nigori.passphrase_type());
+          new_nigori->clear_keystore_decryptor_token();
+          cryptographer->GetKeys(new_nigori->mutable_encryption_keybag());

[rlarocque, 2012/09/13 22:10:51]

It seems you do this almost everywhere.  Why not make it contingent solely on
cryptographer->is_ready()?  Are there cases where you want to avoid calling
GetKeys() on the new (from server) keybag?

[Nicolas Zea, 2012/09/13 22:50:14]

Yes, see the "Note: we only update the encryption keybag..." comment above. In
particular, we don't want to call this if we're not sure that we are properly
updating keystore_decryptor as well (or that it doesn't matter).

+        }
+      } else if (!local_nigori.has_passphrase_type() &&
+                 !server_nigori.has_passphrase_type()) {
+        // Set the explicit passphrase based on the local state. If the server
+        // had set an explict passphrase, we should have pending keys, so
+        // should not reach this code.
+        // Because neither side is migrated, we don't have to worry about the
+        // keystore decryptor token.
+        new_nigori->set_keybag_is_frozen(local_nigori.keybag_is_frozen());
+        cryptographer->GetKeys(new_nigori->mutable_encryption_keybag());
+      } else {
+        // One side is migrated, the other is not. Because we don't have
+        // pending keys, we can just preserve whichever side is migrated.
+        if (local_nigori.has_passphrase_type()) {

[rlarocque, 2012/09/13 22:10:51]

Let's make the division of cases more explicit.  It seems to me that there are
four main cases here:
- both are aware of passphrase_type
- neither are aware of passphrase_type
- local only is aware of passphrase type
- server only is aware of passphrase type

The last two are currently handled inside this else branch. Can they be moved
out to the higher level of else if branches?

[Nicolas Zea, 2012/09/13 22:50:14]

Done.

+          new_nigori->set_keybag_is_frozen(true);
+          new_nigori->set_passphrase_type(local_nigori.passphrase_type());
+          new_nigori->set_keystore_migration_time(
+              local_nigori.keystore_migration_time());
+          new_nigori->mutable_keystore_decryptor_token()->CopyFrom(
+              local_nigori.keystore_decryptor_token());
+          cryptographer->GetKeys(new_nigori->mutable_encryption_keybag());
+        }  // else leave the new nigori with the server state.
+      }
     }
-    nigori_node.Put(SPECIFICS, specifics);
+
+    nigori_node.Put(SPECIFICS, new_specifics);

[rlarocque, 2012/09/13 22:10:51]

SetNigoriSpecifics(), which seems to be used for nigori writing most of the
time, will preserve unknown fields.  This will not.  

I'm guessing this doesn't matter because the unknown fields would have to be
delivered by the server, and the new_specifics here will contain the server's
unkown fields.  Still, just thinking about this makes my head hurt.

[Nicolas Zea, 2012/09/13 22:50:14]

We don't have access to SetNigoriSpecifics here actually (it's a syncapi method,
not a syncable method). That said, I've gone ahead and copied over the entire
local nigori when necessary, which will preserve local unknown fields in those
cases we're not letting server win.

     DVLOG(1) << "Resolving simple conflict, merging nigori nodes: "
              << nigori_node;