[Sync] Add nigori node conflict resolution.

[Sync] Add nigori node conflict resolution.

We merge the set of encryption keys, types, and non-encryption data in the
conflict resolver. If the server's keys are undecryptable we preserve them
until the user provides the passphrase, at which point we store back the
merged set of encryption keys. This way, once everything is resolved,
the default passphrase will be the one that was committed first, and all
clients will have all sets of encryption keys.

BUG=105615
TEST=sync_unit_tests. Manually attempt to set two different passphrases and
encrypted types on synced machines. Ensure everything works.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115248

[Nicolas Zea, 2011-12-13 01:39:54]

PTAL. This basically pulls out and makes a bit more robust the nigori node 
conflict resolution code from http://codereview.chromium.org/8770032/.

[Nicolas Zea, 2011-12-13 01:40:24]

Note that this patch is built on top of http://codereview.chromium.org/8759019

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

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/conflict_resolver.cc:132: // TODO(zea): Find a better
way of doing this. As it stands, we have to
Maybe move this comment down to the set_sync_tabs bit?

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/conflict_resolver.cc:500: Cryptographer*
cryptographer = dir.manager()->GetCryptographer(&trans);
Is manager() or the cryptographer really not available any other way?

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_unittest.cc:871: }
I don't see anything for sync_tabs here?

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:261: // conflict_encryption
period.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:268: // encrypted types, and
eventually re-encrypt everything with the passphrase
We should probably say union instead of merged when talking about encrypted
types.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:285: // We only perform this is the
cryptographer is ready. If not, these are
s/is/if

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:290: // everything itself. It's
possible it could get interrupted during this
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:09]

PTAL

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/conflict_resolver.cc (right):

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/conflict_resolver.cc:132: // TODO(zea): Find a better
way of doing this. As it stands, we have to
On 2011/12/20 17:54:05, timsteele wrote:
> Maybe move this comment down to the set_sync_tabs bit?

Done.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/conflict_resolver.cc:500: Cryptographer*
cryptographer = dir.manager()->GetCryptographer(&trans);
On 2011/12/20 17:54:05, timsteele wrote:
> Is manager() or the cryptographer really not available any other way?

Neither the status controller nor the conflict progress have access to the
manager/cryptographer. Another option would be to plumb in the manager as an
additional parameter from the ResolveConflicts command and ResolveConflict
method, in addition to the ScopedDirLookup, but this seemed simpler, since the
ScopedDirLookup wraps around the manager already.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_unittest.cc:871: }
On 2011/12/20 17:54:05, timsteele wrote:
> I don't see anything for sync_tabs here?

Done.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_util.cc (right):

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:261: // conflict_encryption
On 2011/12/20 17:54:05, timsteele wrote:
> period.

Done.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:268: // encrypted types, and
eventually re-encrypt everything with the passphrase
On 2011/12/20 17:54:05, timsteele wrote:
> We should probably say union instead of merged when talking about encrypted
> types.

Done.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:285: // We only perform this is the
cryptographer is ready. If not, these are
On 2011/12/20 17:54:05, timsteele wrote:
> s/is/if

Done.

http://codereview.chromium.org/8917031/diff/14001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_util.cc:290: // everything itself. It's
possible it could get interrupted during this
On 2011/12/20 17:54:05, timsteele wrote:
> Is this the first official departure from the original design goal of having
any
> client perform re-encryption if necessary?

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.

[Nicolas Zea, 2011-12-20 21:14:30]

Made the plumbing change mentioned, and reverted directory_manager changes. PTAL

[tim (not reviewing), 2011-12-20 21:19:02]

lgtm

[commit-bot: I haz the power, 2011-12-20 21:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/8917031/17003

[commit-bot: I haz the power, 2011-12-21 01:07:59]

Change committed as 115248