Closed Bug 978896 Opened 6 years ago Closed 6 years ago

watch() does not automatically login a signed-in user

Categories

(Core Graveyard :: Identity, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: jedp, Assigned: jedp)

References

Details

Attachments

(1 file, 5 obsolete files)

No description provided.
Status: NEW → ASSIGNED
I've confirmed with both :callahad and :fmarier on IRC that watch should onready() after it calls either onlogin() or onlogout().

Firefox Accounts does not have the concept of the loggedInUser that BrowserID supports, so we don't have the third possible response pattern, namely just onready() when there is no state to work with.
- stylistic fixes to test head.js
- FXA watch() will call onlogout or onlogin before onready
- In case of error, FXA watch() just calls onerror (no onready)
- Tests updated
Attachment #8384864 - Flags: feedback?(ggoncalves)
Fernando, could you give us your thoughts on this approach?
Flags: needinfo?(ferjmoreno)
We're now having this conversation in two places :)

See also Bug 945363 Comment 17 and subsequent discussion.

I'm trying to follow the BrowserID spec:

  https://developer.mozilla.org/en-US/docs/Web/API/navigator.id.watch

"The onready callback will be invoked immediately after any automatic invocations of onlogin, onlogout, or onmatch. By waiting to display UI until onready is called, relying parties can avoid UI flicker in cases where the user agent's preferred state is out of sync with the site's session."
:ggp, if you have patches for bug 947374 and bug 971379 applied, try this - it's rebased over those two
Comment on attachment 8384864 [details] [diff] [review]
watch() should cause onlogin() or onlogout() to fire before onready()

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

Clearing the feedback flag for now as I tried this patch a few days ago and already discussed it on IRC with :jedp. This patch is buggy: it causes the Firefox Accounts login flow to appear spontaneously, even before my call to request().
Attachment #8384864 - Flags: feedback?(ggoncalves)
Gaia test app for FxA watch() and request()
(In reply to Guilherme Gonçalves [:ggp] from comment #7)
> Comment on attachment 8384864 [details] [diff] [review]
> watch() should cause onlogin() or onlogout() to fire before onready()

Hi, Guilherme, and thanks for checking it out!

> Review of attachment 8384864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Clearing the feedback flag for now as I tried this patch a few days ago and
> already discussed it on IRC with :jedp. This patch is buggy: it causes the
> Firefox Accounts login flow to appear spontaneously, even before my call to
> request().

Yup, it does not work as anticipated.  Sorry.

I've created a gaia UI Test app to confirm this (attachment 8388951 [details] in comment 8).  We can iterate with that to solve the problem.

Cheers,
j
This patch needs to be applied over the latest for bug 947374 and bug 971379.

For a gaia test app, see bug 929917 attachment 8389549 [details].

ggp, want to take it for a spin?  Thanks!
Attachment #8384864 - Attachment is obsolete: true
Attachment #8385012 - Attachment is obsolete: true
Attachment #8388951 - Attachment is obsolete: true
Attachment #8389553 - Flags: feedback?(ggoncalves)
Bug 947374 has landed, so you just need to apply bug 971379 if you are running today's build.
Comment on attachment 8389553 [details] [diff] [review]
watch() should cause onlogin() or onlogout() to fire before onready()

Hi, Fernando,

I've tested this with a gaia ui test app and feel confident that it works as it should.  Hence I've canceled f=ggp.  Do you have a moment to review?  Thanks!
j
Attachment #8389553 - Flags: feedback?(ggoncalves) → review?(ferjmoreno)
Comment on attachment 8389553 [details] [diff] [review]
watch() should cause onlogin() or onlogout() to fire before onready()

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

LGTM. Thanks Jed!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +363,5 @@
> +   *
> +   *   refreshAuthentication  - (bool) Force re-auth.
> +   *
> +   *   silent                 - (bool) Prevent any UI interaction.
> +   *                            I.e., try to get an automatic assertion.       

nit: trailing whitespaces.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +177,5 @@
>  
>      // Call logout() on the next tick
>      let runnable = {
>        run: () => {
> +        this.fxAccountsManager.signOut().then(() => { 

nit: trailing whitespace
Attachment #8389553 - Flags: review?(ferjmoreno) → review+
Thanks for your review, Fernando!

Sorry about the trailing whitespace.  Fixed.  (I seem to have got something wrong with my .vimrc; it gives up on highlighting trailing whitespace after a while.)

r=ferjm
Attachment #8389553 - Attachment is obsolete: true
Looks like some tests broke.  Removing checkin-needed and fixing.
Keywords: checkin-needed
Tests needed a tweak.  Should be all good now.
Attachment #8390635 - Attachment is obsolete: true
Attachment #8390991 - Attachment description: watch() should get silent assertion if possible → watch() should get silent assertion if possible; r=ferjm
https://hg.mozilla.org/mozilla-central/rev/52892649259e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.