959088 - browserid_identity doesn't report an auth status of "ok" until a sync key bundle has been fetched.

Status: VERIFIED WORKSFORME

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Have-bid_identity-not-return-a-status-of-LOGIN_FAILE.patch

bid_identity's currentAuthState getter wants to see both a username and a sync key bundle before it reports STATUS_OK.  However, the bundle is fetched from a server, so it is always the case that even when everything is configured correctly, we initially report a status of LOGIN_FAILED_NO_PASSPHRASE.  This causes things like sync setup panes to initially report a bad configuration before eventually changing to STATUS_OK as the remote server responds.

This patch tries to resolve this by the addition of a flag _shouldHaveSyncKeyBundle.  While this flag is false, it reports STATUS_OK so long as we have just the username.  When it is true, it also wants the sync key bundle.  This flag initially starts as false, and is set to true as we attempt to fetch the sync key bundle.

The end result is that STATUS_OK is typically returned as things initialize.  If we fail to fetch a sync key bundle, it eventually will return an error state, but the end result is that if the sync options dialog is displayed before we've managed to grab the sync key bundle, the user doesn't get informed we are in an error state.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 8359108 [details] [diff] [review]
Have-bid_identity-not-return-a-status-of-LOGIN_FAILE.patch

Review of attachment 8359108 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like a reasonable hack.

Consider, though: Sync used to have the concept of being logged in (removed in Bug 590763), which lives on as Status.login. That does mean that the skeleton exists onto which you can graft the multi-state-ness you're trying to represent -- that of being correctly configured, but not yet having done the server dance that would allow you to actually sync.

Whether you choose to address that more thoroughly is essentially a measure of how long you think this code will live :P

See also: Bug 821143.

::: services/sync/modules/browserid_identity.js
@@ +54,5 @@
>    _account: null,
>  
> +  // it takes some time to fetch a sync key bundle, so until this flag is set,
> +  // we don't consider the lack of the bundle to mean that the authState is
> +  // such that we need to authorize.

I'd pick a different word here than "authorize". Perhaps "we don't consider the lack of a keybundle as a failure state"?

@@ +256,5 @@
>        .then(userData => {
>          // both Jelly and FxAccounts give us kA/kB as hex.
>          let kB = Utils.hexToBytes(userData.kB);
>          this._syncKeyBundle = deriveKeyBundle(kB);
> +        this._shouldHaveSyncKeyBundle = true;

I have a vague feeling that this should be set sooner -- before promise resolution -- the idea being that a stalled promise won't leave us reporting A-OK. But nbd.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to Richard Newman [:rnewman] from comment #1)
> Comment on attachment 8359108 [details] [diff] [review]
> Have-bid_identity-not-return-a-status-of-LOGIN_FAILE.patch
> 
> Review of attachment 8359108 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems like a reasonable hack.
> 
> Consider, though: Sync used to have the concept of being logged in (removed
> in Bug 590763), which lives on as Status.login. That does mean that the
> skeleton exists onto which you can graft the multi-state-ness you're trying
> to represent -- that of being correctly configured, but not yet having done
> the server dance that would allow you to actually sync.

IIUC, the code that was removed was only about reflecting this state in the UI - internally there is the services.loggedIn state, but it seems this isn't going to be that helpful without also reinstating the "throbber" - and given that more stuff keep piling on, I think I'd prefer to just stick with the approach in the original patch and revisit this once we have time to breath again...

Comment 3

[Chris Karlof [:ckarlof]]

Comment on attachment 8359108 [details] [diff] [review]
Have-bid_identity-not-return-a-status-of-LOGIN_FAILE.patch

Review of attachment 8359108 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Richard Newman [:rnewman] from comment #1)
> I have a vague feeling that this should be set sooner -- before promise
> resolution -- the idea being that a stalled promise won't leave us reporting
> A-OK. But nbd.

I think setting it sooner would defeat the purpose - the earlier opportunities are still while we expect new network transactions to be made.  But the point is very real, and specifically, the unverified email case will hit this (the promise may never be resolved nor rejected in that case).  I opened bug 961017 for this.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

This is subsumed by the "part 1" patch in bug 959222.

Comment 6

[James Bonacci [:jbonacci]]

OK.