Closed Bug 1152761 Opened 5 years ago Closed 5 years ago

Add local room storage for keys in case recovery is required

Categories

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

defect
Points:
8

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [context])

Attachments

(2 files, 3 obsolete files)

As part of the encryption algorithms, we should be storing room keys locally to enable recovery of the data if the user's key changes - e.g. in the case where an FxA user resets their password.
We should also move the Guest keys into the storage, either in this bug or a separate bug (xref bug 1142522 comment 16).
Rank: 8
Flags: firefox-backlog+
Whiteboard: [context]
Rank: 8 → 6
Assignee: nobody → standard8
This implements the required cache. I've based it on json, as the data is simple, and that's what FxA is using for some of the keys.

The patch causes the keys to be saved, and if decryption fails, then it'll try using the saved key to do the decryption.

At the moment, it doesn't then re-save the data with the encrypted value - I'm pushing that out to bug 1152764 as I'm starting to think we'll want to use the same, or a similar mechanism.
Attachment #8596726 - Flags: review?(mdeboer)
Comment on attachment 8596726 [details] [diff] [review]
Add local storage for Loop's room keys in case recovery is required, and handle the recovery.

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

The tests and API are looking good here. I have a few questions about the new RoomsCache object.

::: browser/components/loop/LoopRooms.jsm
@@ +10,5 @@
>  const {MozLoopService, LOOP_SESSION_TYPE} = Cu.import("resource:///modules/loop/MozLoopService.jsm", {});
>  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>                                    "resource://gre/modules/Promise.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "Task",
>                                    "resource://gre/modules/Task.jsm");

Since we're using `Task.async` here now directly upon module initialization, can you turn this into a regular Cu.import?

@@ +112,5 @@
>      }
>    }
>  };
>  
> +const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";

Can you move this to a separate module and lazily create an instance of the cache when needed?
This module has grown quite a bit, so I think a bit of modularization would be good.

