Break coupling between Room and Crypto.trackRoomDevices (#2998)

`Room` and `Crypto` currently have some tight coupling in the form of a call to
`trackRoomDevices` when out-of-band members are loaded. We can improve this by
instead having Crypto listen out for a `RoomSateEvent.Update` notification.

spec/TestClient.ts

@@ -24,7 +24,7 @@ import MockHttpBackend from "matrix-mock-request";
 import { LocalStorageCryptoStore } from "../src/crypto/store/localStorage-crypto-store";
 import { logger } from "../src/logger";
 import { syncPromise } from "./test-utils/test-utils";
-import { createClient } from "../src/matrix";
+import { createClient, IStartClientOpts } from "../src/matrix";
 import { ICreateClientOpts, IDownloadKeyResult, MatrixClient, PendingEventOrdering } from "../src/client";
 import { MockStorageApi } from "./MockStorageApi";
 import { encodeUri } from "../src/utils";
@@ -79,9 +79,12 @@ export class TestClient {
     /**
      * start the client, and wait for it to initialise.
      */
-    public start(): Promise<void> {
+    public start(opts: IStartClientOpts = {}): Promise<void> {
         logger.log(this + ": starting");
-        this.httpBackend.when("GET", "/versions").respond(200, {});
+        this.httpBackend.when("GET", "/versions").respond(200, {
+            // we have tests that rely on support for lazy-loading members
+            versions: ["r0.5.0"],
+        });
         this.httpBackend.when("GET", "/pushrules").respond(200, {});
         this.httpBackend.when("POST", "/filter").respond(200, { filter_id: "fid" });
         this.expectDeviceKeyUpload();
@@ -93,6 +96,8 @@ export class TestClient {
         this.client.startClient({
             // set this so that we can get hold of failed events
             pendingEventOrdering: PendingEventOrdering.Detached,
+
+            ...opts,
         });
 
         return Promise.all([this.httpBackend.flushAllExpected(), syncPromise(this.client)]).then(() => {

spec/integ/megolm-integ.spec.ts

@@ -1590,4 +1590,92 @@ describe("megolm", () => {
             aliceTestClient.httpBackend.flush("/send/m.room.encrypted/", 1),
         ]);
     });
+
+    describe("Lazy-loading member lists", () => {
+        let p2pSession: Olm.Session;
+
+        beforeEach(async () => {
+            // set up the aliceTestClient so that it is a room with no known members
+            aliceTestClient.expectKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
+            await aliceTestClient.start({ lazyLoadMembers: true });
+            aliceTestClient.client.setGlobalErrorOnUnknownDevices(false);
+
+            aliceTestClient.httpBackend.when("GET", "/sync").respond(200, getSyncResponse([]));
+            await aliceTestClient.flushSync();
+
+            p2pSession = await establishOlmSession(aliceTestClient, testOlmAccount);
+        });
+
+        async function expectMembershipRequest(roomId: string, members: string[]): Promise<void> {
+            const membersPath = `/rooms/${encodeURIComponent(roomId)}/members?not_membership=leave`;
+            aliceTestClient.httpBackend.when("GET", membersPath).respond(200, {
+                chunk: [
+                    testUtils.mkMembershipCustom({
+                        membership: "join",
+                        sender: "@bob:xyz",
+                    }),
+                ],
+            });
+            await aliceTestClient.httpBackend.flush(membersPath, 1);
+        }
+
+        it("Sending an event initiates a member list sync", async () => {
+            // we expect a call to the /members list...
+            const memberListPromise = expectMembershipRequest(ROOM_ID, ["@bob:xyz"]);
+
+            // then a request for bob's devices...
+            aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, getTestKeysQueryResponse("@bob:xyz"));
+
+            // then a to-device with the room_key
+            const inboundGroupSessionPromise = expectSendRoomKey(
+                aliceTestClient.httpBackend,
+                "@bob:xyz",
+                testOlmAccount,
+                p2pSession,
+            );
+
+            // and finally the megolm message
+            const megolmMessagePromise = expectSendMegolmMessage(
+                aliceTestClient.httpBackend,
+                inboundGroupSessionPromise,
+            );
+
+            // kick it off
+            const sendPromise = aliceTestClient.client.sendTextMessage(ROOM_ID, "test");
+
+            await Promise.all([
+                sendPromise,
+                megolmMessagePromise,
+                memberListPromise,
+                aliceTestClient.httpBackend.flush("/keys/query", 1),
+            ]);
+        });
+
+        it("loading the membership list inhibits a later load", async () => {
+            const room = aliceTestClient.client.getRoom(ROOM_ID)!;
+            await Promise.all([room.loadMembersIfNeeded(), expectMembershipRequest(ROOM_ID, ["@bob:xyz"])]);
+
+            // expect a request for bob's devices...
+            aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, getTestKeysQueryResponse("@bob:xyz"));
+
+            // then a to-device with the room_key
+            const inboundGroupSessionPromise = expectSendRoomKey(
+                aliceTestClient.httpBackend,
+                "@bob:xyz",
+                testOlmAccount,
+                p2pSession,
+            );
+
+            // and finally the megolm message
+            const megolmMessagePromise = expectSendMegolmMessage(
+                aliceTestClient.httpBackend,
+                inboundGroupSessionPromise,
+            );
+
+            // kick it off
+            const sendPromise = aliceTestClient.client.sendTextMessage(ROOM_ID, "test");
+
+            await Promise.all([sendPromise, megolmMessagePromise, aliceTestClient.httpBackend.flush("/keys/query", 1)]);
+        });
+    });
 });

src/crypto/index.ts

@@ -89,6 +89,7 @@ import { ISyncResponse } from "../sync-accumulator";
 import { ISignatures } from "../@types/signed";
 import { IMessage } from "./algorithms/olm";
 import { CryptoBackend } from "../common-crypto/CryptoBackend";
+import { RoomState, RoomStateEvent } from "../models/room-state";
 
 const DeviceVerification = DeviceInfo.DeviceVerification;
 
@@ -2606,14 +2607,23 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
             await storeConfigPromise;
         }
 
