Skip to content
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.

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

Add key storage toggle to Encryption settings #29310

Open
wants to merge 73 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
73 commits
Select commit Hold shift + click to select a range
5a74298
Add key storage toggle to Encryption settings
dbkr Jan 27, 2025
98950de
Keys in the acceptable order
dbkr Jan 27, 2025
4eb07d4
Fix some tests
dbkr Jan 28, 2025
97057de
Fix import
dbkr Jan 28, 2025
4ec09f0
Fix toast showing condition
dbkr Jan 28, 2025
4db196e
Fix import order
dbkr Jan 28, 2025
98114c8
Fix playwright tests
dbkr Jan 28, 2025
6a20703
Merge remote-tracking branch 'origin/develop' into dbkr/key_storage_t…
dbkr Feb 5, 2025
b7b2ea3
Fix bits lost in merge
dbkr Feb 5, 2025
e408715
Add key storage delete confirm screen
dbkr Feb 5, 2025
aa6de76
Fix hardcoded Element string
dbkr Feb 5, 2025
5ac2004
Fix type imports
dbkr Feb 5, 2025
1304587
Fix tests
dbkr Feb 5, 2025
a26efc5
Tests for key storage delete panel
dbkr Feb 5, 2025
1b99071
Fix test
dbkr Feb 5, 2025
6b238d1
Type import
dbkr Feb 5, 2025
87d44a7
Test for the view model
dbkr Feb 6, 2025
4ea6a33
Merge remote-tracking branch 'origin/develop' into dbkr/key_storage_t…
dbkr Feb 6, 2025
df4c23b
Fix type import
dbkr Feb 6, 2025
4a3a373
Actually fix type imports
dbkr Feb 6, 2025
e70afdb
Test updating
dbkr Feb 6, 2025
40f9bd9
Add playwright test & clarify slightly confusing comment
dbkr Feb 6, 2025
16c76cb
Show the advnced section whatever the state of key storage
dbkr Feb 6, 2025
f818d6e
Update screenshots
dbkr Feb 6, 2025
fc9bc09
Copy css to its own file
dbkr Feb 7, 2025
25f8fe2
Add missing doc & merge loading states
dbkr Feb 7, 2025
1178d77
Add tsdoc & loading alt text to spinner
dbkr Feb 7, 2025
7c2d9f4
Turn comments into proper tsdoc
dbkr Feb 7, 2025
cfd55a6
Switch to TypedEventEmitter and remove unnecessary loading state
dbkr Feb 7, 2025
f586c43
Add screenshot
dbkr Feb 7, 2025
e8483e0
Use higher level interface
dbkr Feb 7, 2025
8ca4a8b
Merge the two hooks in EncryptionUserSettingsTab
dbkr Feb 7, 2025
2ef05c5
Remove unused import
dbkr Feb 7, 2025
7149b3d
Don't check key backup enabled state separately
dbkr Feb 7, 2025
64f84cb
Update snapshot
dbkr Feb 7, 2025
0c5f2b0
Merge branch 'develop' into dbkr/key_storage_toggle
dbkr Feb 7, 2025
9c4625d
Use fixed recovery key function
dbkr Feb 7, 2025
f0d9e05
Amalgamate duplicated CSS files
dbkr Feb 10, 2025
de18311
Merge remote-tracking branch 'origin/develop' into dbkr/key_storage_t…
dbkr Feb 10, 2025
c4525b9
Have "key storage disabled" as a separate state
dbkr Feb 10, 2025
d1aef9f
Update snapshot
dbkr Feb 10, 2025
96a70f2
Fix... bad merge?
dbkr Feb 10, 2025
98edffa
Add backup enabled mock to more tests
dbkr Feb 10, 2025
a5cec9e
More snapshots
dbkr Feb 10, 2025
0162c82
Use defer util
dbkr Feb 10, 2025
446fca2
Merge branch 'dbkr/key_storage_toggle' into dbkr/key_storage_toggle_2
dbkr Feb 18, 2025
2c5e81f
Update to use EncryptionCardButtons
dbkr Feb 18, 2025
c0e54d9
Update snapshots
dbkr Feb 18, 2025
8c86402
Merge remote-tracking branch 'origin/develop' into dbkr/key_storage_t…
dbkr Feb 20, 2025
91c0adc
Use EncryptionCardEmphasisedContent
dbkr Feb 20, 2025
d7683e1
Update snapshots
dbkr Feb 20, 2025
2f8fb06
Update snapshot
dbkr Feb 20, 2025
819868c
Try screenshot from CI playwright
dbkr Feb 20, 2025
c83fcfd
Try playwright screenshots again
dbkr Feb 20, 2025
1b8cc7c
More screenshots
dbkr Feb 20, 2025
0108ba6
Rename to match files
dbkr Feb 26, 2025
a015d75
Test that 4S secrets are deleted
dbkr Feb 26, 2025
9d3e419
Make description clearer
dbkr Feb 26, 2025
90b5632
Fix typo & move related states together
dbkr Feb 26, 2025
4eebdb9
Add comment
dbkr Feb 26, 2025
ee925b3
More comments
dbkr Feb 26, 2025
2ff4a7a
Fix hook docs
dbkr Feb 26, 2025
defd1c7
restoreAllMocks
dbkr Feb 26, 2025
10487ea
Merge branch 'develop' into dbkr/key_storage_toggle_2
dbkr Feb 26, 2025
e7f36ed
Update snapshot
dbkr Feb 27, 2025
e6aa2da
Switch icon
dbkr Feb 27, 2025
bf5a22a
Update snapshot
dbkr Feb 27, 2025
fa7ee98
Missing copyright
dbkr Feb 28, 2025
1baaa10
Re-order states
dbkr Feb 28, 2025
b4750ae
Remove phantom space
dbkr Feb 28, 2025
5b0ccf7
Clarify 'button'
dbkr Feb 28, 2025
2c8bff1
Clarify docs more
dbkr Feb 28, 2025
f5d94dc
Explain thinking behind updating
dbkr Feb 28, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ import {
} from "../../crypto/utils";

test.describe("Encryption tab", () => {
test.use({
displayName: "Alice",
});
test.use({ displayName: "Alice" });

let recoveryKey: GeneratedSecretStorageKey;
let expectedBackupVersion: string;
Expand Down Expand Up @@ -111,4 +109,36 @@ test.describe("Encryption tab", () => {
// The user is prompted to reset their identity
await expect(dialog.getByText("Forgot your recovery key? You’ll need to reset your identity.")).toBeVisible();
});

test("should warn before turning off key storage", { tag: "@screenshot" }, async ({ page, app, util }) => {
await verifySession(app, recoveryKey.encodedPrivateKey);
await util.openEncryptionTab();

await page.getByRole("checkbox", { name: "Allow key storage" }).click();

await expect(
page.getByRole("heading", { name: "Are you sure you want to turn off key storage and delete it?" }),
).toBeVisible();

await expect(util.getEncryptionTabContent()).toMatchScreenshot("delete-key-storage-confirm.png");

const deleteRequestPromises = [
page.waitForRequest((req) => req.url().endsWith("/account_data/m.cross_signing.master")),
page.waitForRequest((req) => req.url().endsWith("/account_data/m.cross_signing.self_signing")),
page.waitForRequest((req) => req.url().endsWith("/account_data/m.cross_signing.user_signing")),
page.waitForRequest((req) => req.url().endsWith("/account_data/m.megolm_backup.v1")),
page.waitForRequest((req) => req.url().endsWith("/account_data/m.secret_storage.default_key")),
page.waitForRequest((req) => req.url().includes("/account_data/m.secret_storage.key.")),
];

await page.getByRole("button", { name: "Delete key storage" }).click();

await expect(page.getByRole("checkbox", { name: "Allow key storage" })).not.toBeChecked();

for (const prom of deleteRequestPromises) {
const request = await prom;
expect(request.method()).toBe("PUT");
expect(request.postData()).toBe(JSON.stringify({}));
}
});
});
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions res/css/_components.pcss
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
@import "./components/views/settings/devices/_FilteredDeviceListHeader.pcss";
@import "./components/views/settings/devices/_SecurityRecommendations.pcss";
@import "./components/views/settings/devices/_SelectableDeviceTile.pcss";
@import "./components/views/settings/encryption/_KeyStoragePanel.pcss";
@import "./components/views/settings/shared/_SettingsSubsection.pcss";
@import "./components/views/settings/shared/_SettingsSubsectionHeading.pcss";
@import "./components/views/spaces/_QuickThemeSwitcher.pcss";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright 2025 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

