Skip to content

Reset cross-signing before backup when resetting both - #28402

Merged
uhoreg merged 5 commits into
element-hq:developfrom
uhoreg:reset_4s_fix
Nov 19, 2024
Merged

Reset cross-signing before backup when resetting both#28402
uhoreg merged 5 commits into
element-hq:developfrom
uhoreg:reset_4s_fix

Conversation

@uhoreg

@uhoreg uhoreg commented Nov 6, 2024

Copy link
Copy Markdown
Member

so that the backup gets signed by the new cross-signing key

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)

@uhoreg

uhoreg commented Nov 8, 2024

Copy link
Copy Markdown
Member Author

The Jest test looks like just a flaky test. No idea why SonarCloud isn't reporting anything. And earlier version of this PR had 70% test coverage. I've since added another test, but I don't know if that's enough for SonarCloud until SonarCloud returns its result. I think that this PR is otherwise ready for review.

@uhoreg
uhoreg marked this pull request as ready for review November 8, 2024 02:28
@uhoreg
uhoreg requested review from a team as code owners November 8, 2024 02:28
@t3chguy

t3chguy commented Nov 11, 2024

Copy link
Copy Markdown
Member

If a jest test fails/flakes then Sonar won't run as it won't have a complete junit report to work from

@florianduros
florianduros removed their request for review November 14, 2024 10:13
* storage first, then cross-signing, then key backup.
*/
logger.log("Forcing secret storage reset");
await crypto.bootstrapSecretStorage({

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.

Why not add the resetCrossSigning as an option to bootstrapSecretStorage?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mainly just to avoid making such an invasive change.

@BillCarsonFr BillCarsonFr left a comment

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.

Just a question, otherwise ok

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants