Switch crypto.spec.ts away from TestClient and matrix-mock-request. (#3142)

I became sufficiently annoyed with matrix-mock-request that I decided to replace it with fetch-mock, which is what we use in matrix-react-sdk and is generally more powerful, easier to use, and actually maintained.

Unfortunately, we have a TestClient utility which is widely used and quite tightly integrated with matrix-mock-request. It wasn't going to be possible to change TestClient without changing all the tests that use it.

I also don't find TestClient particularly easy to use - it does a lot of stuff which I'm not convinced ought to be done for every single test.

So... I've introduced a couple of new classes (SyncResponder, E2EKeyReceiver) which do some of the useful bits of TestClient, but in a more granular way, and have switched crypto.spec.ts over so that rather than instantiating a TestClient for each test, it creates a MatrixClient directly and intercepts the endpoints necessary.

package.json

@@ -84,6 +84,7 @@
         "@matrix-org/olm": "https://gitlab.matrix.org/api/v4/projects/27/packages/npm/@matrix-org/olm/-/@matrix-org/olm-3.2.14.tgz",
         "@types/bs58": "^4.0.1",
         "@types/content-type": "^1.1.5",
+        "@types/debug": "^4.1.7",
         "@types/domexception": "^4.0.0",
         "@types/jest": "^29.0.0",
         "@types/node": "18",
@@ -96,6 +97,7 @@
         "babelify": "^10.0.0",
         "better-docs": "^2.4.0-beta.9",
         "browserify": "^17.0.0",
+        "debug": "^4.3.4",
         "docdash": "^2.0.0",
         "domexception": "^4.0.0",
         "eslint": "8.33.0",

spec/TestClient.ts

@@ -24,6 +24,8 @@ import "./olm-loader";
 
 import MockHttpBackend from "matrix-mock-request";
 
+import type { IDeviceKeys, IOneTimeKey } from "../src/@types/crypto";
+import type { IE2EKeyReceiver } from "./test-utils/E2EKeyReceiver";
 import { LocalStorageCryptoStore } from "../src/crypto/store/localStorage-crypto-store";
 import { logger } from "../src/logger";
 import { syncPromise } from "./test-utils/test-utils";
@@ -31,14 +33,18 @@ import { createClient, IStartClientOpts } from "../src/matrix";
 import { ICreateClientOpts, IDownloadKeyResult, MatrixClient, PendingEventOrdering } from "../src/client";
 import { MockStorageApi } from "./MockStorageApi";
 import { encodeUri } from "../src/utils";
-import { IDeviceKeys, IOneTimeKey } from "../src/crypto/dehydration";
 import { IKeyBackupSession } from "../src/crypto/keybackup";
 import { IKeysUploadResponse, IUploadKeysRequest } from "../src/client";
+import { ISyncResponder } from "./test-utils/SyncResponder";
 
 /**
  * Wrapper for a MockStorageApi, MockHttpBackend and MatrixClient
+ *
+ * @deprecated Avoid using this; it is tied too tightly to matrix-mock-request and is generally inconvenient to use.
+ *    Instead, construct a MatrixClient manually, use fetch-mock-jest to intercept the HTTP requests, and
+ *    use things like {@link E2EKeyReceiver} and {@link SyncResponder} to manage the requests.
  */
-export class TestClient {
+export class TestClient implements IE2EKeyReceiver, ISyncResponder {
     public readonly httpBackend: MockHttpBackend;
     public readonly client: MatrixClient;
     public deviceKeys?: IDeviceKeys | null;
@@ -243,8 +249,22 @@ export class TestClient {
         return this.deviceKeys!.keys[keyId];
     }
 
+    /** Next time we see a sync request (or immediately, if there is one waiting), send the given response
+     *
+     * Calling this will register a response for `/sync`, and then, in the background, flush a single `/sync` request.
+     * Try calling {@link syncPromise} to wait for the sync to complete.
+     *
+     * @param response - response to /sync request
+     */
+    public sendOrQueueSyncResponse(syncResponse: object): void {
+        this.httpBackend.when("GET", "/sync").respond(200, syncResponse);
+        this.httpBackend.flush("/sync", 1);
+    }
+
     /**
      * flush a single /sync request, and wait for the syncing event
+     *
+     * @deprecated: prefer to use {@link #sendOrQueueSyncResponse} followed by {@link syncPromise}.
      */
     public flushSync(): Promise<void> {
         logger.log(`${this}: flushSync`);

spec/integ/olm-encryption-spec.ts

@@ -29,13 +29,13 @@ limitations under the License.
 import "../olm-loader";
 
 import type { Session } from "@matrix-org/olm";
+import type { IDeviceKeys, IOneTimeKey } from "../../src/@types/crypto";
 import { logger } from "../../src/logger";
 import * as testUtils from "../test-utils/test-utils";
 import { TestClient } from "../TestClient";
 import { CRYPTO_ENABLED, IClaimKeysRequest, IQueryKeysRequest, IUploadKeysRequest } from "../../src/client";
 import { ClientEvent, IContent, ISendEventResponse, MatrixClient, MatrixEvent } from "../../src/matrix";
 import { DeviceInfo } from "../../src/crypto/deviceinfo";
-import { IDeviceKeys, IOneTimeKey } from "../../src/crypto/dehydration";
 
 let aliTestClient: TestClient;
 const roomId = "!room:localhost";

spec/test-utils/E2EKeyReceiver.ts

@@ -0,0 +1,159 @@
+/*
+Copyright 2023 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 debugFunc from "debug";
+import { Debugger } from "debug";
+import fetchMock from "fetch-mock-jest";
+
+import type { IDeviceKeys, IOneTimeKey } from "../../src/@types/crypto";
+
+/** Interface implemented by classes that intercept `/keys/upload` requests from test clients to catch the uploaded keys
+ *
+ * Common interface implemented by {@link TestClient} and {@link E2EKeyReceiver}
+ */
+export interface IE2EKeyReceiver {
+    /**
+     * get the uploaded ed25519 device key
+     *
+     * @returns base64 device key
+     */
+    getSigningKey(): string;
+
+    /**
+     * get the uploaded curve25519 device key
+     *
+     * @returns base64 device key
+     */
+    getDeviceKey(): string;
+
+    /**
+     * Wait for one-time-keys to be uploaded, then return them.
+     *
+     * @returns Promise for the one-time keys
+     */
+    awaitOneTimeKeyUpload(): Promise<Record<string, IOneTimeKey>>;
+}
+
+/** E2EKeyReceiver: An object which intercepts `/keys/uploads` fetches via fetch-mock.
+ *
+ * It stashes the uploaded keys for use elsewhere in the tests.
+ */
+export class E2EKeyReceiver implements IE2EKeyReceiver {
+    private readonly debug: Debugger;
+
+    private deviceKeys: IDeviceKeys | null = null;
+    private oneTimeKeys: Record<string, IOneTimeKey> = {};
+    private readonly oneTimeKeysPromise: Promise<void>;
+
+    /**
+     * Construct a new E2EKeyReceiver.
+     *
+     * It will immediately register an intercept of `/keys/uploads` requests for the given homeserverUrl.
+     * Only /upload requests made to this server will be intercepted: this allows a single test to use more than one
+     * client and have the keys collected separately.
+     *
+     * @param homeserverUrl - the Homeserver Url of the client under test.
+     */
+    public constructor(homeserverUrl: string) {
+        this.debug = debugFunc(`e2e-key-receiver:[${homeserverUrl}]`);
+
+        // set up a listener for /keys/upload.
+        this.oneTimeKeysPromise = new Promise((resolveOneTimeKeys) => {
+            const listener = (url: string, options: RequestInit) =>
+                this.onKeyUploadRequest(resolveOneTimeKeys, options);
+
+            // catch both r0 and v3 variants
+            fetchMock.post(new URL("/_matrix/client/r0/keys/upload", homeserverUrl).toString(), listener);
+            fetchMock.post(new URL("/_matrix/client/v3/keys/upload", homeserverUrl).toString(), listener);
+        });
+    }
+
+    private async onKeyUploadRequest(onOnTimeKeysUploaded: () => void, options: RequestInit): Promise<object> {
+        const content = JSON.parse(options.body as string);
+
+        // device keys may only be uploaded once
+        if (content.device_keys && Object.keys(content.device_keys).length > 0) {
+            if (this.deviceKeys) {
+                throw new Error("Application attempted to upload E2E device keys multiple times");
+            }
+            this.debug(`received device keys`);
+            this.deviceKeys = content.device_keys;
+        }
+
+        if (content.one_time_keys && Object.keys(content.one_time_keys).length > 0) {
+            // this is a one-time-key upload
+
+            // if we already have a batch of one-time keys, then slow-roll the response,
+            // otherwise the client ends up tight-looping one-time-key-uploads and filling the logs with junk.
+            if (Object.keys(this.oneTimeKeys).length > 0) {
+                this.debug(`received second batch of one-time keys: blocking response`);
+                await new Promise(() => {});
+            }
+
+            this.debug(`received ${Object.keys(content.one_time_keys).length} one-time keys`);
+            Object.assign(this.oneTimeKeys, content.one_time_keys);
+            onOnTimeKeysUploaded();
+        }
+
+        return {
+            one_time_key_counts: {
+                signed_curve25519: Object.keys(this.oneTimeKeys).length,
+            },
+        };
+    }
+
+    /** Get the uploaded Ed25519 key
+     *
+     * If device keys have not yet been uploaded, throws an error
+     */
+    public getSigningKey(): string {
+        if (!this.deviceKeys) {
+            throw new Error("Device keys not yet uploaded");
+        }
+        const keyIds = Object.keys(this.deviceKeys.keys).filter((v) => v.startsWith("ed25519:"));
+        if (keyIds.length != 1) {
+            throw new Error(`Expected exactly 1 ed25519 key uploaded, got ${keyIds}`);
+        }
+        return this.deviceKeys.keys[keyIds[0]];
+    }
+
+    /** Get the uploaded Curve25519 key
+     *
+     * If device keys have not yet been uploaded, throws an error
+     */
+    public getDeviceKey(): string {
+        if (!this.deviceKeys) {
+            throw new Error("Device keys not yet uploaded");
+        }
+        const keyIds = Object.keys(this.deviceKeys.keys).filter((v) => v.startsWith("curve25519:"));
+        if (keyIds.length != 1) {
+            throw new Error(`Expected exactly 1 curve25519 key uploaded, got ${keyIds}`);
+        }
+        return this.deviceKeys.keys[keyIds[0]];
+    }
+
+    /**
+     * If one-time keys have already been uploaded, return them. Otherwise,
+     * set up an expectation that the keys will be uploaded, and wait for
+     * that to happen.
+     *
+     * @returns Promise for the one-time keys
+     */
+    public async awaitOneTimeKeyUpload(): Promise<Record<string, IOneTimeKey>> {
+        await this.oneTimeKeysPromise;
+        return this.oneTimeKeys;
+    }
+}

spec/test-utils/SyncResponder.ts

@@ -0,0 +1,131 @@
+/*
+Copyright 2023 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 debugFunc from "debug";
+import { Debugger } from "debug";
+import fetchMock from "fetch-mock-jest";
+import { MockResponse } from "fetch-mock";
+
+/** Interface implemented by classes that intercept `/sync` requests from test clients
+ *
+ * Common interface implemented by {@link TestClient} and {@link SyncResponder}
+ */
+export interface ISyncResponder {
+    /** Next time we see a sync request (or immediately, if there is one waiting), send the given response
+     *
+     * @param response - response to /sync request
+     */
+    sendOrQueueSyncResponse(response: object): void;
+}
+
+enum SyncResponderState {
+    IDLE,
+    WAITING_FOR_REQUEST,
+    WAITING_FOR_RESPONSE,
+}
+
+/** SyncResponder: An object which intercepts `/sync` fetches via fetch-mock.
+ *
+ * Two modes are possible:
+ *  * A response can be queued up; the next call to `/sync` will return it.
+ *  * If a call to `/sync` arrives before a response is queued, it will block until a call to {@link #sendOrQueueSyncResponse}.
+ */
+export class SyncResponder implements ISyncResponder {
+    private readonly debug: Debugger;
+    private state: SyncResponderState = SyncResponderState.IDLE;
+
+    /*
+     * properties that are only valid in WAITING_FOR_REQUEST
+     */
+
+    /** the response to be sent when the request is made */
+    private pendingResponse: object | null = null;
+
+    /*
+     * properties that are only valid in WAITING_FOR_RESPONSE
+     */
+
+    /** a callback to be called with a response once one is registered.
+     *
+     * It will release the /sync request and update the state.
+     */
+    private onResponseReceived: ((response: object) => void) | null = null;
+
+    /**
+     * Construct a new SyncResponder.
+     *
+     * It will immediately register an intercept of `/sync` requests for the given homeserverUrl.
+     * Only /sync requests made to this server will be intercepted: this allows a single test to use more than one
+     * client and have overlapping /sync requests.
+     *
+     * @param homeserverUrl - the Homeserver Url of the client under test.
+     */
+    public constructor(homeserverUrl: string) {
+        this.debug = debugFunc(`sync-responder:[${homeserverUrl}]`);
+        fetchMock.get("begin:" + new URL("/_matrix/client/r0/sync?", homeserverUrl).toString(), (_url, _options) =>
+            this.onSyncRequest(),
+        );
+    }
+
+    private async onSyncRequest(): Promise<MockResponse> {
+        switch (this.state) {
+            case SyncResponderState.IDLE: {
+                this.debug("Got /sync request: waiting for response to be ready");
+                const res = await new Promise<object>((resolve) => {
+                    this.onResponseReceived = resolve;
+                    this.state = SyncResponderState.WAITING_FOR_RESPONSE;
+                });
+                this.debug("Responding to /sync");
+                this.state = SyncResponderState.IDLE;
+                this.onResponseReceived = null;
+                return res;
+            }
+
+            case SyncResponderState.WAITING_FOR_REQUEST: {
+                this.debug("Got /sync request: responding immediately with queued response");
+                const res = this.pendingResponse!;
+                this.state = SyncResponderState.IDLE;
+                this.pendingResponse = null;
+                return res;
+            }
+
+            default:
+                // we must already be in WAITING_FOR_RESPONSE, ie we already have a /sync request in progress
+                throw new Error(`Got unexpected /sync request in state ${this.state}`);
+        }
+    }
+
+    /** Next time we see a sync request (or immediately, if there is one waiting), send the given response
+     *
+     * @param response - response to /sync request
+     */
+    public sendOrQueueSyncResponse(response: object): void {
+        switch (this.state) {
+            case SyncResponderState.IDLE:
+                this.pendingResponse = response;
+                this.state = SyncResponderState.WAITING_FOR_REQUEST;
+                break;
+
+            case SyncResponderState.WAITING_FOR_RESPONSE:
+                this.onResponseReceived!(response);
+                break;
+
+            default:
+                // we already have a response queued
+                throw new Error(`Cannot queue more than one /sync response`);
+        }
+    }
+}

src/@types/crypto.ts

@@ -1,5 +1,5 @@
 /*
-Copyright 2022 The Matrix.org Foundation C.I.C.
+Copyright 2022-2023 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.
@@ -15,6 +15,7 @@ limitations under the License.
 */
 
 import type { IClearEvent } from "../models/event";
+import type { ISignatures } from "./signed";
 
 export type OlmGroupSessionExtraData = {
     untrusted?: boolean;
@@ -70,3 +71,25 @@ export interface IMegolmSessionData extends Extensible {
 }
 
 /* eslint-enable camelcase */
+
+/** the type of the `device_keys` parameter on `/_matrix/client/v3/keys/upload`
+ *
+ * @see https://spec.matrix.org/v1.5/client-server-api/#post_matrixclientv3keysupload
+ */
+export interface IDeviceKeys {
+    algorithms: Array<string>;
+    device_id: string; // eslint-disable-line camelcase
+    user_id: string; // eslint-disable-line camelcase
+    keys: Record<string, string>;
+    signatures?: ISignatures;
+}
+
+/** the type of the `one_time_keys` and `fallback_keys` parameters on `/_matrix/client/v3/keys/upload`
+ *
+ * @see https://spec.matrix.org/v1.5/client-server-api/#post_matrixclientv3keysupload
+ */
+export interface IOneTimeKey {
+    key: string;
+    fallback?: boolean;
+    signatures?: ISignatures;
+}

src/client.ts

@@ -20,7 +20,7 @@ limitations under the License.
 
 import { Optional } from "matrix-events-sdk";
 
-import type { IMegolmSessionData } from "./@types/crypto";
+import type { IDeviceKeys, IMegolmSessionData, IOneTimeKey } from "./@types/crypto";
 import { ISyncStateData, SyncApi, SyncApiOptions, SyncState } from "./sync";
 import {
     EventStatus,
@@ -85,13 +85,7 @@ import { keyFromAuthData } from "./crypto/key_passphrase";
 import { User, UserEvent, UserEventHandlerMap } from "./models/user";
 import { getHttpUriForMxc } from "./content-repo";
 import { SearchResult } from "./models/search-result";
-import {
+import { DEHYDRATION_ALGORITHM, IDehydratedDevice, IDehydratedDeviceKeyInfo } from "./crypto/dehydration";
-    DEHYDRATION_ALGORITHM,
-    IDehydratedDevice,
-    IDehydratedDeviceKeyInfo,
-    IDeviceKeys,
-    IOneTimeKey,
-} from "./crypto/dehydration";
 import {
     IKeyBackupInfo,
     IKeyBackupPrepareOpts,

src/crypto/dehydration.ts

@@ -16,6 +16,7 @@ limitations under the License.
 
 import anotherjson from "another-json";
 
+import type { IDeviceKeys, IOneTimeKey } from "../@types/crypto";
 import { decodeBase64, encodeBase64 } from "./olmlib";
 import { IndexedDBCryptoStore } from "../crypto/store/indexeddb-crypto-store";
 import { decryptAES, encryptAES } from "./aes";
@@ -23,7 +24,6 @@ import { logger } from "../logger";
 import { ISecretStorageKeyInfo } from "./api";
 import { Crypto } from "./index";
 import { Method } from "../http-api";
-import { ISignatures } from "../@types/signed";
 
 export interface IDehydratedDevice {
     device_id: string; // eslint-disable-line camelcase
@@ -38,20 +38,6 @@ export interface IDehydratedDeviceKeyInfo {
     passphrase?: string;
 }
 
-export interface IDeviceKeys {
-    algorithms: Array<string>;
-    device_id: string; // eslint-disable-line camelcase
-    user_id: string; // eslint-disable-line camelcase
-    keys: Record<string, string>;
-    signatures?: ISignatures;
-}
-
-export interface IOneTimeKey {
-    key: string;
-    fallback?: boolean;
-    signatures?: ISignatures;
-}
-
 export const DEHYDRATION_ALGORITHM = "org.matrix.msc2697.v1.olm.libolm_pickle";
 
 const oneweek = 7 * 24 * 60 * 60 * 1000;

src/crypto/index.ts

@@ -20,7 +20,7 @@ limitations under the License.
 import anotherjson from "another-json";
 import { v4 as uuidv4 } from "uuid";
 
-import type { IEventDecryptionResult, IMegolmSessionData } from "../@types/crypto";
+import type { IDeviceKeys, IEventDecryptionResult, IMegolmSessionData, IOneTimeKey } from "../@types/crypto";
 import type { PkDecryption, PkSigning } from "@matrix-org/olm";
 import { EventType, ToDeviceMessageId } from "../@types/event";
 import { TypedReEmitter } from "../ReEmitter";
@@ -63,7 +63,7 @@ import { ToDeviceChannel, ToDeviceRequests, Request } from "./verification/reque
 import { IllegalMethod } from "./verification/IllegalMethod";
 import { KeySignatureUploadError } from "../errors";
 import { calculateKeyCheck, decryptAES, encryptAES } from "./aes";
-import { DehydrationManager, IDeviceKeys, IOneTimeKey } from "./dehydration";
+import { DehydrationManager } from "./dehydration";
 import { BackupManager } from "./backup";
 import { IStore } from "../store";
 import { Room, RoomEvent } from "../models/room";

src/crypto/olmlib.ts

@@ -21,10 +21,10 @@ limitations under the License.
 import anotherjson from "another-json";
 
 import type { PkSigning } from "@matrix-org/olm";
+import type { IOneTimeKey } from "../@types/crypto";
 import { OlmDevice } from "./OlmDevice";
 import { DeviceInfo } from "./deviceinfo";
 import { logger } from "../logger";
-import { IOneTimeKey } from "./dehydration";
 import { IClaimOTKsResult, MatrixClient } from "../client";
 import { ISignatures } from "../@types/signed";
 import { MatrixEvent } from "../models/event";

yarn.lock

@@ -1680,6 +1680,13 @@
   resolved "https://registry.yarnpkg.com/@types/content-type/-/content-type-1.1.5.tgz#aa02dca40864749a9e2bf0161a6216da57e3ede5"
   integrity sha512-dgMN+syt1xb7Hk8LU6AODOfPlvz5z1CbXpPuJE5ZrX9STfBOIXF09pEB8N7a97WT9dbngt3ksDCm6GW6yMrxfQ==
 
+"@types/debug@^4.1.7":
+  version "4.1.7"
+  resolved "https://registry.yarnpkg.com/@types/debug/-/debug-4.1.7.tgz#7cc0ea761509124709b8b2d1090d8f6c17aadb82"
+  integrity sha512-9AonUzyTjXXhEOa0DnqpzZi6VHlqKMswga9EXjpXnnqxwLtdvPPtlO8evrI5D9S6asFRCQ6v+wpiUKbw+vKqyg==
+  dependencies:
+    "@types/ms" "*"
+
 "@types/domexception@^4.0.0":
   version "4.0.0"
   resolved "https://registry.yarnpkg.com/@types/domexception/-/domexception-4.0.0.tgz#bb19c920c81c3f1408b46d021fb79467ff2d32fd"
@@ -1745,6 +1752,11 @@
   resolved "https://registry.yarnpkg.com/@types/json5/-/json5-0.0.29.tgz#ee28707ae94e11d2b827bcbe5270bcea7f3e71ee"
   integrity sha512-dRLjCWHYg4oaA77cxO64oO+7JwCwnIzkZPdrrC71jQmQtlhM556pwKo5bUzqvZndkVbeFLIIi+9TC40JNF5hNQ==
 
+"@types/ms@*":
+  version "0.7.31"
+  resolved "https://registry.yarnpkg.com/@types/ms/-/ms-0.7.31.tgz#31b7ca6407128a3d2bbc27fe2d21b345397f6197"
+  integrity sha512-iiUgKzV9AuaEkZqkOLDIvlQiL6ltuZd9tGcW3gwpnX8JbuiuhFlEGmmFXEXkN50Cvq7Os88IY2v0dkDqXYWVgA==
+
 "@types/node@*", "@types/node@18":
   version "18.13.0"
   resolved "https://registry.yarnpkg.com/@types/node/-/node-18.13.0.tgz#0400d1e6ce87e9d3032c19eb6c58205b0d3f7850"