Closed Bug 1152764 Opened 4 years ago Closed 4 years ago

Hello should encrypt room context information for rooms that aren't encrypted

Categories

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

defect
Points:
5

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [context])

User Story

This bug should:

- Re-encrypt roomKeys for rooms where the decryption with FxA key failed, and the fallback was used.
- Encrypt unencrypted rooms.

We'll need to consider some sort of rate-limiting or back-off to avoid overloading the server when everyone upgrades.

Attachments

(2 files, 1 obsolete file)

As part of adding encryption, we should start to automatically migrate non-encrypted context information (the room name) to being encrypted.

This will need to be done client side.

We may also want some rate limiting in here to avoid overloading the server when everyone upgrades.
Rank: 8
Flags: firefox-backlog+
Whiteboard: [context]
Rank: 8 → 7
User Story: (updated)
Assignee: nobody → standard8
Iteration: --- → 41.1 - May 25
Ok, I think this is in a good enough position that its ready for review. I'm a little cautious as there's some error flows that I think are handled well enough, but we might want more. In any case, probably good enough to get it out and see how it behaves.
Attachment #8605352 - Flags: review?(mdeboer)
Blocks: 1164821
Comment on attachment 8605352 [details] [diff] [review]
Loop should encrypt room context information for rooms that aren't encrypted.

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

Almost there! There a few question that I think we should tackle before landing.

