From 869576747c22f0f4ebf9ee8dfc2b0b18927eaebb Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 26 Jan 2024 10:21:33 +0000 Subject: [PATCH] Refactor `MatrixClient.encryptAndSendEvent` (#4031) * Replace `pendingEventEncryption` with a Set We don't actually need the promise, so no need to save it. This also fixes a resource leak, where we would leak a Promise and a HashMap entry on each encrypted event. * Convert `encryptEventIfNeeded` to async function This means that it will always return a promise, so `encryptAndSendEvent` can't tell if we are actually encrypting or not. Hence, also move the `updatePendingEventStatus` into `encryptEventIfNeeded`. * Simplify `encryptAndSendEvent` Rewrite this as async. * Factor out `MatrixClient.shouldEncryptEventForRoom` * Inline a call to `isRoomEncrypted` I want to deprecate this thing --- spec/unit/matrix-client.spec.ts | 26 +++-- spec/unit/queueToDevice.spec.ts | 1 + src/client.ts | 192 ++++++++++++++++++-------------- src/models/event.ts | 2 +- 4 files changed, 128 insertions(+), 93 deletions(-) diff --git a/spec/unit/matrix-client.spec.ts b/spec/unit/matrix-client.spec.ts index 2a79293e2ad..f1382bca563 100644 --- a/spec/unit/matrix-client.spec.ts +++ b/spec/unit/matrix-client.spec.ts @@ -65,7 +65,7 @@ import { PolicyScope, } from "../../src/models/invites-ignorer"; import { IOlmDevice } from "../../src/crypto/algorithms/megolm"; -import { QueryDict } from "../../src/utils"; +import { defer, QueryDict } from "../../src/utils"; import { SyncState } from "../../src/sync"; import * as featureUtils from "../../src/feature"; import { StubStore } from "../../src/store/stub"; @@ -1453,6 +1453,8 @@ describe("MatrixClient", function () { hasEncryptionStateEvent: jest.fn().mockReturnValue(true), } as unknown as Room; + let mockCrypto: Mocked; + let event: MatrixEvent; beforeEach(async () => { event = new MatrixEvent({ @@ -1467,11 +1469,12 @@ describe("MatrixClient", function () { expect(getRoomId).toEqual(roomId); return mockRoom; }; - client.crypto = client["cryptoBackend"] = { - // mock crypto - encryptEvent: () => new Promise(() => {}), + + mockCrypto = { + encryptEvent: jest.fn(), stop: jest.fn(), - } as unknown as Crypto; + } as unknown as Mocked; + client.crypto = client["cryptoBackend"] = mockCrypto; }); function assertCancelled() { @@ -1488,12 +1491,21 @@ describe("MatrixClient", function () { }); it("should cancel an event which is encrypting", async () => { + const encryptEventDefer = defer(); + mockCrypto.encryptEvent.mockReturnValue(encryptEventDefer.promise); + + const statusPromise = testUtils.emitPromise(event, "Event.status"); // @ts-ignore protected method access - client.encryptAndSendEvent(mockRoom, event); - await testUtils.emitPromise(event, "Event.status"); + const encryptAndSendPromise = client.encryptAndSendEvent(mockRoom, event); + await statusPromise; expect(event.status).toBe(EventStatus.ENCRYPTING); client.cancelPendingEvent(event); assertCancelled(); + + // now let the encryption complete, and check that the message is not sent. + encryptEventDefer.resolve(); + await encryptAndSendPromise; + assertCancelled(); }); it("should cancel an event which is not sent", () => { diff --git a/spec/unit/queueToDevice.spec.ts b/spec/unit/queueToDevice.spec.ts index 1099bcb82e8..f9b90304e02 100644 --- a/spec/unit/queueToDevice.spec.ts +++ b/spec/unit/queueToDevice.spec.ts @@ -265,6 +265,7 @@ describe.each([[StoreType.Memory], [StoreType.IndexedDB]])("queueToDevice (%s st }); const mockRoom = { updatePendingEvent: jest.fn(), + hasEncryptionStateEvent: jest.fn().mockReturnValue(false), } as unknown as Room; client.resendEvent(dummyEvent, mockRoom); diff --git a/src/client.ts b/src/client.ts index 44d9fbd33a9..78011c1b83c 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1305,7 +1305,13 @@ export class MatrixClient extends TypedEventEmitter>(); + + /** IDs of events which are currently being encrypted. + * + * This is part of the cancellation mechanism: if the event is no longer listed here when encryption completes, + * that tells us that it has been cancelled, and we should not send it. + */ + private eventsBeingEncrypted = new Set(); private useE2eForGroupCall = true; private toDeviceMessageQueue: ToDeviceMessageQueue; @@ -4448,9 +4454,10 @@ export class MatrixClient extends TypedEventEmitter { - let cancelled = false; - // Add an extra Promise.resolve() to turn synchronous exceptions into promise rejections, - // so that we can handle synchronous and asynchronous exceptions with the - // same code path. - return Promise.resolve() - .then(() => { - const encryptionPromise = this.encryptEventIfNeeded(event, room ?? undefined); - if (!encryptionPromise) return null; // doesn't need encryption - - this.pendingEventEncryption.set(event.getId()!, encryptionPromise); - this.updatePendingEventStatus(room, event, EventStatus.ENCRYPTING); - return encryptionPromise.then(() => { - if (!this.pendingEventEncryption.has(event.getId()!)) { - // cancelled via MatrixClient::cancelPendingEvent - cancelled = true; - return; - } - this.updatePendingEventStatus(room, event, EventStatus.SENDING); - }); - }) - .then(() => { - if (cancelled) return {} as ISendEventResponse; - let promise: Promise | null = null; - if (this.scheduler) { - // if this returns a promise then the scheduler has control now and will - // resolve/reject when it is done. Internally, the scheduler will invoke - // processFn which is set to this._sendEventHttpRequest so the same code - // path is executed regardless. - promise = this.scheduler.queueEvent(event); - if (promise && this.scheduler.getQueueForEvent(event)!.length > 1) { - // event is processed FIFO so if the length is 2 or more we know - // this event is stuck behind an earlier event. - this.updatePendingEventStatus(room, event, EventStatus.QUEUED); - } - } + protected async encryptAndSendEvent(room: Room | null, event: MatrixEvent): Promise { + try { + let cancelled: boolean; + this.eventsBeingEncrypted.add(event.getId()!); + try { + await this.encryptEventIfNeeded(event, room ?? undefined); + } finally { + cancelled = !this.eventsBeingEncrypted.delete(event.getId()!); + } - if (!promise) { - promise = this.sendEventHttpRequest(event); - if (room) { - promise = promise.then((res) => { - room.updatePendingEvent(event, EventStatus.SENT, res["event_id"]); - return res; - }); - } - } + if (cancelled) { + // cancelled via MatrixClient::cancelPendingEvent + return {} as ISendEventResponse; + } - return promise; - }) - .catch((err) => { - this.logger.error("Error sending event", err.stack || err); - try { - // set the error on the event before we update the status: - // updating the status emits the event, so the state should be - // consistent at that point. - event.error = err; - this.updatePendingEventStatus(room, event, EventStatus.NOT_SENT); - } catch (e) { - this.logger.error("Exception in error handler!", (e).stack || err); + // encryptEventIfNeeded may have updated the status from SENDING to ENCRYPTING. If so, we need + // to put it back. + if (event.status === EventStatus.ENCRYPTING) { + this.updatePendingEventStatus(room, event, EventStatus.SENDING); + } + + let promise: Promise | null = null; + if (this.scheduler) { + // if this returns a promise then the scheduler has control now and will + // resolve/reject when it is done. Internally, the scheduler will invoke + // processFn which is set to this._sendEventHttpRequest so the same code + // path is executed regardless. + promise = this.scheduler.queueEvent(event); + if (promise && this.scheduler.getQueueForEvent(event)!.length > 1) { + // event is processed FIFO so if the length is 2 or more we know + // this event is stuck behind an earlier event. + this.updatePendingEventStatus(room, event, EventStatus.QUEUED); } - if (err instanceof MatrixError) { - err.event = event; + } + + if (!promise) { + promise = this.sendEventHttpRequest(event); + if (room) { + promise = promise.then((res) => { + room.updatePendingEvent(event, EventStatus.SENT, res["event_id"]); + return res; + }); } - throw err; - }); - } + } - private encryptEventIfNeeded(event: MatrixEvent, room?: Room): Promise | null { - if (event.isEncrypted()) { - // this event has already been encrypted; this happens if the - // encryption step succeeded, but the send step failed on the first - // attempt. - return null; + return await promise; + } catch (err) { + this.logger.error("Error sending event", err); + try { + // set the error on the event before we update the status: + // updating the status emits the event, so the state should be + // consistent at that point. + event.error = err; + this.updatePendingEventStatus(room, event, EventStatus.NOT_SENT); + } catch (e) { + this.logger.error("Exception in error handler!", e); + } + if (err instanceof MatrixError) { + err.event = event; + } + throw err; } + } - if (event.isRedaction()) { - // Redactions do not support encryption in the spec at this time, - // whilst it mostly worked in some clients, it wasn't compliant. - return null; - } + private async encryptEventIfNeeded(event: MatrixEvent, room?: Room): Promise { + // If the room is unknown, we cannot encrypt for it + if (!room) return; - if (!room || !this.isRoomEncrypted(event.getRoomId()!)) { - return null; - } + if (!this.shouldEncryptEventForRoom(event, room)) return; if (!this.cryptoBackend && this.usingExternalCrypto) { // The client has opted to allow sending messages to encrypted - // rooms even if the room is encrypted, and we haven't setup + // rooms even if the room is encrypted, and we haven't set up // crypto. This is useful for users of matrix-org/pantalaimon - return null; + return; + } + + if (!this.cryptoBackend) { + throw new Error("This room is configured to use encryption, but your client does not support encryption."); + } + + this.updatePendingEventStatus(room, event, EventStatus.ENCRYPTING); + await this.cryptoBackend.encryptEvent(event, room); + } + + /** + * Determine whether a given event should be encrypted when we send it to the given room. + * + * This takes into account event type and room configuration. + */ + private shouldEncryptEventForRoom(event: MatrixEvent, room: Room): boolean { + if (event.isEncrypted()) { + // this event has already been encrypted; this happens if the + // encryption step succeeded, but the send step failed on the first + // attempt. + return false; } if (event.getType() === EventType.Reaction) { @@ -4852,14 +4865,23 @@ export class MatrixClient extends TypedEventEmitter