1277026 - Log out of Sync on FxA remote disconnection

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Edouard Oger [:eoger]]

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.

Comment 1

[Edouard Oger [:eoger]]

Ryan, do we want to disconnect silently the clients or do we show some kind of UI feedback?

Comment 2

[Ryan Feeley [:rfeeley]]

Attachment: notifications.png

Concise disconnection notification.

Comment 3

[Edouard Oger [:eoger]]

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

Comment 4

[Edouard Oger [:eoger]]

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

Comment 5

[Edouard Oger [:eoger]]

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

Comment 6

[Edouard Oger [:eoger]]

You can try these patches against fxa-latest with https://bug1098694.bmoattachments.org/attachment.cgi?id=8759426

Comment 7

[Lina Butler [:lina]]

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?

Comment 8

[Edouard Oger [:eoger]]

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/

Comment 9

[Edouard Oger [:eoger]]

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/

Comment 10

[Edouard Oger [:eoger]]

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/

Comment 11

[Edouard Oger [:eoger]]

https://reviewboard.mozilla.org/r/57408/#review54268

> Why this change?

Removing some boilerplate code: https://bugzilla.mozilla.org/show_bug.cgi?id=982852

Comment 12

[Edouard Oger [:eoger]]

Rebased against fx-team and addressed Kit comments

Comment 13

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

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)

Comment 14

[Edouard Oger [:eoger]]

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/

Comment 15

[Edouard Oger [:eoger]]

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/

Comment 16

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

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.

Comment 17

[Edouard Oger [:eoger]]

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/

Comment 18

[Edouard Oger [:eoger]]

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/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Matthew N. [:MattN]]

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

[Francesco Lodolo [:flod]]

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

[Ryan Feeley [:rfeeley]]

(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".

Comment 23

[Edouard Oger [:eoger]]

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/

Comment 24

[Edouard Oger [:eoger]]

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

[Francesco Lodolo [:flod]]

(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.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/33d0a9cad453
https://hg.mozilla.org/mozilla-central/rev/fce43f1e8289