From 22f10f71b8c505ac6e8e542f62d5b68c0b093d04 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 5 Jan 2023 09:54:44 +0000 Subject: [PATCH 1/7] indirect decryption attempts via Client (#3023) ... to reduce the number of things referring to `client.crypto` --- src/models/room.ts | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index 0faea02c371..ad21e626450 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -441,9 +441,7 @@ export class Room extends ReadReceipt { }); events.forEach(async (serializedEvent: Partial) => { const event = mapper(serializedEvent); - if (event.getType() === EventType.RoomMessageEncrypted && this.client.isCryptoEnabled()) { - await event.attemptDecryption(this.client.crypto!); - } + await client.decryptEventIfNeeded(event); event.setStatus(EventStatus.NOT_SENT); this.addPendingEvent(event, event.getTxnId()!); }); @@ -503,9 +501,8 @@ export class Room extends ReadReceipt { const decryptionPromises = events .slice(readReceiptTimelineIndex) - .filter((event) => event.shouldAttemptDecryption()) .reverse() - .map((event) => event.attemptDecryption(this.client.crypto!, { isRetry: true })); + .map((event) => this.client.decryptEventIfNeeded(event, { isRetry: true })); await Promise.allSettled(decryptionPromises); } @@ -521,9 +518,9 @@ export class Room extends ReadReceipt { const decryptionPromises = this.getUnfilteredTimelineSet() .getLiveTimeline() .getEvents() - .filter((event) => event.shouldAttemptDecryption()) + .slice(0) // copy before reversing .reverse() - .map((event) => event.attemptDecryption(this.client.crypto!, { isRetry: true })); + .map((event) => this.client.decryptEventIfNeeded(event, { isRetry: true })); await Promise.allSettled(decryptionPromises); } From 030abe156350deaa05eb421566a40bd0924f7c03 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 5 Jan 2023 09:54:56 +0000 Subject: [PATCH 2/7] Pass to-device messages into rust crypto-sdk (#3021) We need a separate API, because `ClientEvent.ToDeviceEvent` is only emitted for successfully decrypted to-device events --- spec/unit/rust-crypto.spec.ts | 44 +++++++++++++++++++++++++++++- src/common-crypto/CryptoBackend.ts | 15 ++++++++++ src/crypto/index.ts | 17 +++++++++++- src/event-mapper.ts | 3 ++ src/rust-crypto/rust-crypto.ts | 20 ++++++++++++++ src/sliding-sync-sdk.ts | 13 ++++++--- src/sync.ts | 21 ++++++-------- 7 files changed, 115 insertions(+), 18 deletions(-) diff --git a/spec/unit/rust-crypto.spec.ts b/spec/unit/rust-crypto.spec.ts index 822c08bf6c9..0bcc88d5d26 100644 --- a/spec/unit/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto.spec.ts @@ -22,6 +22,7 @@ import { KeysClaimRequest, KeysQueryRequest, KeysUploadRequest, + OlmMachine, SignatureUploadRequest, } from "@matrix-org/matrix-sdk-crypto-js"; import { Mocked } from "jest-mock"; @@ -29,7 +30,7 @@ import MockHttpBackend from "matrix-mock-request"; import { RustCrypto } from "../../src/rust-crypto/rust-crypto"; import { initRustCrypto } from "../../src/rust-crypto"; -import { HttpApiEvent, HttpApiEventHandlerMap, IHttpOpts, MatrixHttpApi } from "../../src"; +import { HttpApiEvent, HttpApiEventHandlerMap, IHttpOpts, IToDeviceEvent, MatrixHttpApi } from "../../src"; import { TypedEventEmitter } from "../../src/models/typed-event-emitter"; afterEach(() => { @@ -57,6 +58,47 @@ describe("RustCrypto", () => { }); }); + describe("to-device messages", () => { + let rustCrypto: RustCrypto; + + beforeEach(async () => { + const mockHttpApi = {} as MatrixHttpApi; + rustCrypto = (await initRustCrypto(mockHttpApi, TEST_USER, TEST_DEVICE_ID)) as RustCrypto; + }); + + it("should pass through unencrypted to-device messages", async () => { + const inputs: IToDeviceEvent[] = [ + { content: { key: "value" }, type: "org.matrix.test", sender: "@alice:example.com" }, + ]; + const res = await rustCrypto.preprocessToDeviceMessages(inputs); + expect(res).toEqual(inputs); + }); + + it("should pass through bad encrypted messages", async () => { + const olmMachine: OlmMachine = rustCrypto["olmMachine"]; + const keys = olmMachine.identityKeys; + const inputs: IToDeviceEvent[] = [ + { + type: "m.room.encrypted", + content: { + algorithm: "m.olm.v1.curve25519-aes-sha2", + sender_key: "IlRMeOPX2e0MurIyfWEucYBRVOEEUMrOHqn/8mLqMjA", + ciphertext: { + [keys.curve25519.toBase64()]: { + type: 0, + body: "ajyjlghi", + }, + }, + }, + sender: "@alice:example.com", + }, + ]; + + const res = await rustCrypto.preprocessToDeviceMessages(inputs); + expect(res).toEqual(inputs); + }); + }); + describe("outgoing requests", () => { /** the RustCrypto implementation under test */ let rustCrypto: RustCrypto; diff --git a/src/common-crypto/CryptoBackend.ts b/src/common-crypto/CryptoBackend.ts index 82db3f28a9c..a90afdf3a82 100644 --- a/src/common-crypto/CryptoBackend.ts +++ b/src/common-crypto/CryptoBackend.ts @@ -15,6 +15,7 @@ limitations under the License. */ import type { IEventDecryptionResult, IMegolmSessionData } from "../@types/crypto"; +import type { IToDeviceEvent } from "../sync-accumulator"; import { MatrixEvent } from "../models/event"; /** @@ -74,6 +75,20 @@ export interface CryptoBackend extends SyncCryptoCallbacks { /** The methods which crypto implementations should expose to the Sync api */ export interface SyncCryptoCallbacks { + /** + * Called by the /sync loop whenever there are incoming to-device messages. + * + * The implementation may preprocess the received messages (eg, decrypt them) and return an + * updated list of messages for dispatch to the rest of the system. + * + * Note that, unlike {@link ClientEvent.ToDeviceEvent} events, this is called on the raw to-device + * messages, rather than the results of any decryption attempts. + * + * @param events - the received to-device messages + * @returns A list of preprocessed to-device messages. + */ + preprocessToDeviceMessages(events: IToDeviceEvent[]): Promise; + /** * Called by the /sync loop after each /sync response is processed. * diff --git a/src/crypto/index.ts b/src/crypto/index.ts index 24b36b08143..458132e75bb 100644 --- a/src/crypto/index.ts +++ b/src/crypto/index.ts @@ -85,7 +85,7 @@ import { CryptoStore } from "./store/base"; import { IVerificationChannel } from "./verification/request/Channel"; import { TypedEventEmitter } from "../models/typed-event-emitter"; import { IContent } from "../models/event"; -import { ISyncResponse } from "../sync-accumulator"; +import { ISyncResponse, IToDeviceEvent } from "../sync-accumulator"; import { ISignatures } from "../@types/signed"; import { IMessage } from "./algorithms/olm"; import { CryptoBackend, OnSyncCompletedData } from "../common-crypto/CryptoBackend"; @@ -3198,6 +3198,21 @@ export class Crypto extends TypedEventEmitter { + // all we do here is filter out encrypted to-device messages with the wrong algorithm. Decryption + // happens later in decryptEvent, via the EventMapper + return events.filter((toDevice) => { + if ( + toDevice.type === EventType.RoomMessageEncrypted && + !["m.olm.v1.curve25519-aes-sha2"].includes(toDevice.content?.algorithm) + ) { + logger.log("Ignoring invalid encrypted to-device event from " + toDevice.sender); + return false; + } + return true; + }); + } + private onToDeviceEvent = (event: MatrixEvent): void => { try { logger.log( diff --git a/src/event-mapper.ts b/src/event-mapper.ts index 81d3d772a59..87db88d6407 100644 --- a/src/event-mapper.ts +++ b/src/event-mapper.ts @@ -60,6 +60,9 @@ export function eventMapperFor(client: MatrixClient, options: MapperOpts): Event event.setThread(thread); } + // TODO: once we get rid of the old libolm-backed crypto, we can restrict this to room events (rather than + // to-device events), because the rust implementation decrypts to-device messages at a higher level. + // Generally we probably want to use a different eventMapper implementation for to-device events because if (event.isEncrypted()) { if (!preventReEmit) { client.reEmitter.reEmit(event, [MatrixEventEvent.Decrypted]); diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 7231899b52c..9bb75be70ee 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -24,6 +24,7 @@ import { } from "@matrix-org/matrix-sdk-crypto-js"; import type { IEventDecryptionResult, IMegolmSessionData } from "../@types/crypto"; +import type { IToDeviceEvent } from "../sync-accumulator"; import { MatrixEvent } from "../models/event"; import { CryptoBackend, OnSyncCompletedData } from "../common-crypto/CryptoBackend"; import { logger } from "../logger"; @@ -93,6 +94,25 @@ export class RustCrypto implements CryptoBackend { // /////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + /** called by the sync loop to preprocess incoming to-device messages + * + * @param events - the received to-device messages + * @returns A list of preprocessed to-device messages. + */ + public async preprocessToDeviceMessages(events: IToDeviceEvent[]): Promise { + // send the received to-device messages into receiveSyncChanges. We have no info on device-list changes, + // one-time-keys, or fallback keys, so just pass empty data. + const result = await this.olmMachine.receiveSyncChanges( + JSON.stringify(events), + new RustSdkCryptoJs.DeviceLists(), + new Map(), + new Set(), + ); + + // receiveSyncChanges returns a JSON-encoded list of decrypted to-device messages. + return JSON.parse(result); + } + /** called by the sync loop after processing each sync. * * TODO: figure out something equivalent for sliding sync. diff --git a/src/sliding-sync-sdk.ts b/src/sliding-sync-sdk.ts index 18c94c16836..91ff9d7a75e 100644 --- a/src/sliding-sync-sdk.ts +++ b/src/sliding-sync-sdk.ts @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +import type { SyncCryptoCallbacks } from "./common-crypto/CryptoBackend"; import { NotificationCountType, Room, RoomEvent } from "./models/room"; import { logger } from "./logger"; import * as utils from "./utils"; @@ -127,7 +128,7 @@ type ExtensionToDeviceResponse = { class ExtensionToDevice implements Extension { private nextBatch: string | null = null; - public constructor(private readonly client: MatrixClient) {} + public constructor(private readonly client: MatrixClient, private readonly cryptoCallbacks?: SyncCryptoCallbacks) {} public name(): string { return "to_device"; @@ -150,8 +151,12 @@ class ExtensionToDevice implements Extension { const cancelledKeyVerificationTxns: string[] = []; - data.events - ?.map(this.client.getEventMapper()) + let events = data["events"] || []; + if (events.length > 0 && this.cryptoCallbacks) { + events = await this.cryptoCallbacks.preprocessToDeviceMessages(events); + } + events + .map(this.client.getEventMapper()) .map((toDeviceEvent) => { // map is a cheap inline forEach // We want to flag m.key.verification.start events as cancelled @@ -373,7 +378,7 @@ export class SlidingSyncSdk { this.slidingSync.on(SlidingSyncEvent.Lifecycle, this.onLifecycle.bind(this)); this.slidingSync.on(SlidingSyncEvent.RoomData, this.onRoomData.bind(this)); const extensions: Extension[] = [ - new ExtensionToDevice(this.client), + new ExtensionToDevice(this.client, this.syncOpts.cryptoCallbacks), new ExtensionAccountData(this.client), new ExtensionTyping(this.client), new ExtensionReceipts(this.client), diff --git a/src/sync.ts b/src/sync.ts index 0ba52ba8c0c..11fe3cc102b 100644 --- a/src/sync.ts +++ b/src/sync.ts @@ -48,6 +48,7 @@ import { IStrippedState, ISyncResponse, ITimeline, + IToDeviceEvent, } from "./sync-accumulator"; import { MatrixEvent } from "./models/event"; import { MatrixError, Method } from "./http-api"; @@ -1170,19 +1171,15 @@ export class SyncApi { } // handle to-device events - if (Array.isArray(data.to_device?.events) && data.to_device!.events.length > 0) { - const cancelledKeyVerificationTxns: string[] = []; - data.to_device!.events.filter((eventJSON) => { - if ( - eventJSON.type === EventType.RoomMessageEncrypted && - !["m.olm.v1.curve25519-aes-sha2"].includes(eventJSON.content?.algorithm) - ) { - logger.log("Ignoring invalid encrypted to-device event from " + eventJSON.sender); - return false; - } + if (data.to_device && Array.isArray(data.to_device.events) && data.to_device.events.length > 0) { + let toDeviceMessages: IToDeviceEvent[] = data.to_device.events; - return true; - }) + if (this.syncOpts.cryptoCallbacks) { + toDeviceMessages = await this.syncOpts.cryptoCallbacks.preprocessToDeviceMessages(toDeviceMessages); + } + + const cancelledKeyVerificationTxns: string[] = []; + toDeviceMessages .map(client.getEventMapper({ toDevice: true })) .map((toDeviceEvent) => { // map is a cheap inline forEach From 695b773f8bedea5f6b4072aef602ac2a057f3538 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 5 Jan 2023 15:27:09 +0100 Subject: [PATCH 3/7] Fix false key requests after verifying new device (#3029) --- .../setDeviceVerification.spec.ts | 56 ------------------- src/crypto/index.ts | 3 - 2 files changed, 59 deletions(-) delete mode 100644 spec/unit/crypto/verification/setDeviceVerification.spec.ts diff --git a/spec/unit/crypto/verification/setDeviceVerification.spec.ts b/spec/unit/crypto/verification/setDeviceVerification.spec.ts deleted file mode 100644 index e1c07222663..00000000000 --- a/spec/unit/crypto/verification/setDeviceVerification.spec.ts +++ /dev/null @@ -1,56 +0,0 @@ -/* -Copyright 2022 The Matrix.org Foundation C.I.C. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -import "../../../olm-loader"; - -import { CRYPTO_ENABLED, MatrixClient } from "../../../../src/client"; -import { TestClient } from "../../../TestClient"; - -const Olm = global.Olm; - -describe("crypto.setDeviceVerification", () => { - const userId = "@alice:example.com"; - const deviceId1 = "device1"; - let client: MatrixClient; - - if (!CRYPTO_ENABLED) { - return; - } - - beforeAll(async () => { - await Olm.init(); - }); - - beforeEach(async () => { - client = new TestClient(userId, deviceId1).client; - await client.initCrypto(); - }); - - it("client should provide crypto", () => { - expect(client.crypto).not.toBeUndefined(); - }); - - describe("when setting an own device as verified", () => { - beforeEach(async () => { - jest.spyOn(client.crypto!, "cancelAndResendAllOutgoingKeyRequests"); - await client.crypto!.setDeviceVerification(userId, deviceId1, true); - }); - - it("cancelAndResendAllOutgoingKeyRequests should be called", () => { - expect(client.crypto!.cancelAndResendAllOutgoingKeyRequests).toHaveBeenCalled(); - }); - }); -}); diff --git a/src/crypto/index.ts b/src/crypto/index.ts index 458132e75bb..441d569c4ac 100644 --- a/src/crypto/index.ts +++ b/src/crypto/index.ts @@ -2225,9 +2225,6 @@ export class Crypto extends TypedEventEmitter Date: Thu, 5 Jan 2023 15:00:37 +0000 Subject: [PATCH 4/7] Fix outgoing messages for rust-crypto (#3025) It turns out that MatrixClient uses a `FetchHttpApi` instance with `opts.onlyData = true`, so it was returning the json-parsed response rather than the raw response. Change the way we call `authedRequest` so that we get the raw body back. --- spec/unit/rust-crypto.spec.ts | 7 ++++--- src/rust-crypto/index.ts | 2 +- src/rust-crypto/rust-crypto.ts | 36 +++++++++++++++++----------------- 3 files changed, 23 insertions(+), 22 deletions(-) diff --git a/spec/unit/rust-crypto.spec.ts b/spec/unit/rust-crypto.spec.ts index 0bcc88d5d26..69dde71239e 100644 --- a/spec/unit/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto.spec.ts @@ -30,7 +30,7 @@ import MockHttpBackend from "matrix-mock-request"; import { RustCrypto } from "../../src/rust-crypto/rust-crypto"; import { initRustCrypto } from "../../src/rust-crypto"; -import { HttpApiEvent, HttpApiEventHandlerMap, IHttpOpts, IToDeviceEvent, MatrixHttpApi } from "../../src"; +import { HttpApiEvent, HttpApiEventHandlerMap, IToDeviceEvent, MatrixClient, MatrixHttpApi } from "../../src"; import { TypedEventEmitter } from "../../src/models/typed-event-emitter"; afterEach(() => { @@ -48,7 +48,7 @@ describe("RustCrypto", () => { let rustCrypto: RustCrypto; beforeEach(async () => { - const mockHttpApi = {} as MatrixHttpApi; + const mockHttpApi = {} as MatrixClient["http"]; rustCrypto = (await initRustCrypto(mockHttpApi, TEST_USER, TEST_DEVICE_ID)) as RustCrypto; }); @@ -62,7 +62,7 @@ describe("RustCrypto", () => { let rustCrypto: RustCrypto; beforeEach(async () => { - const mockHttpApi = {} as MatrixHttpApi; + const mockHttpApi = {} as MatrixClient["http"]; rustCrypto = (await initRustCrypto(mockHttpApi, TEST_USER, TEST_DEVICE_ID)) as RustCrypto; }); @@ -132,6 +132,7 @@ describe("RustCrypto", () => { baseUrl: "https://example.com", prefix: "/_matrix", fetchFn: httpBackend.fetchFn as typeof global.fetch, + onlyData: true, }); // for these tests we use a mock OlmMachine, with an implementation of outgoingRequests that diff --git a/src/rust-crypto/index.ts b/src/rust-crypto/index.ts index e3b3cb67cd8..4c826078f9f 100644 --- a/src/rust-crypto/index.ts +++ b/src/rust-crypto/index.ts @@ -23,7 +23,7 @@ import { RUST_SDK_STORE_PREFIX } from "./constants"; import { IHttpOpts, MatrixHttpApi } from "../http-api"; export async function initRustCrypto( - http: MatrixHttpApi, + http: MatrixHttpApi, userId: string, deviceId: string, ): Promise { diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 9bb75be70ee..b48de46970b 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -28,7 +28,7 @@ import type { IToDeviceEvent } from "../sync-accumulator"; import { MatrixEvent } from "../models/event"; import { CryptoBackend, OnSyncCompletedData } from "../common-crypto/CryptoBackend"; import { logger } from "../logger"; -import { IHttpOpts, IRequestOpts, MatrixHttpApi, Method } from "../http-api"; +import { IHttpOpts, MatrixHttpApi, Method } from "../http-api"; import { QueryDict } from "../utils"; /** @@ -54,7 +54,7 @@ export class RustCrypto implements CryptoBackend { public constructor( private readonly olmMachine: RustSdkCryptoJs.OlmMachine, - private readonly http: MatrixHttpApi, + private readonly http: MatrixHttpApi, _userId: string, _deviceId: string, ) {} @@ -181,21 +181,21 @@ export class RustCrypto implements CryptoBackend { } } - private async rawJsonRequest( - method: Method, - path: string, - queryParams: QueryDict, - body: string, - opts: IRequestOpts = {}, - ): Promise { - // unbeknownst to HttpApi, we are sending JSON - if (!opts.headers) opts.headers = {}; - opts.headers["Content-Type"] = "application/json"; - - // we use the full prefix - if (!opts.prefix) opts.prefix = ""; - - const resp = await this.http.authedRequest(method, path, queryParams, body, opts); - return await resp.text(); + private async rawJsonRequest(method: Method, path: string, queryParams: QueryDict, body: string): Promise { + const opts = { + // inhibit the JSON stringification and parsing within HttpApi. + json: false, + + // nevertheless, we are sending, and accept, JSON. + headers: { + "Content-Type": "application/json", + "Accept": "application/json", + }, + + // we use the full prefix + prefix: "", + }; + + return await this.http.authedRequest(method, path, queryParams, body, opts); } } From d02559cf3c04e4863fdb3874ad0e159a0101d36d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 5 Jan 2023 15:02:19 +0000 Subject: [PATCH 5/7] Make error handling in decryptionLoop more generic (#3024) Not everything is a `DecryptionError`, and there's no real reason that we should only do retries for `DecryptionError`s --- spec/unit/models/event.spec.ts | 32 +++++++++++++++----------------- src/models/event.ts | 21 ++++----------------- 2 files changed, 19 insertions(+), 34 deletions(-) diff --git a/spec/unit/models/event.spec.ts b/spec/unit/models/event.spec.ts index da492b54225..244f9214521 100644 --- a/spec/unit/models/event.spec.ts +++ b/spec/unit/models/event.spec.ts @@ -16,7 +16,6 @@ limitations under the License. import { MatrixEvent, MatrixEventEvent } from "../../../src/models/event"; import { emitPromise } from "../../test-utils/test-utils"; -import { EventType } from "../../../src"; import { Crypto } from "../../../src/crypto"; describe("MatrixEvent", () => { @@ -88,22 +87,6 @@ describe("MatrixEvent", () => { expect(ev.getWireContent().ciphertext).toBeUndefined(); }); - it("should abort decryption if fails with an error other than a DecryptionError", async () => { - const ev = new MatrixEvent({ - type: EventType.RoomMessageEncrypted, - content: { - body: "Test", - }, - event_id: "$event1:server", - }); - await ev.attemptDecryption({ - decryptEvent: jest.fn().mockRejectedValue(new Error("Not a DecryptionError")), - } as unknown as Crypto); - expect(ev.isEncrypted()).toBeTruthy(); - expect(ev.isBeingDecrypted()).toBeFalsy(); - expect(ev.isDecryptionFailure()).toBeFalsy(); - }); - describe("applyVisibilityEvent", () => { it("should emit VisibilityChange if a change was made", async () => { const ev = new MatrixEvent({ @@ -134,6 +117,21 @@ describe("MatrixEvent", () => { }); }); + it("should report decryption errors", async () => { + const crypto = { + decryptEvent: jest.fn().mockRejectedValue(new Error("test error")), + } as unknown as Crypto; + + await encryptedEvent.attemptDecryption(crypto); + expect(encryptedEvent.isEncrypted()).toBeTruthy(); + expect(encryptedEvent.isBeingDecrypted()).toBeFalsy(); + expect(encryptedEvent.isDecryptionFailure()).toBeTruthy(); + expect(encryptedEvent.getContent()).toEqual({ + msgtype: "m.bad.encrypted", + body: "** Unable to decrypt: Error: test error **", + }); + }); + it("should retry decryption if a retry is queued", async () => { const eventAttemptDecryptionSpy = jest.spyOn(encryptedEvent, "attemptDecryption"); diff --git a/src/models/event.ts b/src/models/event.ts index 1f1c3cfa366..e4ac9691666 100644 --- a/src/models/event.ts +++ b/src/models/event.ts @@ -828,17 +828,7 @@ export class MatrixEvent extends TypedEventEmittere).name !== "DecryptionError") { - // not a decryption error: log the whole exception as an error - // (and don't bother with a retry) - const re = options.isRetry ? "re" : ""; - // For find results: this can produce "Error decrypting event (id=$ev)" and - // "Error redecrypting event (id=$ev)". - logger.error(`Error ${re}decrypting event (${this.getDetails()})`, e); - this.decryptionPromise = null; - this.retryDecryption = false; - return; - } + const detailedError = e instanceof DecryptionError ? (e).detailedString : String(e); err = e as Error; @@ -858,10 +848,7 @@ export class MatrixEvent extends TypedEventEmittere).detailedString, - ); + logger.log(`Error decrypting event (${this.getDetails()}), but retrying: ${detailedError}`); continue; } @@ -870,9 +857,9 @@ export class MatrixEvent extends TypedEventEmittere).detailedString); + logger.warn(`Error decrypting event (${this.getDetails()}): ${detailedError}`); - res = this.badEncryptedMessage((e).message); + res = this.badEncryptedMessage(String(e)); } // at this point, we've either successfully decrypted the event, or have given up From bb23df942382cb3b061034b2105ae2e7c7ac0191 Mon Sep 17 00:00:00 2001 From: Kerry Date: Fri, 6 Jan 2023 09:00:12 +1300 Subject: [PATCH 6/7] Add alt event type matching in Relations model (#3018) * allow alt event types in relations model * remove unneccesary checks on remove relation * comment * assert on event emitted --- spec/unit/relations.spec.ts | 96 ++++++++++++++++++++++++++++++++++++- src/models/relations.ts | 21 +++----- 2 files changed, 101 insertions(+), 16 deletions(-) diff --git a/spec/unit/relations.spec.ts b/spec/unit/relations.spec.ts index 91b77dd1212..cf4997c2809 100644 --- a/spec/unit/relations.spec.ts +++ b/spec/unit/relations.spec.ts @@ -14,13 +14,21 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { M_POLL_START } from "matrix-events-sdk"; + import { EventTimelineSet } from "../../src/models/event-timeline-set"; import { MatrixEvent, MatrixEventEvent } from "../../src/models/event"; import { Room } from "../../src/models/room"; -import { Relations } from "../../src/models/relations"; +import { Relations, RelationsEvent } from "../../src/models/relations"; import { TestClient } from "../TestClient"; +import { RelationType } from "../../src"; +import { logger } from "../../src/logger"; describe("Relations", function () { + afterEach(() => { + jest.spyOn(logger, "error").mockRestore(); + }); + it("should deduplicate annotations", function () { const room = new Room("room123", null!, null!); const relations = new Relations("m.annotation", "m.reaction", room); @@ -75,6 +83,92 @@ describe("Relations", function () { } }); + describe("addEvent()", () => { + const relationType = RelationType.Reference; + const eventType = M_POLL_START.stable!; + const altEventTypes = [M_POLL_START.unstable!]; + const room = new Room("room123", null!, null!); + + it("should not add events without a relation", async () => { + // dont pollute console + const logSpy = jest.spyOn(logger, "error").mockImplementation(() => {}); + const relations = new Relations(relationType, eventType, room); + const emitSpy = jest.spyOn(relations, "emit"); + const event = new MatrixEvent({ type: eventType }); + + await relations.addEvent(event); + expect(logSpy).toHaveBeenCalledWith("Event must have relation info"); + // event not added + expect(relations.getRelations().length).toBe(0); + expect(emitSpy).not.toHaveBeenCalled(); + }); + + it("should not add events of incorrect event type", async () => { + // dont pollute console + const logSpy = jest.spyOn(logger, "error").mockImplementation(() => {}); + const relations = new Relations(relationType, eventType, room); + const emitSpy = jest.spyOn(relations, "emit"); + const event = new MatrixEvent({ + type: "different-event-type", + content: { + "m.relates_to": { + event_id: "$2s4yYpEkVQrPglSCSqB_m6E8vDhWsg0yFNyOJdVIb_o", + rel_type: relationType, + }, + }, + }); + + await relations.addEvent(event); + + expect(logSpy).toHaveBeenCalledWith(`Event relation info doesn't match this container`); + // event not added + expect(relations.getRelations().length).toBe(0); + expect(emitSpy).not.toHaveBeenCalled(); + }); + + it("adds events that match alt event types", async () => { + const relations = new Relations(relationType, eventType, room, altEventTypes); + const emitSpy = jest.spyOn(relations, "emit"); + const event = new MatrixEvent({ + type: M_POLL_START.unstable!, + content: { + "m.relates_to": { + event_id: "$2s4yYpEkVQrPglSCSqB_m6E8vDhWsg0yFNyOJdVIb_o", + rel_type: relationType, + }, + }, + }); + + await relations.addEvent(event); + + // event added + expect(relations.getRelations()).toEqual([event]); + expect(emitSpy).toHaveBeenCalledWith(RelationsEvent.Add, event); + }); + + it("should not add events of incorrect relation type", async () => { + const logSpy = jest.spyOn(logger, "error").mockImplementation(() => {}); + const relations = new Relations(relationType, eventType, room); + const event = new MatrixEvent({ + type: eventType, + content: { + "m.relates_to": { + event_id: "$2s4yYpEkVQrPglSCSqB_m6E8vDhWsg0yFNyOJdVIb_o", + rel_type: "m.annotation", + }, + }, + }); + + await relations.addEvent(event); + const emitSpy = jest.spyOn(relations, "emit"); + + expect(logSpy).toHaveBeenCalledWith(`Event relation info doesn't match this container`); + // event not added + expect(relations.getRelations().length).toBe(0); + expect(emitSpy).not.toHaveBeenCalled(); + }); + }); + it("should emit created regardless of ordering", async function () { const targetEvent = new MatrixEvent({ sender: "@bob:example.com", diff --git a/src/models/relations.ts b/src/models/relations.ts index bd6f0093814..069bb0a0c69 100644 --- a/src/models/relations.ts +++ b/src/models/relations.ts @@ -33,6 +33,9 @@ export type EventHandlerMap = { [RelationsEvent.Redaction]: (event: MatrixEvent) => void; }; +const matchesEventType = (eventType: string, targetEventType: string, altTargetEventTypes: string[] = []): boolean => + [targetEventType, ...altTargetEventTypes].includes(eventType); + /** * A container for relation events that supports easy access to common ways of * aggregating such events. Each instance holds events that of a single relation @@ -55,11 +58,13 @@ export class Relations extends TypedEventEmitter Date: Fri, 6 Jan 2023 10:27:35 +0000 Subject: [PATCH 7/7] Fix threaded cache receipt when event holds multiple receipts (#3026) --- spec/integ/matrix-client-syncing.spec.ts | 46 ++++++++++++++++++++++++ src/@types/read_receipts.ts | 8 +++++ src/models/room.ts | 12 +++++-- src/models/thread.ts | 18 +++------- 4 files changed, 68 insertions(+), 16 deletions(-) diff --git a/spec/integ/matrix-client-syncing.spec.ts b/spec/integ/matrix-client-syncing.spec.ts index 0f92dc60d95..75487eb81dd 100644 --- a/spec/integ/matrix-client-syncing.spec.ts +++ b/spec/integ/matrix-client-syncing.spec.ts @@ -1543,6 +1543,52 @@ describe("MatrixClient syncing", () => { }); }); }); + + it("only replays receipts relevant to the current context", async () => { + const THREAD_ID = "$unknownthread:localhost"; + + const receipt = { + type: "m.receipt", + room_id: "!foo:bar", + content: { + "$event1:localhost": { + [ReceiptType.Read]: { + "@alice:localhost": { ts: 666, thread_id: THREAD_ID }, + }, + }, + "$otherevent:localhost": { + [ReceiptType.Read]: { + "@alice:localhost": { ts: 999, thread_id: "$otherthread:localhost" }, + }, + }, + }, + }; + syncData.rooms.join[roomOne].ephemeral.events = [receipt]; + + httpBackend!.when("GET", "/sync").respond(200, syncData); + client!.startClient(); + + return Promise.all([httpBackend!.flushAllExpected(), awaitSyncEvent()]).then(() => { + const room = client?.getRoom(roomOne); + expect(room).toBeInstanceOf(Room); + + expect(room?.cachedThreadReadReceipts.has(THREAD_ID)).toBe(true); + + const thread = room!.createThread(THREAD_ID, undefined, [], true); + + expect(room?.cachedThreadReadReceipts.has(THREAD_ID)).toBe(false); + + const receipt = thread.getReadReceiptForUserId("@alice:localhost"); + + expect(receipt).toStrictEqual({ + data: { + thread_id: "$unknownthread:localhost", + ts: 666, + }, + eventId: "$event1:localhost", + }); + }); + }); }); describe("of a room", () => { diff --git a/src/@types/read_receipts.ts b/src/@types/read_receipts.ts index 34f1c67fad7..3032c5934df 100644 --- a/src/@types/read_receipts.ts +++ b/src/@types/read_receipts.ts @@ -54,3 +54,11 @@ export type Receipts = { [userId: string]: [WrappedReceipt | null, WrappedReceipt | null]; // Pair (both nullable) }; }; + +export type CachedReceiptStructure = { + eventId: string; + receiptType: string | ReceiptType; + userId: string; + receipt: Receipt; + synthetic: boolean; +}; diff --git a/src/models/room.ts b/src/models/room.ts index ad21e626450..972112fdfef 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -54,7 +54,13 @@ import { FILTER_RELATED_BY_SENDERS, ThreadFilterType, } from "./thread"; -import { MAIN_ROOM_TIMELINE, Receipt, ReceiptContent, ReceiptType } from "../@types/read_receipts"; +import { + CachedReceiptStructure, + MAIN_ROOM_TIMELINE, + Receipt, + ReceiptContent, + ReceiptType, +} from "../@types/read_receipts"; import { IStateEventWithRoomId } from "../@types/search"; import { RelationsContainer } from "./relations-container"; import { ReadReceipt, synthesizeReceipt } from "./read-receipt"; @@ -302,7 +308,7 @@ export class Room extends ReadReceipt { private txnToEvent: Record = {}; // Pending in-flight requests { string: MatrixEvent } private notificationCounts: NotificationCount = {}; private readonly threadNotifications = new Map(); - public readonly cachedThreadReadReceipts = new Map(); + public readonly cachedThreadReadReceipts = new Map(); private readonly timelineSets: EventTimelineSet[]; public readonly threadsTimelineSets: EventTimelineSet[] = []; // any filtered timeline sets we're maintaining for this room @@ -2718,7 +2724,7 @@ export class Room extends ReadReceipt { // when the thread is created this.cachedThreadReadReceipts.set(receipt.thread_id!, [ ...(this.cachedThreadReadReceipts.get(receipt.thread_id!) ?? []), - { event, synthetic }, + { eventId, receiptType, userId, receipt, synthetic }, ]); } }); diff --git a/src/models/thread.ts b/src/models/thread.ts index da8ddf0b1f8..25e454c32e2 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -27,7 +27,7 @@ import { RoomState } from "./room-state"; import { ServerControlledNamespacedValue } from "../NamespacedValue"; import { logger } from "../logger"; import { ReadReceipt } from "./read-receipt"; -import { Receipt, ReceiptContent, ReceiptType } from "../@types/read_receipts"; +import { CachedReceiptStructure, ReceiptType } from "../@types/read_receipts"; export enum ThreadEvent { New = "Thread.new", @@ -50,7 +50,7 @@ interface IThreadOpts { room: Room; client: MatrixClient; pendingEventOrdering?: PendingEventOrdering; - receipts?: { event: MatrixEvent; synthetic: boolean }[]; + receipts?: CachedReceiptStructure[]; } export enum FeatureSupport { @@ -317,17 +317,9 @@ export class Thread extends ReadReceipt { * and apply them to the current thread * @param receipts - A collection of the receipts cached from initial sync */ - private processReceipts(receipts: { event: MatrixEvent; synthetic: boolean }[] = []): void { - for (const { event, synthetic } of receipts) { - const content = event.getContent(); - Object.keys(content).forEach((eventId: string) => { - Object.keys(content[eventId]).forEach((receiptType: ReceiptType | string) => { - Object.keys(content[eventId][receiptType]).forEach((userId: string) => { - const receipt = content[eventId][receiptType][userId] as Receipt; - this.addReceiptToStructure(eventId, receiptType as ReceiptType, userId, receipt, synthetic); - }); - }); - }); + private processReceipts(receipts: CachedReceiptStructure[] = []): void { + for (const { eventId, receiptType, userId, receipt, synthetic } of receipts) { + this.addReceiptToStructure(eventId, receiptType as ReceiptType, userId, receipt, synthetic); } }