1191064 - Implement WebChannel interface to fxa-content-server in Fennec

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nick Alexander :nalexander [he/him]]

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.

Comment 1

[Nick Alexander :nalexander [he/him]]

I've grown this ticket to encompass a complete WebChannel implementation for Fennec.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: 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.

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: 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.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: 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.

Comment 5

[Nick Alexander :nalexander [he/him]]

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!

Comment 6

[Nick Alexander :nalexander [he/him]]

Adding a sprinkling of FxA folks who might be interested or good security checks.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

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 :)

Comment 9

[Mark Hammond [:markh] [:mhammond]]

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?

Comment 10

[Nick Alexander :nalexander [he/him]]

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!

Comment 11

[Nick Alexander :nalexander [he/him]]

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

[Pulsebot]

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

Comment 13

[Nick Alexander :nalexander [he/him]]

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.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6c96e106f24d
https://hg.mozilla.org/mozilla-central/rev/e80ad43fbb49
https://hg.mozilla.org/mozilla-central/rev/6fed2baa96c9

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

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

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

NIT: Empty line?

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/eb238855c2a0
https://hg.mozilla.org/integration/fx-team/rev/4e9542200eb1

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eb238855c2a0
https://hg.mozilla.org/mozilla-central/rev/4e9542200eb1