Closed
Bug 1277026
Opened 8 years ago
Closed 8 years ago
Log out of Sync on FxA remote disconnection
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: eoger, Assigned: eoger)
Details
Attachments
(3 files, 1 obsolete file)
This is the Desktop part of https://github.com/mozilla/fxa-auth-server/issues/1139 The FxA Device Manager add the possibility of disconnecting a device remotely. We send a push notification from the server to inform devices to disconnect themselves. The server part is merged and can send these push notifications, we now need to handle them on the client.
Assignee | ||
Comment 1•8 years ago
|
||
Ryan, do we want to disconnect silently the clients or do we show some kind of UI feedback?
Flags: needinfo?(rfeeley)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57408/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57408/
Attachment #8759429 -
Flags: review?(kcambridge)
Attachment #8759430 -
Flags: review?(markh)
Attachment #8759431 -
Flags: review?(markh)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57410/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57410/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57412/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/57412/
Assignee | ||
Comment 6•8 years ago
|
||
You can try these patches against fxa-latest with https://bug1098694.bmoattachments.org/attachment.cgi?id=8759426
Comment 7•8 years ago
|
||
Comment on attachment 8759429 [details] Bug 1277026 - Handle FxA push payloads. https://reviewboard.mozilla.org/r/57408/#review54268 Looks good, Edouard! ::: services/fxaccounts/FxAccountsPush.js:148 (Diff revision 1) > * > * @private > */ > - _onPushMessage() { > + _onPushMessage(message) { > this.log.trace("FxAccountsPushService _onPushMessage"); > - // Use this signal to check the verification state of the account right away > + if (message.data) { Style nit: If you change the condition to `if (!message.data)` and return early, you can remove a level of indentation for the switch. ::: services/fxaccounts/tests/xpcshell/test_push_service.js (Diff revision 1) > > pushService.observe(null, mockPushService.subscriptionChangeTopic, FXA_PUSH_SCOPE_ACCOUNT_UPDATE); > }); > - > - > -function run_test() { Why this change?
Attachment #8759429 -
Flags: review?(kcambridge) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8759429 [details] Bug 1277026 - Handle FxA push payloads. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57408/diff/1-2/
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8759430 [details] Bug 1277026 - Allow FxAccounts#getDeviceId callers to get only the cached deviceId. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57410/diff/1-2/
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8759431 [details] Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57412/diff/1-2/
Assignee | ||
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/57408/#review54268 > Why this change? Removing some boilerplate code: https://bugzilla.mozilla.org/show_bug.cgi?id=982852
Assignee | ||
Comment 12•8 years ago
|
||
Rebased against fx-team and addressed Kit comments
Comment 13•8 years ago
|
||
Comment on attachment 8759431 [details] Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect. https://reviewboard.mozilla.org/r/57412/#review54686 Sorry for the delay. I think we should structure this a little differently. FxAccountsPush is already tightly bound with FxAccounts.jsm, so I think it should arange to disconnect directly and send the observer notification after disconnection - nsBrowserGlue then only has the job of showing the notification, not doing the actual disconnection. ie, something like: * new public method handleDeviceDisconnection(deviceId) - this can then cheat and look for the deviceID directly (ie, without the need to change getDeviceID()) - but if you really do want to reuse getDeviceID like the first patch, you should specify the default for the new param (to make it clear we expect it to be called without any params), document what it does, and probably rename it to skipRegistration or something clearer * it checks the device ID matches, does the signout then notifies a new observer notification. * nsBrowserGlue just shows the notification, so will not need to reference fxAccounts at all. (TBH, I wouldn't even mind this being directly in FxAccountsPush and not have a new public method - I think FxAccountsPush to do everything it needs without any changes to FxAccounts.jsm - but I can see the argument that's not as clean)
Attachment #8759431 -
Flags: review?(markh)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8759429 [details] Bug 1277026 - Handle FxA push payloads. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57408/diff/2-3/
Attachment #8759431 -
Flags: review?(markh)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8759431 [details] Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57412/diff/2-3/
Assignee | ||
Updated•8 years ago
|
Attachment #8759430 -
Attachment is obsolete: true
Attachment #8759430 -
Flags: review?(markh)
Updated•8 years ago
|
Attachment #8759431 -
Flags: review?(markh) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8759431 [details] Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect. https://reviewboard.mozilla.org/r/57412/#review54980 Thanks! ::: browser/components/nsBrowserGlue.js:320 (Diff revision 3) > this._showSyncStartedDoorhanger(); > break; > case "weave:engine:clients:display-uri": > this._onDisplaySyncURI(subject); > break; > + case ON_DEVICE_DISCONNECTED_NOTIFICATION: I think that given we use the literal "fxaccounts:..." string a few lines up we should just do that here too - the name ON_DEVICE_DISCONNECTED_NOTIFICATION doesn't immediately imply it has to do with fxa, whereas the literal string does. Then move it up above "weave:engine..." so the fxaccounts ones are grouped. ::: browser/components/nsBrowserGlue.js:539 (Diff revision 3) > os.addObserver(this, "browser-lastwindow-close-granted", false); > } > os.addObserver(this, "weave:service:ready", false); > os.addObserver(this, "fxaccounts:onverified", false); > os.addObserver(this, "weave:engine:clients:display-uri", false); > + os.addObserver(this, ON_DEVICE_DISCONNECTED_NOTIFICATION, false); similarly, use the literal string and move it up a line so fxaccounts: ones are grouped. ::: browser/components/nsBrowserGlue.js:606 (Diff revision 3) > os.removeObserver(this, "browser-lastwindow-close-granted"); > } > os.removeObserver(this, "weave:service:ready"); > os.removeObserver(this, "fxaccounts:onverified"); > os.removeObserver(this, "weave:engine:clients:display-uri"); > + os.removeObserver(this, ON_DEVICE_DISCONNECTED_NOTIFICATION); ditto ::: services/fxaccounts/FxAccounts.jsm:1502 (Diff revision 3) > + .then(data => data ? data.deviceId : null) > + .then(localDeviceId => { > + if (deviceId == localDeviceId) { > + this.notifyObservers(ON_DEVICE_DISCONNECTED_NOTIFICATION, deviceId); > + return this.signOut(true); > + } else { no else after a return. ::: services/fxaccounts/FxAccountsCommon.js:92 (Diff revision 3) > exports.ONLOGIN_NOTIFICATION = "fxaccounts:onlogin"; > exports.ONVERIFIED_NOTIFICATION = "fxaccounts:onverified"; > exports.ONLOGOUT_NOTIFICATION = "fxaccounts:onlogout"; > // Internal to services/fxaccounts only > exports.ON_FXA_UPDATE_NOTIFICATION = "fxaccounts:update"; > +exports.ON_DEVICE_DISCONNECTED_NOTIFICATION = "fxaccounts:device_disconnected"; but yeah, I think it's fine to add the defn here and use it internally in the fxa code as you have done.
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8759429 [details] Bug 1277026 - Handle FxA push payloads. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57408/diff/3-4/
Assignee | ||
Comment 18•8 years ago
|
||
Comment on attachment 8759431 [details] Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57412/diff/3-4/
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
Pushed by mozilla@noorenberghe.ca: https://hg.mozilla.org/integration/fx-team/rev/0943ef016d1c Handle FxA push payloads. r=kitcambridge https://hg.mozilla.org/integration/fx-team/rev/947617c723cd Disconnect Sync and show a notification on FxA remote disconnect. r=markh
Keywords: checkin-needed
Comment 20•8 years ago
|
||
Backed out for xpcshell test failures like: https://treeherder.mozilla.org/logviewer.html#?job_id=9842432&repo=fx-team https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=947617c723cd remote: https://hg.mozilla.org/integration/fx-team/rev/d2a4d1e3d0dc remote: https://hg.mozilla.org/integration/fx-team/rev/0752098a5555
Comment 21•8 years ago
|
||
I need to ask since I've seen this tendency in the last changes: is having hard-coded "Firefox" and "Sync" in strings, instead of brandName variables, expected and wanted?
Comment 22•8 years ago
|
||
(In reply to Francesco Lodolo [:flod] from comment #21) > I need to ask since I've seen this tendency in the last changes: is having > hard-coded "Firefox" and "Sync" in strings, instead of brandName variables, > expected and wanted? Good question. There is no such thing as a Nightly Account, and when users sign in to Sync from Nightly it will be available from every Firefox. So, in this respect, I think we should hard-code "Firefox".
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8759429 [details] Bug 1277026 - Handle FxA push payloads. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57408/diff/4-5/
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8759431 [details] Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57412/diff/4-5/
Comment 25•8 years ago
|
||
(In reply to Ryan Feeley [:rfeeley] from comment #22) > Good question. There is no such thing as a Nightly Account, and when users > sign in to Sync from Nightly it will be available from every Firefox. So, in > this respect, I think we should hard-code "Firefox". > Firefox will begin syncing momentarily. To clarify, this landed in a previous bug, it's not part of this bug. > This computer has been successfully disconnected from Firefox Sync. There are branding strings for these too https://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/syncBrand.dtd Getting those entities in a .properties might be an overhead, and I'm mostly wondering if there is a clear criteria to determine when hard-coding is acceptable.
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 26•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/33d0a9cad453 Handle FxA push payloads. r=kitcambridge https://hg.mozilla.org/integration/fx-team/rev/fce43f1e8289 Disconnect Sync and show a notification on FxA remote disconnect. r=markh
Keywords: checkin-needed
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/33d0a9cad453 https://hg.mozilla.org/mozilla-central/rev/fce43f1e8289
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in
before you can comment on or make changes to this bug.
Description
•