Support MSC4287 m.key_backup stable prefix as well as the unstable prefix (#33034)

* Support MSC4287 m.key_backup stable prefix as well as the unstable prefix

* Update comments to avoid talking about EX since this now applies more widely

* Make comments about m.key_backup more helpful

* Improve comment on recheckBackupDisabled

* Explicitly say backup is disabled only if enabled is actually set to false

apps/web/playwright/e2e/crypto/toasts.spec.ts

@@ -120,7 +120,8 @@ test.describe("'Turn on key storage' toast", () => {
     test("should show toast if key storage is off but account data is missing", async ({ app, page }) => {
         // Given the backup is disabled but we didn't set account data saying that is expected
         await disableKeyBackup(app);
-        await botClient.setAccountData("m.org.matrix.custom.backup_disabled", { disabled: false });
+        await botClient.setAccountData("m.org.matrix.custom.backup_disabled", {} as any as { disabled: boolean });
+        await botClient.setAccountData("m.key_backup", {} as any as { enabled: boolean });
 
         // Wait for the account data setting to stick
         await new Promise((resolve) => setTimeout(resolve, 2000));

apps/web/src/components/viewmodels/settings/encryption/KeyStoragePanelViewModel.ts

@@ -10,7 +10,11 @@ import { CryptoEvent } from "matrix-js-sdk/src/crypto-api";
 import { logger } from "matrix-js-sdk/src/logger";
 
 import { useMatrixClientContext } from "../../../../contexts/MatrixClientContext";
-import { DeviceListener, BACKUP_DISABLED_ACCOUNT_DATA_KEY } from "../../../../device-listener";
+import {
+    DeviceListener,
+    ACCOUNT_DATA_KEY_M_KEY_BACKUP,
+    ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE,
+} from "../../../../device-listener";
 import { useEventEmitterAsyncState } from "../../../../hooks/useEventEmitter";
 import { resetKeyBackupAndWait } from "../../../../utils/crypto/resetKeyBackup";
 
@@ -112,18 +116,22 @@ export function useKeyStoragePanelViewModel(): KeyStoragePanelState {
                             await resetKeyBackupAndWait(crypto);
                         }
 
-                        // Set the flag so that EX no longer thinks the user wants backup disabled
+                        // Set the flag to say that the user wants backup enabled
-                        await matrixClient.setAccountData(BACKUP_DISABLED_ACCOUNT_DATA_KEY, { disabled: false });
+                        await matrixClient.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP, { enabled: true });
+                        await matrixClient.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE, {
+                            disabled: false,
+                        });
                     } else {
                         logger.info("User requested disabling key backup");
                         // This method will delete the key backup as well as server side recovery keys and other
                         // server-side crypto data.
                         await crypto.disableKeyStorage();
 
-                        // Set a flag to say that the user doesn't want key backup.
+                        // Set the flag to say that the user wants backup disabled
-                        // Element X uses this to determine whether to set up automatically,
+                        await matrixClient.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP, { enabled: false });
-                        // so this will stop EX turning it back on spontaneously.
+                        await matrixClient.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE, {
-                        await matrixClient.setAccountData(BACKUP_DISABLED_ACCOUNT_DATA_KEY, { disabled: true });
+                            disabled: true,
+                        });
                     }
                 });
             } finally {

apps/web/src/device-listener/DeviceListener.ts

@@ -149,7 +149,8 @@ export class DeviceListener {
     }
 
     /**
-     * Set the account data "m.org.matrix.custom.backup_disabled" to { "disabled": true }.
+     * Set the account data indicate that the user has chosen to disable key
+     * backup.
      */
     public async recordKeyBackupDisabled(): Promise<void> {
         await this.currentDevice?.recordKeyBackupDisabled();

apps/web/src/device-listener/DeviceListenerCurrentDevice.ts

@@ -29,12 +29,11 @@ import { isSecretStorageBeingAccessed } from "../SecurityManager";
 const KEY_BACKUP_POLL_INTERVAL = 5 * 60 * 1000;
 
 /**
- * Unfortunately-named account data key used by Element X to indicate that the user
+ * Account data key used to indicate that the user has chosen to enable or
- * has chosen to disable server side key backups.
+ * disable server side key backups.
- *
- * We need to set and honour this to prevent Element X from automatically turning key backup back on.
  */
-export const BACKUP_DISABLED_ACCOUNT_DATA_KEY = "m.org.matrix.custom.backup_disabled";
+export const ACCOUNT_DATA_KEY_M_KEY_BACKUP = "m.key_backup";
+export const ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE = "m.org.matrix.custom.backup_disabled";
 
 /**
  * Account data key to indicate whether the user has chosen to enable or disable recovery.
@@ -148,10 +147,12 @@ export class DeviceListenerCurrentDevice {
     }
 
     /**
-     * Set the account data "m.org.matrix.custom.backup_disabled" to `{ "disabled": true }`.
+     * Set the account data indicate that the user has chosen to disable key
+     * backup.
      */
     public async recordKeyBackupDisabled(): Promise<void> {
-        await this.client.setAccountData(BACKUP_DISABLED_ACCOUNT_DATA_KEY, { disabled: true });
+        await this.client.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP, { enabled: false });
+        await this.client.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE, { disabled: true });
     }
 
     /**
@@ -295,13 +296,22 @@ export class DeviceListenerCurrentDevice {
     }
 
     /**
-     * Fetch the account data for `backup_disabled`. If this is the first time,
+     * Fetch the account data for `m.key_backup`. If this is the first time,
      * fetch it from the server (in case the initial sync has not finished).
      * Otherwise, fetch it from the store as normal.
+     *
+     * Returns true if `m.key_backup` has `enabled: false`.
      */
     public async recheckBackupDisabled(): Promise<boolean> {
-        const backupDisabled = await this.client.getAccountDataFromServer(BACKUP_DISABLED_ACCOUNT_DATA_KEY);
+        const keyBackup = await this.client.getAccountDataFromServer(ACCOUNT_DATA_KEY_M_KEY_BACKUP);
-        return !!backupDisabled?.disabled;
+        if (keyBackup) {
+            return keyBackup.enabled === false;
+        }
+
+        const keyBackupDisabledUnstable = await this.client.getAccountDataFromServer(
+            ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE,
+        );
+        return !!keyBackupDisabledUnstable?.disabled;
     }
 
     /**
@@ -350,7 +360,8 @@ export class DeviceListenerCurrentDevice {
             ev.getType().startsWith("m.secret_storage.") ||
             ev.getType().startsWith("m.cross_signing.") ||
             ev.getType() === "m.megolm_backup.v1" ||
-            ev.getType() === BACKUP_DISABLED_ACCOUNT_DATA_KEY ||
+            ev.getType() === ACCOUNT_DATA_KEY_M_KEY_BACKUP ||
+            ev.getType() === ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE ||
             ev.getType() === RECOVERY_ACCOUNT_DATA_KEY
         ) {
             this.deviceListener.recheck();

apps/web/test/unit-tests/DeviceListener-test.ts

@@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Com
 Please see LICENSE files in the repository root for full details.
 */
 
