diff --git a/spec/integ/crypto/cross-signing.spec.ts b/spec/integ/crypto/cross-signing.spec.ts index 84210901c..80a9d2588 100644 --- a/spec/integ/crypto/cross-signing.spec.ts +++ b/spec/integ/crypto/cross-signing.spec.ts @@ -269,7 +269,7 @@ describe("cross-signing", () => { // a *different* device. Then, when we call `bootstrapCrossSigning` again, it should do the honours. mockSetupCrossSigningRequests(); - const accountDataAccumulator = new AccountDataAccumulator(); + const accountDataAccumulator = new AccountDataAccumulator(syncResponder); accountDataAccumulator.interceptGetAccountData(); const authDict = { type: "test" }; diff --git a/spec/integ/crypto/crypto.spec.ts b/spec/integ/crypto/crypto.spec.ts index 24d03321a..1594753e5 100644 --- a/spec/integ/crypto/crypto.spec.ts +++ b/spec/integ/crypto/crypto.spec.ts @@ -1760,7 +1760,7 @@ describe("crypto", () => { beforeEach(async () => { createSecretStorageKey.mockClear(); - accountDataAccumulator = new AccountDataAccumulator(); + accountDataAccumulator = new AccountDataAccumulator(syncResponder); expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} }); await startClientAndAwaitFirstSync(); }); @@ -1787,28 +1787,18 @@ describe("crypto", () => { repeat: 1, overwriteRoutes: true, }); - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); if (content.key) { return content.key; } } } - function awaitMegolmBackupKeyUpload(): Promise> { - return new Promise((resolve) => { - // Called when the megolm backup key is uploaded - fetchMock.put( - `express:/_matrix/client/v3/user/:userId/account_data/m.megolm_backup.v1`, - (url: string, options: RequestInit) => { - const content = JSON.parse(options.body as string); - // update account data for sync response - accountDataAccumulator.accountDataEvents.set("m.megolm_backup.v1", content); - resolve(content.encrypted); - return {}; - }, - { overwriteRoutes: true }, - ); + async function awaitMegolmBackupKeyUpload(): Promise> { + const content = await accountDataAccumulator.interceptSetAccountData("m.megolm_backup.v1", { + repeat: 1, + overwriteRoutes: true, }); + return content.encrypted; } function awaitAccountDataUpdate(type: string): Promise { @@ -1866,9 +1856,6 @@ describe("crypto", () => { // wait for bootstrapSecretStorage to finished await bootstrapPromise; - // Return the newly created key in the sync response - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); - // Finally ensure backup is working await aliceClient.getCrypto()!.checkKeyBackupAndEnable(); @@ -1928,9 +1915,6 @@ describe("crypto", () => { expect(keyContent.iv).toBeDefined(); expect(keyContent.mac).toBeDefined(); - // Return the newly created key in the sync response - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); - // Finally, wait for bootstrapSecretStorage to finished await bootstrapPromise; @@ -1950,9 +1934,6 @@ describe("crypto", () => { // Wait for the key to be uploaded in the account data await awaitSecretStorageKeyStoredInAccountData(); - // Return the newly created key in the sync response - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); - // Wait for bootstrapSecretStorage to finished await bootstrapPromise; @@ -1971,9 +1952,6 @@ describe("crypto", () => { // Wait for the key to be uploaded in the account data await awaitSecretStorageKeyStoredInAccountData(); - // Return the newly created key in the sync response - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); - // Wait for bootstrapSecretStorage to finished await bootstrapPromise; @@ -1985,9 +1963,6 @@ describe("crypto", () => { // Wait for the key to be uploaded in the account data await awaitSecretStorageKeyStoredInAccountData(); - // Return the newly created key in the sync response - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); - // Wait for bootstrapSecretStorage to finished await bootstrapPromise; @@ -2009,9 +1984,6 @@ describe("crypto", () => { // Wait for the key to be uploaded in the account data const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData(); - // Return the newly created key in the sync response - accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); - // Wait for the cross signing keys to be uploaded const [masterKey, userSigningKey, selfSigningKey] = await Promise.all([ awaitCrossSigningKeyUpload("master"), diff --git a/spec/test-utils/AccountDataAccumulator.ts b/spec/test-utils/AccountDataAccumulator.ts index ce74603b1..2e1d6d528 100644 --- a/spec/test-utils/AccountDataAccumulator.ts +++ b/spec/test-utils/AccountDataAccumulator.ts @@ -19,16 +19,23 @@ import fetchMock from "fetch-mock-jest"; import { type ISyncResponder } from "./SyncResponder"; /** - * An object which intercepts `account_data` get and set requests via fetch-mock. + * An object which intercepts `account_data` get and set requests via fetch-mock. + * + * To use this, call {@link interceptSetAccountData} for each type of account date that should be handled. The updated + * account data will be stored in {@link accountDataEvents}; it will also trigger a sync response echoing the updated + * data. + * + * Optionally, you can also call {@link interceptGetAccountData}. */ export class AccountDataAccumulator { /** * The account data events to be returned by the sync. * Will be updated when fetchMock intercepts calls to PUT `/_matrix/client/v3/user/:userId/account_data/`. - * Will be used by `sendSyncResponseWithUpdatedAccountData` */ public accountDataEvents: Map = new Map(); + public constructor(private syncResponder: ISyncResponder) {} + /** * Intercept requests to set a particular type of account data. * @@ -53,6 +60,9 @@ export class AccountDataAccumulator { // update account data for sync response this.accountDataEvents.set(type!, content); resolve(content); + + // return a sync response + this.sendSyncResponseWithUpdatedAccountData(); return {}; }, opts, @@ -90,9 +100,9 @@ export class AccountDataAccumulator { /** * Send a sync response the current account data events. */ - public sendSyncResponseWithUpdatedAccountData(syncResponder: ISyncResponder): void { + private sendSyncResponseWithUpdatedAccountData(): void { try { - syncResponder.sendOrQueueSyncResponse({ + this.syncResponder.sendOrQueueSyncResponse({ next_batch: 1, account_data: { events: Array.from(this.accountDataEvents, ([type, content]) => ({ diff --git a/spec/test-utils/mockEndpoints.ts b/spec/test-utils/mockEndpoints.ts index 6e4a5612c..2acb205e5 100644 --- a/spec/test-utils/mockEndpoints.ts +++ b/spec/test-utils/mockEndpoints.ts @@ -22,8 +22,9 @@ import { type KeyBackupInfo } from "../../src/crypto-api"; * Mock out the endpoints that the js-sdk calls when we call `MatrixClient.start()`. * * @param homeserverUrl - the homeserver url for the client under test + * @param userId - the local user's ID. Defaults to `@alice:localhost`. */ -export function mockInitialApiRequests(homeserverUrl: string) { +export function mockInitialApiRequests(homeserverUrl: string, userId: string = "@alice:localhost") { fetchMock.getOnce( new URL("/_matrix/client/versions", homeserverUrl).toString(), { versions: ["v1.1"] }, @@ -35,7 +36,7 @@ export function mockInitialApiRequests(homeserverUrl: string) { { overwriteRoutes: true }, ); fetchMock.postOnce( - new URL("/_matrix/client/v3/user/%40alice%3Alocalhost/filter", homeserverUrl).toString(), + new URL(`/_matrix/client/v3/user/${encodeURIComponent(userId)}/filter`, homeserverUrl).toString(), { filter_id: "fid" }, { overwriteRoutes: true }, ); diff --git a/spec/unit/matrix-client.spec.ts b/spec/unit/matrix-client.spec.ts index 17951cc6e..35a2016a0 100644 --- a/spec/unit/matrix-client.spec.ts +++ b/spec/unit/matrix-client.spec.ts @@ -47,6 +47,7 @@ import { ClientPrefix, ConditionKind, ContentHelpers, + createClient, Direction, EventTimeline, EventTimelineSet, @@ -81,6 +82,8 @@ import { KnownMembership } from "../../src/@types/membership"; import { type RoomMessageEventContent } from "../../src/@types/events"; import { mockOpenIdConfiguration } from "../test-utils/oidc.ts"; import { type CryptoBackend } from "../../src/common-crypto/CryptoBackend"; +import { SyncResponder } from "../test-utils/SyncResponder.ts"; +import { mockInitialApiRequests } from "../test-utils/mockEndpoints.ts"; jest.useFakeTimers(); @@ -125,6 +128,18 @@ type WrappedRoom = Room & { _state: Map; }; +/** A list of methods to run after the current test */ +const afterTestHooks: (() => Promise | void)[] = []; + +afterEach(async () => { + fetchMock.reset(); + jest.restoreAllMocks(); + for (const hook of afterTestHooks) { + await hook(); + } + afterTestHooks.length = 0; +}); + describe("convertQueryDictToMap", () => { it("returns an empty map when dict is undefined", () => { expect(convertQueryDictToMap(undefined)).toEqual(new Map()); @@ -165,6 +180,11 @@ describe("MatrixClient", function () { const userId = "@alice:bar"; const identityServerUrl = "https://identity.server"; const identityServerDomain = "identity.server"; + + /** + * @deprecated this is hard to use correctly; better to create a regular client with {@link createClient} + * and use fetch-mock. + */ let client: MatrixClient; let store: Store; let scheduler: MatrixScheduler; @@ -2259,7 +2279,7 @@ describe("MatrixClient", function () { }); describe("checkTurnServers", () => { - beforeAll(() => { + beforeEach(() => { mocked(supportsMatrixCall).mockReturnValue(true); }); @@ -2659,10 +2679,164 @@ describe("MatrixClient", function () { }); }); - describe("delete account data", () => { - afterEach(() => { - jest.spyOn(featureUtils, "buildFeatureSupportMap").mockRestore(); + describe("setAccountData", () => { + const TEST_HOMESERVER_URL = "https://alice-server.com"; + + /** Create and start a MatrixClient, connected to the `TEST_HOMESERVER_URL` */ + async function setUpClient(): Promise { + // anything that we don't have a specific matcher for silently returns a 404 + fetchMock.catch(404); + fetchMock.config.warnOnFallback = false; + + mockInitialApiRequests(TEST_HOMESERVER_URL, userId); + + const client = createClient({ baseUrl: TEST_HOMESERVER_URL, userId }); + await client.startClient(); + + // Remember to stop the client again, to stop it spamming logs and HTTP requests + afterTestHooks.push(() => client.stopClient()); + return client; + } + + it("falls back to raw request if called before the client is started", async () => { + // GIVEN a bunch of setup + const client = createClient({ baseUrl: TEST_HOMESERVER_URL, userId }); + + const eventType = "im.vector.test"; + const content = { a: 1 }; + const testresponse = { test: 1 }; + + // ... including an expected REST request + const url = new URL( + `/_matrix/client/v3/user/${encodeURIComponent(client.getSafeUserId())}/account_data/${eventType}`, + TEST_HOMESERVER_URL, + ).toString(); + fetchMock.put({ url, name: "put-account-data" }, testresponse); + + // suppress the expected warning on the console + jest.spyOn(console, "warn").mockImplementation(); + + // WHEN we call `setAccountData` ... + const result = await client.setAccountData(eventType, content); + + // THEN, method should have returned the right thing + expect(result).toEqual(testresponse); + + // and the REST call should have happened, and had the correct content + const lastCall = fetchMock.lastCall("put-account-data"); + expect(lastCall).toBeDefined(); + expect(lastCall?.[1]?.body).toEqual(JSON.stringify(content)); + + // and a warning should have been logged + // eslint-disable-next-line no-console + expect(console.warn).toHaveBeenCalledWith( + expect.stringContaining("Calling `setAccountData` before the client is started"), + ); }); + + it("makes a request to the server, and waits for the /sync response", async () => { + // GIVEN a bunch of setup + const syncResponder = new SyncResponder(TEST_HOMESERVER_URL); + const client = await setUpClient(); + + const eventType = "im.vector.test"; + const content = { a: 1 }; + const testresponse = { test: 1 }; + + // ... including an expected REST request + const url = new URL( + `/_matrix/client/v3/user/${encodeURIComponent(client.getSafeUserId())}/account_data/${eventType}`, + TEST_HOMESERVER_URL, + ).toString(); + fetchMock.put({ url, name: "put-account-data" }, testresponse); + + // WHEN we call `setAccountData` ... + const setProm = client.setAccountData(eventType, content); + + // THEN, the REST call should have happened, and had the correct content + const lastCall = fetchMock.lastCall("put-account-data"); + expect(lastCall).toBeDefined(); + expect(lastCall?.[1]?.body).toEqual(JSON.stringify(content)); + + // Even after waiting a bit, the method should not yet have returned + await jest.advanceTimersByTimeAsync(10); + let finished = false; + setProm.finally(() => (finished = true)); + expect(finished).toBeFalsy(); + + // ... and `getAccountData` still returns the wrong thing + expect(client.getAccountData(eventType)).not.toBeDefined(); + + // WHEN the update arrives over /sync + const content2 = { a: 2 }; + + syncResponder.sendOrQueueSyncResponse({ + account_data: { events: [{ type: eventType, content: content2 }] }, + }); + + // THEN the method should complete, and `getAccountData` returns the new data + expect(await setProm).toEqual(testresponse); + expect(client.getAccountData(eventType)?.event).toEqual({ type: eventType, content: content2 }); + }); + + it("does nothing if the data matches what is there", async () => { + // GIVEN a running matrix client ... + const syncResponder = new SyncResponder(TEST_HOMESERVER_URL); + const client = await setUpClient(); + + // ... which has previously received the account data over /sync + const eventType = "im.vector.test"; + const content = { a: 1, b: 2 }; + + // Keys in a different order, to check that doesn't matter. + const syncedContent = { b: 2, a: 1 }; + syncResponder.sendOrQueueSyncResponse({ + account_data: { events: [{ type: eventType, content: syncedContent }] }, + }); + await jest.advanceTimersByTimeAsync(1); + + // Check that getAccountData is ready + expect(client.getAccountData(eventType)?.event).toEqual({ type: eventType, content: syncedContent }); + + // WHEN we call `setAccountData` ... + await client.setAccountData(eventType, content); + + // THEN there should be no REST call + expect(fetchMock.calls(/account_data/).length).toEqual(0); + }); + }); + + describe("delete account data", () => { + const TEST_HOMESERVER_URL = "https://alice-server.com"; + + /** Create and start a MatrixClient, connected to the `TEST_HOMESERVER_URL` */ + async function setUpClient(versionsResponse: object = { versions: ["1"] }): Promise { + // anything that we don't have a specific matcher for silently returns a 404 + fetchMock.catch(404); + fetchMock.config.warnOnFallback = false; + + fetchMock.getOnce(new URL("/_matrix/client/versions", TEST_HOMESERVER_URL).toString(), versionsResponse, { + overwriteRoutes: true, + }); + fetchMock.getOnce( + new URL("/_matrix/client/v3/pushrules/", TEST_HOMESERVER_URL).toString(), + {}, + { overwriteRoutes: true }, + ); + fetchMock.postOnce( + new URL(`/_matrix/client/v3/user/${encodeURIComponent(userId)}/filter`, TEST_HOMESERVER_URL).toString(), + { filter_id: "fid" }, + { overwriteRoutes: true }, + ); + + const client = createClient({ baseUrl: TEST_HOMESERVER_URL, userId }); + await client.startClient(); + + // Remember to stop the client again, to stop it spamming logs and HTTP requests + afterTestHooks.push(() => client.stopClient()); + return client; + } + it("makes correct request when deletion is supported by server in unstable versions", async () => { const eventType = "im.vector.test"; const versionsResponse = { @@ -2671,17 +2845,17 @@ describe("MatrixClient", function () { "org.matrix.msc3391": true, }, }; - const requestSpy = jest.spyOn(client.http, "authedRequest").mockResolvedValue(versionsResponse); - const unstablePrefix = "/_matrix/client/unstable/org.matrix.msc3391"; - const path = `/user/${encodeURIComponent(userId)}/account_data/${eventType}`; + const client = await setUpClient(versionsResponse); + + const url = new URL( + `/_matrix/client/unstable/org.matrix.msc3391/user/${encodeURIComponent(userId)}/account_data/${eventType}`, + TEST_HOMESERVER_URL, + ).toString(); + fetchMock.delete({ url, name: "delete-data" }, {}); - // populate version support - await client.getVersions(); await client.deleteAccountData(eventType); - expect(requestSpy).toHaveBeenCalledWith(Method.Delete, path, undefined, undefined, { - prefix: unstablePrefix, - }); + expect(fetchMock.calls("delete-data").length).toEqual(1); }); it("makes correct request when deletion is supported by server based on matrix version", async () => { @@ -2690,34 +2864,43 @@ describe("MatrixClient", function () { // so mock the support map to fake stable support const stableSupportedDeletionMap = new Map(); stableSupportedDeletionMap.set(featureUtils.Feature.AccountDataDeletion, featureUtils.ServerSupport.Stable); - jest.spyOn(featureUtils, "buildFeatureSupportMap").mockResolvedValue(new Map()); - const requestSpy = jest.spyOn(client.http, "authedRequest").mockImplementation(() => Promise.resolve()); - const path = `/user/${encodeURIComponent(userId)}/account_data/${eventType}`; + jest.spyOn(featureUtils, "buildFeatureSupportMap").mockResolvedValue(stableSupportedDeletionMap); + + const client = await setUpClient(); + + const url = new URL( + `/_matrix/client/v3/user/${encodeURIComponent(userId)}/account_data/${eventType}`, + TEST_HOMESERVER_URL, + ).toString(); + fetchMock.delete({ url, name: "delete-data" }, {}); - // populate version support - await client.getVersions(); await client.deleteAccountData(eventType); - expect(requestSpy).toHaveBeenCalledWith(Method.Delete, path, undefined, undefined, undefined); + expect(fetchMock.calls("delete-data").length).toEqual(1); }); it("makes correct request when deletion is not supported by server", async () => { const eventType = "im.vector.test"; - const versionsResponse = { - versions: ["1"], - unstable_features: { - "org.matrix.msc3391": false, - }, - }; - const requestSpy = jest.spyOn(client.http, "authedRequest").mockResolvedValue(versionsResponse); - const path = `/user/${encodeURIComponent(userId)}/account_data/${eventType}`; - // populate version support - await client.getVersions(); - await client.deleteAccountData(eventType); + const syncResponder = new SyncResponder(TEST_HOMESERVER_URL); + const client = await setUpClient(); + + const url = new URL( + `/_matrix/client/v3/user/${encodeURIComponent(userId)}/account_data/${eventType}`, + TEST_HOMESERVER_URL, + ).toString(); + fetchMock.put({ url, name: "put-account-data" }, {}); + + const setProm = client.deleteAccountData(eventType); + syncResponder.sendOrQueueSyncResponse({ + account_data: { events: [{ type: eventType, content: {} }] }, + }); + await setProm; // account data updated with empty content - expect(requestSpy).toHaveBeenCalledWith(Method.Put, path, undefined, {}); + const lastCall = fetchMock.lastCall("put-account-data"); + expect(lastCall).toBeDefined(); + expect(lastCall?.[1]?.body).toEqual("{}"); }); }); diff --git a/src/client.ts b/src/client.ts index 8f6fc0408..e5f22933b 100644 --- a/src/client.ts +++ b/src/client.ts @@ -52,7 +52,7 @@ import { type GroupCallEventHandlerEventHandlerMap, } from "./webrtc/groupCallEventHandler.ts"; import * as utils from "./utils.ts"; -import { noUnsafeEventProps, type QueryDict, replaceParam, safeSet, sleep } from "./utils.ts"; +import { deepCompare, defer, noUnsafeEventProps, type QueryDict, replaceParam, safeSet, sleep } from "./utils.ts"; import { Direction, EventTimeline } from "./models/event-timeline.ts"; import { type IActionsObject, PushProcessor } from "./pushprocessor.ts"; import { AutoDiscovery, type AutoDiscoveryAction } from "./autodiscovery.ts"; @@ -2149,14 +2149,72 @@ export class MatrixClient extends TypedEventEmitter( + public async setAccountData( + eventType: K, + content: AccountDataEvents[K] | Record, + ): Promise { + // If the sync loop is not running, fall back to setAccountDataRaw. + if (!this.clientRunning) { + logger.warn( + "Calling `setAccountData` before the client is started: `getAccountData` may return inconsistent results.", + ); + return await retryNetworkOperation(5, () => this.setAccountDataRaw(eventType, content)); + } + + // If the account data is already correct, then we cannot expect an update over sync, and the operation + // is, in any case, a no-op. + // + // NB that we rely on this operation being synchronous to avoid a race condition: there must be no `await` + // between here and `this.addListener` below, in case we miss an update. + const existingData = this.store.getAccountData(eventType); + if (existingData && deepCompare(existingData.event.content, content)) return {}; + + // Create a promise which will resolve when the update is received + const updatedDefer = defer(); + function accountDataListener(event: MatrixEvent): void { + // Note that we cannot safely check that the content matches what we expected, because there is a race: + // * We set the new content + // * Another client sets alternative content + // * Then /sync returns, but only reflects the latest content. + // + // Of course there is room for debate over what we should actually do in that case -- a subsequent + // `getAccountData` isn't going to return the expected value, but whose fault is that? Databases are hard. + // + // Anyway, what we *shouldn't* do is get stuck in a loop. I think the best we can do is check that the event + // type matches. + if (event.getType() === eventType) updatedDefer.resolve(); + } + this.addListener(ClientEvent.AccountData, accountDataListener); + + try { + const result = await retryNetworkOperation(5, () => this.setAccountDataRaw(eventType, content)); + await updatedDefer.promise; + return result; + } finally { + this.removeListener(ClientEvent.AccountData, accountDataListener); + } + } + + /** + * Set account data event for the current user, without waiting for the remote echo. + * + * @param eventType - The event type + * @param content - the contents object for the event + */ + public setAccountDataRaw( eventType: K, content: AccountDataEvents[K] | Record, ): Promise { @@ -2164,9 +2222,8 @@ export class MatrixClient extends TypedEventEmitter { - return this.http.authedRequest(Method.Put, path, undefined, content); - }); + + return this.http.authedRequest(Method.Put, path, undefined, content); } /**