MatrixClient.setAccountData: await remote echo. (#4695)

* Rewrite `deleteAccountData` test

use fetch-mock rather than whatever this was

* `MatrixClient.setAccountData`: await remote echo

Wait for the echo to come back from the server before we assume the account
data has been successfully set

* Update integration tests

Fix up the integ tests which call `setAccountData` and now need a sync
response.

* Address review comment

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" };

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<Record<string, {}>> {
-            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<Record<string, {}>> {
+            const content = await accountDataAccumulator.interceptSetAccountData("m.megolm_backup.v1", {
+                repeat: 1,
+                overwriteRoutes: true,
             });
+            return content.encrypted;
         }
 
         function awaitAccountDataUpdate(type: string): Promise<void> {
@@ -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"),

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<string, any> = 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]) => ({

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 },
     );

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<string, any>;
 };
 
+/** A list of methods to run after the current test */
+const afterTestHooks: (() => Promise<void> | 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<MatrixClient> {
+            // 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<MatrixClient> {
+            // 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("{}");
         });
     });