Log out of Sync on FxA remote disconnection

RESOLVED FIXED in Firefox 50

Status

()

Firefox
Sync
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: eoger, Assigned: eoger)

Tracking

unspecified
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

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

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Ryan, do we want to disconnect silently the clients or do we show some kind of UI feedback?
Flags: needinfo?(rfeeley)
Created attachment 8759347 [details]
notifications.png

Concise disconnection notification.
Flags: needinfo?(rfeeley)
(Assignee)

Comment 3

2 years ago
Created attachment 8759429 [details]
Bug 1277026 - Handle FxA push payloads.

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

2 years ago
Created attachment 8759430 [details]
Bug 1277026 - Allow FxAccounts#getDeviceId callers to get only the cached deviceId.

Review commit: https://reviewboard.mozilla.org/r/57410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57410/
(Assignee)

Comment 5

2 years ago
Created attachment 8759431 [details]
Bug 1277026 - Disconnect Sync and show a notification on FxA remote disconnect.

Review commit: https://reviewboard.mozilla.org/r/57412/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57412/
(Assignee)

Comment 6

2 years ago
You can try these patches against fxa-latest with https://bug1098694.bmoattachments.org/attachment.cgi?id=8759426
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

2 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

2 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

2 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 12

2 years ago
Rebased against fx-team and addressed Kit comments
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

2 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

2 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

2 years ago
Attachment #8759430 - Attachment is obsolete: true
Attachment #8759430 - Flags: review?(markh)

Updated

2 years ago
Attachment #8759431 - Flags: review?(markh) → review+
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

2 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

2 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

2 years ago
Keywords: checkin-needed

Comment 19

2 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
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?
(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

2 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

2 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/
(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

2 years ago
Keywords: checkin-needed

Comment 26

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/33d0a9cad453
https://hg.mozilla.org/mozilla-central/rev/fce43f1e8289
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.