Process m.room.encryption events before emitting RoomMember events

spec/integ/megolm-integ.spec.ts

@@ -21,16 +21,19 @@ import * as testUtils from "../test-utils/test-utils";
 import { TestClient } from "../TestClient";
 import { logger } from "../../src/logger";
 import {
+    IClaimOTKsResult,
     IContent,
+    IDownloadKeyResult,
     IEvent,
-    IClaimOTKsResult,
     IJoinedRoom,
+    IndexedDBCryptoStore,
     ISyncResponse,
-    IDownloadKeyResult,
+    IUploadKeysRequest,
     MatrixEvent,
     MatrixEventEvent,
-    IndexedDBCryptoStore,
     Room,
+    RoomMember,
+    RoomStateEvent,
 } from "../../src/matrix";
 import { IDeviceKeys } from "../../src/crypto/dehydration";
 import { DeviceInfo } from "../../src/crypto/deviceinfo";
@@ -327,7 +330,9 @@ describe("megolm", () => {
         const room = aliceTestClient.client.getRoom(ROOM_ID)!;
         const event = room.getLiveTimeline().getEvents()[0];
         expect(event.isEncrypted()).toBe(true);
-        const decryptedEvent = await testUtils.awaitDecryption(event);
+
+        // it probably won't be decrypted yet, because it takes a while to process the olm keys
+        const decryptedEvent = await testUtils.awaitDecryption(event, { waitOnDecryptionFailure: true });
         expect(decryptedEvent.getContent().body).toEqual('42');
     });
 
