replace prepareKeyBackupVersion by resetKeyBackup #3662

BillCarsonFr · 2023-08-16T14:36:33Z

Fixes https://github.com/vector-im/crypto-internal/issues/107

Checklist

Tests written for new code (and old code if feasible)
Linter and other CI checks pass
Sign-off given on the changes (see CONTRIBUTING.md)

This change is marked as an internal change (Task), so will not be included in the changelog.

src/crypto-api.ts

src/crypto/index.ts

src/rust-crypto/backup.ts

src/client.ts

src/crypto-api.ts

src/crypto/backup.ts

src/rust-crypto/backup.ts

src/rust-crypto/rust-crypto.ts

src/rust-crypto/backup.ts

richvdh · 2023-08-22T14:08:35Z

src/rust-crypto/rust-crypto.ts

+            if (setupNewKeyBackup) {
+                await this.resetKeyBackup();
+            }


this seems to be adding new functionality, which doesn't really fit with the title of this PR (which implies we are just replacing the existing implementation). Maybe we should separate the new rust impl from the changes to the existing code? It's hard to review at the moment as it feels like we're doing two quite different things.

I think that's the main point of the PR? to have boostraping complete now and creating a complete backup?

I think that's the main point of the PR? to have boostraping complete now and creating a complete backup?

Yeah, that's probably fair. But then we should rename the PR to reflect what it's actually doing.

The problem is, I'm struggling to come up with a short summary of the PR. "Add new CryptoApi.resetKeyBackup and CryptoApi.deleteKeyBackupVersion APIs, and enable backups in RustCrypto.bootstrapSecretStorage". What a mouthful!

All of which is to say... I feel like this PR is doing lots of things, and we should probably have broken it down. Which may be one of the reasons it has had 119 comments so far, and multiple rounds of review.

It may be too late now, but I think we can learn from this in the future.

src/client.ts

src/crypto-api.ts

spec/integ/crypto/crypto.spec.ts

richvdh · 2023-08-30T14:31:10Z

spec/integ/crypto/crypto.spec.ts

