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 CryptoApi.resetEncryption #4614

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

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Jan 14, 2025

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.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Task element-hq/element-web#28977
Require #4615
Add CryptoApi.resetEncryption.

The goal of this new api is to give an easy to reset the current encryption state of the user:

  • Disable backing up room keys and delete any existing backup.
  • Remove the default secret storage key from the account data (ie: the recovery key).
  • Reset the cross-signing keys.
  • Re-enable backing up room keys if enabled before.

@florianduros florianduros changed the title Add CryptoApi#resetEncryption Add CryptoApi.resetEncryption Jan 14, 2025
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from f99e069 to 4029eee Compare January 14, 2025 09:35
@florianduros florianduros added T-Feature Request to add a new feature which does not exist right now T-Enhancement and removed T-Feature Request to add a new feature which does not exist right now labels Jan 14, 2025
@florianduros florianduros force-pushed the florianduros/reset-encryption branch 2 times, most recently from 18fb6e6 to b033611 Compare January 15, 2025 14:04
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from b033611 to c1462b7 Compare January 15, 2025 14:36
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from c1462b7 to dbb5c0f Compare January 15, 2025 14:45
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from dbb5c0f to 0420edf Compare January 15, 2025 14:47
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from 0420edf to 6ea0cd1 Compare January 15, 2025 14:48
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from 6ea0cd1 to dbd54dc Compare January 15, 2025 14:54
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from dbd54dc to 9f87c04 Compare January 15, 2025 15:15
@florianduros florianduros force-pushed the florianduros/reset-encryption branch from 9f87c04 to be9f401 Compare January 15, 2025 15:23
Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

Looks like it does what it says it does!

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

generally looks good, a few nits, mostly around the tests

src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
Comment on lines +1919 to +1927
// We consider the key backup as trusted
jest.spyOn(RustBackupManager.prototype, "isKeyBackupTrusted").mockResolvedValue({
trusted: true,
matchesDecryptionKey: true,
});

const rustCrypto = await makeTestRustCrypto(makeMatrixHttpApi(), undefined, undefined, secretStorage);
// We have a key backup
expect(await rustCrypto.getActiveSessionBackupVersion()).not.toBeNull();
Copy link
Member

Choose a reason for hiding this comment

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

it might be good to collect all the fetchMock calls together, rather than having this in the middle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't get it, sorry. rustCrypto.getActiveSessionBackupVersion() is not a fetchMock

spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
spec/unit/rust-crypto/rust-crypto.spec.ts Outdated Show resolved Hide resolved
Comment on lines +1919 to +1923
// We consider the key backup as trusted
jest.spyOn(RustBackupManager.prototype, "isKeyBackupTrusted").mockResolvedValue({
trusted: true,
matchesDecryptionKey: true,
});
Copy link
Member

Choose a reason for hiding this comment

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

is this used before or after the reset?

Can we not use the actual backup data in the response to /_matrix/client/v3/room_keys/version so we don't need to mock this?

Copy link
Contributor Author

@florianduros florianduros Jan 20, 2025

Choose a reason for hiding this comment

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

It's used before the reset in order to have a trusted backup.

I suppose there is a better way to have a trusted backup returned by /_matrix/client/v3/room_keys/version?
The backup returned by /_matrix/client/v3/room_keys/version is not trusted in the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants