[Sync] Add nigori node conflict resolution.

chrome/browser/sync/engine/syncer_util.cc

Index: chrome/browser/sync/engine/syncer_util.cc
diff --git a/chrome/browser/sync/engine/syncer_util.cc b/chrome/browser/sync/engine/syncer_util.cc
index 1be2e0c33e5ef9e56f511617a8dd6e0fac880e1a..d1a923a774eff2c0de4355dec8dfe9e84ae64bd5 100644
--- a/chrome/browser/sync/engine/syncer_util.cc
+++ b/chrome/browser/sync/engine/syncer_util.cc
@@ -253,6 +253,51 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
   if (!entry->Get(IS_UNAPPLIED_UPDATE))
     return SUCCESS;  // No work to do.
   syncable::Id id = entry->Get(ID);
+  const sync_pb::EntitySpecifics& specifics = entry->Get(SERVER_SPECIFICS);
+
+  // We intercept updates to the Nigori node, update the Cryptographer and
+  // encrypt any unsynced changes here because there is no Nigori
+  // ChangeProcessor. We never put the nigori node in a state of
+  // conflict_encryption

[tim (not reviewing), 2011/12/20 17:54:05]

period.

[Nicolas Zea, 2011/12/20 19:54:10]

Done.

+  //
+  // We always update the cryptographer with the server's nigori node,
+  // even if we have a locally modified nigori node (we manually merge nigori
+  // data in the conflict resolver in that case). This handles the case where
+  // two clients both set a different passphrase. The second client to attempt
+  // to commit will go into a state of having pending keys, merge the set of
+  // encrypted types, and eventually re-encrypt everything with the passphrase

[tim (not reviewing), 2011/12/20 17:54:05]

We should probably say union instead of merged when talking about encrypted
types.

[Nicolas Zea, 2011/12/20 19:54:10]

Done.

+  // of the first client and commit the set of merged encryption keys. Until the
+  // second client provides the pending passphrase, the cryptographer will
+  // preserve the encryption keys based on the local passphrase, while the
+  // nigori node will preserve the server encryption keys.
+  //
+  // If non-encryption changes are made to the nigori node, they will be
+  // lost as part of conflict resolution. This is intended, as we place a higher
+  // priority on preserving the server's passphrase change to preserving local
+  // non-encryption changes. Next time the non-encryption changes are made to
+  // the nigori node (e.g. on restart), they will commit without issue.
+  if (specifics.HasExtension(sync_pb::nigori)) {
+    const sync_pb::NigoriSpecifics& nigori =
+        specifics.GetExtension(sync_pb::nigori);
+    cryptographer->Update(nigori);
+
+    // Make sure any unsynced changes are properly encrypted as necessary.
+    // We only perform this is the cryptographer is ready. If not, these are

[tim (not reviewing), 2011/12/20 17:54:05]

s/is/if

[Nicolas Zea, 2011/12/20 19:54:10]

Done.

+    // re-encrypted at SetPassphrase time (via ReEncryptEverything).
+    if (cryptographer->is_ready()) {
+      // Note that we don't bother to re-encrypt any data for which IS_UNSYNCED
+      // == false. The machine that turned on encryption should re-encrypt
+      // everything itself. It's possible it could get interrupted during this

[tim (not reviewing), 2011/12/20 17:54:05]

Is this the first official departure from the original design goal of having any
client perform re-encryption if necessary?

[Nicolas Zea, 2011/12/20 19:54:10]

The logic to re-encrypt on restart has been there since I started working on
encryption. This logic to re-encrypt unsynced changes we're about to commit was
added when the CONFLICT_ENCRYPTION state was added some time ago.

Perhaps the "re-encrypt" here is misleading. This covers the case where we have
newly encrypted types with the same passphrase, and this client has new items of
that type which have not been committed. If we didn't perform this re-encryption
here, we would not commit those items until restart (due to the logic in
GetCommitIds that filters out items needing encryption).

I've updated the comment to try to make this clearer.

+      // process, but we currently reencrypt everything at startup as well,
+      // so as soon as a client is restarted with this datatype encrypted, all
+      // the data should be updated as necessary.
+
+      // If this fails, something is wrong with the cryptographer, but there's
+      // nothing we can do about it here.
+      syncable::ProcessUnsyncedChangesForEncryption(trans,
+                                                    cryptographer);
+    }
+  }
 
   if (entry->Get(IS_UNSYNCED)) {
     DVLOG(1) << "Skipping update, returning conflict for: " << id
@@ -289,44 +334,6 @@ UpdateAttemptResponse SyncerUtil::AttemptToUpdateEntry(
     }
   }
 
-  // We intercept updates to the Nigori node, update the Cryptographer and
-  // encrypt any unsynced changes here because there is no Nigori
-  // ChangeProcessor.
-  const sync_pb::EntitySpecifics& specifics = entry->Get(SERVER_SPECIFICS);
-  if (specifics.HasExtension(sync_pb::nigori)) {
-    const sync_pb::NigoriSpecifics& nigori =
-        specifics.GetExtension(sync_pb::nigori);
-    cryptographer->Update(nigori);
-
-    // Make sure any unsynced changes are properly encrypted as necessary.
-    const syncable::ModelTypeSet encrypted_types =
-        cryptographer->GetEncryptedTypes();
-    if (!VerifyUnsyncedChangesAreEncrypted(trans, encrypted_types) &&
-        (!cryptographer->is_ready() ||
-         !syncable::ProcessUnsyncedChangesForEncryption(trans,
-                                                        cryptographer))) {
-      // We were unable to encrypt the changes, possibly due to a missing
-      // passphrase. We return conflict, even though the conflict is with the
-      // unsynced change and not the nigori node. We ensure foward progress
-      // because the cryptographer already has the pending keys set, so once
-      // the new passphrase is entered we should be able to encrypt properly.
-      // And, because this update will not be applied yet, next time around
-      // we will properly encrypt all appropriate unsynced data.
-      // Note: we return CONFLICT_ENCRYPTION instead of CONFLICT. See
-      // explanation below.
-      DVLOG(1) << "Marking nigori node update as conflicting due to being "
-               << "unable to encrypt all necessary unsynced changes.";
-      return CONFLICT_ENCRYPTION;
-    }
-
-    // Note that we don't bother to encrypt any synced data that now requires
-    // encryption. The machine that turned on encryption should encrypt
-    // everything itself. It's possible it could get interrupted during this
-    // process, but we currently reencrypt everything at startup as well,
-    // so as soon as a client is restarted with this datatype encrypted, all the
-    // data should be updated as necessary.
-  }
-
   // Only apply updates that we can decrypt. If we can't decrypt the update, it
   // is likely because the passphrase has not arrived yet. Because the
   // passphrase may not arrive within this GetUpdates, we can't just return