Closed
Bug 1191064
Opened 9 years ago
Closed 9 years ago
Implement WebChannel interface to fxa-content-server in Fennec
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox43 fixed)
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: nalexander, Assigned: nalexander)
References
Details
Attachments
(3 files)
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•9 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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
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•9 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•9 years ago
|
||
Adding a sprinkling of FxA folks who might be interested or good security checks.
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8660944 -
Flags: review?(markh) → review+
Comment 9•9 years ago
|
||
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•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Component: Firefox Accounts → General
Product: Android Background Services → Firefox for Android
Assignee | ||
Comment 10•9 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•9 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•9 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•9 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
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•9 years ago
|
Attachment #8660942 -
Flags: review?(s.kaspari) → review+
Comment 15•9 years ago
|
||
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•9 years ago
|
||
https://reviewboard.mozilla.org/r/19223/#review17521 ::: mobile/android/modules/FxAccountsWebChannel.jsm:71 (Diff revision 1) > + /** NIT: Empty line?
Comment 17•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8660944 -
Flags: review?(s.kaspari) → review+
Comment 18•9 years ago
|
||
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•9 years ago
|
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/eb238855c2a0 https://hg.mozilla.org/integration/fx-team/rev/4e9542200eb1
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eb238855c2a0 https://hg.mozilla.org/mozilla-central/rev/4e9542200eb1
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•