Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
andybalaam wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion apps/web/playwright/e2e/crypto/toasts.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,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, toasts }) => {
// 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));
Expand Down
16 changes: 13 additions & 3 deletions apps/web/src/components/viewmodels/settings/encryption/KeyStoragePanelViewModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -113,7 +117,10 @@ export function useKeyStoragePanelViewModel(): KeyStoragePanelState {
}

// Set the flag so that EX no longer thinks the user wants backup disabled
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
Expand All @@ -123,7 +130,10 @@ export function useKeyStoragePanelViewModel(): KeyStoragePanelState {
// 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 });
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 {
Expand Down
2 changes: 1 addition & 1 deletion apps/web/src/device-listener/DeviceListener.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ export class DeviceListener {
}

/**
* Set the account data "m.org.matrix.custom.backup_disabled" to { "disabled": true }.
* Set the account data "m.key_backup" to { "enabled": false }.
*/
public async recordKeyBackupDisabled(): Promise<void> {
await this.currentDevice?.recordKeyBackupDisabled();
Expand Down
28 changes: 18 additions & 10 deletions apps/web/src/device-listener/DeviceListenerCurrentDevice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ import { asyncSomeParallel } from "../utils/arrays";
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 by 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.
Expand Down Expand Up @@ -130,10 +129,11 @@ export class DeviceListenerCurrentDevice {
}

/**
* Set the account data "m.org.matrix.custom.backup_disabled" to `{ "disabled": true }`.
* Set the account data "m.key_backup" to `{ "enabled": false }`.
*/
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 });
}

/**
Expand Down Expand Up @@ -284,8 +284,15 @@ export class DeviceListenerCurrentDevice {
* Otherwise, fetch it from the store as normal.
*/
public async recheckBackupDisabled(): Promise<boolean> {
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;
}

const keyBackupDisabledUnstable = await this.client.getAccountDataFromServer(
ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE,
);
return !!keyBackupDisabledUnstable?.disabled;
}

/**
Expand Down Expand Up @@ -334,7 +341,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();
Expand Down
89 changes: 65 additions & 24 deletions apps/web/test/unit-tests/DeviceListener-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@
} 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 } from "../../src/device-listener";
import {
DeviceListener,
ACCOUNT_DATA_KEY_M_KEY_BACKUP,
ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE,
} from "../../src/device-listener";
import { MatrixClientPeg } from "../../src/MatrixClientPeg";
import * as SetupEncryptionToast from "../../src/toasts/SetupEncryptionToast";
import * as UnverifiedSessionToast from "../../src/toasts/UnverifiedSessionToast";
Expand Down Expand Up @@ -430,7 +434,12 @@
mockCrypto!.getSecretStorageStatus.mockResolvedValue(readySecretStorageStatus);
mockCrypto!.getSessionBackupPrivateKey.mockResolvedValue(null);
mockClient.getAccountDataFromServer.mockImplementation((eventType) =>
eventType === BACKUP_DISABLED_ACCOUNT_DATA_KEY ? ({ disabled: true } as any) : null,
eventType === ACCOUNT_DATA_KEY_M_KEY_BACKUP ? ({ enabled: false } as any) : null,
);
mockClient.getAccountDataFromServer.mockImplementation((eventType) =>
eventType === ACCOUNT_DATA_KEY_M_KEY_BACKUP_DISABLED_UNSTABLE
? ({ disabled: true } as any)
: null,
);

await createAndStart();
Expand Down Expand Up @@ -537,6 +546,10 @@
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 () => {
Expand Down Expand Up @@ -568,7 +581,7 @@

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
Expand All @@ -581,11 +594,16 @@
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();
Expand All @@ -596,9 +614,16 @@

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,
);
mockClient.getAccountDataFromServer.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();
Expand Down Expand Up @@ -627,11 +652,16 @@

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();
Expand All @@ -643,11 +673,16 @@
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();
Expand Down Expand Up @@ -1195,10 +1230,16 @@

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();

Expand Down Expand Up @@ -1252,7 +1293,7 @@

mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(null);
mockClient.isKeyBackupKeyStored.mockResolvedValue({});
expect(await deviceListener.keyStorageOutOfSyncNeedsBackupReset(true)).toBe(true);

Check failure on line 1296 in apps/web/test/unit-tests/DeviceListener-test.ts

View workflow job for this annotation

GitHub Actions / Jest (Element Web) (2)

DeviceListener › key storage out of sync › needs backup reset › should need resetting if backup key is missing locally and user forgot 4S key 

expect(received).toBe(expected) // Object.is equality

Expected: true
Received: false
    at Object.toBe (test/unit-tests/DeviceListener-test.ts:1296:88)
});

it("should need resetting if backup key is missing locally and in 4s", async () => {
Expand All @@ -1263,7 +1304,7 @@

mockCrypto.getSessionBackupPrivateKey.mockResolvedValue(null);
mockClient.isKeyBackupKeyStored.mockResolvedValue(null);
expect(await deviceListener.keyStorageOutOfSyncNeedsBackupReset(false)).toBe(true);

Check failure on line 1307 in apps/web/test/unit-tests/DeviceListener-test.ts

View workflow job for this annotation

GitHub Actions / Jest (Element Web) (2)

DeviceListener › key storage out of sync › needs backup reset › should need resetting if backup key is missing locally and in 4s 

expect(received).toBe(expected) // Object.is equality

Expected: true
Received: false
    at Object.toBe (test/unit-tests/DeviceListener-test.ts:1307:89)
});
});

Expand Down
7 changes: 7 additions & 0 deletions ...est/unit-tests/components/viewmodels/settings/encryption/KeyStoragePanelViewModel-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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,
});
});
});
Loading