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

Factor out crypto setup process into a store #28675

Merged
merged 7 commits into from
Dec 11, 2024
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Dec 6, 2024

To make components pure and avoid react 18 dev mode problems due to components making requests when mounted.

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)

To make components pure and avoid react 18 dev mode problems due
to components making requests when mounted.
this.emit("update");
this.onFinished?.(true);
} catch (e) {
if (this.isTokenLogin) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the status is not "error" if there is a login token?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish I knew: this comment is all there was so I moved it over. Possibly because if the user's using token login and the grace period didn't work, nothing else will so it's better to just carry on and bug the user later to set up crypto rather than worry them? Maybe the real answer is that we should drop support for HSes without the key upload grace period.

this.stores = stores;
this.onFinished = onFinished;

this.doSetup().catch(() => logger.error("Initial crypto setup failed"));
Copy link
Member

Choose a reason for hiding this comment

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

Why we are not waiting for doSetup to finish?

Copy link
Member Author

@dbkr dbkr Dec 9, 2024

Choose a reason for hiding this comment

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

It just seemed unnecessary and would conflate the interface I think. Added a comment.

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

I am not really familiar with this store pattern for react :/
Don't know what a pure component should be, but FWIW looks ok.

I think that several things moved/refactored are deprecated and at some point should be droped (don't need to save password there is the grace period, and reauth not need for initial setup of cross signing keys https://github.com/matrix-org/matrix-spec-proposals/blob/hughns/device-signing-upload-uia/proposals/3967-device-signing-upload-uia.md

this.emit("update");

try {
await createCrossSigning(this.client, this.isTokenLogin, this.stores.accountPasswordStore.getPassword());
Copy link
Member

Choose a reason for hiding this comment

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

The password thing look like something that should be removed. Because MSC3967 has been merged and re-auth will not be required to setup cross signing for the first time.
And also anyhow the grace period is managed by the server.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was hoping we could remove this soon. I wouldn't do that in a refactor though.

@dbkr dbkr added this pull request to the merge queue Dec 11, 2024
Merged via the queue into develop with commit b330de5 Dec 11, 2024
33 checks passed
@dbkr dbkr deleted the dbkr/initialcryptosetupstore branch December 11, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants