Refactor sync passphrase setup flow and fix passphrase tests

Refactor sync passphrase setup flow and fix passphrase tests

The passphrase sync integration tests are in a state of disrepair due to
several recent changes to the sync implementation. In particular,
passphrase sync uses two similar methods called OnPassphraseRequired
and OnPassphraseFailed to differentiate between cases where a passphrase is
required for a first attempt at decryption and cases where decryption with a
cached passphrase failed and the user needs to be prompted.

This patch refactors the "for_decryption" boolean flag into an enum called
PassphraseRequiredReason, thereby eliminating the need for OnPassphraseFailed.

It also fixes the tests and updates the test framework to account for
scenarios in which a client is waiting for a passphrase that has been
set by one of its partners, and enables it to exit early when forward
progress is impossible without a call to SetPassphrase.

BUG=78840, 80180, 81018
TEST=sync_integration_tests

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

[Raghu Simha, 2011-04-28 02:43:16]

Lingesh, please review.

[Raghu Simha, 2011-04-28 17:11:41]

+tim.

[tim (not reviewing), 2011-04-28 17:13:53]

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.h:97: virtual void
OnPassphraseFailed() = 0;
This is what OnPassphraseRequired is for above...  we already added a bool to
that method to distinguish for_decryption.  Is there now a third state?

[tim (not reviewing), 2011-04-28 17:18:00]

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/profile_sy...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/profile_sy...
chrome/browser/sync/profile_sync_service.cc:655: OnPassphraseRequired(true);
This seems like a step in the wrong direction.  We've tried to reduce the amount
of variables like this the PSS keeps around, and now we have synonyms with
uber-subtle differences :(   There has to be another way.

[Raghu Simha, 2011-04-28 17:44:21]

A couple of wordy comments :)

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.h:97: virtual void
OnPassphraseFailed() = 0;
On 2011/04/28 17:13:53, timsteele wrote:
> This is what OnPassphraseRequired is for above...  we already added a bool to
> that method to distinguish for_decryption.  Is there now a third state?

