[Sync] Initial support for encrypting any datatype (no UI hookup yet).

[Sync] Initial support for encrypting any datatype (no UI hookup yet).

BUG=59242
TEST=unit,sync_unit,sync_integration

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

[Nicolas Zea, 2011-02-09 01:26:32]

+rsimha for sync integration test modifications
+tim for sync internals

I've todo'd a number of unit tests and sync integration tests that are still
needed. But, at this point, all the functionality should be there and working,
so I'm not expecting the need to perform any significant refactors.

Note that this will have no effect on the UI at the moment and passwords aren't
affected, so it should be completely compatible with how things stand now.

[tim (not reviewing), 2011-02-09 23:54:10]

Just started looking at this puppy!

Adding Albert to take a quick peek, if he has a chance.

[tim (not reviewing), 2011-02-11 06:52:31]

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:394: DCHECK(type !=
syncable::UNSPECIFIED);
DCHECK_NE

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_change_processor.cc:190:
profile_->GetProfileSyncService()->cros_user() != "test user") {
hmm... there's got to be a better way...

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:1034: ModelTypeSet encrypted_datatypes;
Why do we store this in 3 places again?  The fact that we have to keep 3
replicas in sync freaks me out :(  Can we eliminate at least one of them?
Reading the nigori node is pretty quick...

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:1073: void SetEncryptedDataTypes(const
ModelTypeSet& encrypted_types);
Why have get/set operations on the transaction when you can get the directory,
which is where these forward to?

[Raghu Simha, 2011-02-14 05:29:52]

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:292: // Sync has finished
encrypting the datatypes. There is nothing to do.
This seems wrong. Just because you're in RunStateChangeMachine() in the
WAITING_FOR_ENCRYPTION state doesn't necessarily mean that encryption is
complete. I'd imagine this state would be implemented similar to
WAITING_FOR_INITIAL_SYNC, where you check if a condition is satisfied before
calling SignalStateCompleteWithNextState. In this case, I'd imagine the
condition to be IsTypeEncrypted(), although you'd need to devise a way of
knowing which type you're waiting on.

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:628: }
The above block seems to be doing the same as IsTypeEncrypted(). Why not replace
it with a call to the method?

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/nigori_util.cc:38: if(nigori.encrypt_bookmarks())
This file has a bunch of lint errors:
"Missing space before ( in if("

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/multi...
File chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/multi...
chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc:52:
ASSERT_TRUE(AwaitQuiescence());
In this one specific case, you may be able to get away with calling
AwaitGroupSyncCycleCompletion from Client 0, since it is the only client that
makes a local change that needs to be synced (line 46).

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_sessions_sync_test.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_sessions_sync_test.cc:81:
ASSERT_TRUE(IsEncrypted(1));
It's really nice to see a bunch of new tests where almost every line is an
ASSERT_. Makes for very readable tests, and very readable logs in case of a
failure.

[Nicolas Zea, 2011-02-14 21:18:40]

Please take another look. Should have all unit tests in now (added some
functionality to syncapi_unittest in particular). Still need to have all the
integration tests for the other datatypes have at least some encryption
functionality.

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.cc:394: DCHECK(type !=
syncable::UNSPECIFIED);
On 2011/02/11 06:52:31, timsteele wrote:
> DCHECK_NE

Done.

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_change_processor.cc (right):

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_change_processor.cc:190:
profile_->GetProfileSyncService()->cros_user() != "test user") {
On 2011/02/11 06:52:31, timsteele wrote:
> hmm... there's got to be a better way...

Done.

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:1034: ModelTypeSet encrypted_datatypes;
On 2011/02/11 06:52:31, timsteele wrote:
> Why do we store this in 3 places again?  The fact that we have to keep 3
> replicas in sync freaks me out :(  Can we eliminate at least one of them?
> Reading the nigori node is pretty quick...

Done.

http://codereview.chromium.org/6465005/diff/2001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/syncable.h:1073: void SetEncryptedDataTypes(const
ModelTypeSet& encrypted_types);
On 2011/02/11 06:52:31, timsteele wrote:
> Why have get/set operations on the transaction when you can get the directory,
> which is where these forward to?

Done.

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:292: // Sync has finished
encrypting the datatypes. There is nothing to do.
On 2011/02/14 05:29:52, rsimha wrote:
> This seems wrong. Just because you're in RunStateChangeMachine() in the
> WAITING_FOR_ENCRYPTION state doesn't necessarily mean that encryption is
> complete. I'd imagine this state would be implemented similar to
> WAITING_FOR_INITIAL_SYNC, where you check if a condition is satisfied before
> calling SignalStateCompleteWithNextState. In this case, I'd imagine the
> condition to be IsTypeEncrypted(), although you'd need to devise a way of
> knowing which type you're waiting on.

Done.

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.cc:628: }
On 2011/02/14 05:29:52, rsimha wrote:
> The above block seems to be doing the same as IsTypeEncrypted(). Why not
replace
> it with a call to the method?

