1369834 - Add a Firefox Accounts Verify Login Push Notification

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, enhancement)

Description

[Sean McArthur [:seanmonstar]]

We want to add the ability for already signed-in Firefoxes to be usable in a 2FA fashion for signing in to new devices, as described in this document: https://docs.google.com/document/u/1/d/16fdARc6MC9XO7FBG9YJ4bpeqhHcv1G3ua0uzeS4EckY/edit

To get the verify login (signin confirmation) push notifications working in the browser, here's a breakdown of the work to do:

- We want to add a new `case ON_VERIFY_LOGIN_NOTIFICATION` to this switch: https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsPush.js#166
- We need a new constant, `ON_VERIFY_LOGIN_NOTIFICATION`, with the text `"fxaccounts:verify_login"`, taken from the feature document. FxA constants live in here: https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccountsCommon.js#98
- There should be a new method that is called from inside the `case ON_VERIFY_LOGIN_NOTIFICATION` statement, just like the other cases, such as `this._onVerifyLoginNotification(payload.data)`.
- In this new `_onVerifyLoginNotification` method, `AlertsService` should be used to show a notification, using content from the payload. You can see an example of this in https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#2309
  - You can import the `AlertsService` like this: https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#18
  - The notification should have a click callback to open a URL that was also sent in the payload data.
- Add tests! Something like https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/tests/browser/browser_device_connected.js, but for browser_verify_login.

When you have code and browser built using it, you can open the Browser Console (be sure to enable Chrome Debugging in DevTools), and use this to simulate notifications: https://gist.github.com/seanmonstar/70637ef4c3c174373dc463f1b9b938b3

This only describes the work needed for Desktop. I'll find the needed work for Android as well.

Comment 1

[Princi Vershwal]

Attachment: Patch for Bug 1369834.

Comment 2

[Edouard Oger [:eoger]]

Comment on attachment 8880558 [details] [diff] [review]
Patch for Bug 1369834.

Review of attachment 8880558 [details] [diff] [review]:
-----------------------------------------------------------------

This needs work.
Don't forget to run |./mach eslint| before you submit a patch for review.

::: browser/components/nsBrowserGlue.js
@@ +2291,5 @@
> +  _onVerifyLoginNotification(payloadData) {
> +    let tab;
> +    let url = payloadData.url;
> +    let imageURL;
> +      imageURL = "chrome://branding/content/icon64.png";

Why separate the declaration and the initialization? On macOS the icon is actually not needed, only Windows needs to display it

@@ +2295,5 @@
> +      imageURL = "chrome://branding/content/icon64.png";
> +    let clickCallback = async (subject, topic, data) => {
> +      if (topic != "alertclickcallback")
> +        return;
> +      let win = RecentWindow.getMostRecentBrowserWindow({private: false});

You should set win in openTab() instead of retrieving the Window object again

@@ +2300,5 @@
> +      const focusTab = async (url) => {
> +        tab.setAttribute("attention", true);
> +        return tab;
> +      };
> +      if (topic == "alertclickcallback") {

You are testing topic twice in that function.

@@ +2301,5 @@
> +        tab.setAttribute("attention", true);
> +        return tab;
> +      };
> +      if (topic == "alertclickcallback") {
> +        const firstTab = await focusTab(url);

I don't think you need a function focusTab here, also firstTab === tab, there's no need to create another variable.

@@ +2309,5 @@
> +
> +    function openTab() {
> +      let win = RecentWindow.getMostRecentBrowserWindow({private: false});
> +      if (!win) {
> +        win = this._openURLInNewWindow(url);

You need to await on the promise _openURLInNewWindow returns

@@ +2318,5 @@
> +      }
> +    };
> +
> +    try {
> +      AlertsService.showAlertNotification(imageURL, payloadData.title, payloadData.body, true, null, clickCallback);

Should be done once the tab is actually shown otherwise the click callback will be prone to race conditions (what if you click on the notification and the tab isn't there yet?).

@@ +2319,5 @@
> +    };
> +
> +    try {
> +      AlertsService.showAlertNotification(imageURL, payloadData.title, payloadData.body, true, null, clickCallback);
> +      openTab();

This function should be inlined.

::: services/fxaccounts/FxAccountsPush.js
@@ +10,4 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://services-sync/util.js");
>  Cu.import("resource://gre/modules/FxAccountsCommon.js");
> +XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");

This import is not needed here.

@@ +245,4 @@
>          });
>      });
>    },
> +

