Lazy loading: fix end-to-end encryption rooms

spec/unit/room.spec.js

@@ -387,7 +387,7 @@ describe("Room", function() {
         let events = null;
 
         beforeEach(function() {
-            room = new Room(roomId, null, {timelineSupport: timelineSupport});
+            room = new Room(roomId, null, null, {timelineSupport: timelineSupport});
             // set events each time to avoid resusing Event objects (which
             // doesn't work because they get frozen)
             events = [
@@ -469,7 +469,7 @@ describe("Room", function() {
 
     describe("compareEventOrdering", function() {
         beforeEach(function() {
-            room = new Room(roomId, null, {timelineSupport: true});
+            room = new Room(roomId, null, null, {timelineSupport: true});
         });
 
         const events = [
@@ -658,7 +658,7 @@ describe("Room", function() {
 
         beforeEach(function() {
             // no mocking
-            room = new Room(roomId, userA);
+            room = new Room(roomId, null, userA);
         });
 
         describe("Room.recalculate => Stripped State Events", function() {
@@ -1192,7 +1192,7 @@ describe("Room", function() {
     describe("addPendingEvent", function() {
         it("should add pending events to the pendingEventList if " +
                       "pendingEventOrdering == 'detached'", function() {
-            const room = new Room(roomId, userA, {
+            const room = new Room(roomId, null, userA, {
                 pendingEventOrdering: "detached",
             });
             const eventA = utils.mkMessage({
@@ -1218,7 +1218,7 @@ describe("Room", function() {
 
         it("should add pending events to the timeline if " +
                       "pendingEventOrdering == 'chronological'", function() {
-            room = new Room(roomId, userA, {
+            room = new Room(roomId, null, userA, {
                 pendingEventOrdering: "chronological",
             });
             const eventA = utils.mkMessage({
@@ -1242,7 +1242,7 @@ describe("Room", function() {
 
     describe("updatePendingEvent", function() {
         it("should remove cancelled events from the pending list", function() {
-            const room = new Room(roomId, userA, {
+            const room = new Room(roomId, null, userA, {
                 pendingEventOrdering: "detached",
             });
             const eventA = utils.mkMessage({
@@ -1278,7 +1278,7 @@ describe("Room", function() {
 
 
         it("should remove cancelled events from the timeline", function() {
-            const room = new Room(roomId, userA);
+            const room = new Room(roomId, null, userA);
             const eventA = utils.mkMessage({
                 room: roomId, user: userA, event: true,
             });
@@ -1311,53 +1311,93 @@ describe("Room", function() {
         });
     });
 
-    describe("loadOutOfBandMembers", function() {
+    describe("loadMembersIfNeeded", function() {
+        function createClientMock(serverResponse, storageResponse = null) {
+            return {
+                getEventMapper: function() {
+                    // events should already be MatrixEvents
+                    return function(event) {return event;};
+                },
+                _http: {
+                    serverResponse,
+                    authedRequest: function() {
+                        if (this.serverResponse instanceof Error) {
+                            return Promise.reject(this.serverResponse);
+                        } else {
+                            return Promise.resolve({chunk: this.serverResponse});
+                        }
+                    },
+                },
+                store: {
+                    storageResponse,
+                    storedMembers: null,
+                    getOutOfBandMembers: function() {
+                        if (this.storageResponse instanceof Error) {
+                            return Promise.reject(this.storageResponse);
+                        } else {
+                            return Promise.resolve(this.storageResponse);
+                        }
+                    },
+                    setOutOfBandMembers: function(roomId, memberEvents) {
+                        this.storedMembers = memberEvents;
+                        return Promise.resolve();
+                    },
+                },
+            };
+        }
+
         const memberEvent = utils.mkMembership({
             user: "@user_a:bar", mship: "join",
             room: roomId, event: true, name: "User A",
         });
 
-        it("should apply member events", async function() {
-            const room = new Room(roomId, null);
-            await room.loadOutOfBandMembers(Promise.resolve([memberEvent]));
+        it("should load members from server on first call", async function() {
+            const client = createClientMock([memberEvent]);
+            const room = new Room(roomId, client, null, {lazyLoadMembers: true});
+            await room.loadMembersIfNeeded();
             const memberA = room.getMember("@user_a:bar");
             expect(memberA.name).toEqual("User A");
+            const storedMembers = client.store.storedMembers;
+            expect(storedMembers.length).toEqual(1);
+            expect(storedMembers[0].event_id).toEqual(memberEvent.getId());
         });
 
-        it("should apply first call, not first resolved promise", async function() {
+        it("should take members from storage if available", async function() {
             const memberEvent2 = utils.mkMembership({
                 user: "@user_a:bar", mship: "join",
                 room: roomId, event: true, name: "Ms A",
             });
-            const room = new Room(roomId, null);
-
-            const promise2 = Promise.resolve([memberEvent2]);
-            const promise1 = promise2.then(() => [memberEvent]);
+            const client = createClientMock([memberEvent2], [memberEvent]);
+            const room = new Room(roomId, client, null, {lazyLoadMembers: true});
 
-            await room.loadOutOfBandMembers(promise1);
-            await room.loadOutOfBandMembers(promise2);
+            await room.loadMembersIfNeeded();
 
             const memberA = room.getMember("@user_a:bar");
             expect(memberA.name).toEqual("User A");
         });
 
-        it("should revert needs loading on error", async function() {
-            const room = new Room(roomId, null);
+        it("should allow retry on error", async function() {
+            const client = createClientMock(new Error("server says no"));
+            const room = new Room(roomId, client, null, {lazyLoadMembers: true});
             let hasThrown = false;
             try {
-                await room.loadOutOfBandMembers(Promise.reject(new Error("bugger")));
+                await room.loadMembersIfNeeded();
             } catch(err) {
                 hasThrown = true;
             }
             expect(hasThrown).toEqual(true);
-            expect(room.needsOutOfBandMembers()).toEqual(true);
+
+            client._http.serverResponse = [memberEvent];
+            await room.loadMembersIfNeeded();
+            const memberA = room.getMember("@user_a:bar");
+            expect(memberA.name).toEqual("User A");
         });
     });
 
     describe("getMyMembership", function() {
         it("should return synced membership if membership isn't available yet",
         async function() {
-            const room = new Room(roomId, userA);
+            const room = new Room(roomId, null, userA);
             room.setSyncedMembership("invite");
             expect(room.getMyMembership()).toEqual("invite");
             room.addLiveEvents([utils.mkMembership({

src/client.js

@@ -754,56 +754,6 @@ MatrixClient.prototype.getRoom = function(roomId) {
     return this.store.getRoom(roomId);
 };
 
-MatrixClient.prototype._loadMembers = async function(room) {
-    const roomId = room.roomId;
-    // were the members loaded from the server?
-    let fromServer = false;
-    let rawMembersEvents = await this.store.getOutOfBandMembers(roomId);
-    if (rawMembersEvents === null) {
-        fromServer = true;
-        const lastEventId = room.getLastEventId();
-        const response = await this.members(roomId, "join", "leave", lastEventId);
-        rawMembersEvents = response.chunk;
-        console.log(`LL: got ${rawMembersEvents.length} members from server`);
-    }
-    const memberEvents = rawMembersEvents.map(this.getEventMapper());
-    return {memberEvents, fromServer};
-};
-
-/**
- * Preloads the member list for the given room id,
- * in case lazy loading of memberships is in use.
- * @param {string} roomId The room ID
- */
-MatrixClient.prototype.loadRoomMembersIfNeeded = async function(roomId) {
-    const room = this.getRoom(roomId);
-    if (!room || !room.needsOutOfBandMembers()) {
-        return;
-    }
-    // intercept whether we need to store oob members afterwards
-    let membersNeedStoring = false;
-    // Note that we don't await _loadMembers here first.
-    // setLazyLoadedMembers sets a flag before it awaits the promise passed in
-    // to avoid a race when calling membersNeedLoading/loadOutOfBandMembers
-    // in fast succession, before the first promise resolves.
-    const membersPromise = this._loadMembers(room)
-        .then(({memberEvents, fromServer}) => {
-            membersNeedStoring = fromServer;
-            return memberEvents;
-        });
-    await room.loadOutOfBandMembers(membersPromise);
-    // if loadOutOfBandMembers throws, this wont be called
-    // but that's fine as we don't want to store members
-    // that caused an error.
-    if (membersNeedStoring) {
-        const rawMembersEvents = room.currentState.getMembers()
-            .filter((m) => m.isOutOfBand())
-            .map((m) => m.events.member.event);
-        console.log(`LL: telling backend to store ${rawMembersEvents.length} members`);
-        await this.store.setOutOfBandMembers(roomId, rawMembersEvents);
-    }
-};
-
 /**
  * Retrieve all known rooms.
  * @return {Room[]} A list of rooms, or an empty list if there is no data store.

src/crypto/algorithms/megolm.js

@@ -535,8 +535,9 @@ MegolmEncryption.prototype._checkForUnknownDevices = function(devicesInRoom) {
  * @return {module:client.Promise} Promise which resolves to a map
  *     from userId to deviceId to deviceInfo
  */
-MegolmEncryption.prototype._getDevicesInRoom = function(room) {
-    const roomMembers = utils.map(room.getEncryptionTargetMembers(), function(u) {
+MegolmEncryption.prototype._getDevicesInRoom = async function(room) {
+    const members = await room.getEncryptionTargetMembers();
+    const roomMembers = utils.map(members, function(u) {
         return u.userId;
     });
 
@@ -555,29 +556,28 @@ MegolmEncryption.prototype._getDevicesInRoom = function(room) {
     // common and then added new devices before joining this one? --Matthew
     //
     // yup, see https://github.com/vector-im/riot-web/issues/2305 --richvdh
-    return this._crypto.downloadKeys(roomMembers, false).then((devices) => {
-        // remove any blocked devices
-        for (const userId in devices) {
-            if (!devices.hasOwnProperty(userId)) {
+    const devices = await this._crypto.downloadKeys(roomMembers, false);
+    // remove any blocked devices
+    for (const userId in devices) {
+        if (!devices.hasOwnProperty(userId)) {
+            continue;
+        }
+
+        const userDevices = devices[userId];
+        for (const deviceId in userDevices) {
+            if (!userDevices.hasOwnProperty(deviceId)) {
                 continue;
             }
 
-            const userDevices = devices[userId];
-            for (const deviceId in userDevices) {
-                if (!userDevices.hasOwnProperty(deviceId)) {
-                    continue;
-                }
-
-                if (userDevices[deviceId].isBlocked() ||
-                    (userDevices[deviceId].isUnverified() && isBlacklisting)
-                   ) {
-                    delete userDevices[deviceId];
-                }
+            if (userDevices[deviceId].isBlocked() ||
+                (userDevices[deviceId].isUnverified() && isBlacklisting)
+               ) {
+                delete userDevices[deviceId];
             }
         }
+    }
 
-        return devices;
-    });
+    return devices;
 };
 
 /**

src/crypto/algorithms/olm.js

@@ -83,60 +83,62 @@ OlmEncryption.prototype._ensureSession = function(roomMembers) {
  *
  * @return {module:client.Promise} Promise which resolves to the new event body
  */
-OlmEncryption.prototype.encryptMessage = function(room, eventType, content) {
+OlmEncryption.prototype.encryptMessage = async function(room, eventType, content) {
     // pick the list of recipients based on the membership list.
     //
     // TODO: there is a race condition here! What if a new user turns up
     // just as you are sending a secret message?
 
-    const users = utils.map(room.getEncryptionTargetMembers(), function(u) {
+    const members = await room.getEncryptionTargetMembers();
+
+    const users = utils.map(members, function(u) {
         return u.userId;
     });
 
     const self = this;
-    return this._ensureSession(users).then(function() {
-        const payloadFields = {
-            room_id: room.roomId,
-            type: eventType,
-            content: content,
-        };
-
-        const encryptedContent = {
-            algorithm: olmlib.OLM_ALGORITHM,
-            sender_key: self._olmDevice.deviceCurve25519Key,
-            ciphertext: {},
-        };
-
-        const promises = [];
-
-        for (let i = 0; i < users.length; ++i) {
-            const userId = users[i];
-            const devices = self._crypto.getStoredDevicesForUser(userId);
-
-            for (let j = 0; j < devices.length; ++j) {
-                const deviceInfo = devices[j];
-                const key = deviceInfo.getIdentityKey();
-                if (key == self._olmDevice.deviceCurve25519Key) {
-                    // don't bother sending to ourself
-                    continue;
-                }
-                if (deviceInfo.verified == DeviceVerification.BLOCKED) {
-                    // don't bother setting up sessions with blocked users
-                    continue;
-                }
-
-                promises.push(
-                    olmlib.encryptMessageForDevice(
-                        encryptedContent.ciphertext,
-                        self._userId, self._deviceId, self._olmDevice,
-                        userId, deviceInfo, payloadFields,
-                    ),
-                );
+    await this._ensureSession(users);
+
+    const payloadFields = {
+        room_id: room.roomId,
+        type: eventType,
+        content: content,
+    };
+
+    const encryptedContent = {
+        algorithm: olmlib.OLM_ALGORITHM,
+        sender_key: self._olmDevice.deviceCurve25519Key,
+        ciphertext: {},
+    };
+
+    const promises = [];
+
+    for (let i = 0; i < users.length; ++i) {
+        const userId = users[i];
+        const devices = self._crypto.getStoredDevicesForUser(userId);
+
+        for (let j = 0; j < devices.length; ++j) {
+            const deviceInfo = devices[j];
+            const key = deviceInfo.getIdentityKey();
+            if (key == self._olmDevice.deviceCurve25519Key) {
+                // don't bother sending to ourself
+                continue;
             }
+            if (deviceInfo.verified == DeviceVerification.BLOCKED) {
+                // don't bother setting up sessions with blocked users
+                continue;
+            }
+
+            promises.push(
+                olmlib.encryptMessageForDevice(
+                    encryptedContent.ciphertext,
+                    self._userId, self._deviceId, self._olmDevice,
+                    userId, deviceInfo, payloadFields,
+                ),
+            );
         }
+    }
 
-        return Promise.all(promises).return(encryptedContent);
-    });
+    return await Promise.all(promises).return(encryptedContent);
 };
 
 /**