FxA should not fire ONLOGIN twice after an RP-triggered refreshAuthentication

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

Firefox OS
FxA
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Sam Penrose, Assigned: Sam Penrose)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
0) As of bug 1030227 , when an RP calls request({refreshAuthentication:0}) and the user enters the correct password, its onlogin fires:

  https://github.com/mozilla/gecko-dev/blob/master/toolkit/identity/FirefoxAccounts.jsm#L175

while a separate call to the onlogin for every RP, including the request caller, is also triggered thanks to the change in the bug. The underlying mechanism is tricky:

1) When the refreshAuthentication UI exits after a successful challenge, the blob returned to Gecko here:

  https://github.com/mozilla/gecko-dev/blob/master/b2g/components/FxAccountsMgmtService.jsm#L72

looks like this:

  {"id":"<id>","data":{"method":"signIn","email":"<email>","password":"<pw>"}}

according to the current pattern, data.method should be refreshAuthentication (but see below). I believe I have traced the problem as far as:

  https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/fxa_ui.js#L87

(in other words, data on that line is the above blob coming from one of the fxam_* modules).

2) However, this bug is masking a crasher in FxAccountsMgmtService.jsm:

  https://github.com/mozilla/gecko-dev/blob/master/b2g/components/FxAccountsMgmtService.jsm#L140

which is that FxAccountsManager does not have a refreshAuthentication method.

3) FxAccountsManager started the UI in its _refreshAuthentication method (note the underscore), which is a hint that the [event returned by the UI / method called in FxAccountsManager] should be spelled "refreshSucceeded" or similar; the current event naming pattern does not distinguish between "pleaseCallUI" and "uiReturned" which is bad IMHO.

4) That's a lot of refactoring. For now I think the simple, 2.0-uplift-friendly  fix is to stop ONLOGIN firing when we have a refreshAuthentication in progress.

5) Sadly, ONLOGIN fires in FxAccounts.jsm, which has no concept of refreshAuthentication because shoot me now. Also, our control flow is promise-based, which means we need promise-friendly state management. Therefore Jed I propose to move the ugly this._refreshing flag I added to FxAccountsManager to FxAccounts, using it to suppress the ONLOGIN event. The resulting patch will have about 6 single-line edits and solve this problem.

6) Alternately, we could set a flag in FirefoxAccounts.jsm when it gets the RP's request(), but:

  i. b2g/components/FxAccountsMgmtService.jsm also listens to ONLOGIN
  ii. ONLOGIN might get a new listener
  iii. ONLOGIN after refreshAuthentication is a lie anyway
  iv. flags are ugly, having 2 is worse than having 1.

Thoughts?
(Assignee)

Comment 1

4 years ago
Oh, in point 3) I left out the punchline:

... Because FxAccountsMgmtService.jsm erroneously and thankfully calls FxAccountsManager.signIn() instead of the non-existent .refreshAuthentication(), FxAccountsManager thinks we are logging in, and calls FxAccounts.setSignedInUser().
(Assignee)

Comment 2

4 years ago
Another thought as I remember to ni? Jed for Comment 0. It would be cleaner to patch the Gaia system app to return {data: {method: 'refreshAuthentication'}} in step 1) above, but that's a challenging review/uplift for 2.0, and we'd need to first land and uplift a Gecko patch to FxAccountsMgmtService.jsm that discarded the event instead of crashing. Maybe; I'm about 80% sure that would work. On balance I still favor the proposal in point 5) above.
Flags: needinfo?(jparsons)
This approach makes sense to me (point 5 in the Description).  And I agree that confining the fixes to gecko will make 2.0 uplift easier.  Furthermore, there are, as you've said elsewhere, other larger refactors potentially coming up for subsequent versions, so investing in deeper structural changes here only to discard them later would seem pointless.
Flags: needinfo?(jparsons)
(Assignee)

Comment 4

4 years ago
Created attachment 8448232 [details] [diff] [review]
1031580-move-refresh-flag.patch

This patch is a simpler version of the proposal I made in Comment 0, point 5). It turns out that FxAccountsManager directly calls the saving of state method in FxAccounts which emits the event*. Since no state has changed, we can simply not re-save it, which bypasses the firing of ONLOGIN. xpcshell tests pass and behavior verified manually.

* which seems worth reconsidering
Attachment #8448232 - Flags: review?(jparsons)
Comment on attachment 8448232 [details] [diff] [review]
1031580-move-refresh-flag.patch

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

Wow, short and sweet!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +140,5 @@
>          user.email = user.email || aEmail;
> +
> +        if (this._refreshing) { // No need to update local state
> +          return Promise.resolve({user: this._user});
> +        }

Wow, fantastic that it was this simple.  Could you add some more comments, though, to explain for future generations?  A reference to the bug would be sufficient, I think.
Attachment #8448232 - Flags: review?(jparsons) → review+
(Assignee)

Comment 6

4 years ago
Thanks, Jed; comment added.

https://tbpl.mozilla.org/?tree=Try&rev=86efe5057416
(Assignee)

Comment 7

4 years ago
Created attachment 8448836 [details] [diff] [review]
1031580-move-refresh-flag.patch
Attachment #8448232 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
blocking-b2g: --- → 2.0?
Keywords: checkin-needed

Updated

4 years ago
Target Milestone: --- → 2.0 S5 (4july)
https://hg.mozilla.org/mozilla-central/rev/213b52d04d79
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Sam, just want to check, is it blocking Bug 1000323 or other 2.0 blocking bugs?  
If not, and *if* it is not deemed as a blocker, we can uplift to 2.0 by the uplift request route.
Flags: needinfo?(spenrose)
(Assignee)

Comment 11

4 years ago
(In reply to No-Jun Park [:njpark] from comment #10)
> Sam, just want to check, is it blocking Bug 1000323 or other 2.0 blocking
> bugs?  
> If not, and *if* it is not deemed as a blocker, we can uplift to 2.0 by the
> uplift request route.

Guilherme, this is your call.
Flags: needinfo?(spenrose) → needinfo?(ggoncalves)
It does block bug 1000323.
Flags: needinfo?(ggoncalves)
Blocks: 1000323
2.0+ as this blocks a blocker. There's also a small fix ready to go. Let's get this landed.
blocking-b2g: 2.0? → 2.0+
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e7a7d248e40
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox31: --- → wontfix
status-firefox32: --- → fixed
status-firefox33: --- → fixed
You need to log in before you can comment on or make changes to this bug.