Un-need newlines

@@ +250,5 @@
>  };
>  
>  // Service registration below registers with FxAccountsComponents.manifest
>  const components = [FxAccountsPushService];
> +this.NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
\ No newline at end of file

This line doesn't need to be changed (newline?)

::: services/fxaccounts/tests/browser/browser_verify_login.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const { MockRegistrar } =

Un-used import

@@ +10,5 @@
> +
> +  info("testVerifyNewSignin");
> +  gBrowser.selectedBrowser.loadURI("about:robots");
> +  await waitForDocLoadComplete();
> +  

Trailing spaces

@@ +29,5 @@
> +
> +add_task(async function() {
> +  let msg = {
> +        data: {
> +          deviceName: 'Laptop', 

Trailing spaces here and bellow

@@ +31,5 @@
> +  let msg = {
> +        data: {
> +          deviceName: 'Laptop', 
> +          url: 'http://localhost/newLogin', 
> +          title:'Sign-in Request',

":" without spaces, this will not pass eslint.

@@ +35,5 @@
> +          title:'Sign-in Request',
> +          body:'New sign-in request on Intel Mac OS X 10.12'
> +        }
> +      };
> +  await testVerifyNewSignin(JSON.stringify(msg.data));

Just inline that function if you're using it once

::: services/fxaccounts/tests/browser/head.js
@@ +35,4 @@
>    });
>  }
>  
> +const alertCallbacks = []

I don't feel really confident about this way of mocking alerts service, let's revisit that on the 2nd review.

@@ +54,5 @@
> +  if (!alertsServiceCID) {
> +    alertsServiceCID = MockRegistrar.register("@mozilla.org/alerts-service;1", alertsService);
> +    registerCleanupFunction(() => {
> +      MockRegistrar.unregister(alertsServiceCID);
> +      while (alertCallbacks.length) {

Why are the callbacks called in the cleanup function?

@@ +65,4 @@
>  // Keep a set of progress listeners for waitForDocLoadComplete() to make sure
>  // they're not GC'ed before we saw the page load.
>  waitForDocLoadComplete.listeners = new Set();
> +registerCleanupFunction(() => waitForDocLoadComplete.listeners.clear());
\ No newline at end of file

This line doesn't need to be changed either (newline at the end of the file maybe)

::: services/fxaccounts/tests/xpcshell/test_push_service.js
@@ +347,4 @@
>    });
>  
>    pushService.observe(null, mockPushService.subscriptionChangeTopic, FXA_PUSH_SCOPE_ACCOUNT_UPDATE);
> +});
\ No newline at end of file

No need for change here

Comment 3

[Sean McArthur [:seanmonstar]]

Comment on attachment 8880558 [details] [diff] [review]
Patch for Bug 1369834.

Review of attachment 8880558 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +2291,5 @@
> +  _onVerifyLoginNotification(payloadData) {
> +    let tab;
> +    let url = payloadData.url;
> +    let imageURL;
> +      imageURL = "chrome://branding/content/icon64.png";

Just a drive-by note: it would seem that determining whether to show the `imageURL` could be handled once in `showAlertNotification`, and then not have if checks everywhere else. It would also mean that if macOS every gained the ability to show an image, all those cases would just work.

@@ +2318,5 @@
> +      }
> +    };
> +
> +    try {
> +      AlertsService.showAlertNotification(imageURL, payloadData.title, payloadData.body, true, null, clickCallback);

Is there anything to worry about with regards to the tab not loading, being blocked by something, or otherwise keeping us from alerting the user? Or is that situation just a "oh well, too bad"?

::: services/fxaccounts/tests/browser/head.js
@@ +35,4 @@
>    });
>  }
>  
> +const alertCallbacks = []

