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

Fix issue in sync when crypto is not supported by client #2715

Merged
merged 3 commits into from
Sep 30, 2022

Conversation

stas-demydiuk
Copy link
Contributor

@stas-demydiuk stas-demydiuk commented Sep 30, 2022

When the Matrix client is initialized without crypto support it now breaks in the sync as starting from matrix-js-sdk 19.7.0 the additional code was added that directly access the client's crypto feature.

This PR is aimed to fix the issue by introducing the additional check that crypto is actually available before accessing it

Signed-off-by: Stanislav Demydiuk [email protected]

Checklist

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

Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Fix issue in sync when crypto is not supported by client (#2715). Contributed by @stas-demydiuk.

@stas-demydiuk stas-demydiuk requested a review from a team as a code owner September 30, 2022 07:31
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Sep 30, 2022
@t3chguy t3chguy requested review from t3chguy and removed request for germain-gg, florianduros and robintown September 30, 2022 07:52
@t3chguy t3chguy added the backport staging Label to automatically backport PR to staging branch label Sep 30, 2022
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request!

I believe it would be important to add a smoke test to ensure that the js-sdk can process a sync when the crypto layer is missing

If I look a little further at this, I can see that the type definition here fails to mention that the crypto property can be undefined (see

public crypto: Crypto; // XXX: Intended private, used in code.
) I would suggest changing that too

src/sync.ts Outdated Show resolved Hide resolved
@t3chguy
Copy link
Member

t3chguy commented Sep 30, 2022

/me will handle this and write a test, thanks for review @gsouquet and thanks @stas-demydiuk for the report & fix

@germain-gg germain-gg dismissed their stale review September 30, 2022 08:00

t3chguy will handle tests

@t3chguy t3chguy requested review from germain-gg and removed request for t3chguy September 30, 2022 08:03
Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Good luck with the strict null checks

@t3chguy t3chguy enabled auto-merge (squash) September 30, 2022 08:10
@t3chguy t3chguy merged commit 9bb5afe into matrix-org:develop Sep 30, 2022
RiotRobot pushed a commit that referenced this pull request Sep 30, 2022
Co-authored-by: Michael Telatynski <[email protected]>
(cherry picked from commit 9bb5afe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport staging Label to automatically backport PR to staging branch T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants