Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 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
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 @@ -111,4 +111,21 @@ 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();
Comment thread
dbkr marked this conversation as resolved.

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

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

await expect(page.getByRole("checkbox", { name: "Allow key storage" })).not.toBeChecked();
});
Comment on lines +115 to +130
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it would be good if we confirmed that the switch actually worked, rather than just exposing some nice UI?

});
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.
3 changes: 2 additions & 1 deletion 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 Expand Up @@ -357,9 +358,9 @@
@import "./views/settings/_UserProfileSettings.pcss";
@import "./views/settings/encryption/_AdvancedPanel.pcss";
@import "./views/settings/encryption/_ChangeRecoveryKey.pcss";
@import "./views/settings/encryption/_DestructiveComponent.pcss";
@import "./views/settings/encryption/_EncryptionCard.pcss";
@import "./views/settings/encryption/_RecoveryPanelOutOfSync.pcss";
@import "./views/settings/encryption/_ResetIdentityPanel.pcss";
@import "./views/settings/tabs/_SettingsBanner.pcss";
@import "./views/settings/tabs/_SettingsIndent.pcss";
@import "./views/settings/tabs/_SettingsSection.pcss";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.mx_KeyBackupPanel_toggleRow {
flex-direction: row;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't have a strong opinion on this, but having a .pcss file which doesn't correspond to a specific component seems like a deviation from our normal way of doing things.

Wouldn't the normal way be to have a DestructiveEncryptionCard or something?

Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@
* Please see LICENSE files in the repository root for full details.
*/

.mx_ResetIdentityPanel {
.mx_ResetIdentityPanel_content {
/**
* Shared by multiple components that confirm a destructive action in the user settings dialog.
*/
.mx_DestructiveComponent {
.mx_DestructiveComponent_content {
display: flex;
flex-direction: column;
gap: var(--cpd-space-3x);
Expand All @@ -17,7 +20,7 @@
}
}

.mx_ResetIdentityPanel_footer {
.mx_DestructiveComponent_footer {
display: flex;
flex-direction: column;
gap: var(--cpd-space-4x);
Expand Down
7 changes: 5 additions & 2 deletions src/DeviceListener.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,11 @@ export default class DeviceListener {
// prompt the user to enter their recovery key.
showSetupEncryptionToast(SetupKind.KEY_STORAGE_OUT_OF_SYNC);
} else if (defaultKeyId === null) {
// the user just hasn't set up 4S yet: prompt them to do so
showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY);
// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to backups)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to backups)
// the user just hasn't set up 4S yet: prompt them to do so (unless they've explicitly said no to key storage)

const disabledEvent = cli.getAccountData("m.org.matrix.custom.backup_disabled");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could we add a constant somewhere for this name, so that we don't need to hardcode it in 15 different places?

if (!disabledEvent || !disabledEvent.getContent()?.disabled) {
showSetupEncryptionToast(SetupKind.SET_UP_RECOVERY);
}
} else {
// some other condition... yikes! Show the 'set up encryption' toast: this is what we previously did
// in 'other' situations. Possibly we should consider prompting for a full reset in this case?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
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 key storage is enabled, or 'undefined' if the state is still loading.
*/
isEnabled: boolean | undefined;
Comment thread
dbkr marked this conversation as resolved.
Comment on lines +14 to +17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

again, I'd like us to be clearer about what "enabled" means here. Naively, I assumed it meant m.org.matrix.custom.backup_disabled, but that doesn't seem to be the case.


/**
* 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 {
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();
}

// 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 });
} 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");
}
Comment on lines +93 to +108
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we need to disable dehydration and delete the dehydrated device key here too.

This whole thing could usefully become crypto.deleteSecretStorage.


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

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,78 @@
/*
* 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";
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";

interface Props {
/**
* Called when the user either cancels the operation or key storage has been disabled
*/
onFinish: () => void;
Comment thread
dbkr marked this conversation as resolved.
}

/**
* Confirms that the user really wants to turn off and delete their key storage
*/
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")}
className="mx_DestructiveComponent"
>
<div className="mx_DestructiveComponent_content">
{_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>
</div>
<div className="mx_DestructiveComponent_footer">
<Button destructive={true} onClick={onDeleteClick} disabled={busy}>
{_t("settings|encryption|delete_key_storage|confirm")}
</Button>
<Button kind="tertiary" onClick={onFinish}>
{_t("action|cancel")}
</Button>
</div>
</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;
Comment thread
dbkr marked this conversation as resolved.
}

/**
* This component allows the user to set up or change their recovery key.
*/
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_KeyBackupPanel_toggleRow">
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't the class here be about KeyStoragePanel rather than KeyBackupPanel ?

<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>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,9 @@ export function ResetIdentityPanel({ onCancelClick, onFinish, variant }: ResetId
? _t("settings|encryption|advanced|breadcrumb_title_forgot")
: _t("settings|encryption|advanced|breadcrumb_title")
}
className="mx_ResetIdentityPanel"
className="mx_DestructiveComponent"
>
<div className="mx_ResetIdentityPanel_content">
<div className="mx_DestructiveComponent_content">
<VisualList>
<VisualListItem Icon={CheckIcon} success={true}>
{_t("settings|encryption|advanced|breadcrumb_first_description")}
Expand All @@ -74,7 +74,7 @@ export function ResetIdentityPanel({ onCancelClick, onFinish, variant }: ResetId
</VisualList>
{variant === "compromised" && <span>{_t("settings|encryption|advanced|breadcrumb_warning")}</span>}
</div>
<div className="mx_ResetIdentityPanel_footer">
<div className="mx_DestructiveComponent_footer">
<Button
destructive={true}
onClick={async (evt) => {
Expand Down
Loading