-        if (!this.lazyLoadMembers) {
+        logger.log(`Enabling encryption in ${roomId}`);
-            logger.log(
-                "Enabling encryption in " + roomId + "; " + "starting to track device lists for all users therein",
-            );
 
+        // we don't want to force a download of the full membership list of this room, but as soon as we have that
+        // list we can start tracking the device list.
+        if (room.membersLoaded()) {
             await this.trackRoomDevicesImpl(room);
         } else {
-            logger.log("Enabling encryption in " + roomId);
+            // wait for the membership list to be loaded
+            const onState = (_state: RoomState): void => {
+                room.off(RoomStateEvent.Update, onState);
+                if (room.membersLoaded()) {
+                    this.trackRoomDevicesImpl(room).catch((e) => {
+                        logger.error(`Error enabling device tracking in ${roomId}`, e);
+                    });
+                }
+            };
+            room.on(RoomStateEvent.Update, onState);
         }
     }
 
@@ -2622,6 +2632,8 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
      *
      * @param roomId - The room ID to start tracking devices in.
      * @returns when all devices for the room have been fetched and marked to track
+     * @deprecated there's normally no need to call this function: device list tracking
+     *    will be enabled as soon as we have the full membership list.
      */
     public trackRoomDevices(roomId: string): Promise<void> {
         const room = this.clientStore.getRoom(roomId);

src/models/room-state.ts

@@ -623,6 +623,16 @@ export class RoomState extends TypedEventEmitter<EmittedEvents, EventHandlerMap>
         return this.oobMemberFlags.status === OobStatus.NotStarted;
     }
 
+    /**
+     * Check if loading of out-of-band-members has completed
+     *
+     * @returns true if the full membership list of this room has been loaded. False if it is not started or is in
+     *    progress.
+     */
+    public outOfBandMembersReady(): boolean {
+        return this.oobMemberFlags.status === OobStatus.Finished;
+    }
+
     /**
      * Mark this room state as waiting for out-of-band members,
      * ensuring it doesn't ask for them to be requested again

src/models/room.ts

@@ -888,6 +888,20 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         return { memberEvents, fromServer };
     }
 
+    /**
+     * Check if loading of out-of-band-members has completed
+     *
+     * @returns true if the full membership list of this room has been loaded (including if lazy-loading is disabled).
+     *    False if the load is not started or is in progress.
+     */
+    public membersLoaded(): boolean {
+        if (!this.opts.lazyLoadMembers) {
+            return true;
+        }
+
+        return this.currentState.outOfBandMembersReady();
+    }
+
     /**
      * Preloads the member list in case lazy loading
      * of memberships is in use. Can be called multiple times,
@@ -909,10 +923,6 @@ export class Room extends ReadReceipt<RoomEmittedEvents, RoomEventHandlerMap> {
         const inMemoryUpdate = this.loadMembers()
             .then((result) => {
                 this.currentState.setOutOfBandMembers(result.memberEvents);
-                // now the members are loaded, start to track the e2e devices if needed
-                if (this.client.isCryptoEnabled() && this.client.isRoomEncrypted(this.roomId)) {
-                    this.client.crypto!.trackRoomDevices(this.roomId);
-                }
                 return result.fromServer;
             })
             .catch((err) => {