Implement WebChannel interface to fxa-content-server in Fennec

RESOLVED FIXED in Firefox 43

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: nalexander, Assigned: nalexander)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
Firefox 43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
The fxa-content-server /settings end point allows to change the password (invalidating the existing Sync keys) and to delete the Firefox Account from the fxa-auth-server entirely.  The content server implemented this in https://github.com/mozilla/fxa-content-server/issues/2152.

This ticket tracks implementing support for these actions in Fennec, or for hiding the functionality in the Fennec context.
(Assignee)

Comment 1

2 years ago
I've grown this ticket to encompass a complete WebChannel implementation for Fennec.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Summary: Implement or hide support for changing the password and deleting the account in Fennec → Implement WebChannel interface to fxa-content-server in Fennec
(Assignee)

Comment 2

2 years ago
Created attachment 8660942 [details]
MozReview Request: Bug 1191064 - Part 1: Add Fennec version of FxAccountsWebChannel. r?sebastian,markh

Bug 1191064 - Part 1: Add Fennec version of FxAccountsWebChannel. r?sebastian,markh

This ticket does the following things:

* register early.  If the first page that Gecko loads is
  about:accounts, the channel needs to be in place.  If we delay this,
  we can and do miss content server messages.

* listen to the following messages:

  CAN_LINK_ACCOUNT: 'fxaccounts:can_link_account'
  CHANGE_PASSWORD: 'fxaccounts:change_password'
  DELETE_ACCOUNT: 'fxaccounts:delete_account'
  LOADED: 'fxaccounts:loaded'
  LOGIN: 'fxaccounts:login'

The list of messages is from
https://github.com/mozilla/fxa-content-server/blob/2a78a14dafe396ce2bfb1572ad418f14cfab1355/app/scripts/models/auth_brokers/fx-desktop-v2.js#L24
via
https://github.com/mozilla/fxa-content-server/blob/2a78a14dafe396ce2bfb1572ad418f14cfab1355/app/scripts/models/auth_brokers/fx-fennec-v1.js

This patch implements only LOADED, LOGIN, and CHANGE_PASSWORD.  The
messages have the following behaviour:

A LOADED message is ferried to the individual XUL <browser> element it
originated from.  In general, WebChannel is a global listener: it does
not matter where a message originates.  We want to have fine-grained
control over when an embedding <iframe> is displayed (as opposed to
loaded, in the Gecko sense of loaded).  The fxa-content-server
participates in this exchange via the LOADED message; we complete the
loop by specially handling LOADED.

A LOGIN or CHANGE_PASSWORD message either creates a new Android
Account in the Engaged state, or moves an existing Android Account to
the Engaged state.  An Android sync is not yet requested -- we'll
arrange that from the Java side.
Attachment #8660942 - Flags: review?(s.kaspari)
Attachment #8660942 - Flags: review?(markh)
(Assignee)

Comment 3

2 years ago
Created attachment 8660943 [details]
MozReview Request: Bug 1191064 - Part 2: Handle relinking Firefox Accounts on Fennec. r?sebastian,markh

Bug 1191064 - Part 2: Handle relinking Firefox Accounts on Fennec. r?sebastian,markh

The desired behaviour:

* If we have no account now, and had no account or the same account in
  the past -- no message, allow.
* If we have no account now, but had a different account in the past -- prompt.
* If we have an account, and this is the same account -- no message, allow.
* If we have an account, and this is not the same account -- toast and
  never allow.
Attachment #8660943 - Flags: review?(s.kaspari)
Attachment #8660943 - Flags: review?(markh)
(Assignee)

Comment 4

2 years ago
Created attachment 8660944 [details]
MozReview Request: Bug 1191064 - Part 3: Handle removing Android Accounts from fxa-content-server. r?sebastian,markh

Bug 1191064 - Part 3: Handle removing Android Accounts from fxa-content-server. r?sebastian,markh

This adds a new JS to Java ping-pong; exposes it via Accounts.jsm; and
uses it in response to the fxa-content-server message.
Attachment #8660944 - Flags: review?(s.kaspari)
Attachment #8660944 - Flags: review?(markh)
(Assignee)

Comment 5

2 years ago
markh, sebastian: there's some context missing here (see earlier WIP commits, pushed to review but not yet landed or flagged) and some follow-up to continue with.  I wanted to get some of this out for early feedback while I polish and complete about:accounts.

