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

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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.

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: watch() should cause onlogin() or onlogout() to fire before onready()

- 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

Comment 3

[Sam Penrose]

Fernando, could you give us your thoughts on this approach?

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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."

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Alternate patch, rebased over bugs 947374 and 971379

:ggp, if you have patches for bug 947374 and bug 971379 applied, try this - it's rebased over those two

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

I've replied at https://bugzilla.mozilla.org/show_bug.cgi?id=945363#c20

Comment 7

[Guilherme Gonçalves [:ggp]]

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().

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: Gaia UI test app for FxA watch() and request()

Gaia test app for FxA watch() and request()

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(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

Comment 10

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: watch() should cause onlogin() or onlogout() to fire before onready()

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!

Comment 11

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Bug 947374 has landed, so you just need to apply bug 971379 if you are running today's build.

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 14

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: watch() should get silent assertion if possible

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

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Looks like some tests broke.  Removing checkin-needed and fixing.

Comment 16

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: watch() should get silent assertion if possible; r=ferjm

Tests needed a tweak.  Should be all good now.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52892649259e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52892649259e