Done.

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/nigori_util.cc:38: if(nigori.encrypt_bookmarks())
On 2011/02/14 05:29:52, rsimha wrote:
> This file has a bunch of lint errors:
> "Missing space before ( in if("

Done.

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/multi...
File chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/multi...
chrome/test/live_sync/multiple_client_live_sessions_sync_test.cc:52:
ASSERT_TRUE(AwaitQuiescence());
On 2011/02/14 05:29:52, rsimha wrote:
> In this one specific case, you may be able to get away with calling
> AwaitGroupSyncCycleCompletion from Client 0, since it is the only client that
> makes a local change that needs to be synced (line 46).

Done.

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/two_c...
File chrome/test/live_sync/two_client_live_sessions_sync_test.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/test/live_sync/two_c...
chrome/test/live_sync/two_client_live_sessions_sync_test.cc:81:
ASSERT_TRUE(IsEncrypted(1));
On 2011/02/14 05:29:52, rsimha wrote:
> It's really nice to see a bunch of new tests where almost every line is an
> ASSERT_. Makes for very readable tests, and very readable logs in case of a
> failure.

Done.

[Raghu Simha, 2011-02-14 22:56:51]

Test changes LGTM pending green trybot runs.

http://codereview.chromium.org/6465005/diff/11004/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/6465005/diff/11004/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:195: syncable::ModelType
waiting_for_encryption_type_;
It looks like you're always setting this member before reading its value, but
I'd still recommend initializing it in the constructor.

[tim (not reviewing), 2011-02-15 00:10:56]

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:1907: <<
syncable::ModelTypeToString(*iter);
Does SyncBackendHost enforce that registrar can't change while re-encryption is
in process?  'Cause it seems like this call to Encrypt could be referring to an
older version of routing info.  Not sure if that breaks anything but maybe we
don't want the NOTREACHED()?

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/nigori_util.cc:16: ModelTypeSet
GetEncryptedDataTypesFromSyncer(BaseTransaction* const trans) {
nit - ...FromDirectory or just GetEncryptedDataTypes? Just since there's no
syncer involved...

[Nicolas Zea, 2011-02-15 19:52:18]

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:1907: <<
syncable::ModelTypeToString(*iter);
Good point. It's probably better to just remove any types that we don't have
routing info for (with a warning), and continue. Done.

On 2011/02/15 00:10:57, timsteele wrote:
> Does SyncBackendHost enforce that registrar can't change while re-encryption
is
> in process?  'Cause it seems like this call to Encrypt could be referring to
an
> older version of routing info.  Not sure if that breaks anything but maybe we
> don't want the NOTREACHED()?

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/nigori_util.cc (right):

http://codereview.chromium.org/6465005/diff/20001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/nigori_util.cc:16: ModelTypeSet
GetEncryptedDataTypesFromSyncer(BaseTransaction* const trans) {
On 2011/02/15 00:10:57, timsteele wrote:
> nit - ...FromDirectory or just GetEncryptedDataTypes? Just since there's no
> syncer involved...

Done.

http://codereview.chromium.org/6465005/diff/11004/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_harness.h (right):

http://codereview.chromium.org/6465005/diff/11004/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_harness.h:195: syncable::ModelType
waiting_for_encryption_type_;
On 2011/02/14 22:56:51, rsimha wrote:
> It looks like you're always setting this member before reading its value, but
> I'd still recommend initializing it in the constructor.

Done.

[tim (not reviewing), 2011-02-15 19:58:04]

Couple nits and I think this LGTM!
Nice work.

Make sure we update the server protos ASAP after this lands. (raz or albert can
help if you don't have a checkout).

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/nigori_specifics.proto:41: optional bool
encrypt_passwords = 5;
I don't think we want this bool?  You can't have passwords without encryption
right...

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/sync.proto:31: optional EncryptedData encrypted =
1;
Add a comment here describing how this field works.

[Nicolas Zea, 2011-02-15 23:30:51]

Done! Committing once trybots go green.

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/nigori_specifics.proto:41: optional bool
encrypt_passwords = 5;
On 2011/02/15 19:58:04, timsteele wrote:
> I don't think we want this bool?  You can't have passwords without encryption
> right...

Done.

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/6465005/diff/36002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/sync.proto:31: optional EncryptedData encrypted =
1;
On 2011/02/15 19:58:04, timsteele wrote:
> Add a comment here describing how this field works.

Done.