Update the Nigori node when the passphrase changes.

Update the Nigori node when the passphrase changes.

BUG=32410
TEST=unittests

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

[albertb, 2010-06-25 19:54:38]

Hi Nick,

I'm debating whether to make the SyncFrontend an UnrecoverableErrorHandler and
use it to forward failure to look up the Nigori node back to the PSS. What do
you think?

Alternatively, I guess I could create an MA and a CP for that node and trigger
UEs through those. It might help to keep most of the crypto code together
instead of sprinkled around syncapi.cc. Also, because of the new gaia passphrase
requirements, since chainging the passphrase would mean having to login again,
we'd always be running the MA when that happens.

Thanks!
Albert

[ncarter (slow), 2010-06-25 22:47:38]

LGTM with thoughts.

http://codereview.chromium.org/2858027/diff/1/2
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2858027/diff/1/2#newcode1533
chrome/browser/sync/engine/syncapi.cc:1533: // TODO(albertb): Plumb an
UnrecoverableError all the way back to the PSS.
I'd be fine with UREH being plumbed here.

http://codereview.chromium.org/2858027/diff/1/3
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/2858027/diff/1/3#newcode220
chrome/browser/sync/profile_sync_service_password_unittest.cc:220:
nigori_node.Put(SPECIFICS, nigori_specifics);
This is the first time I've noticed how in this file we test things at the
ProfileSyncService layer, but also directly create entries in the syncable DB
(sidestepping the limitations imposed by the syncapi).  It's kind of cool.  The
other test types had fakey interceptions of GetSyncIdForTaggedNode, a method
that owes its existence to this problem.  I'd support getting rid of
GetSyncIdForTaggedNode in favor of what you do here; it's more realistic.  And
this is making me what else we could stand to gain (in terms of testability,
etc) by breaking down some of the syncapi/syncable barrier.

As a side note, it's shocking how many lines of code it is to set up a single
syncable object that passes the tree invariants, don't you think?

[albertb, 2010-06-28 17:33:09]

Thanks, will submit when the trybots turn green.

http://codereview.chromium.org/2858027/diff/1/2
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/2858027/diff/1/2#newcode1533
chrome/browser/sync/engine/syncapi.cc:1533: // TODO(albertb): Plumb an
UnrecoverableError all the way back to the PSS.
On 2010/06/25 22:47:38, ncarter wrote:
> I'd be fine with UREH being plumbed here.
Sounds good. I'll plumb it in another patch.

http://codereview.chromium.org/2858027/diff/1/3
File chrome/browser/sync/profile_sync_service_password_unittest.cc (right):

http://codereview.chromium.org/2858027/diff/1/3#newcode220
chrome/browser/sync/profile_sync_service_password_unittest.cc:220:
nigori_node.Put(SPECIFICS, nigori_specifics);
On 2010/06/25 22:47:38, ncarter wrote:
> This is the first time I've noticed how in this file we test things at the
> ProfileSyncService layer, but also directly create entries in the syncable DB
> (sidestepping the limitations imposed by the syncapi).  It's kind of cool. 
The
> other test types had fakey interceptions of GetSyncIdForTaggedNode, a method
> that owes its existence to this problem.  I'd support getting rid of
> GetSyncIdForTaggedNode in favor of what you do here; it's more realistic.  And
> this is making me what else we could stand to gain (in terms of testability,
> etc) by breaking down some of the syncapi/syncable barrier.

Zork deserves the credit to coming up with this way of creating the permanent
password node, I just added code to also create the Nigori node. Regarding
testing, in addition to the syncapi/syncable barrier, I think one of the big
obstacles is the prevalent use of static methods and constructors. Skrul filed a
bug a couple months ago with a few good suggestions: crbug.com/42152

> As a side note, it's shocking how many lines of code it is to set up a single
> syncable object that passes the tree invariants, don't you think?

To be honest, I am shocked how many lines of code it takes to do anything with
sync :)