Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Set up key backup using non-deprecated APIs (2nd take) #12098

Merged
merged 24 commits into from
Jan 10, 2024

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jan 2, 2024

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)

Fixes element-hq/element-web#26323

Put back the changes that were on #12005 without the regression that causes unwanted backup resets


Here's what your changelog entry will look like:

🐛 Bug Fixes

@BillCarsonFr BillCarsonFr added T-Task Refactoring, enabling or disabling functionality, other engineering tasks T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems labels Jan 2, 2024
@BillCarsonFr BillCarsonFr requested a review from a team as a code owner January 2, 2024 19:29
@BillCarsonFr BillCarsonFr changed the base branch from develop to valere/better_e2e_playwrigth_test January 2, 2024 19:30
@BillCarsonFr BillCarsonFr marked this pull request as draft January 2, 2024 19:33
@BillCarsonFr BillCarsonFr changed the base branch from valere/better_e2e_playwrigth_test to develop January 4, 2024 16:34
…log.tsx

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
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.

a few more suggestions for clarity, but lgtm otherwise

const secretStorageAlreadySetup = await cli.hasSecretStorageKey();

if (!secretStorageAlreadySetup) {
// bootstrap secret storage, that will also create a backup version
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
// bootstrap secret storage, that will also create a backup version
// bootstrap secret storage; that will also create a backup version

if (!secretStorageAlreadySetup) {
// bootstrap secret storage, that will also create a backup version
await accessSecretStorage(async (): Promise<void> => {
// do nothing, all is now setup correctly
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
// do nothing, all is now setup correctly
// do nothing, all is now set up correctly

Comment on lines 88 to 89
// Secret storage exists, we need to ensure that we can write to it before
// we create a new backup version. It ensures that we can write to it and keep it in sync.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, only just seeing this now. It rather duplicates the comment below.

Suggested change
// Secret storage exists, we need to ensure that we can write to it before
// we create a new backup version. It ensures that we can write to it and keep it in sync.

@@ -116,6 +117,10 @@ export function createTestClient(): MatrixClient {
bootstrapCrossSigning: jest.fn(),
hasSecretStorageKey: jest.fn(),

secretStorage: {
get: jest.fn(),
} as unknown as ServerSideSecretStorage,
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised this typecast is necessary: it doesn't seem to be for any of the other properties. Can we get rid of it?

Suggested change
} as unknown as ServerSideSecretStorage,
},

Copy link
Member

Choose a reason for hiding this comment

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

(also remove the import of ServerSideSecretStorage)

await currentDialogLocator.getByRole("button", { name: "Copy", exact: true }).click();

// copy the recovery key to use it later
securityKey = await app.getClipboard();
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
securityKey = await app.getClipboard();
const securityKey = await app.getClipboard();


let securityKey = "";

const currentDialogLocator = page.locator(".mx_Dialog");
Copy link
Member

@richvdh richvdh Jan 9, 2024

Choose a reason for hiding this comment

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

suggest moving this (const currentDialogLocator = page.locator(".mx_Dialog");) down to where it is used.

Comment on lines 35 to 44
// Create a backup
const tab = await app.settings.openUserSettings("Security & Privacy");

await expect(tab.getByRole("heading", { name: "Secure Backup" })).toBeVisible();

let securityKey = "";

const currentDialogLocator = page.locator(".mx_Dialog");

await tab.getByRole("button", { name: "Set up", exact: true }).click();
Copy link
Member

Choose a reason for hiding this comment

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

Lots of whitespace here, and I don't think it helps readability.

Suggested change
// Create a backup
const tab = await app.settings.openUserSettings("Security & Privacy");
await expect(tab.getByRole("heading", { name: "Secure Backup" })).toBeVisible();
let securityKey = "";
const currentDialogLocator = page.locator(".mx_Dialog");
await tab.getByRole("button", { name: "Set up", exact: true }).click();
// Create a backup
const tab = await app.settings.openUserSettings("Security & Privacy");
await expect(tab.getByRole("heading", { name: "Secure Backup" })).toBeVisible();
await tab.getByRole("button", { name: "Set up", exact: true }).click();


await tab.getByRole("button", { name: "Set up", exact: true }).click();

// It's the first time and secure storage not setup, so it will create one
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
// It's the first time and secure storage not setup, so it will create one
// It's the first time and secure storage is not set up, so it will create one

Comment on lines 72 to 77
await tab.getByRole("button", { name: "Delete Backup", exact: true }).click();

await expect(currentDialogLocator.getByRole("heading", { name: "Delete Backup" })).toBeVisible();

// Delete it
await currentDialogLocator.getByTestId("dialog-primary-button").click(); // Click "Delete Backup"
Copy link
Member

@richvdh richvdh Jan 9, 2024

Choose a reason for hiding this comment

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

Again, too much whitespace. And the comment is in the wrong place.

Suggested change
await tab.getByRole("button", { name: "Delete Backup", exact: true }).click();
await expect(currentDialogLocator.getByRole("heading", { name: "Delete Backup" })).toBeVisible();
// Delete it
await currentDialogLocator.getByTestId("dialog-primary-button").click(); // Click "Delete Backup"
// Delete it
await tab.getByRole("button", { name: "Delete Backup", exact: true }).click();
await expect(currentDialogLocator.getByRole("heading", { name: "Delete Backup" })).toBeVisible();
await currentDialogLocator.getByTestId("dialog-primary-button").click(); // Click "Delete Backup"

await currentDialogLocator.getByTestId("dialog-cancel-button").click();

// go back to the settings to check that no backup was created (the setup button should still be there)
const tab2 = await app.settings.openUserSettings("Security & Privacy");
Copy link
Member

Choose a reason for hiding this comment

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

why do we create a new tab2 here, but not the other two times we re-open the settings? Surely either we can re-use tab or we can't?

@BillCarsonFr BillCarsonFr added this pull request to the merge queue Jan 9, 2024
@t3chguy t3chguy removed this pull request from the merge queue due to a manual request Jan 9, 2024
@t3chguy t3chguy added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@t3chguy t3chguy added this pull request to the merge queue Jan 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2024
@t3chguy t3chguy added this pull request to the merge queue Jan 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 10, 2024
@t3chguy t3chguy added this pull request to the merge queue Jan 10, 2024
Merged via the queue into develop with commit 0337bd1 Jan 10, 2024
24 checks passed
@t3chguy t3chguy deleted the valere/fix_reset_backup_after_verif branch January 10, 2024 11:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element-R: "Set up key backup" button doesn't work
4 participants