-import { type Mocked, mocked } from "jest-mock";
+import { type Mocked, mocked, type MockedObject } from "jest-mock";
 import {
     MatrixEvent,
     type Room,
@@ -25,7 +25,12 @@ import {
 } from "matrix-js-sdk/src/crypto-api";
 import { type CryptoSessionStateChange } from "@matrix-org/analytics-events/types/typescript/CryptoSessionStateChange";
 
-import { DeviceListener, BACKUP_DISABLED_ACCOUNT_DATA_KEY, RECOVERY_ACCOUNT_DATA_KEY } from "../../src/device-listener";
+import {
+    DeviceListener,
+    ACCOUNT_DATA_KEY_M_KEY_BACKUP,
+    ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE,
+    RECOVERY_ACCOUNT_DATA_KEY,
+} from "../../src/device-listener";
 import { MatrixClientPeg } from "../../src/MatrixClientPeg";
 import * as SetupEncryptionToast from "../../src/toasts/SetupEncryptionToast";
 import * as UnverifiedSessionToast from "../../src/toasts/UnverifiedSessionToast";
@@ -322,8 +327,10 @@ describe("DeviceListener", () => {
 
             // @ts-ignore implementing a function with complex return type
             mockClient!.getAccountDataFromServer.mockImplementation(async (key) => {
-                if (key === BACKUP_DISABLED_ACCOUNT_DATA_KEY) {
+                if (key === ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE) {
                     return { disabled: true };
+                } else if (key == ACCOUNT_DATA_KEY_M_KEY_BACKUP) {
+                    return { enabled: false };
                 } else if (key === RECOVERY_ACCOUNT_DATA_KEY) {
                     return null;
                 } else {
@@ -513,9 +520,7 @@ describe("DeviceListener", () => {
                 it("does not show an out-of-sync toast when the backup key is missing locally but backup is purposely disabled", async () => {
                     mockCrypto!.getSecretStorageStatus.mockResolvedValue(readySecretStorageStatus);
                     mockCrypto!.getSessionBackupPrivateKey.mockResolvedValue(null);
-                    mockClient.getAccountDataFromServer.mockImplementation((eventType) =>
+                    mockKeyBackupFromServer(mockClient, false);
-                        eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY ? ({ disabled: true } as any) : null,
-                    );
 
                     await createAndStart();
 
@@ -621,6 +626,10 @@ describe("DeviceListener", () => {
             expect(mockClient.setAccountData).toHaveBeenCalledWith("m.org.matrix.custom.backup_disabled", {
                 disabled: true,
             });
+
+            expect(mockClient.setAccountData).toHaveBeenCalledWith("m.key_backup", {
+                enabled: false,
+            });
         });
 
         it("sets the recovery account data when we call recordRecoveryDisabled", async () => {
@@ -652,7 +661,7 @@ describe("DeviceListener", () => {
 
                 it("shows the 'Turn on key storage' toast if we never explicitly turned off key storage", async () => {
                     // Given key backup is off but the account data saying we turned it off is not set
-                    // (m.org.matrix.custom.backup_disabled)
+                    // (m.key_backup or m.org.matrix.custom.backup_disabled)
                     mockClient.getAccountData.mockReturnValue(undefined);
 
                     // When we launch the DeviceListener
@@ -665,11 +674,16 @@ describe("DeviceListener", () => {
                 it("shows the 'Turn on key storage' toast if we turned on key storage", async () => {
                     // Given key backup is off but the account data says we turned it on (this should not happen - the
                     // account data should only be updated if we turn on key storage)
-                    mockClient.getAccountData.mockImplementation((eventType) =>
+                    mockClient.getAccountData.mockImplementation((eventType) => {
-                        eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY
+                        switch (eventType) {
-                            ? new MatrixEvent({ content: { disabled: false } })
+                            case ACCOUNT_DATA_KEY_M_KEY_BACKUP:
-                            : undefined,
+                                return new MatrixEvent({ content: { enabled: true } });
-                    );
+                            case ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE:
+                                return new MatrixEvent({ content: { disabled: false } });
+                            default:
+                                return undefined;
+                        }
+                    });
 
                     // When we launch the DeviceListener
                     await createAndStart();
@@ -680,9 +694,7 @@ describe("DeviceListener", () => {
 
                 it("does not show the 'Turn on key storage' toast if we turned off key storage", async () => {
                     // Given key backup is off but the account data saying we turned it off is set
-                    mockClient.getAccountDataFromServer.mockImplementation((eventType) =>
+                    mockKeyBackupFromServer(mockClient, false);
-                        eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY ? ({ disabled: true } as any) : null,
-                    );
 
                     // When we launch the DeviceListener
                     await createAndStart();
@@ -711,11 +723,16 @@ describe("DeviceListener", () => {
 
                 it("does not show the 'Turn on key storage' toast if we turned on key storage", async () => {
                     // Given key backup is on and the account data says we turned it on
-                    mockClient.getAccountData.mockImplementation((eventType) =>
+                    mockClient.getAccountData.mockImplementation((eventType) => {
-                        eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY
+                        switch (eventType) {
-                            ? new MatrixEvent({ content: { disabled: false } })
+                            case ACCOUNT_DATA_KEY_M_KEY_BACKUP:
-                            : undefined,
+                                return new MatrixEvent({ content: { enabled: true } });
-                    );
+                            case ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE:
+                                return new MatrixEvent({ content: { disabled: false } });
+                            default:
+                                return undefined;
+                        }
+                    });
 
                     // When we launch the DeviceListener
                     await createAndStart();
@@ -727,11 +744,16 @@ describe("DeviceListener", () => {
                 it("does not show the 'Turn on key storage' toast if we turned off key storage", async () => {
                     // Given key backup is on but the account data saying we turned it off is set (this should never
                     // happen - it should only be set when we turn off key storage or dismiss the toast)
-                    mockClient.getAccountData.mockImplementation((eventType) =>
+                    mockClient.getAccountData.mockImplementation((eventType) => {
-                        eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY
+                        switch (eventType) {
-                            ? new MatrixEvent({ content: { disabled: true } })
+                            case ACCOUNT_DATA_KEY_M_KEY_BACKUP:
-                            : undefined,
+                                return new MatrixEvent({ content: { enabled: false } });
-                    );
+                            case ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE:
+                                return new MatrixEvent({ content: { disabled: true } });
+                            default:
+                                return undefined;
+                        }
+                    });
 
                     // When we launch the DeviceListener
                     await createAndStart();
@@ -1279,10 +1301,16 @@ describe("DeviceListener", () => {
 
             it("does not show the 'set up recovery' toast if the user has chosen to disable key storage", async () => {
                 mockClient!.getAccountData.mockImplementation((k: string) => {
-                    if (k === "m.org.matrix.custom.backup_disabled") {
+                    switch (k) {
-                        return new MatrixEvent({ content: { disabled: true } });
+                        case "m.org.matrix.custom.backup_disabled":
+                            return new MatrixEvent({ content: { disabled: true } });
+
+                        case "m.key_backup":
+                            return new MatrixEvent({ content: { enabled: false } });
+
+                        default:
+                            return undefined;
                     }
-                    return undefined;
                 });
                 await createAndStart();
 
@@ -1295,18 +1323,15 @@ describe("DeviceListener", () => {
         describe("needs backup reset", () => {
             it("should not need resetting if backup disabled", async () => {
                 const deviceListener = await createAndStart();
-                mockClient.getAccountDataFromServer.mockResolvedValue({
+                mockKeyBackupFromServer(mockClient, false);
-                    disabled: true,
+
-                });
                 expect(await deviceListener.keyStorageOutOfSyncNeedsBackupReset(false)).toBe(false);
                 expect(await deviceListener.keyStorageOutOfSyncNeedsBackupReset(true)).toBe(false);
             });
 
             it("should not need resetting if backup key is present locally or in 4S, and user has 4S key", async () => {
                 const deviceListener = await createAndStart();
-                mockClient.getAccountDataFromServer.mockResolvedValue({
+                mockKeyBackupFromServer(mockClient, true);
-                    disabled: false,
-                });
 
                 mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(null);
                 mockClient.isKeyBackupKeyStored.mockResolvedValue({});
@@ -1319,9 +1344,7 @@ describe("DeviceListener", () => {
 
             it("should not need resetting if backup key is present locally and user forgot 4S key", async () => {
                 const deviceListener = await createAndStart();
-                mockClient.getAccountDataFromServer.mockResolvedValue({
+                mockKeyBackupFromServer(mockClient, true);
-                    disabled: false,
-                });
 
                 mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(new Uint8Array());
                 mockClient.isKeyBackupKeyStored.mockResolvedValue(null);
@@ -1330,9 +1353,7 @@ describe("DeviceListener", () => {
 
             it("should need resetting if backup key is missing locally and user forgot 4S key", async () => {
                 const deviceListener = await createAndStart();
-                mockClient.getAccountDataFromServer.mockResolvedValue({
+                mockKeyBackupFromServer(mockClient, true);
-                    disabled: false,
-                });
 
                 mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(null);
                 mockClient.isKeyBackupKeyStored.mockResolvedValue({});
@@ -1341,9 +1362,7 @@ describe("DeviceListener", () => {
 
             it("should need resetting if backup key is missing locally and in 4s", async () => {
                 const deviceListener = await createAndStart();
-                mockClient.getAccountDataFromServer.mockResolvedValue({
+                mockKeyBackupFromServer(mockClient, true);
-                    disabled: false,
-                });
 
                 mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(null);
                 mockClient.isKeyBackupKeyStored.mockResolvedValue(null);
@@ -1421,3 +1440,16 @@ describe("DeviceListener", () => {
         });
     });
 });
+
+function mockKeyBackupFromServer(client: MockedObject<MatrixClient>, enabled: boolean) {
+    client.getAccountDataFromServer.mockImplementation(async (eventType: string) => {
+        switch (eventType) {
+            case ACCOUNT_DATA_KEY_M_KEY_BACKUP:
+                return { enabled } as any;
+            case ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE:
+                return { disabled: !enabled } as any;
+            default:
+                return null;
+        }
+    });
+}

apps/web/test/unit-tests/components/viewmodels/settings/encryption/KeyStoragePanelViewModel-test.ts

@@ -128,6 +128,10 @@ describe("KeyStoragePanelViewModel", () => {
         expect(mocked(matrixClient.setAccountData)).toHaveBeenCalledWith("m.org.matrix.custom.backup_disabled", {
             disabled: false,
         });
+
+        expect(mocked(matrixClient.setAccountData)).toHaveBeenCalledWith("m.key_backup", {
+            enabled: true,
+        });
     });
 
     it("should delete key storage when disabling", async () => {
@@ -145,5 +149,8 @@ describe("KeyStoragePanelViewModel", () => {
         expect(mocked(matrixClient.setAccountData)).toHaveBeenCalledWith("m.org.matrix.custom.backup_disabled", {
             disabled: true,
         });
+        expect(mocked(matrixClient.setAccountData)).toHaveBeenCalledWith("m.key_backup", {
+            enabled: false,
+        });
     });
 });