Don't create Olm sessions proactively

In what I hoped would be a five-minute refactor to help clean up an annoying
not-really-used codepath, but turned into a bit of a hackathon on the tests,
create Olm sessions lazily in Olm rooms, just as we do in megolm rooms, which
allows us to avoid having to get the member list before configuring e2e in a
room.

lib/client.js

@@ -453,18 +453,8 @@ MatrixClient.prototype.setRoomEncryption = function(roomId, config) {
     if (!this._crypto) {
         throw new Error("End-to-End encryption disabled");
     }
-
+    this._crypto.setRoomEncryption(roomId, config);
-    var roomMembers = [];
+    return q();
-    var room = this.getRoom(roomId);
-    if (!room) {
-        console.warn("Enabling encryption in unknown room " + roomId);
-    } else {
-       roomMembers = utils.map(room.getJoinedMembers(), function(u) {
-           return u.userId;
-       });
-    }
-
-    return this._crypto.setRoomEncryption(roomId, config, roomMembers);
 };
 
 /**

lib/crypto-algorithms/base.js

@@ -20,8 +20,6 @@ limitations under the License.
  *
  * @module crypto-algorithms/base
  */
-var q = require("q");
-
 var utils = require("../utils");
 
 /**
@@ -63,20 +61,6 @@ var EncryptionAlgorithm = function(params) {
 /** */
 module.exports.EncryptionAlgorithm = EncryptionAlgorithm;
 
-/**
- * Initialise this EncryptionAlgorithm instance for a particular room.
- *
- * <p>This will be called once per EncryptionAlgorithm, just after the
- * constructor is called.
- *
- * @param {string[]} roomMembers list of currently-joined users in the room
- * @return {module:client.Promise} Promise which resolves when setup is complete
- */
-EncryptionAlgorithm.prototype.initRoomEncryption = function(roomMembers) {
-    return q();
-};
-
-
 /**
  * Encrypt a message event
  *

lib/crypto-algorithms/olm.js

@@ -41,19 +41,37 @@ var base = require("./base");
  */
 function OlmEncryption(params) {
     base.EncryptionAlgorithm.call(this, params);
+    this._sessionPrepared = false;
+    this._prepPromise = null;
 }
 utils.inherits(OlmEncryption, base.EncryptionAlgorithm);
 
 /**
- * @inheritdoc
+ * @private
+
  * @param {string[]} roomMembers list of currently-joined users in the room
  * @return {module:client.Promise} Promise which resolves when setup is complete
  */