This is a mostly stand-alone WebChannel listener for Fennec.  It creates, updates, and deletes native Android Accounts in response to an embedded accounts.firefox.com page.  The embedding happens in the (not yet flagged) about:accounts code; the details are not important right now, modulo the LOADED message (which is commented here).

Follow-up that I know of: 

* adding device handshake / capabilities WebChannel messaging (not yet implemented on server).
* radiating account removals and changes through-out Gecko (so, for example, we can drive about:accounts pages to appropriate endpoints as the account status changes).
* handling changes to the WebChannel URL (via about:config).  This is important for self-hosters (and correctness).

Thanks for your feedback!
(Assignee)

Comment 6

2 years ago
Adding a sprinkling of FxA folks who might be interested or good security checks.
Comment on attachment 8660942 [details]
MozReview Request: Bug 1191064 - Part 1: Add Fennec version of FxAccountsWebChannel. r?sebastian,markh

https://reviewboard.mozilla.org/r/19223/#review17103

::: mobile/android/modules/FxAccountsWebChannel.jsm:156
(Diff revision 1)
> +            Accounts.getFirefoxAccount().then(account => {

should this be a promise-chain with a trailing .catch? I'm wondering if some unexpected error in one of the Acocunts methods might end up unreported?

Desktop has this issue too :(

::: mobile/android/modules/FxAccountsWebChannel.jsm:158
(Diff revision 1)
> +                Accounts.createFirefoxAccountFromJSON(data).then(success => {

It seems strange that a password change request may end up calling createFirefoxAccountFromJSON?

::: mobile/android/modules/FxAccountsWebChannel.jsm:167
(Diff revision 1)
> +                Accounts.updateFirefoxAccountFromJSON(data).then(success => {

I can't find this function yet (maybe it is in a later patch?) but I'd think it would want to handle the fact that the new account may not match the existing account (the name implies it might not)

::: mobile/android/modules/FxAccountsWebChannel.jsm:202
(Diff revision 1)
> +// (eg, it uses the observer service to tell interested parties of interesting

This comment referencing the "observer service" probably needs to be updated

Looks good to me, and my comments are at your discretion.
Attachment #8660942 - Flags: review?(markh) → review+
Comment on attachment 8660943 [details]
MozReview Request: Bug 1191064 - Part 2: Handle relinking Firefox Accounts on Fennec. r?sebastian,markh

https://reviewboard.mozilla.org/r/19225/#review17107

::: mobile/android/modules/FxAccountsWebChannel.jsm:214
(Diff revision 1)
> +                  // In future, we should use a UID for this comparison.

why not use the uid now?

::: mobile/android/modules/FxAccountsWebChannel.jsm:225
(Diff revision 1)
> +                      callback: () => { 

trailing whitespace :)

::: mobile/android/modules/FxAccountsWebChannel.jsm:267
(Diff revision 1)
> +            this._helpers.setPreviousAccountNameHashPref(data.email);

It seems alittle wrong that you are setting this hash before you know if you can act on it (ie, what if you fail a few lines later?)

If you turn this into a promise chain (ie, returning the create/update promises) you could do this in a later .then() *and* still have a final .catch to log the errors :)
Attachment #8660943 - Flags: review?(markh) → review+
Attachment #8660944 - Flags: review?(markh) → review+
Comment on attachment 8660944 [details]
MozReview Request: Bug 1191064 - Part 3: Handle removing Android Accounts from fxa-content-server. r?sebastian,markh

https://reviewboard.mozilla.org/r/19227/#review17113

::: mobile/android/base/AccountsHelper.java:174
(Diff revision 1)
> +                            if (callback != null) {

Do you want to log anything here?

::: mobile/android/base/AccountsHelper.java:183
(Diff revision 1)
> +                Log.w(LOGTAG, "Got exception updating Firefox Account from JSON; ignoring.", e);

I know nothing about your logging modules, but from the other files I'd expect e.toString() here too?

::: mobile/android/modules/FxAccountsWebChannel.jsm:313
(Diff revision 1)
> +              }).catch(e => {

I wonder if returning the promise from Accounts.deleteFirefoxAccount and having the .catch on the "outer" promise makes sense?
(Assignee)

Updated

2 years ago
Depends on: 1204930
(Assignee)

Updated

2 years ago
Blocks: 1204930
No longer depends on: 1204930
(Assignee)

Updated

2 years ago
Component: Firefox Accounts → General
Product: Android Background Services → Firefox for Android
(Assignee)

Updated

2 years ago
Blocks: 1205011
(Assignee)

Comment 10

2 years ago
https://reviewboard.mozilla.org/r/19223/#review17103

> should this be a promise-chain with a trailing .catch? I'm wondering if some unexpected error in one of the Acocunts methods might end up unreported?
> 
> Desktop has this issue too :(

It's not 100% awesome 'cuz the error messaging really wants (account, success), but I made this a full chain.  It's good form.

> It seems strange that a password change request may end up calling createFirefoxAccountFromJSON?

I agree.  I separated out CHANGE_PASSWORD.

> I can't find this function yet (maybe it is in a later patch?) but I'd think it would want to handle the fact that the new account may not match the existing account (the name implies it might not)

Earlier patch, actually -- reviewed long ago by sebastian.  In response to your comment, I fail if the email differs.  (But not the UID -- more below.)

Your perspective on the promises chain and the high-level gotchas was valuable.  Thanks, markh!
(Assignee)

Comment 11

2 years ago
https://reviewboard.mozilla.org/r/19225/#review17107

> why not use the uid now?

Two reasons.  First, fxa-content-server doesn't provide it: https://github.com/mozilla/fxa-content-server/blob/2a78a14dafe396ce2bfb1572ad418f14cfab1355/app/scripts/models/auth_brokers/fx-desktop.js#L78.  (I just filed https://github.com/mozilla/fxa-content-server/issues/3062 to improve this.)

Second, it is technically possible to have a corrupt Android Account on Fennec which can't provide its UID.  For platform reasons, we always get an email.  This can be improved, and it is a remote possibility.  Future work, no matter what.

> It seems alittle wrong that you are setting this hash before you know if you can act on it (ie, what if you fail a few lines later?)
> 
> If you turn this into a promise chain (ie, returning the create/update promises) you could do this in a later .then() *and* still have a final .catch to log the errors :)

Good idea: done.

Comment 12

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/6c96e106f24d
https://hg.mozilla.org/integration/fx-team/rev/e80ad43fbb49
https://hg.mozilla.org/integration/fx-team/rev/6fed2baa96c9
(Assignee)

Comment 13

2 years ago
sebastian: I landed this all r=markh 'cuz I expect to be working on this code (with your reviews) for the next several weeks.  We'll have plenty of follow-ups to address issues and share knowledge.
https://hg.mozilla.org/mozilla-central/rev/6c96e106f24d
https://hg.mozilla.org/mozilla-central/rev/e80ad43fbb49
https://hg.mozilla.org/mozilla-central/rev/6fed2baa96c9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(Assignee)

Updated

2 years ago
Depends on: 1205334
Attachment #8660942 - Flags: review?(s.kaspari) → review+
Comment on attachment 8660942 [details]
MozReview Request: Bug 1191064 - Part 1: Add Fennec version of FxAccountsWebChannel. r?sebastian,markh

https://reviewboard.mozilla.org/r/19223/#review17519
https://reviewboard.mozilla.org/r/19223/#review17521

::: mobile/android/modules/FxAccountsWebChannel.jsm:71
(Diff revision 1)
> +  /**

NIT: Empty line?
Comment on attachment 8660943 [details]
MozReview Request: Bug 1191064 - Part 2: Handle relinking Firefox Accounts on Fennec. r?sebastian,markh

https://reviewboard.mozilla.org/r/19225/#review17523
Attachment #8660943 - Flags: review?(s.kaspari) → review+
Attachment #8660944 - Flags: review?(s.kaspari) → review+
Comment on attachment 8660944 [details]
MozReview Request: Bug 1191064 - Part 3: Handle removing Android Accounts from fxa-content-server. r?sebastian,markh

https://reviewboard.mozilla.org/r/19227/#review17525
(Assignee)

Updated

2 years ago
Depends on: 1191068
(Assignee)

Updated

2 years ago
No longer blocks: 1204930
Depends on: 1204930
(Assignee)

Updated

2 years ago
Depends on: 1206086

Comment 19

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/eb238855c2a0
https://hg.mozilla.org/integration/fx-team/rev/4e9542200eb1
https://hg.mozilla.org/mozilla-central/rev/eb238855c2a0
https://hg.mozilla.org/mozilla-central/rev/4e9542200eb1
You need to log in before you can comment on or make changes to this bug.