Closed Bug 1466933 Opened 6 years ago Closed 6 years ago

Implement FxA device commands

Categories

(Firefox :: Sync, enhancement, P1)

Product:
Firefox
Component:
Sync
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(1 file)

Bug 1466933 - Implement FxA commands.
59 bytes, text/x-review-board-request
rfkelly
: review+
markh
: review+
Details 
FxA commands is an alternative to FxA messages (first implemented in bug 1442133).
More info about the feature here: https://github.com/mozilla/fxa-auth-server/pull/2449
Here's a first draft of the patch.

To do:
- Verify that we re-encrypt the send tab keys if kSync changes.
- Update tests once we're happy with the new code.
- Check missed messages every once in a while (every startup or something)
Priority: -- → P1
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review258104

I left a bunch of nits, but this looks really good overall!  I'm r- for now based on the following small issues, which I think are hopefully easily addressed:

* More considerations around failure and error-handling, both during the crypto bits and when talking to the FxA service.
* Use of a higher-level encryption helper routine if we have one, rather than directly manipulating "IV" and "hmac" and friends.
* Inclusion of a "kid" field in the encrypted payload (or alternately, a story about why that's not worthwhile).
* The addition of logging to help testers ensure that the right code paths are being followed.

One final consideration is how to trigger the periodic fetching of missed commands without hammering the FxA servers.  One option, if possible, may be to check for misses messages as part of a user-initiated sync but not as part of a regular background sync.  This would help unstick users who are mashing "sync now" after a sent tab failed to show up.  But I don't think that's a blocker.

::: browser/base/content/browser-sync.js:333
(Diff revision 2)
>        devices = []; // We can still run in degraded mode.
>      }
>      const toSendMessages = [];
>      for (const client of clients) {
>        const device = devices.find(d => d.id == client.fxaDeviceId);
> -      if (device && fxAccounts.messages.canReceiveSendTabMessages(device)) {
> +      if (device && fxAccounts.commands.sendTab.isDeviceCompatible(device)) {

Similar to my previous comment, it will be important for testers to be able to tell whether a tab was send via the new mechanism or the old.  Can we add a log message here indicating which branch was taking, so that it can be checked as part of the test plan?

::: services/fxaccounts/FxAccounts.jsm:1055
(Diff revision 2)
>      // We are now ready for business. This should only be invoked once
>      // per setSignedInUser(), regardless of whether we've rebooted since
>      // setSignedInUser() was called.
>      await this.notifyObservers(ONVERIFIED_NOTIFICATION);
> +    // Re-trigger the device registration to add the "send tab" available command.
> +    await this.updateDeviceRegistration();

To make sure I understand why this is necessary...

You make an initial device registration as soon as the user signs in, in order to receive push notifications of verification etc.  But you can't include the send-tab command in that one because you don't yet have the keys.  So once you have the keys, you update it to include the send-tab command.  Accurate?

::: services/fxaccounts/FxAccountsClient.jsm:488
(Diff revision 2)
>     *         Resolves to the request's response, (which should be an empty object)
>     */
> -  sendMessage(sessionTokenHex, topic, to, data) {
> +  invokeCommand(sessionTokenHex, command, target, payload) {
>      const body = {
> -      topic,
> -      to,
> +      command,
> +      target,

Our of curiosity, how do you feel about the naming conventions in this API?  E.g. would "targetDevice" or similar be more useful than just "target"?

::: services/fxaccounts/FxAccountsCommands.js:44
(Diff revision 2)
> +    const client = this._fxAccounts.getAccountsClient();
> +    try {
> +      await client.invokeCommand(sessionToken, command, device.id, payload);
> +      log.info(`Payload sent to device ${device.id}.`);
> +    } catch (e) {
> +      log.error(`Could not send data to device ${device.id}.`, e);

This could fail for at least two reasons:

* The device no longer supports the "open-uri" command, e.g. it deactivated it since you last checked.
* The pushbox feature has been preffed off on the backend, and you got a "feature not enabled" error.

Is there something we could do in response to those issues, e.g. fall back to sending the tab via the old mechanism?

IIUC, with the current implementation, a failure here will be logged and ignored, and the user won't see any feedback that their send-tab attempt failed.

I can also imagine future command implementations which might want to know about and respond to errors.  Perhaps it should be re-thrown here so that calling code can take whatever action is appropriate.

::: services/fxaccounts/FxAccountsCommands.js:83
(Diff revision 2)
> +        throw new Error("No device registration.");
> +      }
> +      const handledCommands = device.handledCommands || [];
> +      const lastCommandIndex = device.lastCommandIndex || 0;
> +      const {index, messages} = this._fetchRemoteCommands(lastCommandIndex);
> +      const missedMessages = messages.filter(m => !handledCommands.includes(m.index));

If `handledCommands` and `messages` were both in sorted order, it would be possible to do a more efficient filter here than the nested loop implied by  `filter` and `includes`.  But perhaps that's not really worth worrying about if the length of `handledCommands` will always be small?

::: services/fxaccounts/FxAccountsCommands.js:85
(Diff revision 2)
> +      const handledCommands = device.handledCommands || [];
> +      const lastCommandIndex = device.lastCommandIndex || 0;
> +      const {index, messages} = this._fetchRemoteCommands(lastCommandIndex);
> +      const missedMessages = messages.filter(m => !handledCommands.includes(m.index));
> +      await updateUserData({
> +        device: {...device, lastCommandIndex: index, handledCommands: []}

Aha, I see, we have to ensure this gets called regularly to keep `handledCommands` from growing without bound.

Perhaps, when adding an item to `handledCommands`, you could check if its length has grown beyond some reasonable threshold, and call `fetchMissedRemoteCommands` if so.  That would give us more confidence that `handledCommands` will not grow too large.

::: services/fxaccounts/FxAccountsCommands.js:111
(Diff revision 2)
> +      opts.limit = limit;
> +    }
> +    return client.getCommands(sessionToken, opts);
> +  }
> +
> +  async _handleCommand(messages) {

Naming nit: this should be "handleCommands" if it accepts a list of messages.

::: services/fxaccounts/FxAccountsCommands.js:117
(Diff revision 2)
> +    const fxaDevices = await this._fxAccounts.getDeviceList();
> +    for (const {data} of messages) {
> +      let {command, payload, sender} = data;
> +      if (sender) {
> +        sender = fxaDevices.find(d => d.id == sender);
> +      }

Again, let me know if you have any feedback on the naming of things in this API, before we commit to it on either client or server.

::: services/fxaccounts/FxAccountsCommands.js:149
(Diff revision 2)
> +    };
> +    const bytes = encoder.encode(JSON.stringify(data));
> +    for (let device of to) {
> +      const encrypted = await this._encrypt(bytes, device);
> +      const payload = {encrypted};
> +      await this._commands.send(COMMAND_SENDTAB, device, payload); // FxA needs an object.

> // FxA needs an object.

It's not too late to change that if it's annoying for you here, would just a string be better?

::: services/fxaccounts/FxAccountsCommands.js:154
(Diff revision 2)
> +      await this._commands.send(COMMAND_SENDTAB, device, payload); // FxA needs an object.
> +    }
> +  }
> +
> +  isDeviceCompatible(device) {
> +    return device.availableCommands && device.availableCommands[COMMAND_SENDTAB];

If the command metadata included a "kid" field, you could do a quick check here to determine whether you can actually read the other device's keys.

::: services/fxaccounts/FxAccountsCommands.js:171
(Diff revision 2)
> +    const tabSender = {
> +      id: sender ? sender.id : "",
> +      name: sender ? sender.name : ""
> +    };
> +    const {title, url: uri} = data.entries[current];
> +    Observers.notify("fxaccounts:messages:display-tabs", [{uri, title, tabSender}]);

The name "fxaccounts:messages:display-tabs" seems like a hold-over from the previous implementation, should it now be somthing like "fxaccounts:commands:display-tabs" or similar?

::: services/fxaccounts/FxAccountsCommands.js:172
(Diff revision 2)
> +      id: sender ? sender.id : "",
> +      name: sender ? sender.name : ""
> +    };
> +    const {title, url: uri} = data.entries[current];
> +    Observers.notify("fxaccounts:messages:display-tabs", [{uri, title, tabSender}]);
> +  }

For QA testing purposes, it will be important that we can distinguish between tabs received via the old mechanism and tabs received via the new.

Could we please add some sort of success log message here indicating that the tab was received via device commands infra?  Then in our test plan, we can have a step where the tester views sync logs to confirm that the tab arrived via the expected mechanism.

::: services/fxaccounts/FxAccountsCommands.js:184
(Diff revision 2)
> +    const {IV, hmac, ciphertext} = JSON.parse(bundle);
> +    const {kSync} = await this._fxAccounts.getKeys();
> +    const syncKeyBundle = BulkKeyBundle.fromHexKey(kSync);
> +    if (this._ciphertextHMAC(syncKeyBundle, ciphertext) != hmac) {
> +      throw new Error("HMAC mismatch!");
> +    }

I'm surprised we don't have a helper function for doing the combination of:

* Extract IV, hmac, ciphertext from payload
* Check hmac
* Decrypt payload

If we really don't have one, it's probably worth adding one in `Weave.Crypto`, otherwise we risk diverging from how it's done in other places.  For example, I know we have a bug on file about using a constant-time comparison in the HMAC check, which is not done here.

::: services/fxaccounts/FxAccountsCommands.js:191
(Diff revision 2)
> +    let {publicKey, authSecret} = JSON.parse(cleartext);
> +    authSecret = urlsafeBase64Decode(authSecret);
> +    publicKey = urlsafeBase64Decode(publicKey);
> +
> +    const {ciphertext: encrypted} = await PushCrypto.encrypt(bytes, publicKey, authSecret);
> +    return urlsafeBase64Encode(encrypted);

What happens if this throws, for example because:

* kSync changed, but one or the other of the devices are still using the old value?
* a buggy client put bad JSON in its open-uri commmand metadata?

::: services/fxaccounts/FxAccountsCommands.js:251
(Diff revision 2)
> +    const ciphertext = await Weave.Crypto.encrypt(JSON.stringify(keyToEncrypt), keyBundle.encryptionKeyB64, IV);
> +    const hmac = this._ciphertextHMAC(keyBundle, ciphertext);
> +    return JSON.stringify({
> +      IV,
> +      hmac,
> +      ciphertext

Since the lifetime of this data is not managed by tokenserver, and hence not cleared on password reset, I suggest including a "kid" field in this bundle to indicate which key was used to encrypt it.  This will make it easier for other clients to detect key mis-matches without relying on a try-it-and-see-if-it-failed approach.

For bonus points you could calculate the "kid" in the same way we do for oauth scoped-keys flow:

  https://github.com/mozilla/fxa-crypto-relier/blob/master/src/deriver/ScopedKeys.js#L137

Although I'm not sure if you know the keyRotationTimestamp (aka "generation number") in the front-end code here.

::: services/fxaccounts/FxAccountsCommands.js:261
(Diff revision 2)
> +    let hasher = keyBundle.sha256HMACHasher;
> +    if (!hasher) {
> +      throw new Error("Cannot compute HMAC without an HMAC key.");
> +    }
> +    return CommonUtils.bytesAsHex(Utils.digestBytes(ciphertext, hasher));
> +  }

Again, this feels like it's too low-level and we should be using some shared code for this from `Weave.Crypto`.

::: services/fxaccounts/FxAccountsPush.js:166
(Diff revision 2)
>      this.log.debug(`push command: ${payload.command}`);
>      switch (payload.command) {
> +      case COMMAND_RECEIVED:
> +        const url = new URL(payload.data.url);
> +        const index = url.searchParams.get("index");
> +        this.fxAccounts.commands.consumeRemoteCommand(index);

This makes me think we should just tell you the index, rather than forcing you to parse it out of a URL.  Thoughts?
Attachment #8983555 - Flags: review?(rfkelly) → review-
> Similar to my previous comment,

Lol sorry, I forgot these would appear in code order, rather than in the order I created them...
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review258104

> To make sure I understand why this is necessary...
> 
> You make an initial device registration as soon as the user signs in, in order to receive push notifications of verification etc.  But you can't include the send-tab command in that one because you don't yet have the keys.  So once you have the keys, you update it to include the send-tab command.  Accurate?

Exact, I'd be easier to understand if we had a state machine on desktop but this what's happening :)

> Our of curiosity, how do you feel about the naming conventions in this API?  E.g. would "targetDevice" or similar be more useful than just "target"?

I'm pretty happy with the naming conventions so far, nothing to report on my side

> If `handledCommands` and `messages` were both in sorted order, it would be possible to do a more efficient filter here than the nested loop implied by  `filter` and `includes`.  But perhaps that's not really worth worrying about if the length of `handledCommands` will always be small?

Yup I don't except more than 10 unhandled commands here.

> Aha, I see, we have to ensure this gets called regularly to keep `handledCommands` from growing without bound.
> 
> Perhaps, when adding an item to `handledCommands`, you could check if its length has grown beyond some reasonable threshold, and call `fetchMissedRemoteCommands` if so.  That would give us more confidence that `handledCommands` will not grow too large.

Smart!

> > // FxA needs an object.
> 
> It's not too late to change that if it's annoying for you here, would just a string be better?

I'd be nice but I expect in the future we might want to send objects :) If we could send both that'd be fine, but I don't really mind to be honest.

> This makes me think we should just tell you the index, rather than forcing you to parse it out of a URL.  Thoughts?

I think iOS wanted the full URL, but maybe their requirements changed?
Thanks for the thorough review as always Ryan, it was very helpful.
I've amended my patch with your suggestions and added tests!
> > This makes me think we should just tell you the index, rather than forcing you to parse it out of a URL.  Thoughts?
> I think iOS wanted the full URL, but maybe their requirements changed?

To clarify, I'm suggesting we give you the index *as well as* the full URL.  I've added an "index" field to the message sent by auth-server and it should be live on the dev box now.
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review259084

Appologies, I thought I had published this days ago, but apparently I didn't click the right button or something.  Anyway, here's my comments from the other day...

-----

I left some tiny nits, but this LGTM! r+

::: browser/components/nsBrowserGlue.js:489
(Diff revision 3)
>          data = JSON.parse(data);
>          if (data.isLocalDevice) {
>            this._onDeviceDisconnected();
>          }
>          break;
> -      case "fxaccounts:messages:display-tabs":
> +      case "fxaccounts:commands:display-tabs":

A tiny nit: I think it'd be useful for the naming here to be consistent with the value we use in the `COMMAND_SENDTA` constant, e.g. "fxaccounts:commands:open-uri" and "https://identity.mozilla.com/cmd/open-uri".  I don't feel strongly about "open-uri" vs "display-tabs" vs something else entirely, but having them match in both plases will make it easier to navigate the code in future.

::: services/fxaccounts/FxAccountsCommands.js:28
(Diff revision 3)
> +  constructor(fxAccounts) {
> +    this._fxAccounts = fxAccounts;
> +    this.sendTab = new SendTab(this, fxAccounts);
> +  }
> +
> +  async send(command, device, payload) {

Tiny nit: you might consider naming this "invokeCommand" for consistency with the method it ends up calling.

::: services/fxaccounts/FxAccountsCommands.js:49
(Diff revision 3)
> +      return false;
> +    }
> +    log.info(`Consuming command with index ${index}.`);
> +    const {messages} = await this._fetchRemoteCommands(index, 1);
> +    if (messages.length != 1) {
> +      log.warn(`Should have retrieved 1 and only 1 message, got ${messages.length}.`);

Should we bail out if the length is zero, rather than doing no-op writes to the stored user data?

::: services/fxaccounts/FxAccountsCommands.js:127
(Diff revision 3)
> +            await this.sendTab.handle(sender, payload);
> +          } catch (e) {
> +            log.error(`Error while handling incoming Send Tab payload.`, e);
> +          }
> +        default:
> +          log.info(`Unknown commands: ${command}.`);

nit: it's an "Unknown command" singular, not multiple commands.

::: services/fxaccounts/FxAccountsPush.js:167
(Diff revision 3)
> -    }
>      this.log.debug(`push command: ${payload.command}`);
>      switch (payload.command) {
> +      case COMMAND_RECEIVED:
> +        const url = new URL(payload.data.url);
> +        const index = url.searchParams.get("index");

There should now be `payload.data.index` property that you can use directly, rather than parsing this out of the URL.

::: services/fxaccounts/FxAccountsWebChannel.jsm:441
(Diff revision 3)
>          log.info("changePassword ignoring unsupported field", name);
>        }
>      }
> -    return this._fxAccounts.updateUserAccountData(newCredentials)
> -      .then(() => this._fxAccounts.updateDeviceRegistration());
> +    await this._fxAccounts.updateUserAccountData(newCredentials);
> +    // Force the keys derivation, to be able to register a send-tab command
> +    // in updateDeviceRegistration.

Hrm, if this invariant is important for `updateDeviceRegistration`, then I wonder if we should move it into `updateDeviceRegistration` rather than having to remember to do it here (and potentially at other locations in future).

::: services/fxaccounts/tests/xpcshell/test_commands.js:74
(Diff revision 3)
> +add_task(async function test_commands_fetchMissedRemoteCommands() {
> +  const accountState = {
> +    data: {
> +      device: {
> +        handledCommands: [10, 11],
> +        lastCommandIndex: 11,

I don't think it matters for your test, but is `lastCommandIndex` is 11, then the server should not be returning commands with index 8, 9, etc, should it?
Attachment #8983555 - Flags: review?(rfkelly) → review+
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review259896

Nice work Ed! This looks great to me and most of my comments are nits, but I haven't given r+ because of my concerns re the pref flipping back to false (which seems like something we should be prepared for, especially if the proverbial hits the fan at some point during a rollout). Let me know if I've missed something here...

::: browser/base/content/browser-sync.js:334
(Diff revision 4)
>      }
> -    const toSendMessages = [];
> +    const fxaCommandsDevices = [];
> +    const oldSendTabClients = [];
>      for (const client of clients) {
>        const device = devices.find(d => d.id == client.fxaDeviceId);
> -      if (device && fxAccounts.messages.canReceiveSendTabMessages(device)) {
> +      if (device && (await fxAccounts.commands.sendTab.isDeviceCompatible(device))) {

It might make more sense to console.error and continue if !device, as that would imply something bad we should look into? (it's a bit if a pity we don't just carefully use log.jsm in this module, but there's no need to do that here)

::: browser/base/content/browser-sync.js:346
(Diff revision 4)
> +      console.log(`Sending a tab to ${fxaCommandsDevices.map(d => d.name).join(", ")} using FxA commands.`);
> +      const report = await fxAccounts.commands.sendTab.send(fxaCommandsDevices, {url, title});
> +      for (let {device, error} of report.failed) {
> +        console.error(`Failed to send a tab with FxA commands for ${device.name}.
> +                       Falling back on the Sync back-end`, error);
> +        oldSendTabClients.push(clients.find(c => c.fxaDeviceId == device.id));

similarly here - it seems unlikely, but .find returning undefined seems bad and worth making noise about?

::: browser/base/content/browser-sync.js:602
(Diff revision 4)
>      }
>      const state = UIState.get();
>      if (state.status == UIState.STATUS_SIGNED_IN) {
>        this.updateSyncStatus({ syncing: true });
> -      Services.tm.dispatchToMainThread(() => Weave.Service.sync());
> +      Services.tm.dispatchToMainThread(() => {
> +        fxAccounts.commands.fetchMissedRemoteCommands();

We should add a comment about why we do this here but not for a scheduled sync.

::: browser/components/nsBrowserGlue.js:486
(Diff revision 4)
>          if (data.isLocalDevice) {
>            this._onDeviceDisconnected();
>          }
>          break;
> -      case "fxaccounts:messages:display-tabs":
> +      case "fxaccounts:commands:display-tabs":
>        case "weave:engine:clients:display-uris":

I agree with Ryan re the names - it's a shame (although understandable) that we chose to not use FxAccountsCommon here to avoid pulling it into this code for perf reasons, but we should use the same string values like we do for all other fxa-specific notifications)

::: services/fxaccounts/FxAccounts.jsm:1052
(Diff revision 4)
>      await currentState.updateUserAccountData(updateData);
>      // We are now ready for business. This should only be invoked once
>      // per setSignedInUser(), regardless of whether we've rebooted since
>      // setSignedInUser() was called.
>      await this.notifyObservers(ONVERIFIED_NOTIFICATION);
> +    // Re-trigger the device registration to add the "send tab" available command.

I think the comment here should be made a bit more general - something like "some parts of the device registration may depends on having keys available, so re-register now they are" - it also makes it clearer why "send tab" is only available at this time.

However, what's the story for users who already are logged in? ISTM we should also bump DEVICE_REGISTRATION_VERSION for these users (and as I mention later, I'm also concerned about what happens when the pref is flipped from true back to false) - but below I have another idea that might kill both birds with one stone

::: services/fxaccounts/FxAccounts.jsm:1689
(Diff revision 4)
>    getPushSubscription() {
>      return this.fxaPushService.getSubscription();
>    },
>  
> -  // Once FxA messages is stable, remove this, hardcode the capabilities,
> -  // and reset the device registration version.
> +  async availableCommands() {
> +    if (!Services.prefs.getBoolPref("identity.fxaccounts.commands.enabled", true)) {

It doesn't look like we do the right thing here if the pref flips back to false after we've registered. I thought about abusing DEVICE_REGISTRATION_VERSION and a bitmask for this, but that doesn't really scale for future commands that will probably have the same basic issue of landing behind a pref.

How about something like:

* As we register a device, we *also* write the keys returned by availableCommands() into our local device data (ie, store it in signedInUser.json)
* We introduce a new function, something like checkDeviceUpdateNeeded() or similar. The existing code checking DEVICE_REGISTRATION_VERSION is moved into that function.
* That new function also checks if the keys in signedInUser.json match what's currently returned by availableCommands() and updates accordingly.
* The post-verification check just includes a call to this new function - the fact a user becomes verified ends up having the side-effect of changing the availableCommands() result so magically does the right thing.

::: services/fxaccounts/FxAccounts.jsm:1697
(Diff revision 4)
> -      return [CAPABILITY_MESSAGES, CAPABILITY_MESSAGES_SENDTAB];
> +    const sendTabKey = await this.commands.sendTab.getEncryptedKey();
> +    if (!sendTabKey) { // This will happen if the account is not verified yet.
> +      return {};
>      }
> -    return [];
> +    return {
> +      [COMMAND_SENDTAB]: sendTabKey

The array literal seems wrong here (and TIL that the JSON is identical with and without it - but it is confusing)

::: services/fxaccounts/FxAccountsCommands.js:133
(Diff revision 4)
> +      }
> +    }
> +  }
> +}
> +
> +class SendTab {

I think some comments about how send tab is implemented, and particularly around the encryption would make sense here.

::: services/fxaccounts/FxAccountsCommands.js:164
(Diff revision 4)
> +        const encrypted = await this._encrypt(bytes, device);
> +        const payload = {encrypted};
> +        await this._commands.send(COMMAND_SENDTAB, device, payload); // FxA needs an object.
> +        report.succeeded.push(device);
> +      } catch (error) {
> +        report.failed.push({device, error});

I think it probably makes sense to log here too - the consumers don't have good logging (eg, it only goes to the browser consile, which isn't that useful for us when we get bug reports with sync logs attached)

It also seems unfortunate that we will be unable to send a command when we are offline and that (IIUC) the user will get no feedback on that. I guess the fact we fallback to a command is OK in the short-term and for sendtab, do you think we should do anything better here in a followup? I suspect we *must* do something better as soon as we use this for anything other than sendtab.

::: services/fxaccounts/FxAccountsCommands.js:189
(Diff revision 4)
> +    }
> +    const bytes = await this._decrypt(encrypted);
> +    const decoder = new TextDecoder("utf8");
> +    const data = JSON.parse(decoder.decode(bytes));
> +    const current = data.hasOwnProperty("current") ? data.current :
> +                                                        data.entries.length - 1;

nit: this indentation seems strange

::: services/fxaccounts/FxAccountsCommands.js:288
(Diff revision 4)
> +    });
> +  }
> +}
> +
> +function urlsafeBase64Encode(buffer) {
> +  return ChromeUtils.base64URLEncode(new Uint8Array(buffer), { pad: false });

nit: remove spaces in object literal

::: services/fxaccounts/FxAccountsCommon.js:71
(Diff revision 4)
>  exports.ON_PASSWORD_CHANGED_NOTIFICATION = "fxaccounts:password_changed";
>  exports.ON_PASSWORD_RESET_NOTIFICATION = "fxaccounts:password_reset";
>  exports.ON_ACCOUNT_DESTROYED_NOTIFICATION = "fxaccounts:account_destroyed";
>  exports.ON_COLLECTION_CHANGED_NOTIFICATION = "sync:collection_changed";
>  exports.ON_VERIFY_LOGIN_NOTIFICATION = "fxaccounts:verify_login";
> +exports.COMMAND_RECEIVED = "fxaccounts:command_received";

It looks like this should be ON_COMMAND_RECEIVED?
Attachment #8983555 - Flags: review?(markh)
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review259896

> It doesn't look like we do the right thing here if the pref flips back to false after we've registered. I thought about abusing DEVICE_REGISTRATION_VERSION and a bitmask for this, but that doesn't really scale for future commands that will probably have the same basic issue of landing behind a pref.
> 
> How about something like:
> 
> * As we register a device, we *also* write the keys returned by availableCommands() into our local device data (ie, store it in signedInUser.json)
> * We introduce a new function, something like checkDeviceUpdateNeeded() or similar. The existing code checking DEVICE_REGISTRATION_VERSION is moved into that function.
> * That new function also checks if the keys in signedInUser.json match what's currently returned by availableCommands() and updates accordingly.
> * The post-verification check just includes a call to this new function - the fact a user becomes verified ends up having the side-effect of changing the availableCommands() result so magically does the right thing.

How about we just bump the device registration version/turn the pref to true in a second patch, and add a pref observer that just resets the device registration version?

> The array literal seems wrong here (and TIL that the JSON is identical with and without it - but it is confusing)

This allows us to use COMMAND_SENDTAB as a variable name, the produced JSON will be:
{"https://identity.mozilla.com/cmd/open-uri": {keys}}
Without the brackets we would get:
{COMMAND_SENDTAB: {keys}}

> I think it probably makes sense to log here too - the consumers don't have good logging (eg, it only goes to the browser consile, which isn't that useful for us when we get bug reports with sync logs attached)
> 
> It also seems unfortunate that we will be unable to send a command when we are offline and that (IIUC) the user will get no feedback on that. I guess the fact we fallback to a command is OK in the short-term and for sendtab, do you think we should do anything better here in a followup? I suspect we *must* do something better as soon as we use this for anything other than sendtab.

Yeah we should probably try to come up with a buffering system once we use FxA commands for something less trivial than send tab.
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review260196

::: services/fxaccounts/FxAccountsCommands.js:171
(Diff revisions 4 - 5)
>          const encrypted = await this._encrypt(bytes, device);
>          const payload = {encrypted};
> -        await this._commands.send(COMMAND_SENDTAB, device, payload); // FxA needs an object.
> +        await this._commands.invoke(COMMAND_SENDTAB, device, payload); // FxA needs an object.
>          report.succeeded.push(device);
>        } catch (error) {
> +        log.info("Error while invoking a send tab command.", error);

nit: we might as well use log.error() here so we get an error log
Attachment #8983555 - Flags: review?(markh)
(In reply to Edouard Oger [:eoger] from comment #13)
> How about we just bump the device registration version/turn the pref to true
> in a second patch, and add a pref observer that just resets the device
> registration version?

That seems a little messy - eg, what happens if we end up with a different reason for bumping the device registration in the meantime? And wouldn't that imply we need to persist that version somewhere?

Another idea I had was to or in a bitmask (eg, 0x80000000) if the pref is set, but all those solutions smell a little to me and don't really scale well into future commands. I'd rather not use the excuse that FxAccounts is messy to make it even more messy. OTOH, if my suggestion ends up making things messier or more complex I'll re-think it, but I don't think it's that onerous and should actually make it cleaner and easier to rationalize about.
> That seems a little messy - eg, what happens if we end up with a different reason for bumping the device registration in the meantime? And wouldn't that imply we need to persist that version somewhere?

In any case the keys are already saved in signedInUser.json, there's no re-generating keys again, we'd just pick them up and include them in the payload sent to FxA.
At the moment the device registration version system does exactly what we need, except handling pref changes, that's why I'm suggesting adding a pref observer on top of it.
A pref observer only works for prefs adjusted while running. I'm also thinking of a new build coming out with the pref flipped due to a decision to re-disable this due to problems found during rollout.

I agree we don't want to regenerate keys, but ISTM that if we know what keys we previously registered with and are capable of knowing what keys we'd register with now, that's all the info we need to do the right thing without relying on pref observers that may miss the change or by decrementing a value that's designed to be a constant that only ever increments in later builds.
> I'm also thinking of a new build coming out with the pref flipped due to a decision to re-disable this due to problems found during rollout.

I haven't considered that, thank you!
Comment on attachment 8983555 [details]
Bug 1466933 - Implement FxA commands.

https://reviewboard.mozilla.org/r/249410/#review260594

Thanks heaps Ed!
Attachment #8983555 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b636a45b545e
Implement FxA commands. r=markh,rfkelly
https://hg.mozilla.org/mozilla-central/rev/b636a45b545e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1483979
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: