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.

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

Jump to bottom

Add Recovery section in the new user settings Encryption tab #28673

Open
wants to merge 42 commits into
base: develop
Choose a base branch
from
Open

Add Recovery section in the new user settings Encryption tab #28673

wants to merge 42 commits into from

Conversation

florianduros
Copy link
Member

@florianduros florianduros commented Dec 6, 2024 •
edited
Loading

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

First of all, I'm sorry for the size of the PR.

Task #26468
Figma design
Add a new encryption tab with a new recovery section.

Implemented

  • Add the crypto team as code owner of this new encryption tab

The encryption tab has the following behaviour:

  • If the encryption is not setup, we display a verify button to launch the Set up encryption dialog. Only this button and his description is displayed. The user must verify his device or set up the encryption before playing with the encryption settings.
  • Otherwise, display the Recovery section (the Key storage and Advanced section in the future)
  • If the user want to set up or change the recovery key when clicking on the corresponding button of the recovery section, the content of the encryption tab is replaced by a breadcrumb to do it.

The recovery section has the following behaviour:

  • If the recovery key is not setup, we ask to setup a recovery key
  • If the secrets (cross-signing keys) are not cached locally, we ask to enter the recovery key
  • Otherwise, display the "change recovery key" button

Not implemented

This PR DON'T (because the PR is already massive and theses tasks should be done in other PRs):

  • Implement the "Key storage is out of sync" toast
  • Implement the new "Enter your recovery key" dialog and the "Forgot your recovery key? You’ll need to reset your identity." dialog
  • Add dot to the User settings tab

@florianduros florianduros force-pushed the florianduros/encryption-tab branch 3 times, most recently from 7ee9a42 to a90f51f Compare December 6, 2024 11:06
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from a90f51f to b3dc812 Compare December 6, 2024 14:19
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from b3dc812 to 98d86e0 Compare December 6, 2024 16:21
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from 98d86e0 to 2710462 Compare December 10, 2024 15:02
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from 2710462 to 5e31204 Compare December 11, 2024 14:30
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from 5e31204 to 54ede8b Compare December 11, 2024 14:49
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from 54ede8b to 1811aa6 Compare December 11, 2024 17:22
@florianduros florianduros force-pushed the florianduros/encryption-tab branch 5 times, most recently from 97b9f95 to 9aecfcc Compare December 12, 2024 09:16
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from 38de793 to 380ef62 Compare January 9, 2025 14:37
@florianduros
Use matrixClient.secretStorage.getDefaultKeyId instead of `matrixCl…
8bd5d6a 
…ient.getCrypto().checkKeyBackupAndEnable` to know if we need to set up a recovery key
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from 380ef62 to 8bd5d6a Compare January 9, 2025 14:51
@florianduros
Merge branch 'develop' into florianduros/encryption-tab
086f28e 
# Conflicts:
#	playwright/snapshots/settings/account-user-settings-tab.spec.ts/account-linux.png
#	playwright/snapshots/settings/account-user-settings-tab.spec.ts/account-smallscreen-linux.png
@florianduros
Update existing screenshot to add encryption tab.
0c18708
@florianduros florianduros requested a review from richvdh January 9, 2025 17:06
@florianduros florianduros force-pushed the florianduros/encryption-tab branch from cbf06fd to e5dea48 Compare January 10, 2025 15:24
richvdh
richvdh reviewed Jan 13, 2025
View reviewed changes
src/components/views/settings/encryption/ChangeRecoveryKey.tsx Outdated Show resolved Hide resolved
playwright/e2e/settings/encryption-user-tab/recovery.spec.ts Outdated Show resolved Hide resolved
src/components/views/settings/encryption/RecoveryPanel.tsx Outdated Show resolved Hide resolved
src/components/views/settings/encryption/RecoveryPanel.tsx Outdated Show resolved Hide resolved
src/components/views/settings/encryption/RecoveryPanel.tsx Outdated Show resolved Hide resolved
src/components/views/settings/encryption/RecoveryPanel.tsx Outdated Show resolved Hide resolved
src/components/views/settings/encryption/ChangeRecoveryKey.tsx Outdated Show resolved Hide resolved
src/components/views/settings/tabs/user/EncryptionUserSettingsTab.tsx Outdated Show resolved Hide resolved
playwright/e2e/settings/encryption-user-tab/index.ts Outdated Show resolved Hide resolved
src/components/views/settings/encryption/ChangeRecoveryKey.tsx Outdated
Comment on lines 33 to 36
* - `save_key_setup_flow`: The user is asked to save the new recovery key during the setup flow.
* - `save_key_change_flow`: The user is asked to save the new recovery key during the change key flow.
* - `confirm`: The user is asked to confirm the new recovery key.
* - `confirm_key_setup_flow`: The user is asked to confirm the new recovery key during the set up flow.
* - `confirm_key_change_flow`: The user is asked to confirm the new recovery key during the change key flow.
Copy link
Member

Choose a reason for hiding this comment

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

I have to say, I wonder if it wouldn't be simpler to just have three states (inform_user, save_key, confirm_key) and distinguish between the two flows by looking at the userHasRecoveryKey prop.

On the other hand, we should probably just get on and land this rather than messing with it forever. A thought for the future though.

@florianduros florianduros force-pushed the florianduros/encryption-tab branch from cee03f0 to bef3165 Compare January 13, 2025 14:21
@florianduros florianduros requested a review from richvdh January 13, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t3chguy t3chguy t3chguy left review comments

@dbkr dbkr dbkr approved these changes

@richvdh richvdh Awaiting requested review from richvdh

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
T-Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@florianduros @dbkr @richvdh @t3chguy