::: browser/components/loop/modules/LoopRooms.jsm
@@ +201,5 @@
>    /**
> +   * Handles the encryption queue. Takes the next item off the queue,
> +   * restarts the timer if necessary.
> +   */
> +  handleEncryptionQueue: Task.async(function* () {

Please make this a normal Function. Since a timer fires this, we don't really need to deal result and/ or error propagation back to the caller.

Plus I think `purgeEncryptionQueue` might be a better name as 'handle' seems a bit too generic.

@@ +203,5 @@
> +   * restarts the timer if necessary.
> +   */
> +  handleEncryptionQueue: Task.async(function* () {
> +    let roomToken = this.encryptionQueue.queue.shift();
> +

nit: please loose this newline so that the logging clearly relates to this statement.

@@ +208,5 @@
> +    MozLoopService.log.debug("Encrypting", roomToken);
> +
> +    // Performed in sync fashion so that we don't queue a timer until it has
> +    // completed, and to make it easier to run tests.
> +    let roomData = this.rooms.get(roomToken);

In this case you need to add a guard for when no roomData is returned.

@@ +211,5 @@
> +    // completed, and to make it easier to run tests.
> +    let roomData = this.rooms.get(roomToken);
> +
> +    try {
> +      yield this.encryptRoom(roomData);

Can you just replace this block with a call to `this.update(roomToken, {}, err => {});`?

@@ +214,5 @@
> +    try {
> +      yield this.encryptRoom(roomData);
> +    } catch (error) {
> +      MozLoopService.log.error("Upgrade encryption of room failed", error);
> +      // No need to remove the room from the list as that's done in the list

list above?

@@ +231,5 @@
> +   * Queues a room for encryption sometime in the future. This is done so as
> +   * not to overload the server or the browser when we initially request the
> +   * list of rooms.
> +   *
> +   * @param  {String} roomToken The token for the room that needs encrypting.

nit: superfluous ws.

@@ +241,5 @@
> +
> +    // Set up encryption to happen at a random time later.
> +    if (!this.encryptionQueue.timer) {
> +      let waitTime = (MAX_TIME_BEFORE_ENCRYPTION - MIN_TIME_BEFORE_ENCRYPTION) *
> +        Math.random() + MIN_TIME_BEFORE_ENCRYPTION;

Why's the randomness necessary? Can you add a comment above here that explains this waitTime magic?

@@ +826,5 @@
>      } else {
> +      // room.roomName is the final fallback as this is pre-encryption support.
> +      room.decryptedContext.roomName = roomData.roomName ||
> +                                       room.decryptedContext.roomName ||
> +                                       room.roomName;

Do we have a bug on file that will eventually allow us to simplify this?

@@ +874,5 @@
> +   */
> +  encryptRoom: Task.async(function* (room) {
> +    return new Promise((resolve, reject) => {
> +      // All we need to do is to perform an update with no data specified,
> +      // and it'll handle the encryption for us.

...therefore I don't think it's necessary to dedicate a function to this operation.
As a side note, functions that return a Promise do not need to be async generators.

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +11,5 @@
>  
> +let timerArgs = [];
> +
> +timerHandlers.startTimer = function(callback, delay) {
> +  timerArgs.push({callback, delay});

What does this do? You're not testing the array's contents in this test, so I'm wondering if this should in fact just call the callback right away.

::: browser/components/loop/test/xpcshell/test_looprooms_encryption_in_fxa.js
@@ +6,5 @@
>  
> +let timerArgs = [];
> +
> +timerHandlers.startTimer = function(callback, delay) {
> +  timerArgs.push({callback, delay});

Same question.

::: browser/components/loop/test/xpcshell/test_looprooms_upgrade_to_encryption.js
@@ +133,5 @@
> +  // We have to decrypt the info, no other way.
> +  for (let room of rooms) {
> +    let roomData = yield loopCrypto.decryptBytes(room.roomKey, room.context.value);
> +
> +    Assert.deepEqual(JSON.parse(roomData), {roomName: kRoomsResponses.get(room.roomToken).roomName},

nit: formatting.
Attachment #8605352 - Flags: review?(mdeboer) → review-
(In reply to Mike de Boer [:mikedeboer] from comment #2)
> ::: browser/components/loop/modules/LoopRooms.jsm
> @@ +201,5 @@
> >    /**
> > +   * Handles the encryption queue. Takes the next item off the queue,
> > +   * restarts the timer if necessary.
> > +   */
> > +  handleEncryptionQueue: Task.async(function* () {
> 
> Please make this a normal Function. Since a timer fires this, we don't
> really need to deal result and/ or error propagation back to the caller.

I forgot the reason for this originally - its for the xpcshell tests. We need to know when the update has finished so that the tests can be deterministic. I've added a comment.

> ::: browser/components/loop/test/xpcshell/test_looprooms.js
> > +timerHandlers.startTimer = function(callback, delay) {
> > +  timerArgs.push({callback, delay});
> 
> What does this do? You're not testing the array's contents in this test, so
> I'm wondering if this should in fact just call the callback right away.
> 

Your're right, too much c&p ;-)
Updated for review comments.
Attachment #8605352 - Attachment is obsolete: true
Attachment #8607541 - Flags: review?(mdeboer)
Comment on attachment 8607541 [details] [diff] [review]
Loop should encrypt room context information for rooms that aren't encrypted. v2

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

Nice! I think we're there. I was debating to have the timer bookkeeping done in the encryptionQueue struct as well, since that's the only place where it's used, but it's not important enough to hold things up longer here.

Thanks for this.

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +8,5 @@
>  Cu.import("resource:///modules/loop/LoopRooms.jsm");
>  Cu.import("resource:///modules/Chat.jsm");
>  Cu.import("resource://gre/modules/Promise.jsm");
>  
> +timerHandlers.startTimer = function(callback, delay) {

nit: `timerHandlers.startTimer = callback => callback();

::: browser/components/loop/test/xpcshell/test_looprooms_encryption_in_fxa.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +timerHandlers.startTimer = function(callback, delay) {

nit: `timerHandlers.startTimer = callback => callback();

::: browser/components/loop/test/xpcshell/test_looprooms_upgrade_to_encryption.js
@@ +87,5 @@
> +      }
> +    });
> +  });
> +
> +  loopServer.registerPathHandler("/rooms/error401", (req, res) => {

I don't think you're using these two handlers... please remove them.
Attachment #8607541 - Flags: review?(mdeboer) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/1a2ab884ad45
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8607541 [details] [diff] [review]
Loop should encrypt room context information for rooms that aren't encrypted. v2

Approval Request Comment
[Feature/regressing bug #]: Context in Conversations for Hello.
[User impact if declined]: Rooms details won't be encrypted until they are edited - With the context work, we now have encryption so that room context isn't stored on our servers unencrypted. With this patch, we're moving the room name over to encrypted as well.
[Describe test coverage new/current, TreeHerder]: Landed in m-c, has unit tests.
[Risks and why]: Minor. Small chance of issues with the upgrade algorithm, but there's unit tests covering, so all should be fine.
[String/UUID change made/needed]: None
Attachment #8607541 - Flags: approval-mozilla-aurora?
Attachment #8607541 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This or bug 1164821 is hitting xpcshell permafail on Aurora.
https://treeherder.mozilla.org/logviewer.html#?job_id=848721&repo=mozilla-aurora

Please take a look ASAP or I'll backout both later today.
Flags: needinfo?(standard8)
Flags: needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
It seems that Array.prototype.includes is only available in nightly builds (bug 1070767). That doesn't look like its going to go out further anytime soon, so I think we should just swap back to indexOf.
Flags: needinfo?(standard8)
Attachment #8610410 - Flags: review?(mdeboer)
Attachment #8610410 - Flags: review?(mdeboer) → review+
Comment on attachment 8610410 [details] [diff] [review]
Fix includes -> indexOf

Approval Request Comment
[Feature/regressing bug #]: Fixes this bug on aurora - moves to an older API that's supported on non-nightly.
[User impact if declined]: See previous approval req
[Describe test coverage new/current, TreeHerder]: Covered by unit tests
[Risks and why]: minimal - reverting to older API
[String/UUID change made/needed]: None
Attachment #8610410 - Flags: approval-mozilla-aurora?
Comment on attachment 8610410 [details] [diff] [review]
Fix includes -> indexOf

Trivial bustage fixes don't need to get approval.
Attachment #8610410 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.