.mx_KeyStoragePanel_toggleRow {
flex-direction: row;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/*
Copyright 2025 New Vector Ltd.

SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
Please see LICENSE files in the repository root for full details.
*/

import { useCallback, useEffect, useState } from "react";
import { logger } from "matrix-js-sdk/src/logger";

import { useMatrixClientContext } from "../../../../contexts/MatrixClientContext";

interface KeyStoragePanelState {
/**
* Whether the app's "key storage" option should show as enabled to the user,
* or 'undefined' if the state is still loading.
*/
Comment on lines +14 to +17
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/element-hq/element-web/pull/29113/files#r1956081855 asked to make this clearer: I've added details but I'd argue that if this is asking for exactly what state it means the keys are in, that detail doesn't belong here: it's a view controller, so its job is to specify how the UI looks, not what that might happen to imply under the hood.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that that's fair. I think in its place it might be nice to add a comment in the checkState code which sets it. At the moment, that would look like:

// We tell the user that backup is enabled if there is a backup present on the server, irrespective of whether
// we are actually using it for backup or recovery.

isEnabled: boolean | undefined;

/**
* A function that can be called to enable or disable key storage.
* @param enable True to turn key storage on or false to turn it off
*/
setEnabled: (enable: boolean) => void;

/**
* True if the state is still loading for the first time
*/
loading: boolean;

/**
* True if the status is in the process of being changed
*/
busy: boolean;
}

export function useKeyStoragePanelViewModel(): KeyStoragePanelState {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function useKeyStoragePanelViewModel(): KeyStoragePanelState {
/** Returns a ViewModel for use in {@link KeyStoragePanel} and {@link DeleteKeyStoragePanel}. */
export function useKeyStoragePanelViewModel(): KeyStoragePanelState {

const [isEnabled, setIsEnabled] = useState<boolean | undefined>(undefined);
const [loading, setLoading] = useState(true);
// Whilst the change is being made, the toggle will reflect the pending value rather than the actual state
const [pendingValue, setPendingValue] = useState<boolean | undefined>(undefined);

const matrixClient = useMatrixClientContext();

const checkStatus = useCallback(async () => {
const crypto = matrixClient.getCrypto();
if (!crypto) {
logger.error("Can't check key backup status: no crypto module available");
return;
}
const info = await crypto.getKeyBackupInfo();
setIsEnabled(Boolean(info?.version));
}, [matrixClient]);

useEffect(() => {
(async () => {
await checkStatus();
setLoading(false);
})();
}, [checkStatus]);

const setEnabled = useCallback(
async (enable: boolean) => {
setPendingValue(enable);
try {
const crypto = matrixClient.getCrypto();
if (!crypto) {
logger.error("Can't change key backup status: no crypto module available");
return;
}
if (enable) {
const currentKeyBackup = await crypto.checkKeyBackupAndEnable();
if (currentKeyBackup === null) {
await crypto.resetKeyBackup();
}
Comment on lines +73 to +75
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you explain why this is necessary, and why only when currentKeyBackup is null?


// resetKeyBackup fires this off in the background without waiting, so we need to do it
// explicitly and wait for it, otherwise it won't be enabled yet when we check again.
await crypto.checkKeyBackupAndEnable();

// Set the flag so that EX no longer thinks the user wants backup disabled
await matrixClient.setAccountData("m.org.matrix.custom.backup_disabled", { disabled: false });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we get a symbolic constant somewhere for m.org.matrix.custom.backup_disabled, please?

} else {
// Get the key backup version we're using
const info = await crypto.getKeyBackupInfo();
if (!info?.version) {
logger.error("Can't delete key backup version: no version available");
return;
}

// Bye bye backup
await crypto.deleteKeyBackupVersion(info.version);

// also turn off 4S, since this is also storing keys on the server.
// Delete the cross signing keys from secret storage
await matrixClient.deleteAccountData("m.cross_signing.master");
await matrixClient.deleteAccountData("m.cross_signing.self_signing");
await matrixClient.deleteAccountData("m.cross_signing.user_signing");
// and the key backup key (we just turned it off anyway)
await matrixClient.deleteAccountData("m.megolm_backup.v1");

// Delete the key information
const defaultKey = await matrixClient.secretStorage.getDefaultKeyId();
if (defaultKey) {
await matrixClient.deleteAccountData(`m.secret_storage.key.${defaultKey}`);

// ...and the default key pointer
await matrixClient.deleteAccountData("m.secret_storage.default_key");
}

// finally, 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("m.org.matrix.custom.backup_disabled", { disabled: true });
Comment on lines +94 to +114
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't we talk about disabling dehydration, and making this all a method in the js-sdk?

}

await checkStatus();
} finally {
setPendingValue(undefined);
}
},
[setPendingValue, checkStatus, matrixClient],
);

return { isEnabled: pendingValue ?? isEnabled, setEnabled, loading, busy: pendingValue !== undefined };
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright 2025 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

import { Breadcrumb, Button, VisualList, VisualListItem } from "@vector-im/compound-web";
import CrossIcon from "@vector-im/compound-design-tokens/assets/web/icons/close";
import ErrorIcon from "@vector-im/compound-design-tokens/assets/web/icons/error-solid";
import React, { useCallback, useState } from "react";

import { _t } from "../../../../languageHandler";
import { EncryptionCard } from "./EncryptionCard";
import { useKeyStoragePanelViewModel } from "../../../viewmodels/settings/encryption/KeyStoragePanelViewModel";
import SdkConfig from "../../../../SdkConfig";
import { EncryptionCardButtons } from "./EncryptionCardButtons";
import { EncryptionCardEmphasisedContent } from "./EncryptionCardEmphasisedContent";

interface Props {
/**
* Called when the user either cancels the operation or key storage has been disabled
*/
onFinish: () => void;
}

/**
* Confirms that the user really wants to turn off and delete their key storage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Confirms that the user really wants to turn off and delete their key storage
* Confirms that the user really wants to turn off and delete their key storage. Part of the "Encryption" settings tab.

*/
export function DeleteKeyStoragePanel({ onFinish }: Props): JSX.Element {
const { setEnabled } = useKeyStoragePanelViewModel();
const [busy, setBusy] = useState(false);

const onDeleteClick = useCallback(async () => {
setBusy(true);
try {
await setEnabled(false);
} finally {
setBusy(false);
}
onFinish();
}, [setEnabled, onFinish]);

return (
<>
<Breadcrumb
backLabel={_t("action|back")}
onBackClick={onFinish}
pages={[_t("settings|encryption|title"), _t("settings|encryption|delete_key_storage|breadcrumb_page")]}
onPageClick={onFinish}
/>
<EncryptionCard
Icon={ErrorIcon}
destructive={true}
title={_t("settings|encryption|delete_key_storage|title")}
>
<EncryptionCardEmphasisedContent>
{_t("settings|encryption|delete_key_storage|description")}
<VisualList>
<VisualListItem Icon={CrossIcon} destructive={true}>
{_t("settings|encryption|delete_key_storage|list_first")}
</VisualListItem>
<VisualListItem Icon={CrossIcon} destructive={true}>
{_t("settings|encryption|delete_key_storage|list_second", { brand: SdkConfig.get().brand })}
</VisualListItem>
</VisualList>
</EncryptionCardEmphasisedContent>
<EncryptionCardButtons>
<Button destructive={true} onClick={onDeleteClick} disabled={busy}>
{_t("settings|encryption|delete_key_storage|confirm")}
</Button>
<Button kind="tertiary" onClick={onFinish}>
{_t("action|cancel")}
</Button>
</EncryptionCardButtons>
</EncryptionCard>
</>
);
}
73 changes: 73 additions & 0 deletions src/components/views/settings/encryption/KeyStoragePanel.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright 2025 New Vector Ltd.
*
* SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only OR LicenseRef-Element-Commercial
* Please see LICENSE files in the repository root for full details.
*/

import React, { useCallback } from "react";
import { InlineField, InlineSpinner, Label, Root, ToggleControl } from "@vector-im/compound-web";

import type { FormEvent } from "react";
import { SettingsSection } from "../shared/SettingsSection";
import { _t } from "../../../../languageHandler";
import { SettingsHeader } from "../SettingsHeader";
import { useKeyStoragePanelViewModel } from "../../../viewmodels/settings/encryption/KeyStoragePanelViewModel";

interface Props {
/**
* Called when the user turns off the "allow key storage" toggle
*/
onKeyStorageDisableClick: () => void;
}

/**
* This component allows the user to set up or change their recovery key.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* This component allows the user to set up or change their recovery key.
* This component allows the user to set up or change their recovery key.
*
* It is used within the "Encryption" settings tab.

*/
export const KeyStoragePanel: React.FC<Props> = ({ onKeyStorageDisableClick }) => {
const { isEnabled, setEnabled, loading, busy } = useKeyStoragePanelViewModel();

const onKeyBackupChange = useCallback(
(e: FormEvent<HTMLInputElement>) => {
if (e.currentTarget.checked) {
setEnabled(true);
} else {
onKeyStorageDisableClick();
}
},
[setEnabled, onKeyStorageDisableClick],
);

if (loading) {
return <InlineSpinner aria-label={_t("common|loading")} />;
}

return (
<SettingsSection
legacy={false}
heading={
<SettingsHeader
hasRecommendedTag={isEnabled === false}
label={_t("settings|encryption|key_storage|title")}
/>
}
subHeading={_t("settings|encryption|key_storage|description", undefined, {
a: (sub) => (
<a href="https://element.io/help#encryption5" target="_blank" rel="noreferrer noopener">
{sub}
</a>
),
})}
>
<Root className="mx_KeyStoragePanel_toggleRow">
<InlineField
name="keyStorage"
control={<ToggleControl name="keyStorage" checked={isEnabled} onChange={onKeyBackupChange} />}
>
<Label>{_t("settings|encryption|key_storage|allow_key_storage")}</Label>
</InlineField>
{busy && <InlineSpinner />}
</Root>
</SettingsSection>
);
};
Loading
Loading