ProfileSyncServiceHarness::OnStateChanged is called when
SyncFrontend::OnPassphraseAccepted() is triggered by both,
Core::NotifyPassphraseRequired() and by Core::NotifyPassphraseFailed(). The crux
of this patch is that we need a way to differentiate between the two. This is
because in the first case (required), forward progress is possible, while in the
latter case (failed), forward progress is impossible and we must exit early from
AwaitStatusChangeWithTimeout.

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/profile_sy...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/profile_sy...
chrome/browser/sync/profile_sync_service.cc:655: OnPassphraseRequired(true);
On 2011/04/28 17:18:00, timsteele wrote:
> This seems like a step in the wrong direction.  We've tried to reduce the
amount
> of variables like this the PSS keeps around, and now we have synonyms with
> uber-subtle differences :(   There has to be another way.

Like I explained in the other comment, this is to differentiate between cases
where forward progress is possible and cases when it's not. If you look at the
header comments in profile_sync_service_observer.h, it says that "Observers
should query the service to determine what happened". As of today, there is
nothing in ProfileSyncService that the harness can query to find out if an
existing (cached) passphrase simply needs to be tried out, or if it has already
been tried and the attempt failed.

I agree this mechanism doesn't scale well. I'd love to hear your ideas for how
to fix this. The thing is that if we treat both cases identically, every test
that sets a passphrase on one client will have to have other clients wait for up
to 45 seconds before it knows that it should try a new passphrase. Keeping count
of the number of times OnStateChange() was called doesn't work because it can be
called for any number of other reasons.

[Raghu Simha, 2011-04-28 17:57:32]

On 2011/04/28 17:13:53, timsteele wrote:
>
http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
> File chrome/browser/sync/glue/sync_backend_host.h (right):
> 
>
http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host.h:97: virtual void
> OnPassphraseFailed() = 0;
> This is what OnPassphraseRequired is for above...  we already added a bool to
> that method to distinguish for_decryption.  Is there now a third state?

To specifically answer this: Yes, there are now 3 states. However, we can't
differentiate between the 2nd and 3rd states:

- Normal case (for_decryption is false).
- Trying decryption for the first time with a cached passphrase (for_decryption
is true).
- Tried decryption with the cached passphrase, and it didn't work. Need a new
passphrase (for_decryption is still true).

[tim (not reviewing), 2011-04-28 19:20:29]

On 2011/04/28 17:57:32, rsimha wrote:
> On 2011/04/28 17:13:53, timsteele wrote:
> >
>
http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
> > File chrome/browser/sync/glue/sync_backend_host.h (right):
> > 
> >
>
http://codereview.chromium.org/6902101/diff/22/chrome/browser/sync/glue/sync_...
> > chrome/browser/sync/glue/sync_backend_host.h:97: virtual void
> > OnPassphraseFailed() = 0;
> > This is what OnPassphraseRequired is for above...  we already added a bool
to
> > that method to distinguish for_decryption.  Is there now a third state?
> 
> To specifically answer this: Yes, there are now 3 states. However, we can't
> differentiate between the 2nd and 3rd states:
> 
> - Normal case (for_decryption is false).
> - Trying decryption for the first time with a cached passphrase
(for_decryption
> is true).
> - Tried decryption with the cached passphrase, and it didn't work. Need a new
> passphrase (for_decryption is still true).

Without fully groking, if there's a third state, then the proper thing to do
seems to be to turn the bool into something that can represent that (e.g. enum),
vs adding a similar sounding method.

[Raghu Simha, 2011-04-28 19:28:30]

On 2011/04/28 19:20:29, timsteele wrote:

> Without fully groking, if there's a third state, then the proper thing to do
> seems to be to turn the bool into something that can represent that (e.g.
enum),
> vs adding a similar sounding method.

I'm sure that's possible. Let me try going down that path.

[Raghu Simha, 2011-04-28 22:44:37]

On 2011/04/28 19:20:29, timsteele wrote:

> Without fully groking, if there's a third state, then the proper thing to do
> seems to be to turn the bool into something that can represent that (e.g.
enum),
> vs adding a similar sounding method.

I've just uploaded a new patch where the booleans observed_passphrase_failed_
and passphrase_required_for_decryption_ have been replaced by a single 3 state
enum value passphrase_required_reason_. Let me know what you think.

[tim (not reviewing), 2011-04-29 01:21:48]

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.h:113: ENCRYPTION = 1,            // The
cryptographer requires a passphrase for
Since this is in the syncapi ns, we should probably do something like
REASON_ENCRYPTION
REASON_DECRYPTION...

sync_api::ENCRYPTION is kinda ambiguous

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.h:117: DECRYPTION_FAILED = 3,     // The
cryptographer requires a new passphrase
SET_PASSPHRASE_FAILED

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/js_sync_...
File chrome/browser/sync/js_sync_manager_observer_unittest.cc (right):

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:141:
sync_manager_observer_.OnPassphraseRequired(sync_api::UNKNOWN);
should add syncapi.h include

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:72:
passphrase_required_reason_(sync_api::UNKNOWN),
i think I liked NO_REASON better

[Raghu Simha, 2011-04-29 18:59:14]

+zea as reviewer. Patch still in progress.

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.h:113: ENCRYPTION = 1,            // The
cryptographer requires a passphrase for
On 2011/04/29 01:21:48, timsteele wrote:
> Since this is in the syncapi ns, we should probably do something like
> REASON_ENCRYPTION
> REASON_DECRYPTION...
> 
> sync_api::ENCRYPTION is kinda ambiguous

Done.

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.h:117: DECRYPTION_FAILED = 3,     // The
cryptographer requires a new passphrase
On 2011/04/29 01:21:48, timsteele wrote:
> SET_PASSPHRASE_FAILED

Done.

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/js_sync_...
File chrome/browser/sync/js_sync_manager_observer_unittest.cc (right):

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:141:
sync_manager_observer_.OnPassphraseRequired(sync_api::UNKNOWN);
On 2011/04/29 01:21:48, timsteele wrote:
> should add syncapi.h include

It's already included. Line 11.

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/6015/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:72:
passphrase_required_reason_(sync_api::UNKNOWN),
On 2011/04/29 01:21:48, timsteele wrote:
> i think I liked NO_REASON better 

Done.

[Raghu Simha, 2011-04-29 20:38:23]

This is now ready for a proper review.

zea: Removal of OnPassphraseFailed.
akalin: js changes.
tim: Everything else.

[Nicolas Zea, 2011-04-29 22:59:29]

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.h:115: // encryption.
Mention that this is the migration/upgrade case, and does not happen in normal
usage.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/js_sync_...
File chrome/browser/sync/js_sync_manager_observer_unittest.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:142:
HasArgsAsList(reason_set_passphrase_failed_args),
I believe the expectations should be in reverse order, so the first result
expected is set last?

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:382: observed_passphrase_required_ =
false;
Add clear of passphrase reason here too?

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:611: //
REASON_SET_PASSPHRASE_FAILED.
That's not true right? If the passphrase changes and we can't decrypt, syncapi
will trigger OnPassphraseRequired(reason..decryption)

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:514: bool
observed_passphrase_required_;
Do we need this anymore? Can't we always use passphrase_required_reason?

[tim (not reviewing), 2011-04-29 23:02:12]

I like this new enum.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:606: observed_passphrase_required_ =
true;
can we eliminate this and just check passphrase_reason_ != NOT_REQUIRED ?

We can leave the observed_passphrase_required() method and just change it to
mean this under the covers.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:232:
SignalStateCompleteWithNextState(PASSPHRASE_REQUIRED_FOR_DECRYPTION);
It's a bit unintuitive now that we say if REASON_SET_PASSPHRASE_REQUIRED (and
_not_ REASON_DECRYPTON) then PASSPHRASE_REQUIRED_FOR_DECRYPTION...

