Implement transition from roomName to encrypted context

VERIFIED FIXED in Firefox 40

Status

Hello (Loop)
Client
P1
normal
Rank:
5
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

unspecified
mozilla40
Points:
8
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox40 verified)

Details

(Whiteboard: [context], URL)

Attachments

(2 attachments, 7 obsolete attachments)

9.17 KB, patch
standard8
: review+
Details | Diff | Splinter Review
54.52 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1142525
(Assignee)

Updated

3 years ago
No longer blocks: 1115341
(Assignee)

Updated

3 years ago
See Also: → bug 1142589

Updated

3 years ago
Rank: 5
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [context]
(Assignee)

Updated

3 years ago
Assignee: nobody → standard8
Iteration: --- → 39.3 - 30 Mar
Points: --- → 8
(Assignee)

Updated

3 years ago
Depends on: 1147375
(Assignee)

Updated

3 years ago
Depends on: 1114563, 1146929
(Assignee)

Updated

3 years ago
Depends on: 1147609
(Assignee)

Updated

3 years ago
No longer depends on: 1114563
(Assignee)

Updated

3 years ago
Depends on: 1147940
(Assignee)

Updated

3 years ago
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Created attachment 8586753 [details] [diff] [review]
Patch 1: share utils and crypto content modules with chrome as well
Created attachment 8586756 [details] [diff] [review]
Patch 1: share utils and crypto content modules with chrome as well
Attachment #8586753 - Attachment is obsolete: true
Created attachment 8586780 [details] [diff] [review]
Patch 1.1: share utils and crypto content modules with chrome as well

Now includes working crypto in chrome!
Attachment #8586756 - Attachment is obsolete: true
Attachment #8586780 - Flags: review?(standard8)
(Assignee)

Comment 4

3 years ago
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.
Attachment #8586780 - Flags: review?(standard8) → review-
Created attachment 8587342 [details] [diff] [review]
Patch 1.2: share utils and crypto content modules with chrome as well

lol, removing memoize :P
Attachment #8586780 - Attachment is obsolete: true
Attachment #8587342 - Flags: review?(standard8)
(Assignee)

Comment 6

3 years ago
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.
Attachment #8587342 - Flags: review?(standard8) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8587688 [details] [diff] [review]
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.
(Assignee)

Updated

3 years ago
Depends on: 1151832
(Assignee)

Updated

3 years ago
No longer depends on: 1147940
(Assignee)

Comment 8

3 years ago
Created attachment 8590206 [details] [diff] [review]
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).
Attachment #8587688 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1152761
(Assignee)

Updated

3 years ago
Blocks: 1152764
(Assignee)

Updated

3 years ago
Depends on: 1152947
(Assignee)

Comment 9

3 years ago
Created attachment 8590761 [details] [diff] [review]
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.
Attachment #8590206 - Attachment is obsolete: true
Attachment #8590761 - Flags: review?(mdeboer)
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: "fake@invalid.com" };
> +
> +  // 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.
Attachment #8590761 - Flags: review?(mdeboer) → feedback+
(Assignee)

Comment 11

3 years ago
(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.
(Assignee)

Comment 12

3 years ago
Created attachment 8590813 [details] [diff] [review]
Part 2 Hook up encryption for room contexts in guest mode. v2

Updated for review comments
Attachment #8590761 - Attachment is obsolete: true
Attachment #8590813 - Flags: review?(mdeboer)
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?
Attachment #8590813 - Flags: review?(mdeboer) → review-
(Assignee)

Updated

3 years ago
Depends on: 1153534
(Assignee)

Comment 14

3 years ago
Created attachment 8591220 [details] [diff] [review]
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.
Attachment #8590813 - Attachment is obsolete: true
Attachment #8591220 - Flags: review?(mdeboer)
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.
Attachment #8591220 - Flags: review?(mdeboer) → review+
(Assignee)

Updated

3 years ago
Blocks: 1153788
(Assignee)

Comment 16

3 years ago
(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.
No longer blocks: 1153788
(Assignee)

Updated

3 years ago
Blocks: 1153788
(Assignee)

Comment 18

3 years ago
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.
Flags: qe-verify+
(Assignee)

Updated

3 years ago
Blocks: 1153806
(Assignee)

Comment 19

3 years ago
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
QA Contact: bogdan.maris
(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.
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
Flags: qe-verify+
(Assignee)

Updated

3 years ago
Depends on: 1193765
You need to log in before you can comment on or make changes to this bug.