1142522 - Implement transition from roomName to encrypted context

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

As part of the context-in-conversations work, we need to transition the existing roomName field to be an encrypted context field.

Bug 1141105 is implementing the server-side changes, bug 1141133 is writing the encrypt/decrypt algorithms.

This bug is to start to tie them together.

https://wiki.mozilla.org/Loop/Architecture/Context

Per bug 1141105 comment 2:

A one-cycle transition period probably makes sense, to give people six weeks or so to get their clients updated. Here's what I'd propose:

- When a 39 client finds a room with a roomName, it copies it into the encrypted context and leaves it on the room. Changes to the room name are written to both the roomName and the context fields.

- In 40, we change this behavior so that any context changes instead set roomName to something like "Upgrade Firefox to see room names."

- Sometime in the 41-42 timeframe, we update the server to stop using the roomName field altogether.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: share utils and crypto content modules with chrome as well

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: share utils and crypto content modules with chrome as well

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: share utils and crypto content modules with chrome as well

Now includes working crypto in chrome!

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8586780 [details] [diff] [review]
Patch 1.1: share utils and crypto content modules with chrome as well

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

::: browser/components/loop/content/shared/js/crypto.js
@@ +13,5 @@
> +  var sharedUtils;
> +  if (inChrome) {
> +    this.EXPORTED_SYMBOLS = ["crypto"];
> +    var Cu = Components.utils;
> +    Cu.importGlobalProperties(["crypto"]);

As discussed on irc yesterday, this doesn't quite work due to the duplicate "crypto" symbol.

I think we can either change crypto to loopCrypto for the purposes of in-chrome (I'm thinking of naming it to that in MozLoopService anyway, so that we won't ever have namespace clashes), or find some other nice way of avoiding the clash.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: share utils and crypto content modules with chrome as well

lol, removing memoize :P

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8587342 [details] [diff] [review]
Patch 1.2: share utils and crypto content modules with chrome as well

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

Looks good.

Comment 7

[Mark Banner (:standard8)]

Attachment: WIP Start hooking up backend encryption

This is enough that it creates a key for non-FxA users (I've not written the FxA code yet), then when you create a room it creates a key, encrypts the room name and the key and stores them on the server.

It can also decrypt them and hence it'll survive to some extent after a restart.

There's lots of rough edges here and I'll need to do some extensive rework and extension on this before its near review ready - hence its not worth providing feedback at this stage.

I'm mainly putting up so that Mike can start fiddling with context on the content side of things if he wants to.

Comment 8

[Mark Banner (:standard8)]

Attachment: WIP Part 2 Hook up encryption for room contexts in guest mode.

I think this gets everything working in guest mode, and gets the tests running again, though I need to look through and see if there's more to add.

Next steps are also to look at hooking up the pref as an intermediate to disable encryption and also to at least make sure FxA continues working (I'll probably implement that in a separate patch).

Comment 9

[Mark Banner (:standard8)]

Attachment: Part 2 Hook up encryption for room contexts in guest mode.

This gets everything working in guest mode and completes the tests and updates the ui-showcase.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590761 [details] [diff] [review]
Part 2 Hook up encryption for room contexts in guest mode.

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

I think all the logic is there, but I think it's worth to spend a bit more time in restructuring the newly added things in LoopRooms to use Task.jsm (fewer blocks & braces FTW!)

::: browser/components/loop/LoopRooms.jsm
@@ +18,5 @@
>  });
>  XPCOMUtils.defineLazyGetter(this, "gLoopBundle", function() {
>    return Services.strings.createBundle('chrome://browser/locale/loop/loop.properties');
>  });
> +const loopUtils = Cu.import("resource:///modules/loop/utils.js", {}).utils;

Please make this a LazyGetter.

@@ +19,5 @@
>  XPCOMUtils.defineLazyGetter(this, "gLoopBundle", function() {
>    return Services.strings.createBundle('chrome://browser/locale/loop/loop.properties');
>  });
> +const loopUtils = Cu.import("resource:///modules/loop/utils.js", {}).utils;
> +const loopCrypto = Cu.import("resource:///modules/loop/crypto.js", {}).LoopCrypto;

Please make this a LazyModuleGetter.

@@ +147,5 @@
>    /**
> +   * Gets or creates a room key for a room.
> +   *
> +   * It assumes that the room data is
> +   * decrypted.

nit: I think this'll fit on one line.

@@ +149,5 @@
> +   *
> +   * It assumes that the room data is
> +   * decrypted.
> +   *
> +   * @param {Object} roomData The roomData to get the key for.

Please add a '@return' block here.

@@ +161,5 @@
> +
> +      loopCrypto.generateKey().then(key => {
> +        resolve(key);
> +      }, error => {
> +        reject(error);

No need need to wrap this in a function:

`loopCrypto.generateKey().then(resolve, reject);`

@@ +215,5 @@
> +   *                                any decrypted data.
> +   *                   - all: The roomData with both encrypted and decrypted
> +   *                          information.
> +   */
> +  promiseEncryptRoomData: function(roomData) {

Instead of using promises here, can you convert the functions that you only use internally to generators that simply yield for each async operation?

You can check out `return Task.async` for this, which returns a Promise as well, so the public API can invoke them when they use CPS.

For an example you can see GoogleImporter.jsm.

@@ +219,5 @@
> +  promiseEncryptRoomData: function(roomData) {
> +    return new Promise((resolve, reject) => {
> +      // For now, disable encryption/context if context is disabled, or if
> +      // FxA is turned on.
> +      if (!Services.prefs.getBoolPref("loop.contextInConverations.enabled") ||

Please use `MozLoopService.getLoopPref()`

@@ +507,4 @@
>  
> +      eventEmitter.emit("add", room);
> +      callback(null, room);
> +    }).catch(error => {

nit: might as well change this to just `callback`.

@@ +696,5 @@
> +      let data = JSON.parse(response.body);
> +      extend(newRoomData, data);
> +      this.rooms.set(roomToken, newRoomData);
> +      callback(null, newRoomData);
> +    }).catch(error => callback(error));

nit: same.

::: browser/components/loop/MozLoopService.jsm
@@ +52,5 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/Timer.jsm");
>  Cu.import("resource://gre/modules/FxAccountsOAuthClient.jsm");
> +const loopUtils = Cu.import("resource:///modules/loop/utils.js", {}).utils;
> +const loopCrypto = Cu.import("resource:///modules/loop/crypto.js", {}).LoopCrypto;

Please make these lazy too.

@@ +1335,5 @@
> +      }
> +
> +      // XXX Temporarily save in preferences until we've got some
> +      // extra storage.
> +      if (!Services.prefs.prefHasUserValue("loop.key")) {

I think you can do `if (!MozLoopService.getLoopPref("key"))`

@@ +1338,5 @@
> +      // extra storage.
> +      if (!Services.prefs.prefHasUserValue("loop.key")) {
> +        // Get a new value.
> +        loopCrypto.generateKey().then(key => {
> +          Services.prefs.setCharPref("loop.key", key);

MozLoopService.setLoopPref("key", key, Ci.nsIPrefBranch.PREF_STRING);

This one is actually more verbose, so debatable.

@@ +1347,5 @@
> +        });
> +        return;
> +      }
> +
> +      resolve(Services.prefs.getCharPref("loop.key"));

MozLoopService.getLoopPref("key");

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +172,5 @@
>  
> +// This compares rooms by normalizing the room fields so that the contents
> +// are the same between the two rooms - except for the room name
> +// (see normalizeRoom). This means we can detect if fields are missing, but
> +// we don't need to worry about the values being different, for examlpe, in the

nit: 'example'.

::: browser/components/loop/test/xpcshell/test_loopservice_encryptionkey.js
@@ +31,5 @@
> +  // Set the userProfile to look like we're logged into FxA.
> +  MozLoopServiceInternal.fxAOAuthTokenData = { token_type: "bearer" };
> +  MozLoopServiceInternal.fxAOAuthProfile = { email: "[e-mail]" };
> +
> +  // Currently unimplemented add a test when we implement the code.

nit: add a comma for improved readability.

@@ +37,5 @@
> +    /unimplemented/, "should reject as unimplemented");
> +});
> +
> +function run_test()
> +{

nit: brace on same line please.

::: browser/components/loop/ui/fake-mozLoop.js
@@ +5,5 @@
>  // Sample from https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms
>  var fakeRooms = [
>    {
>      "roomToken": "_nxD4V4FflQ",
> +    decryptedContext: {

nit: for posterity, you might want to quote this property as well.

Comment 11

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> ::: browser/components/loop/MozLoopService.jsm
> > +      // XXX Temporarily save in preferences until we've got some
> > +      // extra storage.
> > +      if (!Services.prefs.prefHasUserValue("loop.key")) {
> 
> I think you can do `if (!MozLoopService.getLoopPref("key"))`

I don't want to do that here as getLoopPref logs an error.

Comment 12

[Mark Banner (:standard8)]

Attachment: Part 2 Hook up encryption for room contexts in guest mode. v2

Updated for review comments

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8590813 [details] [diff] [review]
Part 2 Hook up encryption for room contexts in guest mode. v2

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

I saw a few issues and one suggestion that'd make the code on LoopRooms look even better. That is an opinion, of course ;-)

Only LoopRooms.jsm needs to be updated, the rest is already a-ok.

::: browser/components/loop/LoopRooms.jsm
@@ +183,5 @@
> +   */
> +  promiseDecryptRoomKey: Task.async(function* (encryptedKey) {
> +    let profileKey = yield MozLoopService.promiseProfileEncryptionKey();
> +
> +    return yield loopCrypto.decryptBytes(profileKey, encryptedKey);

I kinda like the pattern:

```js
let result = yield loopCrypto.decryptBytes(profileKey, encryptedKey);
return result;
```

But on the other hand I also like the conciseness of this notation :)
Up to you!

@@ +221,5 @@
> +    if (!newRoomData.context) {
> +      newRoomData.context = {};
> +    }
> +
> +    // First get the room key

nit: missing dot.

@@ +256,5 @@
> +   */
> +  promiseDecryptRoomData: Task.async(function* (roomData) {
> +    if (!roomData.context) {
> +      resolve(roomData);
> +      return;

how does this work? You don't have a `resolve` function anymore.

@@ +260,5 @@
> +      return;
> +    }
> +
> +    if (!roomData.context.wrappedKey) {
> +      reject(new Error("Missing wrappedKey"));

same here for `reject`... are you capturing this in a unit test?

@@ +273,5 @@
> +
> +    roomData.roomKey = key;
> +    roomData.decryptedContext = JSON.parse(decryptedData);
> +
> +    // Ensure no existing key on the url.

nit: can you rephrase this a bit?

@@ +285,5 @@
> +  /**
> +   * Saves room information and notifies updates to any listeners.
> +   *
> +   * @param  {Object}  roomData The new room data to save.
> +   * @param  {Boolean} isUpdate true if this is an update, false if its an add.

nit: one space too many between `@param` and `{`

@@ +476,4 @@
>  
> +      eventEmitter.emit("add", room);
> +      callback(null, room);
> +    }).catch(callback);

What do you think of the following:

```js
Task.spawn(function* () {
  let result = yield this.promiseEncryptRoomData(room);

  // Save both sets of data...
  room = result.all;
  // ...but only send the encrypted data.
  let response = yield MozLoopService.hawkRequest(this.sessionType,
    "/rooms", "POST", result.encrypted);

  extend(room, JSON.parse(response.body));
  // Do not keep this value - it is a request to the server.
  delete room.expiresIn;
  this.rooms.set(room.roomToken, room);

  if (this.sessionType == LOOP_SESSION_TYPE.GUEST) {
    this.setGuestCreatedRoom(true);
  }

  eventEmitter.emit("add", room);
  callback(null, room);
}.bind(this)).catch(callback);
```

@@ +638,5 @@
> +    } else {
> +      roomData.decryptedContext.roomName = newRoomName;
> +    }
> +
> +    let newRoomData;

...and use the same treatment here?

Comment 14

[Mark Banner (:standard8)]

Attachment: Part 2 Hook up encryption for room contexts in guest mode. v3

This updates for the review comments. It also updates test_looprooms.js for tests for decryption.

It turns out I had to do quite a bit of work to get the tests working as there were various side-effects happening - especially in normalizeRooms - which were affecting room objects as the tests progressed and causing confusion. So I've made that return complete clones rather than adjusted objects.

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8591220 [details] [diff] [review]
Part 2 Hook up encryption for room contexts in guest mode. v3

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

LGTM. r=me with comments addressed.

::: browser/components/loop/LoopRooms.jsm
@@ +462,5 @@
>        return;
>      }
>  
> +    Task.spawn(function* () {
> +      let result = yield this.promiseEncryptRoomData(room);

You can write this as:

`let {all, encrypted} = yield this.promiseEncryptRoomData(room);`

@@ +645,5 @@
> +      roomData.decryptedContext.roomName = newRoomName;
> +    }
> +
> +    Task.spawn(function* () {
> +      let result = yield this.promiseEncryptRoomData(roomData);

Same here.

::: browser/components/loop/MozLoopService.jsm
@@ +1330,5 @@
> +  promiseProfileEncryptionKey: function() {
> +    return new Promise((resolve, reject) => {
> +      if (this.userProfile) {
> +        // We're an FxA user.
> +        // XXX Implement this for FxA.

Please add a bug no.

@@ +1336,5 @@
> +        return;
> +      }
> +
> +      // XXX Temporarily save in preferences until we've got some
> +      // extra storage.

What kind of extra storage? Will this ever happen?

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +16,4 @@
>    ["_nxD4V4FflQ", {
>      roomToken: "_nxD4V4FflQ",
> +    // Encrypted with roomKey "FliIGLUolW-xkKZVWstqKw"
> +    // Guest key "uGIs-kGbYt1hBBwjyW7MLQ"

Can you put this in a constant above and reference that here as well as use that below in the setCharPref?

@@ +453,5 @@
>  });
>  
>  // Test if the rooms cache is refreshed after FxA signin or signout.
>  add_task(function* test_refresh() {
> +  // XXX Temporarily whilst FxA encryption isn't handled.

Please add a bug no.

::: browser/components/loop/test/xpcshell/test_loopservice_encryptionkey.js
@@ +37,5 @@
> +    /unimplemented/, "should reject as unimplemented");
> +});
> +
> +function run_test() {
> +  do_register_cleanup(function() {

You can remove the `run_test()` function and declare the `do_register_cleanup()` function at the top of this file.

Comment 16

[Mark Banner (:standard8)]

(In reply to Mike de Boer [:mikedeboer] from comment #15)
> @@ +1336,5 @@
> > +        return;
> > +      }
> > +
> > +      // XXX Temporarily save in preferences until we've got some
> > +      // extra storage.
> 
> What kind of extra storage? Will this ever happen?

Something non-prefs. This will be part of bug 1152761 where we need to store room keys anyway.

Comment 17

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/e9281275b655
https://hg.mozilla.org/integration/fx-team/rev/a58bfd07a6e8

Comment 18

[Mark Banner (:standard8)]

For qe-verify:

At the moment the context in conversations work is hidden behind a pref and requires the dev server to run, i.e. the prefs (suggest a separate profile):

loop.contextInConverations.enabled;true
loop.server;https://loop-dev.stage.mozaws.net/v0

To verify:

- Urls are no longer displayed in the panel list
- Only new/changed rooms get encrypted currently, bug 1152764 will extend this to more.
- Only guest rooms will be encrypted. FxA ones are bug 1153788
- For new rooms:
-- Copying a url gives an extended url (compared to previous)
-- Opening an extended url gives the room name on the standalone UI
-- Removing the part of the url from the # onwards stops the room name being displayed on the standalone UI and gives an error message.

Existing rooms will remain the same with the shorter url unless a change of room name occurs. At that time, it will be encrypted with an extend url etc.

Comment 19

[Mark Banner (:standard8)]

I had to push a unit test-bustage fix for browser_UITour_loop.js. Now to find out why run_all_loop_tests.sh didn't pick it up...

https://hg.mozilla.org/integration/fx-team/rev/5109c1949c7a

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9281275b655
https://hg.mozilla.org/mozilla-central/rev/a58bfd07a6e8
https://hg.mozilla.org/mozilla-central/rev/5109c1949c7a

Comment 21

[Bogdan Maris, Desktop QA]

(In reply to Mark Banner (:standard8) from comment #18)
> For qe-verify:
> 
> At the moment the context in conversations work is hidden behind a pref and
> requires the dev server to run, i.e. the prefs (suggest a separate profile):
> 
> loop.contextInConverations.enabled;true
> loop.server;https://loop-dev.stage.mozaws.net/v0
> 
> To verify:
> 
> - Urls are no longer displayed in the panel list
> - Only new/changed rooms get encrypted currently, bug 1152764 will extend
> this to more.
> - Only guest rooms will be encrypted. FxA ones are bug 1153788
> - For new rooms:
> -- Copying a url gives an extended url (compared to previous)
> -- Opening an extended url gives the room name on the standalone UI
> -- Removing the part of the url from the # onwards stops the room name being
> displayed on the standalone UI and gives an error message.
> 
> Existing rooms will remain the same with the shorter url unless a change of
> room name occurs. At that time, it will be encrypted with an extend url etc.

Tested side by side old Nightly from 2015-04-13 and latest Nightly&Aurora across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 64-bit) to compare builds before and after the fix. Marking as verified based on my testing.