-OlmEncryption.prototype.initRoomEncryption = function(roomMembers) {
+OlmEncryption.prototype._ensureSession = function(roomMembers) {
-    var crypto = this._crypto;
+    if (this._prepPromise) {
-    return crypto.downloadKeys(roomMembers, true).then(function(res) {
+        // prep already in progress
-        return crypto.ensureOlmSessionsForUsers(roomMembers);
+        return this._prepPromise;
+    }
+
+    if (this._sessionPrepared) {
+        // prep already done
+        return q();
+    }
+
+    var self = this;
+    this._prepPromise = self._crypto.downloadKeys(roomMembers, true).then(function(res) {
+        return self._crypto.ensureOlmSessionsForUsers(roomMembers);
+    }).then(function() {
+        self._sessionPrepared = true;
+    }).finally(function() {
+        self._prepPromise = null;
     });
+    return this._prepPromise;
 };
 
 /**
@@ -75,14 +93,16 @@ OlmEncryption.prototype.encryptMessage = function(room, eventType, content) {
         return u.userId;
     });
 
+    var self = this;
+    return this._ensureSession(users).then(function() {
         var participantKeys = [];
         for (var i = 0; i < users.length; ++i) {
             var userId = users[i];
-        var devices = this._crypto.getStoredDevicesForUser(userId);
+            var devices = self._crypto.getStoredDevicesForUser(userId);
             for (var j = 0; j < devices.length; ++j) {
                 var deviceInfo = devices[j];
                 var key = deviceInfo.getIdentityKey();
-            if (key == this._olmDevice.deviceCurve25519Key) {
+                if (key == self._olmDevice.deviceCurve25519Key) {
                     // don't bother setting up session to ourself
                     continue;
                 }
@@ -94,16 +114,14 @@ OlmEncryption.prototype.encryptMessage = function(room, eventType, content) {
             }
         }
 
-    return q(
+        return olmlib.encryptMessageForDevices(
-        olmlib.encryptMessageForDevices(
+            self._deviceId, self._olmDevice, participantKeys, {
-            this._deviceId, this._olmDevice, participantKeys, {
                 room_id: room.roomId,
                 type: eventType,
                 content: content,
             }
-        )
         );
-
+    });
 };
 
 /**

lib/crypto.js

@@ -557,11 +557,8 @@ Crypto.prototype.isSenderKeyVerified = function(userId, algorithm, sender_key) {
  *
  * @param {string} roomId The room ID to enable encryption in.
  * @param {object} config The encryption config for the room.
- * @param {string[]} roomMembers userIds of room members to start sessions with
- *
- * @return {module:client.Promise} A promise that will resolve when encryption is setup.
  */
-Crypto.prototype.setRoomEncryption = function(roomId, config, roomMembers) {
+Crypto.prototype.setRoomEncryption = function(roomId, config) {
     // if we already have encryption in this room, we should ignore this event
     // (for now at least. maybe we should alert the user somehow?)
     var existingConfig = this._sessionStore.getEndToEndRoom(roomId);
@@ -569,7 +566,7 @@ Crypto.prototype.setRoomEncryption = function(roomId, config, roomMembers) {
         if (JSON.stringify(existingConfig) != JSON.stringify(config)) {
             console.error("Ignoring m.room.encryption event which requests " +
                           "a change of config in " + roomId);
-            return q();
+            return;
         }
     }
 
@@ -592,7 +589,6 @@ Crypto.prototype.setRoomEncryption = function(roomId, config, roomMembers) {
         roomId: roomId,
     });
     this._roomAlgorithms[roomId] = alg;
-    return alg.initRoomEncryption(roomMembers);
 };
 
 

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

@@ -131,7 +131,7 @@ function bobUploadsKeys() {
  *
  * @return {promise} resolves once the http request has completed.
  */
-function aliQueryKeys() {
+function expectAliQueryKeys() {
     // can't query keys before bob has uploaded them
     expect(bobDeviceKeys).toBeDefined();
 
@@ -151,7 +151,7 @@ function aliQueryKeys() {
  *
  * @return {promise} which resolves once the http request has completed.
  */
-function bobQueryKeys() {
+function expectBobQueryKeys() {
     // can't query keys before ali has uploaded them
     expect(aliDeviceKeys).toBeDefined();
 
@@ -166,6 +166,34 @@ function bobQueryKeys() {
     return bobHttpBackend.flush("/keys/query", 1);
 }
 
+/**
+ * Set an expectation that ali will claim one of bob's keys; then flush the http request.
+ *
+ * @return {promise} resolves once the http request has completed.
+ */
+function expectAliClaimKeys() {
+    // can't query keys before bob has uploaded them
+    expect(bobOneTimeKeys).toBeDefined();
+
+    aliHttpBackend.when("POST", "/keys/claim").respond(200, function(path, content) {
+        expect(content.one_time_keys[bobUserId][bobDeviceId]).toEqual("curve25519");
+        for (var keyId in bobOneTimeKeys) {
+            if (bobOneTimeKeys.hasOwnProperty(keyId)) {
+                if (keyId.indexOf("curve25519:") === 0) {
+                    break;
+                }
+            }
+        }
+        var result = {};
+        result[bobUserId] = {};
+        result[bobUserId][bobDeviceId] = {};
+        result[bobUserId][bobDeviceId][keyId] = bobOneTimeKeys[keyId];
+        return {one_time_keys: result};
+    });
+
+    return aliHttpBackend.flush("/keys/claim", 1);
+}
+
 
 function aliDownloadsKeys() {
     // can't query keys before bob has uploaded them
@@ -180,7 +208,7 @@ function aliDownloadsKeys() {
             display_name: null,
         }]);
     });
-    var p2 = aliQueryKeys();
+    var p2 = expectAliQueryKeys();
 
     // check that the localStorage is updated as we expect (not sure this is
     // an integration test, but meh)
@@ -193,60 +221,91 @@ function aliDownloadsKeys() {
 }
 
 function aliEnablesEncryption() {
-    // can't query keys before bob has uploaded them
+    return aliClient.setRoomEncryption(roomId, {
-    expect(bobOneTimeKeys).toBeDefined();
-
-    aliQueryKeys().catch(test_utils.failTest);
-    aliHttpBackend.when("POST", "/keys/claim").respond(200, function(path, content) {
-        expect(content.one_time_keys[bobUserId][bobDeviceId]).toEqual("curve25519");
-        for (var keyId in bobOneTimeKeys) {
-            if (bobOneTimeKeys.hasOwnProperty(keyId)) {
-                if (keyId.indexOf("curve25519:") === 0) {
-                    break;
-                }
-            }
-        }
-        var result = {};
-        result[bobUserId] = {};
-        result[bobUserId][bobDeviceId] = {};
-        result[bobUserId][bobDeviceId][keyId] = bobOneTimeKeys[keyId];
-        return {one_time_keys: result};
-    });
-    var p = aliClient.setRoomEncryption(roomId, {
         algorithm: "m.olm.v1.curve25519-aes-sha2",
-    }).then(function(res) {
+    }).then(function() {
-        expect(res[aliUserId]).toEqual({});
-        expect(res[bobUserId][bobDeviceId].device).toBeDefined();
-        expect(res[bobUserId][bobDeviceId].sessionId).toBeDefined();
         expect(aliClient.isRoomEncrypted(roomId)).toBeTruthy();
     });
-    aliHttpBackend.flush();
-    return p;
 }
 
 function bobEnablesEncryption() {
-    bobQueryKeys().catch(test_utils.failTest);
     return bobClient.setRoomEncryption(roomId, {
         algorithm: "m.olm.v1.curve25519-aes-sha2",
-    }).then(function(res) {
+    }).then(function() {
-        expect(res[aliUserId][aliDeviceId].device).toBeDefined();
-        expect(res[aliUserId][aliDeviceId].sessionId).toBeDefined();
-        expect(res[bobUserId]).toEqual({});
        expect(bobClient.isRoomEncrypted(roomId)).toBeTruthy();
     });
 }
 
+/**
+ * Ali sends a message, first claiming e2e keys. Set the expectations and
+ * check the results.
+ *
+ * @return {promise} which resolves to the ciphertext for Bob's device.
+ */
+function aliSendsFirstMessage() {
+    return q.all([
+        sendMessage(aliClient),
+        expectAliQueryKeys()
+            .then(expectAliClaimKeys)
+            .then(expectAliSendMessageRequest)
+    ]).spread(function(_, ciphertext) {
+        return ciphertext;
+    });
+}
+
+/**
+ * Ali sends a message without first claiming e2e keys. Set the expectations
+ * and check the results.
+ *
+ * @return {promise} which resolves to the ciphertext for Bob's device.
+ */
 function aliSendsMessage() {
-    return sendMessage(aliHttpBackend, aliClient).then(function(content) {
+    return q.all([
+        sendMessage(aliClient),
+        expectAliSendMessageRequest()
+    ]).spread(function(_, ciphertext) {
+        return ciphertext;
+    });
+}
+
+/**
+ * Bob sends a message, first querying (but not claiming) e2e keys. Set the
+ * expectations and check the results.
+ *
+ * @return {promise} which resolves to the ciphertext for Ali's device.
+ */
+function bobSendsReplyMessage() {
+    return q.all([
+        sendMessage(bobClient),
+        expectBobQueryKeys()
+            .then(expectBobSendMessageRequest)
+    ]).spread(function(_, ciphertext) {
+        return ciphertext;
+    });
+}
+
+/**
+ * Set an expectation that Ali will send a message, and flush the request
+ *
+ * @return {promise} which resolves to the ciphertext for Bob's device.
+ */
+function expectAliSendMessageRequest() {
+    return expectSendMessageRequest(aliHttpBackend).then(function(content) {
         aliMessages.push(content);
         expect(utils.keys(content.ciphertext)).toEqual([bobDeviceCurve25519Key]);
         var ciphertext = content.ciphertext[bobDeviceCurve25519Key];
         expect(ciphertext).toBeDefined();
+        return ciphertext;
     });
 }
 
-function bobSendsMessage() {
+/**
-    return sendMessage(bobHttpBackend, bobClient).then(function(content) {
+ * Set an expectation that Bob will send a message, and flush the request
+ *
+ * @return {promise} which resolves to the ciphertext for Bob's device.
+ */
+function expectBobSendMessageRequest() {
+    return expectSendMessageRequest(bobHttpBackend).then(function(content) {
         bobMessages.push(content);
         var aliKeyId = "curve25519:" + aliDeviceId;
         var aliDeviceCurve25519Key = aliDeviceKeys.keys[aliKeyId];
@@ -257,7 +316,13 @@ function bobSendsMessage() {
     });
 }
 
-function sendMessage(httpBackend, client) {
+function sendMessage(client) {
+    return client.sendMessage(
+        roomId, {msgtype: "m.text", body: "Hello, World"}
+    );
+}
+
+function expectSendMessageRequest(httpBackend) {
     var path = "/send/m.room.encrypted/";
     var sent;
     httpBackend.when("PUT", path).respond(200, function(path, content) {
@@ -266,11 +331,7 @@ function sendMessage(httpBackend, client) {
             event_id: "asdfgh",
         };
     });
-    var p1 = client.sendMessage(
+    return httpBackend.flush(path, 1).then(function() {
-        roomId, {msgtype: "m.text", body: "Hello, World"}
-    );
-    var p2 = httpBackend.flush(path, 1);
-    return q.all([p1, p2]).then(function() {
         return sent;
     });
 }
@@ -459,7 +520,7 @@ describe("MatrixClient crypto", function() {
                 bobDeviceKeys.keys["curve25519:" + bobDeviceId] += "abc";
 
                 return q.all(aliClient.downloadKeys([bobUserId]),
-                             aliQueryKeys());
+                             expectAliQueryKeys());
             })
             .then(function() {
                 // should get an empty list
@@ -481,8 +542,8 @@ describe("MatrixClient crypto", function() {
             .then(bobUploadsKeys)
             .then(aliStartClient)
             .then(aliEnablesEncryption)
-            .then(aliSendsMessage)
+            .then(aliSendsFirstMessage)
-            .catch(test_utils.failTest).done(done);
+            .catch(test_utils.failTest).nodeify(done);
     });
 
     it("Bob receives a message", function(done) {
@@ -490,7 +551,7 @@ describe("MatrixClient crypto", function() {
             .then(bobUploadsKeys)
             .then(aliStartClient)
             .then(aliEnablesEncryption)
-            .then(aliSendsMessage)
+            .then(aliSendsFirstMessage)
             .then(bobStartClient)
             .then(bobRecvMessage)
             .catch(test_utils.failTest).done(done);
@@ -501,13 +562,20 @@ describe("MatrixClient crypto", function() {
             .then(bobUploadsKeys)
             .then(aliStartClient)
             .then(aliEnablesEncryption)
+            .then(aliDownloadsKeys)
             .then(function() {
                 aliClient.setDeviceBlocked(bobUserId, bobDeviceId, true);
-                return sendMessage(aliHttpBackend, aliClient);
+                var p1 = sendMessage(aliClient);
+                var p2 = expectAliQueryKeys()
+                    .then(expectAliClaimKeys)
+                    .then(function() {
+                        return expectSendMessageRequest(aliHttpBackend);
                     }).then(function(sentContent) {
                         // no unblocked devices, so the ciphertext should be empty
                         expect(sentContent.ciphertext).toEqual({});
-            }).catch(test_utils.failTest).done(done);
+                    });
+                return q.all([p1, p2]);
+            }).catch(test_utils.failTest).nodeify(done);
     });
 
     it("Bob receives two pre-key messages", function(done) {
@@ -515,7 +583,7 @@ describe("MatrixClient crypto", function() {
             .then(bobUploadsKeys)
             .then(aliStartClient)
             .then(aliEnablesEncryption)
-            .then(aliSendsMessage)
+            .then(aliSendsFirstMessage)
             .then(bobStartClient)
             .then(bobRecvMessage)
             .then(aliSendsMessage)
@@ -528,11 +596,11 @@ describe("MatrixClient crypto", function() {
             .then(bobUploadsKeys)
             .then(aliStartClient)
             .then(aliEnablesEncryption)
-            .then(aliSendsMessage)
+            .then(aliSendsFirstMessage)
             .then(bobStartClient)
             .then(bobRecvMessage)
             .then(bobEnablesEncryption)
-            .then(bobSendsMessage).then(function(ciphertext) {
+            .then(bobSendsReplyMessage).then(function(ciphertext) {
                 expect(ciphertext.type).toEqual(1);
             }).then(aliRecvMessage)
             .catch(test_utils.failTest).done(done);