+            expect(activeBackup).toStrictEqual(backupVersion);
+        });
+
+        it("Reset key backup should create a new backup and update 4S", async () => {


seems like this isn't part of the tests for bootstrapSecretStorage so should be moved outside the describe block?

refactored.

refactored.

I don't see any changes here. It's still inside the describe("bootstrapSecretStorage" ... ) block.

richvdh · 2023-08-30T14:31:27Z

spec/integ/crypto/crypto.spec.ts

+        });
+
+        it("Reset key backup should create a new backup and update 4S", async () => {
+            // First setup recovery


Suggested change

// First setup recovery

// First set up recovery

also, what is recovery?

Recovery is 4S (cross signing and megolm backup)

Recovery is 4S (cross signing and megolm backup)

Please could the comment say that then?

spec/integ/crypto/crypto.spec.ts

richvdh · 2023-08-31T13:55:53Z

src/crypto/index.ts

+    /**
+     * Deletes the given backup.
+     *
+     * @param version - The version to delete.
+     */
+    public async deleteKeyBackupVersion(version: string): Promise<void> {
+        await this.backupManager.deleteKeyBackupVersion(version);
+    }
+


this still needs to be moved, please.

richvdh · 2023-08-31T15:17:12Z

src/crypto-api.ts

+     * The decryption key will be saved in Secret Storage (the `SecretStorageCallbacks.getSecretStorageKey` Crypto
+     * callback will be called)
+     * and the backup engine will be started.


Suggested change

* The decryption key will be saved in Secret Storage (the `SecretStorageCallbacks.getSecretStorageKey` Crypto

* callback will be called)

* and the backup engine will be started.

* The decryption key will be saved in Secret Storage (the {@link SecretStorageCallbacks#getSecretStorageKey}

* callback will be called) and the backup engine will be started.

richvdh · 2023-08-31T15:18:43Z

spec/test-utils/mockEndpoints.ts

@@ -56,3 +58,35 @@ export function mockSetupCrossSigningRequests(): void {
        {},
    );
 }
+
+/**
+ * Mock out requests to  `/room_keys/version`.


Suggested change

* Mock out requests to `/room_keys/version`.

* Mock out requests to `/room_keys/version`.

richvdh · 2023-08-31T15:19:31Z

src/rust-crypto/backup.ts

+
+/**
+ * Holds information of a created keybackup.
+ * Useful to get the generated private key material and save it securely somewhere.


Suggested change

* Useful to get the generated private key material and save it securely somewhere.

*

* Useful to get the generated private key material and save it securely somewhere.

richvdh · 2023-08-31T15:21:34Z

src/crypto/index.ts

    public async resetKeyBackup(): Promise<void> {
        // Delete existing ones
-        await this.backupManager.deleteKeyBackup();
+        // There is no use case for having several key backup version live server side.


Suggested change

// There is no use case for having several key backup version live server side.

// There is no use case for having several key backup versions live server side.

richvdh · 2023-08-31T15:38:14Z

src/rust-crypto/backup.ts

+    /**
+     * Prepare the keybackup version data, auth_data not signed at this point
+     * @returns a {@link PreparedKeyBackupVersion} with all information about the creation.
+     */
+    private async prepareKeyBackupVersion(): Promise<PreparedKeyBackupVersion> {


(and same naming that legacy code)

this is not necessarily a good thing.

I think my problem is that "prepare" is very vague. We can keep the name of the method if you like but please expand the tsdoc to explain more clearly what it does.

richvdh · 2023-08-31T15:39:49Z

spec/test-utils/mockEndpoints.ts

+ * Returns 404 `M_NOT_FOUND` for GET `/room_keys/version` request until POST `room_keys/version` is called.
+ * Once the POST is done GET `/room_keys/version` will return the posted backup instead of 404
+ *
+ * @param backupVersion The version of the backup to create


done

Not as far as I can see?

richvdh · 2023-08-31T15:39:58Z

spec/test-utils/mockEndpoints.ts

+ * Once the POST is done, `GET /room_keys/version` will return the posted backup
+ * instead of 404.
+ *
+ * @param backupVersion - The version of the backup to create


this still needs an update, per #3662 (comment)

richvdh · 2023-08-31T15:42:17Z

spec/integ/crypto/crypto.spec.ts

+            // Return the newly created key in the sync response
+            sendSyncResponse(secretStorageKey);
+
+            // Wait for the cross signing keys to be uploaded


Suggested change

// Wait for the cross signing keys to be uploaded

// Wait for the cross signing keys and backup key to be uploaded

richvdh · 2023-08-31T15:44:51Z

spec/integ/crypto/crypto.spec.ts

+            expect(activeBackup).toStrictEqual(backupVersion);
+        });
+
+        it("Reset key backup should create a new backup and update 4S", async () => {


refactored.

I don't see any changes here. It's still inside the describe("bootstrapSecretStorage" ... ) block.

BillCarsonFr · 2023-09-01T15:40:26Z

Closed. Devided in 2 PRs:

Deprecate MatrixClient.{prepare,create}KeyBackupVersion in favour of new CryptoApi.resetKeyBackup API #3689
Implement key backup APIs for rust and create backup in bootstrapSecretStorage #3690

replace prepareKeyBackupVersion by resetKeyBackup

46abd56

BillCarsonFr added the T-Task Tasks for the team like planning label Aug 16, 2023

Fix missing implementation

21b8f0f

github-actions bot deployed to PR Documentation Preview August 16, 2023 15:25 View deployment

BillCarsonFr added 3 commits August 17, 2023 16:27

Merge branch 'develop' into valere/element-r/keybackup_reset

f8d3a4f

add test for backup setup

49eaf4b

more test and fix missing await

4582402

github-actions bot deployed to PR Documentation Preview August 18, 2023 13:16 View deployment

BillCarsonFr marked this pull request as ready for review August 18, 2023 14:40

BillCarsonFr requested a review from a team as a code owner August 18, 2023 14:40

BillCarsonFr requested review from t3chguy, kerryarchibald and richvdh and removed request for kerryarchibald August 18, 2023 14:40

t3chguy requested changes Aug 18, 2023

View reviewed changes

BillCarsonFr added 3 commits August 21, 2023 17:51

cleaning and doc

2c6037c

Merge branch 'develop' into valere/element-r/keybackup_reset

1d99a99

fix lint

c95e90a

github-actions bot deployed to PR Documentation Preview August 21, 2023 16:00 View deployment

richvdh reviewed Aug 22, 2023

View reviewed changes

src/rust-crypto/backup.ts Outdated Show resolved Hide resolved

src/rust-crypto/backup.ts Outdated Show resolved Hide resolved

richvdh requested changes Aug 22, 2023

View reviewed changes

Merge branch 'develop' into valere/element-r/keybackup_reset

f6a8467

github-actions bot deployed to PR Documentation Preview August 28, 2023 15:41 View deployment

added some comments

985b5e0

github-actions bot deployed to PR Documentation Preview August 29, 2023 09:42 View deployment

BillCarsonFr added 5 commits August 29, 2023 13:45

Remove unneeded crypto methods to MatrixClient

6c6c0df

fix bad call to disableKeyBackup

511faa0

missing ts-doc

8f28611

ts-doc

319985c

remove a todo

80fce56

Merge branch 'develop' into valere/element-r/keybackup_reset

5a75462

github-actions bot deployed to PR Documentation Preview August 29, 2023 12:32 View deployment

BillCarsonFr requested review from richvdh and t3chguy August 29, 2023 12:35

richvdh requested changes Aug 30, 2023

View reviewed changes

BillCarsonFr and others added 2 commits August 30, 2023 17:20

Merge branch 'develop' into valere/element-r/keybackup_reset

50da327

code review

646de15

github-actions bot deployed to PR Documentation Preview August 30, 2023 16:23 View deployment

BillCarsonFr requested a review from richvdh August 31, 2023 12:29

missing ts-doc

05f80da

github-actions bot deployed to PR Documentation Preview August 31, 2023 13:55 View deployment

richvdh reviewed Aug 31, 2023

View reviewed changes

BillCarsonFr closed this Sep 1, 2023

richvdh mentioned this pull request Sep 4, 2023

Implement key backup APIs for rust and create backup in bootstrapSecretStorage #3690

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace prepareKeyBackupVersion by resetKeyBackup #3662

replace prepareKeyBackupVersion by resetKeyBackup #3662

BillCarsonFr commented Aug 16, 2023 •

edited by github-actions bot

Loading

richvdh Aug 22, 2023

BillCarsonFr Aug 29, 2023

richvdh Aug 31, 2023

richvdh Aug 30, 2023

BillCarsonFr Aug 30, 2023

richvdh Aug 31, 2023

richvdh Aug 30, 2023

richvdh Aug 30, 2023

BillCarsonFr Aug 30, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

richvdh Aug 31, 2023

BillCarsonFr commented Sep 1, 2023

	* Mock out requests to `/room_keys/version`.
	* Mock out requests to `/room_keys/version`.

	* Useful to get the generated private key material and save it securely somewhere.
	*
	* Useful to get the generated private key material and save it securely somewhere.

	// There is no use case for having several key backup version live server side.
	// There is no use case for having several key backup versions live server side.

	// Wait for the cross signing keys to be uploaded
	// Wait for the cross signing keys and backup key to be uploaded

replace prepareKeyBackupVersion by resetKeyBackup #3662

replace prepareKeyBackupVersion by resetKeyBackup #3662

Conversation

BillCarsonFr commented Aug 16, 2023 • edited by github-actions bot Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr commented Sep 1, 2023

BillCarsonFr commented Aug 16, 2023 •

edited by github-actions bot

Loading