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

Replace SecurityCustomisations with CryptoSetupExtension #12342

Merged
merged 41 commits into from Apr 12, 2024

Conversation

thoraj
Copy link
Contributor

@thoraj thoraj commented Mar 14, 2024

Description

This PR is for the changes required for the CryptoSetupExtension. The extension is described in this issue, and also in the corresponding PR in matrix-react-sdk-module-api. The module-api PR has been merged, and the changes were released in v2.4.0.

SecurityCustomisations

We have asked about the deprecation of the SecurityCustomisations here in GH, and in the Element Web Development room (#element-dev:matrix.org). A decision must be made if CryptoSetupExtension will replace SecurityCustomisations immediately, or if there need to be a period of overlap where both are supported? As of 15.03.2024 we have not received any response/information about the matter.

Important this PR will remove the calls to the obsolete SecurityCustomisations, and replace them with the corresponding calls to CryptoSetupExtension (via ModuleRunner). (I have left the old calls as comments)

The rationale is that until Verji asked about the SecurityCustomisations the Element team had no plans to port them since no-one had notified the Element team that they were using them. Furthermore, AFAIK the customisation mechanism as such is deprecated and will be removed in the future.

Dependencies

This PR depends on matrix-react-sdk-module-api v2.4.0+, hence the checks will fail until matrix-react-sdk bumps the dependency from 2.3.0 => 2.4.0

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.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Signed-off-by: Thor Arne Johansen taj@verji.com

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Mar 14, 2024
@thoraj thoraj changed the title Taj/cryptosetup Replace SecurityCustomisations with CryptoSetupExtension Mar 15, 2024
@richvdh
Copy link
Member

richvdh commented Apr 4, 2024

I'm sorry this has taken so long for a decision.

A decision must be made if CryptoSetupExtension will replace SecurityCustomisations immediately, or if there need to be a period of overlap where both are supported?

I discussed this with the team last week; the answer was: we're not aware of anyone other than yourselves that are using SecurityCustomisations. If someone does come out of the woodwork, at least we're providing a replacement. As far as we are concerned, therefore, there is no need to maintain SecurityCustomisations.

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.

Thanks for this! A few comments.

src/Lifecycle.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
test/modules/MockModule.ts Outdated Show resolved Hide resolved
test/modules/MockModule.ts Outdated Show resolved Hide resolved
src/MatrixClientPeg.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
src/modules/ModuleRunner.ts Outdated Show resolved Hide resolved
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.

This is looking really good now, thank you for your perseverance! Just one small suggestion.

@richvdh richvdh changed the title Replace SecurityCustomisations with CryptoSetupExtension Replace SecurityCustomisations with CryptoSetupExtension Apr 11, 2024
@richvdh richvdh added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Apr 11, 2024
thoraj and others added 3 commits April 11, 2024 15:18
Keep the flags to track implementations as direct properties

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@thoraj
Copy link
Contributor Author

thoraj commented Apr 11, 2024

@richvdh Thanks a lot for the very thorough review. I have implemented your last suggestions.

@thoraj thoraj requested a review from richvdh April 11, 2024 13:53
Fix whitespace

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.

Thank you so much for your perseverance!

@richvdh richvdh added this pull request to the merge queue Apr 12, 2024
Merged via the queue into matrix-org:develop with commit 6392759 Apr 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements 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.

None yet

3 participants