[Sync] Add ability to save/restore Nigori to SyncEncryptionHandlerImpl

[Sync] Add ability to save/restore Nigori to SyncEncryptionHandlerImpl

SyncEncryptionHandlerImpl now detects when a user enables passphrase
encryption locally and notifies observers with the updated Nigori
state. An observer can then use this state to restore a future instance
of SyncEncryptionHandler. The ability to save/restore Nigori across
Directory instances is a prerequisite for issuing a lightweight
"dashboard stop and clear" when a user enables passphrase encryption.

Add OnLocalSetCustomPassphrase to SyncEncryptionHandler interface.

Plumb OnLocalSetCustomPassphrase events from SyncEncryptionHandlerImpl
on the sync thread to ProfileSyncService on the UI thread.

Loosen a restriction on ModelNeutralMutableEntry's capabilities.

BUG=490836
Committed: https://crrev.com/358cabb346973315059d9afbff9d9c17c4b706f2
Cr-Commit-Position: refs/heads/master@{#334513}

[maniscalco, 2015-06-10 21:58:11]

maniscalco@chromium.org changed reviewers:
+ zea@chromium.org

[maniscalco, 2015-06-10 21:58:11]

Nicolas, would you please review this change?

It's not as big as it seems. Most of it is boiler plate resulting from a
new method on SyncEncryptionHandler and includes some backend/frontend
plumbing. I plan to send out the remaining plumbing work in a future CL.

[maniscalco, 2015-06-10 22:01:45]

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:800:
syncable::ModelNeutralMutableEntry model_neutral_mutable_entry(
I tried a number of ways of creating the nigori node, but
ModelNeutralMutableEntry seemed like the best fit.  LMK if there's another
approach that's a better fit.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:1602: // Now verify
that we can restore the current state using the captured
I'm interested in your opinion on how to test this.  Currently, we have two test
cases that enable passphrase encryption.  This case and the one below
SetCustomPassAfterMigrationNoKeystoreKey, which I haven't updated, but probably
should.

I chose to test the new save/restore logic as a follow-on step of the existing
test case because it seems that will most closely match how it will be called. 
LMK if you think I should instead break it out into its own case.

[Nicolas Zea, 2015-06-11 17:43:07]

https://codereview.chromium.org/1177853002/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/1177853002/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1417:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
nit: Add a comment with a TODO here and mention the tracking bug?

https://codereview.chromium.org/1177853002/diff/1/components/sync_driver/sync...
File components/sync_driver/sync_frontend.h (right):

https://codereview.chromium.org/1177853002/diff/1/components/sync_driver/sync...
components/sync_driver/sync_frontend.h:140: // |nigori_state| is the updated
nigori state.
Maybe be a bit more specific and explain that nigori_state preserve's the new
encryption keys, and explain what the frontend is expected to do with it.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/public/sy...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/public/sy...
sync/internal_api/public/sync_encryption_handler.h:138: sync_pb::NigoriSpecifics
nigori_specifics;
Do you expect to persist any other information as part of this struct? Does it
make more sense to just pass the specifics (or even a string with the serialized
data)?

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:800:
syncable::ModelNeutralMutableEntry model_neutral_mutable_entry(
On 2015/06/10 22:01:45, maniscalco wrote:
> I tried a number of ways of creating the nigori node, but
> ModelNeutralMutableEntry seemed like the best fit.  LMK if there's another
> approach that's a better fit.

Acknowledged.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:812:
*specifics.mutable_nigori() = nigori_state.nigori_specifics;
You can just do mutable_nigori()->CopyFrom(...)

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1183:
WriteEncryptionStateToNigori(trans);
Is this necessary? Doesn't FinishSetPassphrase already update the nigori node?

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1188:
nigori_state.nigori_specifics = nigori_node.GetNigoriSpecifics();
maybe DCHECK that the nigori reflects a custom passphrase here

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:1602: // Now verify
that we can restore the current state using the captured
On 2015/06/10 22:01:45, maniscalco wrote:
> I'm interested in your opinion on how to test this.  Currently, we have two
test
> cases that enable passphrase encryption.  This case and the one below
> SetCustomPassAfterMigrationNoKeystoreKey, which I haven't updated, but
probably
> should.
> 
> I chose to test the new save/restore logic as a follow-on step of the existing
> test case because it seems that will most closely match how it will be called.

> LMK if you think I should instead break it out into its own case.

I'm okay with this. One question I have though is what is the expected behavior
for a migration to custom passphrase/frozen implicit passphrase? It might be
good to test that as well.

[maniscalco, 2015-06-15 16:49:18]

Thanks for the feedback.  Good point about including frozen implicit passphrase
users.

RFAL!  I've added a question to you about renaming OnLocalSetCustomPassphrase.

https://codereview.chromium.org/1177853002/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/1177853002/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1417:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> nit: Add a comment with a TODO here and mention the tracking bug?

Done.

https://codereview.chromium.org/1177853002/diff/1/components/sync_driver/sync...
File components/sync_driver/sync_frontend.h (right):

https://codereview.chromium.org/1177853002/diff/1/components/sync_driver/sync...
components/sync_driver/sync_frontend.h:140: // |nigori_state| is the updated
nigori state.
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> Maybe be a bit more specific and explain that nigori_state preserve's the new
> encryption keys, and explain what the frontend is expected to do with it.

Done.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/public/sy...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/public/sy...
sync/internal_api/public/sync_encryption_handler.h:138: sync_pb::NigoriSpecifics
nigori_specifics;
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> Do you expect to persist any other information as part of this struct? Does it
> make more sense to just pass the specifics (or even a string with the
serialized
> data)?

Currently, I think the NigoriSpecifics is all I will need.  In a previous
iteration I was passing around an EntryKernel as well (but realized I didn't
need it).  It was handy having the struct because this interface is implemented
in a bunch of places so changing the signature is a pain (gotta update a lot of
code to to change the parameter type or add a second param).

IMO, having a dedicated type here seems like the right thing because it will
allow us to hide some of the details from users of SyncEncryptionHandler.  WDYT?

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:812:
*specifics.mutable_nigori() = nigori_state.nigori_specifics;
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> You can just do mutable_nigori()->CopyFrom(...)

Done.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1183:
WriteEncryptionStateToNigori(trans);
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> Is this necessary? Doesn't FinishSetPassphrase already update the nigori node?

You're right.  FinishSetPassphrase calls SetNigoriSpecificis so this is
unnecessary.  Removed.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1188:
nigori_state.nigori_specifics = nigori_node.GetNigoriSpecifics();
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> maybe DCHECK that the nigori reflects a custom passphrase here

Good idea.  Done.

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

https://codereview.chromium.org/1177853002/diff/1/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:1602: // Now verify
that we can restore the current state using the captured
On 2015/06/11 17:43:07, Nicolas Zea wrote:
> On 2015/06/10 22:01:45, maniscalco wrote:
> > I'm interested in your opinion on how to test this.  Currently, we have two
> test
> > cases that enable passphrase encryption.  This case and the one below
> > SetCustomPassAfterMigrationNoKeystoreKey, which I haven't updated, but
> probably
> > should.
> > 
> > I chose to test the new save/restore logic as a follow-on step of the
existing
> > test case because it seems that will most closely match how it will be
called.
> 
> > LMK if you think I should instead break it out into its own case.
> 
> I'm okay with this. One question I have though is what is the expected
behavior
> for a migration to custom passphrase/frozen implicit passphrase? It might be
> good to test that as well.

I've moved the verification to a new helper method (
VerifyRestoreAfterCustomPassphrase) and now call that method from this case and
SetCustomPassAfterMigrationNoKeystoreKey.

(after offline discussion) Agreed that migration to custom passphrase / frozen
implicit passphrase should invoke the save/restore functionality.  

OK, I've updated SEHI to notify in AttemptToMigrateNigoriToKeystore because it
will cover both the custom and implicit frozen cases.  I've also updated the
migration related tests that end up in these two states.

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/publi...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/publi...
sync/internal_api/public/sync_encryption_handler.h:130: virtual void
OnLocalSetCustomPassphrase(
I'm going to rename this method because its name is no longer accurate.  WDYT
about:

OnLocalSetPassphraseComplete
OnLocalSetPassphrase
OnLocalSetPassphraseEncryption

?

I want the name to capture that we have either a frozen implicit passphrase or
custom passphrase and that the transition was initiated from this device (not a
remote device, not the server).

[Nicolas Zea, 2015-06-15 19:16:02]

Mostly LG

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/publi...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/publi...
sync/internal_api/public/sync_encryption_handler.h:130: virtual void
OnLocalSetCustomPassphrase(
On 2015/06/15 16:49:18, maniscalco wrote:
> I'm going to rename this method because its name is no longer accurate.  WDYT
> about:
> 
> OnLocalSetPassphraseComplete
> OnLocalSetPassphrase
> OnLocalSetPassphraseEncryption
> 
> ?
> 
> I want the name to capture that we have either a frozen implicit passphrase or
> custom passphrase and that the transition was initiated from this device (not
a
> remote device, not the server).

OnLocalSetPassphraseEncryption SGTM

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/sync_...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:794: // See that we don't
already have a nigori node.
nit: See -> Verify

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:817:
WriteEncryptionStateToNigori(&trans);
Is WriteEncryptionStateToNigori expected to change anything?

[maniscalco, 2015-06-15 20:07:26]

Rename done.  PTAL!

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/publi...
File sync/internal_api/public/sync_encryption_handler.h (right):

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/publi...
sync/internal_api/public/sync_encryption_handler.h:130: virtual void
OnLocalSetCustomPassphrase(
On 2015/06/15 19:16:02, Nicolas Zea wrote:
> On 2015/06/15 16:49:18, maniscalco wrote:
> > I'm going to rename this method because its name is no longer accurate. 
WDYT
> > about:
> > 
> > OnLocalSetPassphraseComplete
> > OnLocalSetPassphrase
> > OnLocalSetPassphraseEncryption
> > 
> > ?
> > 
> > I want the name to capture that we have either a frozen implicit passphrase
or
> > custom passphrase and that the transition was initiated from this device
(not
> a
> > remote device, not the server).
> 
> OnLocalSetPassphraseEncryption SGTM

Done.

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/sync_...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:794: // See that we don't
already have a nigori node.
On 2015/06/15 19:16:02, Nicolas Zea wrote:
> nit: See -> Verify

Done.

https://codereview.chromium.org/1177853002/diff/50001/sync/internal_api/sync_...
sync/internal_api/sync_encryption_handler_impl.cc:817:
WriteEncryptionStateToNigori(&trans);
On 2015/06/15 19:16:02, Nicolas Zea wrote:
> Is WriteEncryptionStateToNigori expected to change anything?

It's not.  I probably shouldn't add that call.  Removed.

[Nicolas Zea, 2015-06-15 23:30:56]

LGTM!

[maniscalco, 2015-06-16 00:27:06]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2015-06-16 00:28:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177853002/90001

[commit-bot: I haz the power, 2015-06-16 00:37:13]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-06-16 00:38:19]

Patchset 6 (id:??) landed as
https://crrev.com/358cabb346973315059d9afbff9d9c17c4b706f2
Cr-Commit-Position: refs/heads/master@{#334513}