Invalidate device lists when encryption is enabled in a room

spec/TestClient.js

@@ -52,21 +52,21 @@ export default function TestClient(userId, deviceId, accessToken) {
 /**
  * start the client, and wait for it to initialise.
  *
- * @param {object?} existingDevices  the list of our existing devices to return from
- *    the /query request. Defaults to empty device list
  * @return {Promise}
  */
-TestClient.prototype.start = function(existingDevices) {
+TestClient.prototype.start = function() {
     this.httpBackend.when("GET", "/pushrules").respond(200, {});
     this.httpBackend.when("POST", "/filter").respond(200, { filter_id: "fid" });
-    this.expectKeyUpload(existingDevices);
+    this.expectKeyUpload();
 
     this.client.startClient({
         // set this so that we can get hold of failed events
         pendingEventOrdering: 'detached',
     });
 
-    return this.httpBackend.flush();
+    return this.httpBackend.flush().then(() => {
+        console.log('TestClient[' + this.userId + ']: started');
+    });
 };
 
 /**
@@ -78,22 +78,9 @@ TestClient.prototype.stop = function() {
 
 /**
  * Set up expectations that the client will upload device and one-time keys.
- *
- * @param {object?} existingDevices  the list of our existing devices to return from
- *    the /query request. Defaults to empty device list
  */
-TestClient.prototype.expectKeyUpload = function(existingDevices) {
+TestClient.prototype.expectKeyUpload = function() {
     const self = this;
-    this.httpBackend.when('POST', '/keys/query').respond(200, function(path, content) {
-        expect(Object.keys(content.device_keys)).toEqual([self.userId]);
-        expect(content.device_keys[self.userId]).toEqual({});
-        let res = existingDevices;
-        if (!res) {
-            res = { device_keys: {} };
-            res.device_keys[self.userId] = {};
-        }
-        return res;
-    });
     this.httpBackend.when("POST", "/keys/upload").respond(200, function(path, content) {
         expect(content.one_time_keys).toBe(undefined);
         expect(content.device_keys).toBeTruthy();
@@ -111,6 +98,24 @@ TestClient.prototype.expectKeyUpload = function(existingDevices) {
     });
 };
 
+/**
+ * Set up expectations that the client will query device keys.
+ *
+ * We check that the query contains each of the users in `response`.
+ *
+ * @param {Object} response   response to the query.
+ */
+TestClient.prototype.expectKeyQuery = function(response) {
+    this.httpBackend.when('POST', '/keys/query').respond(
+        200, (path, content) => {
+            Object.keys(response.device_keys).forEach((userId) => {
+                expect(content.device_keys[userId]).toEqual({});
+            });
+            return response;
+        });
+};
+
+
 /**
  * get the uploaded curve25519 device key
  *

spec/integ/matrix-client-crypto.spec.js

@@ -393,7 +393,9 @@ describe("MatrixClient crypto", function() {
 
     afterEach(function() {
         aliTestClient.stop();
+        aliTestClient.httpBackend.verifyNoOutstandingExpectation();
         bobTestClient.stop();
+        bobTestClient.httpBackend.verifyNoOutstandingExpectation();
     });
 
     it('Ali knows the difference between a new user and one with no devices',
@@ -620,16 +622,13 @@ describe("MatrixClient crypto", function() {
             .then(function() {
                 aliTestClient.client.setDeviceBlocked(bobUserId, bobDeviceId, true);
                 const p1 = sendMessage(aliTestClient.client);
-                const p2 = expectAliQueryKeys()
-                    .then(expectAliClaimKeys)
-                    .then(function() {
-                        return expectSendMessageRequest(aliTestClient.httpBackend);
-                    }).then(function(sentContent) {
-                        // no unblocked devices, so the ciphertext should be empty
-                        expect(sentContent.ciphertext).toEqual({});
-                    });
+                const p2 = expectSendMessageRequest(aliTestClient.httpBackend)
+                      .then(function(sentContent) {
+                          // no unblocked devices, so the ciphertext should be empty
+                          expect(sentContent.ciphertext).toEqual({});
+                      });
                 return q.all([p1, p2]);
-            }).catch(testUtils.failTest).nodeify(done);
+            }).nodeify(done);
     });
 
     it("Bob receives two pre-key messages", function(done) {
@@ -685,4 +684,44 @@ describe("MatrixClient crypto", function() {
             }).then(expectAliQueryKeys)
             .nodeify(done);
     });
+
+    it("Ali does a key query when encryption is enabled", function(done) {
+        // enabling encryption in the room should make alice download devices
+        // for both members.
+        q()
+            .then(() => startClient(aliTestClient))
+            .then(() => {
+                const syncData = {
+                    next_batch: '2',
+                    rooms: {
+                        join: {},
+                    },
+                };
+                syncData.rooms.join[roomId] = {
+                    state: {
+                        events: [
+                            testUtils.mkEvent({
+                                type: 'm.room.encryption',
+                                skey: '',
+                                content: {
+                                    algorithm: 'm.olm.v1.curve25519-aes-sha2',
+                                },
+                            }),
+                        ],
+                    },
+                };
+
+                aliTestClient.httpBackend.when('GET', '/sync').respond(
+                    200, syncData);
+                return aliTestClient.httpBackend.flush('/sync', 1);
+            }).then(() => {
+                aliTestClient.expectKeyQuery({
+                    device_keys: {
+                        [aliUserId]: {},
+                        [bobUserId]: {},
+                    },
+                });
+                return aliTestClient.httpBackend.flush('/keys/query', 1);
+            }).nodeify(done);
+    });
 });

spec/integ/matrix-client-methods.spec.js

@@ -351,7 +351,6 @@ describe("MatrixClient", function() {
 
             httpBackend.when("POST", "/keys/query").check(function(req) {
                 expect(req.data).toEqual({device_keys: {
-                    '@alice:localhost': {},
                     'boris': {},
                     'chaz': {},
                 }});

spec/integ/megolm.spec.js

@@ -672,9 +672,7 @@ describe("megolm", function() {
         let inboundGroupSession;
         let decrypted;
 
-        return aliceTestClient.start(
-            getTestKeysQueryResponse(aliceTestClient.userId),
-        ).then(function() {
+        return aliceTestClient.start().then(function() {
             // an encrypted room with just alice
             const syncResponse = {
                 next_batch: 1,
@@ -701,6 +699,13 @@ describe("megolm", function() {
             };
             aliceTestClient.httpBackend.when('GET', '/sync').respond(200, syncResponse);
 
+            // the completion of the first initialsync hould make Alice
+            // invalidate the device cache for all members in e2e rooms (ie,
+            // herself), and do a key query.
+            aliceTestClient.expectKeyQuery(
+                getTestKeysQueryResponse(aliceTestClient.userId),
+            );
+
             return aliceTestClient.httpBackend.flush();
         }).then(function() {
             aliceTestClient.client.setDeviceKnown(aliceTestClient.userId, 'DEVICE_ID');

spec/mock-request.js

@@ -49,17 +49,17 @@ HttpBackend.prototype = {
         const tryFlush = function() {
             // if there's more real requests and more expected requests, flush 'em.
             console.log(
-                "  trying to flush queue => reqs=%s expected=%s [%s]",
-                self.requests.length, self.expectedRequests.length, path,
+                "  trying to flush queue => reqs=[%s] expected=[%s]",
+                self.requests, self.expectedRequests.map((er) => er.path),
             );
             if (self._takeFromQueue(path)) {
                 // try again on the next tick.
-                console.log("  flushed. Trying for more. [%s]", path);
                 flushed += 1;
                 if (numToFlush && flushed === numToFlush) {
-                    console.log("  [%s] Flushed assigned amount: %s", path, numToFlush);
+                    console.log("  Flushed assigned amount: %s", numToFlush);
                     defer.resolve();
                 } else {
+                    console.log("  flushed. Trying for more.");
                     setTimeout(tryFlush, 0);
                 }
             } else if (flushed === 0 && !triedWaiting) {
@@ -68,7 +68,7 @@ HttpBackend.prototype = {
                 setTimeout(tryFlush, 5);
                 triedWaiting = true;
             } else {
-                console.log("  no more flushes. [%s]", path);
+                console.log("  no more flushes.");
                 defer.resolve();
             }
         };

src/crypto/DeviceList.js

@@ -64,7 +64,13 @@ export default class DeviceList {
                 // just wait for the existing download to complete
                 promises.push(this._keyDownloadsInProgressByUser[u]);
             } else {
-                if (forceDownload || !this.getStoredDevicesForUser(u)) {
+                if (forceDownload) {
+                    console.log("Invalidating device list for " + u +
+                                " for forceDownload");
+                    this.invalidateUserDeviceList(u);
+                } else if (!this.getStoredDevicesForUser(u)) {
+                    console.log("Invalidating device list for " + u +
+                                " due to empty cache");
                     this.invalidateUserDeviceList(u);
                 }
                 if (this._pendingUsersWithNewDevices[u]) {

src/crypto/algorithms/olm.js

@@ -64,7 +64,7 @@ OlmEncryption.prototype._ensureSession = function(roomMembers) {
     }
 
     const self = this;
-    this._prepPromise = self._crypto.downloadKeys(roomMembers, true).then(function(res) {
+    this._prepPromise = self._crypto.downloadKeys(roomMembers).then(function(res) {
         return self._crypto.ensureOlmSessionsForUsers(roomMembers);
     }).then(function() {
         self._sessionPrepared = true;

src/crypto/index.js

@@ -86,9 +86,6 @@ function Crypto(baseApis, eventEmitter, sessionStore, userId, deviceId,
     );
 
     if (!myDevices) {
-        // we don't yet have a list of our own devices; make sure we
-        // get one when we flush the pendingUsersWithNewDevices.
-        this._deviceList.invalidateUserDeviceList(this._userId);
         myDevices = {};
     }
 
@@ -545,6 +542,23 @@ Crypto.prototype.setRoomEncryption = function(roomId, config) {
         config: config,
     });
     this._roomEncryptors[roomId] = alg;
+
+    // if encryption was not previously enabled in this room, we will have been
+    // ignoring new device events for these users so far. We may well have
+    // up-to-date lists for some users, for instance if we were sharing other
+    // e2e rooms with them, so there is room for optimisation here, but for now
+    // we just invalidate everyone in the room.
+    if (!existingConfig) {
+        console.log("Enabling encryption in " + roomId + " for the first time; " +
+                    "invalidating device lists for all users therein");
+        const room = this._clientStore.getRoom(roomId);
+        const members = room.getJoinedMembers();
+        members.forEach((m) => {
+            this._deviceList.invalidateUserDeviceList(m.userId);
+        });
+        // the actual refresh happens once we've finished processing the sync,
+        // in _onSyncCompleted.
+    }
 };
 
 
@@ -769,6 +783,8 @@ Crypto.prototype._onSyncCompleted = function(syncData) {
         } else {
             // otherwise, we have to invalidate all devices for all users we
             // share a room with.
+            console.log("Completed first initialsync; invalidating all " +
+                        "device list caches");
             this._invalidateDeviceListForAllActiveUsers();
         }
     }