@@ +115,5 @@
>  
> +const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";
> +
> +/**
> + * RoomsCache is a cache for saving simple rooms data to the disk in case we

s/RoomsCache/RoomKeysCache/

@@ +143,5 @@
> +  this.path = OS.Path.join(options.baseDir, options.filename);
> +  this._cache = null;
> +}
> +
> +RoomsCache.prototype = {

I think the prototype only needs to contain three methods: 

 - get(sessionType, roomToken)
 - set(sessionType, roomToken, roomKey)
 - clear()

Inside the module you can have two private functions that

 1. the cache is read from disk upon `_cache` access - something like `getCache()` that always yields an object.
 2. the cache is stored to disk upon setting a value on `_cache` - something like `persistCache(json)` that stores to disk.

@@ +166,5 @@
> +  _readCache: function() {
> +    return CommonUtils.readJSON(this.path).then(result => {
> +      this._cache = result;
> +    }).catch(error => {
> +      MozLoopService.log.debug("Error reading the cache:", error);

Please only log this when the error is _not_ a FILE_NOT_FOUND error.

@@ +217,5 @@
> +   * @param {String}            roomKey      The encryption key for the room.
> +   * @return {Promise}  A promise that is resolved when the data has been stored.
> +   */
> +  storeRoomKey: Task.async(function* (sessionType, roomToken, roomKey) {
> +    if (sessionType != LOOP_SESSION_TYPE.FXA) {

if this is only going to be used for FXA types, is it needed to keep the sessionType in the cache data format?

@@ +240,5 @@
> +
> +    // Only save it if there's no key, or it is different.
> +    if (!this._cache[sessionType][roomToken].key ||
> +        this._cache[sessionType][roomToken].key != roomKey) {
> +      this._cache[sessionType][roomToken].key = roomKey;

If the only thing we save in the cache is a roomKey, wouldn't it make sense to flatten the format to `this._cache[roomToken] = roomKey`?
Attachment #8596726 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #4)
> @@ +115,5 @@
> >  
> > +const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";
> > +
> > +/**
> > + * RoomsCache is a cache for saving simple rooms data to the disk in case we
> 
> s/RoomsCache/RoomKeysCache/

I don't mind changing this for now, but I was envisioning it might be used later for other things as well and hence I was designing it to be more generic.

> @@ +240,5 @@
> > +
> > +    // Only save it if there's no key, or it is different.
> > +    if (!this._cache[sessionType][roomToken].key ||
> > +        this._cache[sessionType][roomToken].key != roomKey) {
> > +      this._cache[sessionType][roomToken].key = roomKey;
> 
> If the only thing we save in the cache is a roomKey, wouldn't it make sense
> to flatten the format to `this._cache[roomToken] = roomKey`?

I didn't want to do that - if we ended up, for some reason, needing to save other data to this cache then it'd make it harder to upgrade nicely.

With this format, adding in new items is simply a matter of a new key/value pair - if the user/developer happens to swap version to downgrade temporarily, then the old version would still be able to read the key.

It'd also mean that we would hopefully be less likely to need migration routines for different versions.
Updated for comments. I've kept with the generic cache idea, as I think that's the easier thing to do looking forward, at the slight expense of some minor additional complexity.

I'll do the rename if its still wanted though, and change it back if we start using it for more things.
Attachment #8596726 - Attachment is obsolete: true
Attachment #8599815 - Flags: review?(mdeboer)
This time I'll include all the changes...
Attachment #8599815 - Attachment is obsolete: true
Attachment #8599815 - Flags: review?(mdeboer)
Attachment #8599820 - Flags: review?(mdeboer)
Iteration: --- → 40.3 - 11 May
Comment on attachment 8599820 [details] [diff] [review]
Add local storage for Loop's room keys in case recovery is required, and handle the recovery. v2a

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

Cool! Kudos for the test coverage here... quite essential.

Alright, let's ship it! (please do check out the notes I left below)

::: browser/components/loop/modules/LoopRooms.jsm
@@ +306,5 @@
> +
> +    try {
> +      key = yield this.promiseDecryptRoomKey(roomData.context.wrappedKey);
> +    }
> +    catch (error) {

nit: please put all the braces on the same line.

@@ +545,5 @@
>        }
>  
> +      // Now we've got the room token, we can save the key to disk.
> +      yield this.roomsCache.setKey(this.sessionType,
> +        room.roomToken, room.roomKey);

nit: I *think* `all.roomToken` will still fit in the 80-120 char limit.

@@ +760,5 @@
> +      } else {
> +        // This might be an upgrade to encrypted rename, so store the key
> +        // just in case.
> +        yield this.roomsCache.setKey(this.sessionType,
> +          all.roomToken, all.roomKey);

nit: I *think* `all.roomToken` will still fit in the 80-120 char limit.

::: browser/components/loop/modules/LoopRoomsCache.jsm
@@ +40,5 @@
> + *   - {String} filename  The filename for the cache file.
> + */
> +function LoopRoomsCache(options) {
> +  this.baseDir = options.baseDir;
> +  this.path = OS.Path.join(options.baseDir, options.filename);

It feels more 'natural' to have the default filename defined and used in this file.

```js
const LOOP_ROOMS_CACHE_FILENAME = "loopRoomsCache.json";

...

  this.path = OS.Path.join(
    options.baseDir || OS.Constants.Path.profileDir,
    options.filename || LOOP_ROOM_CACHE_FILENAME
  );
```

How does that look? Saves an import in LoopRooms.jsm too.

@@ +51,5 @@
> +   *
> +   * @param  {Object} contents An object to be saved in json format.
> +   * @return {Promise}         A promise that is resolved once the save is complete.
> +   */
> +  _setCache: function(contents) {

You *could* declare this function in the module scope as `function setCache(content) {...}` and call it in `setKey` as: `yield setCache.call(this, cache);`.
The same for _getCache, of course.

Up to you!

@@ +75,5 @@
> +      return (this._cache = yield CommonUtils.readJSON(this.path));
> +    } catch(error) {
> +      // This is really complex due to OSFile's error handling, see bug 1160109.
> +      if ((OS.Constants.libc && error.unixErrno != OS.Constants.libc.ENOENT) ||
> +          (OS.Constants.Win && error.winLastError != OS.Constants.Win.ERROR_FILE_NOT_FOUND)) {

Heh, well, kudos for figuring this mess out!!

@@ +120,5 @@
> +   *
> +   * @param {LOOP_SESSION_TYPE} sessionType  The session type for the room.
> +   * @param {String}            roomToken    The token for the room.
> +   * @param {String}            roomKey      The encryption key for the room.
> +   * @return {Promise}  A promise that is resolved when the data has been stored.

nit: one space too many.
Attachment #8599820 - Flags: review?(mdeboer) → review+
I took a look at this, and came to the conclusion it was some weird interaction between the things implemented for the cache and the FxA side of things. Given that bug 1153806 was cleaning this up, I pushed both to the try server, and the results came out green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02097ee8c5ac

Hence I've re-pushed both patches together, so hopefully they'll stick.
Ok, I eventually figured this out. The issue was that test_roomUpdates wasn't always waiting for all notifications. Due to the new context work, I think it can take slightly longer - possibly more async - to get all the required information. As a result, the expected updates notifications were spilling over into the channel ids test and confusing things.

I've added some waits, and also fixed a couple of test issues. Successful try run here from before I removed all the debug:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60e33633cbbd
Attachment #8601727 - Flags: review?(mdeboer)
Comment on attachment 8601727 [details] [diff] [review]
Bug 1152764 - Loop should encrypt room context information for rooms that aren't encrypted.

Oops, wrong patch.
Attachment #8601727 - Attachment is obsolete: true
Attachment #8601727 - Flags: review?(mdeboer)
This time with the correct patch...
Attachment #8601728 - Flags: review?(mdeboer)
Comment on attachment 8601728 [details] [diff] [review]
Intermittent failure fixes

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

Ship it! Nice changes.

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +494,4 @@
>    function badRoomJoin() {
>      Assert.ok(false, "Unexpected 'joined' event emitted!");
>    }
> +  LoopRooms.on("join", badRoomJoin);

heh, whoopsie :)
Attachment #8601728 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/mozilla-central/rev/08e566362fc9
https://hg.mozilla.org/mozilla-central/rev/5cffb3b5b2fb
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.