Agreed, this is hacky. The problem is a full solution would probably need a lot more work. Consider:

The browser_device_connected test defines an `AlertService`, and then `nsBrowserGlue` loads it into its lazy constant. Once this test is over, even though the test cleanup function has de-registered the `AlertService`, the existing `nsBrowserGlue` instance has it stuck in its lazy constant, and won't get a new one from the browser_verify_login test.

A "proper fix" would probably require telling the `nsBrowserGlue` to evict or invalidate its `AlertsService` constant, so it can look up the new one.

@@ +54,5 @@
> +  if (!alertsServiceCID) {
> +    alertsServiceCID = MockRegistrar.register("@mozilla.org/alerts-service;1", alertsService);
> +    registerCleanupFunction(() => {
> +      MockRegistrar.unregister(alertsServiceCID);
> +      while (alertCallbacks.length) {

They aren't called, they're just cleaned up, so they don't pollute other tests.

Comment 4

[Edouard Oger [:eoger]]

> Just a drive-by note: it would seem that determining whether to show the `imageURL` could be handled once in `showAlertNotification`, and then not have if checks everywhere else. It would also mean that if macOS every gained the ability to show an image, all those cases would just work.

So a little bit of context about notifications: in Windows we don't use native notifications [0]. We use our own XUL-crafted notifications instead.
In this implementation, contrary to macOS, no icon is shown on the side of the notification [1]. This is why we add it ourselves (I agree it should be shown by default, but it's probably not our problem :) ).

[0] https://i.stack.imgur.com/O7JA4.png
[1] http://imgur.com/a/2f8Bk

Comment 5

[Princi Vershwal]

Attachment: pushnotification2.patch

eoger: I have made the changes according to your review. Please have a look.

Comment 6

[Edouard Oger [:eoger]]

Do you mind merging the two patches? It's a bit hard to review this diff.

Comment 7

[Princi Vershwal]

Attachment: pushnotification.patch

Merged. Please check

Comment 8

[Edouard Oger [:eoger]]

Comment on attachment 8882049 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882049 [details] [diff] [review]:
-----------------------------------------------------------------

This still needs a little bit more work but it is definitely on the right track.
The alerts service mocking story is way less complex now which I appreciate, thanks.

::: browser/components/nsBrowserGlue.js
@@ -6,4 @@
>  const Cc = Components.classes;
>  const Cr = Components.results;
>  const Cu = Components.utils;
> -

Un-needed change

@@ +256,4 @@
>      });
>  
>    XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts", "resource://gre/modules/FxAccounts.jsm");
> +  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");

No need to move that import

@@ +1446,4 @@
>      try {
>        // This will throw NS_ERROR_NOT_AVAILABLE if the notification cannot
>        // be displayed per the idl.
> +      this.AlertsService.showAlertNotification(null, title, text,

Un-needed indentation change

@@ +1712,4 @@
>          return;
>        this._openPreferences("sync", { origin: "doorhanger" });
>      }
> +    this.AlertsService.showAlertNotification(null, title, body, true, null, clickCallback);

Ditto

@@ +2286,4 @@
>        if (AppConstants.platform == "win") {
>          imageURL = "chrome://branding/content/icon64.png";
>        }
> +      this.AlertsService.showAlertNotification(imageURL, title, body, true, null, clickCallback);

Ditto

@@ +2290,5 @@
>      } catch (ex) {
>        Cu.reportError("Error displaying tab(s) received by Sync: " + ex);
>      }
>    },
> +  async _onVerifyLoginNotification(payloadData) {

- How about {body, title, url} instead of payloadData.
  => See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment
     "Unpacking fields from objects passed as function parameter"
- Add a newline before to be consistent with the other methods.

@@ +2305,5 @@
> +      tab = tabs[tabs.length - 1];
> +    } else {
> +      tab = win.gBrowser.addTab(url);
> +    }
> +    let clickCallback = async (subject, topic, data) => {

async is not needed here.

@@ +2308,5 @@
> +    }
> +    let clickCallback = async (subject, topic, data) => {
> +      if (topic != "alertclickcallback")
> +        return;
> +      tab.setAttribute("attention", true);

I don't think this is going to work if you're setting that tab as the active a line later.
You should probably drop this statement.

@@ +2315,5 @@
> +
> +    try {
> +      this.AlertsService.showAlertNotification(imageURL, payloadData.title, payloadData.body, true, null, clickCallback);
> +    } catch (ex) {
> +      Cu.reportError("Error notifying signing in to a new devices:: " + ex);

"Error notifying of a verify login event: " (Or something similar)

@@ +2342,4 @@
>      };
>  
>      try {
> +      this.AlertsService.showAlertNotification(null, title, body, true, null, clickCallback);

Ditto

@@ +2358,4 @@
>          return;
>        this._openPreferences("sync", { origin: "devDisconnectedAlert"});
>      }
> +    this.AlertsService.showAlertNotification(null, title, body, true, null, clickCallback);

Ditto

::: services/fxaccounts/tests/browser/browser_verify_login.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Most of the comments I'm making on that file also apply to browser_device_connected.js

@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +const { MockRegistrar } =

This is not used in this new test.

@@ +8,5 @@
> +const gBrowserGlue = Cc["@mozilla.org/browser/browserglue;1"]
> +                     .getService(Ci.nsIObserver);
> +
> +add_task(async function setup() {
> +  const alertsService = {

I wonder if this shouldn't be factored in a head*.js file, since it is used in two tests.

@@ +9,5 @@
> +                     .getService(Ci.nsIObserver);
> +
> +add_task(async function setup() {
> +  const alertsService = {
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAlertsService, Ci.nsISupports]),

I don't think this is needed.

@@ +11,5 @@
> +add_task(async function setup() {
> +  const alertsService = {
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAlertsService, Ci.nsISupports]),
> +    showAlertNotification: (image, title, text, clickable, cookie, clickCallback) => {
> +      // We can't simulate a click on the alert popup,

This comment is a bit off now, we should probably just say that we are invoking the event handler ourselves directly.

@@ +17,5 @@
> +      // We are using this as a function instead of observe.
> +      clickCallback(null, "alertclickcallback", null);
> +    }
> +  };
> +  let inst = alertsService.QueryInterface(Ci.nsIAlertsService);

Nitpick: let's call this alertsService, or alertsServiceMock or something more descriptive than |inst| (same in browser_verify_login.js).

@@ +22,5 @@
> +  gBrowserGlue.observe({wrappedJSObject: inst}, "browser-glue-test", "mock-alerts-service");
> +});
> +
> +add_task(async function() {
> +  let msg = {

The indentation looks wrong.

@@ +41,5 @@
> +  Services.obs.notifyObservers(null, "fxaccounts:verify_login", payloadData);
> +
> +  let tab = await waitForTabPromise;
> +  Assert.ok("Tab successfully opened");
> +  Assert.equal(tab.linkedBrowser.currentURI.spec, JSON.parse(payloadData).url);

Just use msg.data.url directly

::: services/fxaccounts/tests/xpcshell/test_push_service.js
@@ +231,5 @@
>    pushService.observe(msg, mockPushService.pushTopic, FXA_PUSH_SCOPE_ACCOUNT_UPDATE);
>  });
>  
> +add_test(function observePushTopicVerifyLogin() {
> +  var url, title, body;

No "var" please.
Also these should be initialized.

@@ +248,5 @@
> +      return this;
> +    }
> +  };
> +  let obs = (subject, topic, data) => {
> +    Services.obs.removeObserver(obs, topic);

You should probably test |data| here.

Comment 9

[Sean McArthur [:seanmonstar]]

Comment on attachment 8882049 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882049 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +256,4 @@
>      });
>  
>    XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts", "resource://gre/modules/FxAccounts.jsm");
> +  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");

What do you mean with here? This changed from being a module constant to a property on the `BrowserGlue` instance. Perhaps it should be camel case instead, to be more apparent? `this.alertsService`?

@@ +1446,4 @@
>      try {
>        // This will throw NS_ERROR_NOT_AVAILABLE if the notification cannot
>        // be displayed per the idl.
> +      this.AlertsService.showAlertNotification(null, title, text,

I don't see indentation changed. A `this` was added, to access the property of the `BrowserGlue`, instead of a module constant.

Comment 10

[Edouard Oger [:eoger]]

Comment on attachment 8882049 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882049 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +256,4 @@
>      });
>  
>    XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts", "resource://gre/modules/FxAccounts.jsm");
> +  XPCOMUtils.defineLazyServiceGetter(this, "AlertsService", "@mozilla.org/alerts-service;1", "nsIAlertsService");

Sorry, the lack of context and indentation on splinter review threw me off.
Disregard my first comment.

@@ +1446,4 @@
>      try {
>        // This will throw NS_ERROR_NOT_AVAILABLE if the notification cannot
>        // be displayed per the idl.
> +      this.AlertsService.showAlertNotification(null, title, text,

My bad I'm a bit tired. Carry on.

Comment 11

[Sean McArthur [:seanmonstar]]

Comment on attachment 8882049 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882049 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/tests/browser/browser_verify_login.js
@@ +9,5 @@
> +                     .getService(Ci.nsIObserver);
> +
> +add_task(async function setup() {
> +  const alertsService = {
> +    QueryInterface: XPCOMUtils.generateQI([Ci.nsIAlertsService, Ci.nsISupports]),

True, since XPCOM isn't actually converting this. The comment about change `let inst = ...` makes me think also that since this is just sending a plain old JS object, the `inst` line can be removed as well. It was written as an attempt to get the XPCOM machinery to work, but we couldn't do it, so the tests just assume the value is a function instead of a `nsIObserve` instance.

Comment 12

[Princi Vershwal]

Attachment: pushnotification.patch

Thanks for your reviews eoger. Made the changes you suggested. Please see.

Comment 13

[Edouard Oger [:eoger]]

Comment on attachment 8882395 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882395 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/tests/browser/head.js
@@ +39,4 @@
>    });
>  }
>  
> +add_task(async function setup() {

Let's no register that mock for every test in the suite. I would like instead to see a method like "setupMockAlertsService()"

Comment 14

[Princi Vershwal]

Attachment: pushnotification.patch

Done.

Comment 15

[Edouard Oger [:eoger]]

Comment on attachment 8882407 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882407 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/fxaccounts/tests/browser/head.js
@@ +39,4 @@
>    });
>  }
>  
> +/*add_task(async function setup() {

You probably wanted to remove that instead of commenting it.

Comment 16

[Princi Vershwal]

Attachment: pushnotification.patch

Sorry about that.

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8882422 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882422 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comment, looks good to me. Thanks.

::: services/fxaccounts/tests/browser/head.js
@@ +7,5 @@
>   *
>   * @return promise
>   */
> +
> +const gBrowserGlue = Cc["@mozilla.org/browser/browserglue;1"]

This is going to pollute the global namespace, put that inside setupMockAlertsService()

Comment 18

[Princi Vershwal]

Attachment: pushnotification.patch

Hey eoger!! Will this one work?

Comment 19

[Edouard Oger [:eoger]]

Comment on attachment 8882623 [details] [diff] [review]
pushnotification.patch

Review of attachment 8882623 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

Comment 20

[Princi Vershwal]

Thanks eoger!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e31dd5df8ba
Add Firefox Accounts Verify Login Push Notification. r=eoger

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7e31dd5df8ba