Closed Bug 1008901 Opened 5 years ago Closed 5 years ago

Fire 'onlogin' when the account is verified

Categories

(Firefox :: Firefox Accounts, defect, P3)

All
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
Firefox 32

People

(Reporter: ferjm, Assigned: spenrose)

References

Details

(Whiteboard: [fxa4fxos2.0])

Attachments

(1 file, 1 obsolete file)

Currently, RPs requesting a FxAccounts assertion via mozId when there is an unverified account logged in the device will trigger the 'onerror' callback with the 'UNVERIFIED_ACCCOUNT' error. However, we don't fire the 'onlogin' callback when the account is verified *after* triggering the onerror callback if the RP. I think we should trigger the onlogin callback for all active RPs when we receive the ONVERIFIED_NOTIFICATION from FxA.
Assignee: nobody → ferjmoreno
Component: General → FxAccounts
Product: Firefox OS → Core
See also https://bugzilla.mozilla.org/show_bug.cgi?id=1004319#c2 , where we need to trigger an event in FxAccountsManager that results in DOMIdentity.jsm telling each RP to logout. Do you want to assign this one to me and have me do them together? I'm going to work on 1004319 today or maybe tomorrow.
Thanks Sam!

I believe you just need to observe ONVERFIED_NOTIFICATION for this bug and ONLOGOUT_NOTIFICATION for bug 1004319 [1] at [2] and trigger the appropriate RP callbacks.

[1] https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsCommon.js#53
[2] https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/FirefoxAccounts.jsm#62
Assignee: ferjmoreno → spenrose
Actually, after reading bug 1004319 it might need a different approach. https://bugzilla.mozilla.org/show_bug.cgi?id=1004319#c3 but that's something to discuss on that bug :).
Blocks: 1006540
No longer blocks: loop_mobile_ID
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P3
Hardware: x86 → All
Whiteboard: [fxa4fxos2.0]
I tried to signal from FxAccounts.pollEmailStatus() to toolkit, as Fernando suggests. The problem is as follows:

  previously, FxAccountsManager has cached the user with verified:false
  pollEmailStatus() gets verified:true and sets it in FxAccounts
  toolkit's FirefoxAccount calls FxAccountsManager.getAssertion()
  getAssertion() calls this.getAccounts(), which returns the cache
  getAssertion() then returns ERROR_UNVERIFIED_ACCOUNT

I believe the real problem is that FxAccountsManager should not exist. In the short term, we can either have toolkit tell FxAccountsManager to discard its cache via a new parameter to getAssertion(), or we can have pollEmailStatus() tell FxAccountsManager the same thing via a new event, and then have the latter fire ONVERIFIED. I like the second approach better, as it means that ONVERIFIED doesn't fire until services/fxaccounts/*jsm state is coherent. Patch coming, but I'd love feedback. Maybe there is a third approach (register a clear-cache callback with FxAccounts?), or I am missing something about the problem? 

(In reply to Fernando Jiménez Moreno [:ferjm] (work week, not reading bugmail) from comment #2)
> Thanks Sam!
> 
> I believe you just need to observe ONVERFIED_NOTIFICATION for this bug and
> ONLOGOUT_NOTIFICATION for bug 1004319 [1] at [2] and trigger the appropriate
> RP callbacks.
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/services/fxaccounts/
> FxAccountsCommon.js#53
> [2]
> https://mxr.mozilla.org/mozilla-central/source/toolkit/identity/
> FirefoxAccounts.jsm#62
Flags: needinfo?(jparsons)
Attached patch 1008901-verify-onlogin.patch (obsolete) — Splinter Review
This patch is built on top of the latest patch from bug 1004319:

  https://bugzilla.mozilla.org/attachment.cgi?id=8431207

It follows the chained-signal approach I outlined in the last comment. r- without hesitation if you have concerns, and thanks for your time.
Attachment #8435106 - Flags: review?(jparsons)
Flags: needinfo?(jparsons)
Comment on attachment 8435106 [details] [diff] [review]
1008901-verify-onlogin.patch

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

Thanks, Sam.  This looks good to me.  Nice and short and too the point.

I just have a few questions.  r=me with those addressed.

::: toolkit/identity/FirefoxAccounts.jsm
@@ +46,5 @@
>  #endif
>  
>  function FxAccountsService() {
>    Services.obs.addObserver(this, "quit-application-granted", false);
> +  Services.obs.addObserver(this, ONVERIFIED_NOTIFICATION, false);

Isn't this going to be an error if run on desktop or android, since the import of Common is only done for b2g?

@@ +65,5 @@
>      switch (aTopic) {
> +      case ONVERIFIED_NOTIFICATION:
> +        log.debug("Received " + ONVERIFIED_NOTIFICATION + "; firing request()s");
> +        for (let [rpId,] of this._rpFlows) {
> +          this.request(rpId);

I suppose we don't need the options {silent: true} in this case.

@@ +72,2 @@
>        case "quit-application-granted":
>          Services.obs.removeObserver(this, "quit-application-granted");

Also needs: Services.obs.removeObserver(this, ONVERIFIED_NOTIFICATION);
Attachment #8435106 - Flags: review?(jparsons) → review+
Thanks very much, Jed, especially for catching my #ifdef fail. I fixed that. Regarding the .silent, (and per IRC), I am comfortable with the idea that we have written this event-handling code in concert with the event-generating code, which only sends ONVERIFIED when we have a logged-in user, making .silent unnecessary.

https://tbpl.mozilla.org/?tree=Try&rev=cbcf099174ea
Attachment #8435106 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5c5e7c0f31c5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Target Milestone: mozilla33 → mozilla32
Product: Core → Firefox
Target Milestone: mozilla32 → Firefox 32
You need to log in before you can comment on or make changes to this bug.