[Raghu Simha, 2011-04-30 00:43:05]

This might be the one :)

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncapi.h (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncapi.h:115: // encryption.
On 2011/04/29 22:59:30, nzea wrote:
> Mention that this is the migration/upgrade case, and does not happen in normal
> usage.

Done.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/js_sync_...
File chrome/browser/sync/js_sync_manager_observer_unittest.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:142:
HasArgsAsList(reason_set_passphrase_failed_args),
On 2011/04/29 22:59:30, nzea wrote:
> I believe the expectations should be in reverse order, so the first result
> expected is set last?

This is fine, based on the in-person conversation we just had :)

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:382: observed_passphrase_required_ =
false;
On 2011/04/29 22:59:30, nzea wrote:
> Add clear of passphrase reason here too?

Done.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:606: observed_passphrase_required_ =
true;
On 2011/04/29 23:02:13, timsteele wrote:
> can we eliminate this and just check passphrase_reason_ != NOT_REQUIRED ?
> 
> We can leave the observed_passphrase_required() method and just change it to
> mean this under the covers.

Done.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:611: //
REASON_SET_PASSPHRASE_FAILED.
On 2011/04/29 22:59:30, nzea wrote:
> That's not true right? If the passphrase changes and we can't decrypt, syncapi
> will trigger OnPassphraseRequired(reason..decryption)

The comment is outdated, misleading and probably unnecessary. Removed it, since
right now, all we're doing is simply storing the value of |reason|, whatever it
might be.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:514: bool
observed_passphrase_required_;
On 2011/04/29 22:59:30, nzea wrote:
> Do we need this anymore? Can't we always use passphrase_required_reason?

Good point. Removed.

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/6902101/diff/4060/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_harness.cc:232:
SignalStateCompleteWithNextState(PASSPHRASE_REQUIRED_FOR_DECRYPTION);
On 2011/04/29 23:02:13, timsteele wrote:
> It's a bit unintuitive now that we say if REASON_SET_PASSPHRASE_REQUIRED (and
> _not_ REASON_DECRYPTON) then PASSPHRASE_REQUIRED_FOR_DECRYPTION...

Changed to SET_PASSPHRASE_FAILED.

[Nicolas Zea, 2011-05-02 17:39:03]

LGTM

[akalin, 2011-05-02 18:02:47]

On 2011/05/02 17:39:03, nzea wrote:
> LGTM

js changes LGTM

[akalin, 2011-05-02 18:02:53]

http://codereview.chromium.org/6902101/diff/7025/chrome/browser/sync/js_sync_...
File chrome/browser/sync/js_sync_manager_observer_unittest.cc (right):

http://codereview.chromium.org/6902101/diff/7025/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:133: NULL));
indentation (NULL should line up with "OnPassphraseRequired").

http://codereview.chromium.org/6902101/diff/7025/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:143: NULL));
here, too

[Raghu Simha, 2011-05-02 18:20:05]

http://codereview.chromium.org/6902101/diff/7025/chrome/browser/sync/js_sync_...
File chrome/browser/sync/js_sync_manager_observer_unittest.cc (right):

http://codereview.chromium.org/6902101/diff/7025/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:133: NULL));
On 2011/05/02 18:02:53, akalin wrote:
> indentation (NULL should line up with "OnPassphraseRequired").

Nice catch. Done.

http://codereview.chromium.org/6902101/diff/7025/chrome/browser/sync/js_sync_...
chrome/browser/sync/js_sync_manager_observer_unittest.cc:143: NULL));
On 2011/05/02 18:02:53, akalin wrote:
> here, too

Done.

[tim (not reviewing), 2011-05-02 19:37:36]

We should add a method (could be a static function taking a passphrase required
reason, or just a member of PSS) that returns true if the reason is DECRYPTION
or SET_PASSPHRASE_FAILED.  It can be a separate patch, if you promise to do it
:)

LGTM otherwise.

[Raghu Simha, 2011-05-03 03:29:18]

On 2011/05/02 19:37:36, timsteele wrote:
> We should add a method (could be a static function taking a passphrase
required
> reason, or just a member of PSS) that returns true if the reason is DECRYPTION
> or SET_PASSPHRASE_FAILED.  It can be a separate patch, if you promise to do it
> :)

This is now out for review at http://codereview.chromium.org/6910012.