@@ -873,7 +878,12 @@ describe("megolm", () => {
 
         const room = aliceTestClient.client.getRoom(ROOM_ID)!;
         await room.decryptCriticalEvents();
-        expect(room.getLiveTimeline().getEvents()[0].getContent().body).toEqual('42');
+
+        // it probably won't be decrypted yet, because it takes a while to process the olm keys
+        const decryptedEvent = await testUtils.awaitDecryption(
+            room.getLiveTimeline().getEvents()[0], { waitOnDecryptionFailure: true },
+        );
+        expect(decryptedEvent.getContent().body).toEqual('42');
 
         const exported = await aliceTestClient.client.exportRoomKeys();
 
@@ -1012,7 +1022,9 @@ describe("megolm", () => {
         const room = aliceTestClient.client.getRoom(ROOM_ID)!;
         const event = room.getLiveTimeline().getEvents()[0];
         expect(event.isEncrypted()).toBe(true);
-        const decryptedEvent = await testUtils.awaitDecryption(event);
+
+        // it probably won't be decrypted yet, because it takes a while to process the olm keys
+        const decryptedEvent = await testUtils.awaitDecryption(event, { waitOnDecryptionFailure: true });
         expect(decryptedEvent.getRoomId()).toEqual(ROOM_ID);
         expect(decryptedEvent.getContent()).toEqual({});
         expect(decryptedEvent.getClearContent()).toBeUndefined();

spec/test-utils/test-utils.ts

@@ -362,22 +362,28 @@ export class MockStorageApi {
  * @param {MatrixEvent} event
  * @returns {Promise} promise which resolves (to `event`) when the event has been decrypted
  */
-export async function awaitDecryption(event: MatrixEvent): Promise<MatrixEvent> {
+export async function awaitDecryption(
+    event: MatrixEvent, { waitOnDecryptionFailure = false } = {},
+): Promise<MatrixEvent> {
     // An event is not always decrypted ahead of time
     // getClearContent is a good signal to know whether an event has been decrypted
     // already
     if (event.getClearContent() !== null) {
-        return event;
+        if (waitOnDecryptionFailure && event.isDecryptionFailure()) {
+            logger.log(`${Date.now()} event ${event.getId()} got decryption error; waiting`);
+        } else {
+            return event;
+        }
     } else {
-        logger.log(`${Date.now()} event ${event.getId()} is being decrypted; waiting`);
+        logger.log(`${Date.now()} event ${event.getId()} is not yet decrypted; waiting`);
+    }
 
-        return new Promise((resolve) => {
-            event.once(MatrixEventEvent.Decrypted, (ev) => {
-                logger.log(`${Date.now()} event ${event.getId()} now decrypted`);
-                resolve(ev);
-            });
+    return new Promise((resolve) => {
+        event.once(MatrixEventEvent.Decrypted, (ev) => {
+            logger.log(`${Date.now()} event ${event.getId()} now decrypted`);
+            resolve(ev);
         });
-    }
+    });
 }
 
 export const emitPromise = (e: EventEmitter, k: string): Promise<any> => new Promise(r => e.once(k, r));

src/crypto/index.ts

@@ -2552,12 +2552,45 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
      * @param {boolean=} inhibitDeviceQuery true to suppress device list query for
      *   users in the room (for now). In case lazy loading is enabled,
      *   the device query is always inhibited as the members are not tracked.
+     *
+     * @deprecated It is normally incorrect to call this method directly. Encryption
+     *   is enabled by receiving an `m.room.encryption` event (which we may have sent
+     *   previously).
      */
     public async setRoomEncryption(
         roomId: string,
         config: IRoomEncryption,
         inhibitDeviceQuery?: boolean,
     ): Promise<void> {
+        const room = this.clientStore.getRoom(roomId);
+        if (!room) {
+            throw new Error(`Unable to enable encryption tracking devices in unknown room ${roomId}`);
+        }
+        await this.setRoomEncryptionImpl(room, config);
+        if (!this.lazyLoadMembers && !inhibitDeviceQuery) {
+            this.deviceList.refreshOutdatedDeviceLists();
+        }
+    }
+
+    /**
+     * Set up encryption for a room.
+     *
+     * This is called when an <tt>m.room.encryption</tt> event is received. It saves a flag
+     * for the room in the cryptoStore (if it wasn't already set), sets up an "encryptor" for
+     * the room, and enables device-list tracking for the room.
+     *
+     * It does <em>not</em> initiate a device list query for the room. That is normally
+     * done once we finish processing the sync, in onSyncCompleted.
+     *
+     * @param room The room to enable encryption in.
+     * @param config The encryption config for the room.
+     */
+    private async setRoomEncryptionImpl(
+        room: Room,
+        config: IRoomEncryption,
+    ): Promise<void> {
+        const roomId = room.roomId;
+
         // ignore crypto events with no algorithm defined
         // This will happen if a crypto event is redacted before we fetch the room state
         // It would otherwise just throw later as an unknown algorithm would, but we may
@@ -2625,14 +2658,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             logger.log("Enabling encryption in " + roomId + "; " +
                 "starting to track device lists for all users therein");
 
-            await this.trackRoomDevices(roomId);
-            // TODO: this flag is only not used from MatrixClient::setRoomEncryption
-            // which is never used (inside Element at least)
-            // but didn't want to remove it as it technically would
-            // be a breaking change.
-            if (!inhibitDeviceQuery) {
-                this.deviceList.refreshOutdatedDeviceLists();
-            }
+            await this.trackRoomDevicesImpl(room);
         } else {
             logger.log("Enabling encryption in " + roomId);
         }
@@ -2645,15 +2671,30 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
      * @returns {Promise} when all devices for the room have been fetched and marked to track
      */
     public trackRoomDevices(roomId: string): Promise<void> {
+        const room = this.clientStore.getRoom(roomId);
+        if (!room) {
+            throw new Error(`Unable to start tracking devices in unknown room ${roomId}`);
+        }
+        return this.trackRoomDevicesImpl(room);
+    }
+
+    /**
+     * Make sure we are tracking the device lists for all users in this room.
+     *
+     * This is normally called when we are about to send an encrypted event, to make sure
+     * we have all the devices in the room; but it is also called when processing an
+     * m.room.encryption state event (if lazy-loading is disabled), or when members are
+     * loaded (if lazy-loading is enabled), to prepare the device list.
+     *
+     * @param room Room to enable device-list tracking in
+     */
+    private trackRoomDevicesImpl(room: Room): Promise<void> {
+        const roomId = room.roomId;
         const trackMembers = async (): Promise<void> => {
             // not an encrypted room
             if (!this.roomEncryptors.has(roomId)) {
                 return;
             }
-            const room = this.clientStore.getRoom(roomId);
-            if (!room) {
-                throw new Error(`Unable to start tracking devices in unknown room ${roomId}`);
-            }
             logger.log(`Starting to track devices for room ${roomId} ...`);
             const members = await room.getEncryptionTargetMembers();
             members.forEach((m) => {
@@ -2815,17 +2856,14 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             // MatrixClient has already checked that this room should be encrypted,
             // so this is an unexpected situation.
             throw new Error(
-                "Room was previously configured to use encryption, but is " +
+                "Room " + roomId + " was previously configured to use encryption, but is " +
                 "no longer. Perhaps the homeserver is hiding the " +
                 "configuration event.",
             );
         }
 
-        if (!this.roomDeviceTrackingState[roomId]) {
-            this.trackRoomDevices(roomId);
-        }
         // wait for all the room devices to be loaded
-        await this.roomDeviceTrackingState[roomId];
+        await this.trackRoomDevicesImpl(room);
 
         let content = event.getContent();
         // If event has an m.relates_to then we need
@@ -2972,19 +3010,12 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
     /**
      * handle an m.room.encryption event
      *
-     * @param {module:models/event.MatrixEvent} event encryption event
+     * @param room in which the event was received
+     * @param event encryption event to be processed
      */
-    public async onCryptoEvent(event: MatrixEvent): Promise<void> {
-        const roomId = event.getRoomId()!;
+    public async onCryptoEvent(room: Room, event: MatrixEvent): Promise<void> {
         const content = event.getContent<IRoomEncryption>();
-
-        try {
-            // inhibit the device list refresh for now - it will happen once we've
-            // finished processing the sync, in onSyncCompleted.
-            await this.setRoomEncryption(roomId, content, true);
-        } catch (e) {
-            logger.error(`Error configuring encryption in room ${roomId}`, e);
-        }
+        await this.setRoomEncryptionImpl(room, content);
     }
 
     /**

src/sliding-sync-sdk.ts

@@ -703,7 +703,7 @@ export class SlidingSyncSdk {
         const processRoomEvent = async (e: MatrixEvent): Promise<void> => {
             client.emit(ClientEvent.Event, e);
             if (e.isState() && e.getType() == EventType.RoomEncryption && this.opts.crypto) {
-                await this.opts.crypto.onCryptoEvent(e);
+                await this.opts.crypto.onCryptoEvent(room, e);
             }
         };
 

src/sync.ts

@@ -1362,6 +1362,18 @@ export class SyncApi {
                 }
             }
 
+            // process any crypto events *before* emitting the RoomStateEvent events. This
+            // avoids a race condition if the application tries to send a message after the
+            // state event is processed, but before crypto is enabled, which then causes the
+            // crypto layer to complain.
+            if (this.opts.crypto) {
+                for (const e of stateEvents.concat(events)) {
+                    if (e.isState() && e.getType() === EventType.RoomEncryption && e.getStateKey() === "") {
+                        await this.opts.crypto.onCryptoEvent(room, e);
+                    }
+                }
+            }
+
             try {
                 await this.injectRoomEvents(room, stateEvents, events, syncEventData.fromCache);
             } catch (e) {
@@ -1389,21 +1401,11 @@ export class SyncApi {
 
             this.processEventsForNotifs(room, events);
 
-            const processRoomEvent = async (e): Promise<void> => {
-                client.emit(ClientEvent.Event, e);
-                if (e.isState() && e.getType() == "m.room.encryption" && this.opts.crypto) {
-                    await this.opts.crypto.onCryptoEvent(e);
-                }
-            };
-
-            await utils.promiseMapSeries(stateEvents, processRoomEvent);
-            await utils.promiseMapSeries(events, processRoomEvent);
-            ephemeralEvents.forEach(function(e) {
-                client.emit(ClientEvent.Event, e);
-            });
-            accountDataEvents.forEach(function(e) {
-                client.emit(ClientEvent.Event, e);
-            });
+            const emitEvent = (e: MatrixEvent): boolean => client.emit(ClientEvent.Event, e);
+            stateEvents.forEach(emitEvent);
+            events.forEach(emitEvent);
+            ephemeralEvents.forEach(emitEvent);
+            accountDataEvents.forEach(emitEvent);
 
             // Decrypt only the last message in all rooms to make sure we can generate a preview
             // And decrypt all events after the recorded read receipt to ensure an accurate