From 65b0ac8c287ea45561dde0e69745973d5f85229e Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 2 Jun 2026 11:25:03 +0100 Subject: [PATCH] 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 | 3 +- .../encryption/KeyStoragePanelViewModel.ts | 22 ++-- .../web/src/device-listener/DeviceListener.ts | 3 +- .../DeviceListenerCurrentDevice.ts | 33 +++-- .../test/unit-tests/DeviceListener-test.ts | 118 +++++++++++------- .../KeyStoragePanelViewModel-test.ts | 7 ++ 6 files changed, 123 insertions(+), 63 deletions(-) diff --git a/apps/web/playwright/e2e/crypto/toasts.spec.ts b/apps/web/playwright/e2e/crypto/toasts.spec.ts index 010e81a8d4..9267739305 100644 --- a/apps/web/playwright/e2e/crypto/toasts.spec.ts +++ b/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)); diff --git a/apps/web/src/components/viewmodels/settings/encryption/KeyStoragePanelViewModel.ts b/apps/web/src/components/viewmodels/settings/encryption/KeyStoragePanelViewModel.ts index d968b91a7c..ba873b2350 100644 --- a/apps/web/src/components/viewmodels/settings/encryption/KeyStoragePanelViewModel.ts +++ b/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 - await matrixClient.setAccountData(BACKUP_DISABLED_ACCOUNT_DATA_KEY, { disabled: false }); + // Set the flag to say that the user wants backup enabled + 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. - // Element X uses this to determine whether to set up automatically, - // so this will stop EX turning it back on spontaneously. - await matrixClient.setAccountData(BACKUP_DISABLED_ACCOUNT_DATA_KEY, { disabled: true }); + // Set the flag to say that the user wants backup disabled + await matrixClient.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP, { enabled: false }); + await matrixClient.setAccountData(ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE, { + disabled: true, + }); } }); } finally { diff --git a/apps/web/src/device-listener/DeviceListener.ts b/apps/web/src/device-listener/DeviceListener.ts index fea00c0380..f4e3fa3e82 100644 --- a/apps/web/src/device-listener/DeviceListener.ts +++ b/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 { await this.currentDevice?.recordKeyBackupDisabled(); diff --git a/apps/web/src/device-listener/DeviceListenerCurrentDevice.ts b/apps/web/src/device-listener/DeviceListenerCurrentDevice.ts index c66a7e9945..32320edc1f 100644 --- a/apps/web/src/device-listener/DeviceListenerCurrentDevice.ts +++ b/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 - * has chosen to disable server side key backups. - * - * We need to set and honour this to prevent Element X from automatically turning key backup back on. + * Account data key used to indicate that the user has chosen to enable or + * disable server side key backups. */ -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 { - 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 { - const backupDisabled = await this.client.getAccountDataFromServer(BACKUP_DISABLED_ACCOUNT_DATA_KEY); - return !!backupDisabled?.disabled; + const keyBackup = await this.client.getAccountDataFromServer(ACCOUNT_DATA_KEY_M_KEY_BACKUP); + 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(); diff --git a/apps/web/test/unit-tests/DeviceListener-test.ts b/apps/web/test/unit-tests/DeviceListener-test.ts index a8ebd4760e..44962c84aa 100644 --- a/apps/web/test/unit-tests/DeviceListener-test.ts +++ b/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) => - eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY ? ({ disabled: true } as any) : null, - ); + mockKeyBackupFromServer(mockClient, false); 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) => - eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY - ? new MatrixEvent({ content: { disabled: false } }) - : undefined, - ); + mockClient.getAccountData.mockImplementation((eventType) => { + switch (eventType) { + case ACCOUNT_DATA_KEY_M_KEY_BACKUP: + 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) => - eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY ? ({ disabled: true } as any) : null, - ); + mockKeyBackupFromServer(mockClient, false); // 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) => - eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY - ? new MatrixEvent({ content: { disabled: false } }) - : undefined, - ); + mockClient.getAccountData.mockImplementation((eventType) => { + switch (eventType) { + case ACCOUNT_DATA_KEY_M_KEY_BACKUP: + 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) => - eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY - ? new MatrixEvent({ content: { disabled: true } }) - : undefined, - ); + mockClient.getAccountData.mockImplementation((eventType) => { + switch (eventType) { + case ACCOUNT_DATA_KEY_M_KEY_BACKUP: + 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") { - return new MatrixEvent({ content: { disabled: true } }); + switch (k) { + 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({ - disabled: true, - }); + mockKeyBackupFromServer(mockClient, false); + 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({ - disabled: false, - }); + mockKeyBackupFromServer(mockClient, true); 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({ - disabled: false, - }); + mockKeyBackupFromServer(mockClient, true); 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({ - disabled: false, - }); + mockKeyBackupFromServer(mockClient, true); 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({ - disabled: false, - }); + mockKeyBackupFromServer(mockClient, true); mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(null); mockClient.isKeyBackupKeyStored.mockResolvedValue(null); @@ -1421,3 +1440,16 @@ describe("DeviceListener", () => { }); }); }); + +function mockKeyBackupFromServer(client: MockedObject, 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; + } + }); +} diff --git a/apps/web/test/unit-tests/components/viewmodels/settings/encryption/KeyStoragePanelViewModel-test.ts b/apps/web/test/unit-tests/components/viewmodels/settings/encryption/KeyStoragePanelViewModel-test.ts index eb9c81c4a7..dc1faec985 100644 --- a/apps/web/test/unit-tests/components/viewmodels/settings/encryption/KeyStoragePanelViewModel-test.ts +++ b/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, + }); }); });