From ce776b99895de9f63eac134b0304ab865faa348a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 23 Dec 2022 11:03:14 +0000 Subject: [PATCH] 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 | 11 +++-- spec/integ/megolm-integ.spec.ts | 88 +++++++++++++++++++++++++++++++++ src/crypto/index.ts | 22 +++++++-- src/models/room-state.ts | 10 ++++ src/models/room.ts | 18 +++++-- 5 files changed, 137 insertions(+), 12 deletions(-) diff --git a/spec/TestClient.ts b/spec/TestClient.ts index 19a8d42c5..3e672bfb8 100644 --- a/spec/TestClient.ts +++ b/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 { + public start(opts: IStartClientOpts = {}): Promise { 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(() => { diff --git a/spec/integ/megolm-integ.spec.ts b/spec/integ/megolm-integ.spec.ts index 334800333..1bdf3a09f 100644 --- a/spec/integ/megolm-integ.spec.ts +++ b/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 { + 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)]); + }); + }); }); diff --git a/src/crypto/index.ts b/src/crypto/index.ts index 91eb99062..b925483e4 100644 --- a/src/crypto/index.ts +++ b/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 { + 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 { const room = this.clientStore.getRoom(roomId); diff --git a/src/models/room-state.ts b/src/models/room-state.ts index 3940659f2..a17d92b56 100644 --- a/src/models/room-state.ts +++ b/src/models/room-state.ts @@ -623,6 +623,16 @@ export class RoomState extends TypedEventEmitter 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 diff --git a/src/models/room.ts b/src/models/room.ts index e33d571e6..0faea02c3 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -888,6 +888,20 @@ export class Room extends ReadReceipt { 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 { 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) => {