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

Remove deprecated methods and attributes of MatrixClient #4659

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Jan 27, 2025

Task element-hq/element-web#26922
Part of #4653

This PR removes the deprecated methods and attributes of the the MatrixClient. The legacy crypto relies on using deprecated methods of the MatrixClient and the MatrixClient is also using methods from the legacy crypto.

In order to not have a gigantic PR where both the deprecated methods of the MatrixClient and the legacy crypto are removed, only the MatrixClient is changed here (with the related tests). Accordingly, the legacy crypto tests and lint are in failure.

Since the PR will be merged in florianduros/rip-out-legacy-crypto/remove-legacy-crypto and not in develop, we can accept that the lint and the legacy crypto tests are in failure in the feature branch until the legacy crypto is removed.

The unit (not related to legacy crypto) and integ tests are updated to pass. However the cleaning of theses (remove isNewBackendOnly, run on each backend etc) tests will be done in a dedicated PR.

@florianduros florianduros changed the title Remove deprecated methods and attribute of MatrixClient Remove deprecated methods and attributes of MatrixClient Jan 27, 2025
@florianduros florianduros force-pushed the florianduros/rip-out-legacy-crypto/clean-up-matrixclient branch from 5140262 to 808f7ee Compare January 27, 2025 13:27
- `Embedded.spec.ts`: casting since `encryptAndSendToDevices` is removed from `MatrixClient`.
- `room.spec.ts`: remove deprecated usage of `MatrixClient.crypto`
- `matrix-client.spec.ts` & `matrix-client-methods.spec.ts`: remove calls of deprecated methods of `MatrixClient`
@florianduros florianduros force-pushed the florianduros/rip-out-legacy-crypto/clean-up-matrixclient branch from 80891ce to 0b31afd Compare January 27, 2025 15:02
@florianduros florianduros force-pushed the florianduros/rip-out-legacy-crypto/clean-up-matrixclient branch from 65e8795 to 89ac700 Compare January 27, 2025 17:14
@florianduros florianduros marked this pull request as ready for review January 27, 2025 17:21
@florianduros florianduros requested review from a team as code owners January 27, 2025 17:21
@florianduros florianduros requested review from uhoreg, richvdh and dbkr and removed request for a team January 27, 2025 17:21
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Looks sensible to me, will leave your question to the crypto experts. Looking forward to getting a lot of lines out of client.js.

@richvdh
Copy link
Member

richvdh commented Jan 28, 2025

Related: element-hq/element-web#17488

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.

looks good to me in general

src/client.ts Show resolved Hide resolved
Comment on lines -2132 to -2139
* Fetches the latest capabilities from the homeserver, ignoring any cached
* versions. The newly returned version is cached.
*
* @returns A promise which resolves to the capabilities of the homeserver
*/
public fetchCapabilities(): Promise<Capabilities> {
return this.serverCapabilitiesService.fetchCapabilities();
}
Copy link
Member

Choose a reason for hiding this comment

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

It's a shame that git's diff algorithm makes such a mess of this.

If you look at it from the commandline with git show --patience, you get a much nicer diff, showing that fetchCapabilities, initRustCrypto and secretStorage are unchanged. I think the problem is basically that the change is so large that we've exhausted the standard diff algorithm's ability to look ahead to match up lines.

For future reference, it might be nice to break down massive removals like this into separate commits, so that they can be reviewed via the github UI.

Comment on lines -2517 to -2536
/**
* Set the global override for whether the client should ever send encrypted
* messages to unverified devices. This provides the default for rooms which
* do not specify a value.
*
* @param value - whether to blacklist all unverified devices by default
*
* @deprecated Prefer direct access to {@link CryptoApi.globalBlacklistUnverifiedDevices}:
*
* ```javascript
* client.getCrypto().globalBlacklistUnverifiedDevices = value;
* ```
*/
public setGlobalBlacklistUnverifiedDevices(value: boolean): boolean {
if (!this.cryptoBackend) {
throw new Error("End-to-end encryption disabled");
}
this.cryptoBackend.globalBlacklistUnverifiedDevices = value;
return value;
}
Copy link
Member

Choose a reason for hiding this comment

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

It might be worth calling out, for the record, that some of the removed methods in MatrixClient do actually work in Rust Crypto.

It's definitely a good time to clean these out, so not suggesting we change anything; just making a note.

src/client.ts Outdated Show resolved Hide resolved
Comment on lines -1038 to -1039
// the prepare request should complete successfully.
await p;
Copy link
Member

Choose a reason for hiding this comment

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

isn't this still useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it isn't, prepareToEncrypt is not asynchronous.

Copy link
Member

Choose a reason for hiding this comment

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

[that looks like a bug, tbh]

`ICreateClientOpts.deviceToImport` was used in the legacy cryto. The rust crypto doesn't support to import devices in this way.
`globalErrorOnUnknownDevices` is not used in the rust-crypto. The API is marked as unstable, we can remove it.
@florianduros florianduros force-pushed the florianduros/rip-out-legacy-crypto/clean-up-matrixclient branch from 04363e4 to c5d6e35 Compare January 28, 2025 14:03
@florianduros florianduros requested a review from richvdh January 28, 2025 14:09
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.

lgtm

@florianduros florianduros merged commit 89cec0a into florianduros/rip-out-legacy-crypto/remove-legacy-crypto Jan 28, 2025
23 of 27 checks passed
@florianduros florianduros deleted the florianduros/rip-out-legacy-crypto/clean-up-matrixclient branch January 28, 2025 14:27
github-merge-queue bot pushed a commit that referenced this pull request Feb 7, 2025
* Remove deprecated calls in `webrtc/call.ts`

* Throw error when legacy call was used

* Remove `MatrixClient.initLegacyCrypto` (#4620)

* Remove `MatrixClient.initLegacyCrypto`

* Remove `MatrixClient.initLegacyCrypto` in README.md

* Remove tests using `MatrixClient.initLegacyCrypto`

* Remove legacy crypto support in `sync` api (#4622)

* Remove deprecated `DeviceInfo` in `webrtc/call.ts` (#4654)

* chore(legacy call): Remove `DeviceInfo` usage

* refactor(legacy call): throw `GroupCallUnknownDeviceError` at the end of `initOpponentCrypto`

* Remove deprecated methods and attributes of `MatrixClient` (#4659)

* feat(legacy crypto)!: remove deprecated methods of `MatrixClient`

* test(legacy crypto): update existing tests to not use legacy crypto

- `Embedded.spec.ts`: casting since `encryptAndSendToDevices` is removed from `MatrixClient`.
- `room.spec.ts`: remove deprecated usage of `MatrixClient.crypto`
- `matrix-client.spec.ts` & `matrix-client-methods.spec.ts`: remove calls of deprecated methods of `MatrixClient`

* test(legacy crypto): remove test files using `MatrixClient` deprecated methods

* test(legacy crypto): update existing integ tests to run successfully

* feat(legacy crypto!): remove `ICreateClientOpts.deviceToImport`.

`ICreateClientOpts.deviceToImport` was used in the legacy cryto. The rust crypto doesn't support to import devices in this way.

* feat(legacy crypto!): remove `{get,set}GlobalErrorOnUnknownDevices`

`globalErrorOnUnknownDevices` is not used in the rust-crypto. The API is marked as unstable, we can remove it.

* Remove usage of legacy crypto in `event.ts` (#4666)

* feat(legacy crypto!): remove legacy crypto usage in `event.ts`

* test(legacy crypto): update event.spec.ts to not use legacy crypto types

* Remove legacy crypto export in `matrix.ts` (#4667)

* feat(legacy crypto!): remove legacy crypto export in `matrix.ts`

* test(legacy crypto): update `megolm-backup.spec.ts` to import directly `CryptoApi`

* Remove usage of legacy crypto in integ tests (#4669)

* Clean up legacy stores (#4663)

* feat(legacy crypto!): keep legacy methods used in lib olm migration

The rust cryto needs these legacy stores in order to do the migration from the legacy crypto to the rust crypto. We keep the following methods of the stores:
- Used in `libolm_migration.ts`.
- Needed in the legacy store tests.
- Needed in the rust crypto test migration.

* feat(legacy crypto): extract legacy crypto types in legacy stores

In order to be able to delete the legacy crypto, these stores shouldn't rely on the legacy crypto. We need to extract the used types.

* feat(crypto store): remove `CryptoStore` functions used only by tests

* test(crypto store): use legacy `MemoryStore` type

* Remove deprecated methods of `CryptoBackend` (#4671)

* feat(CryptoBackend)!: remove deprecated methods

* feat(rust-crypto)!: remove deprecated methods of `CryptoBackend`

* test(rust-crypto): remove tests of deprecated methods of `CryptoBackend`

* Remove usage of legacy crypto in `embedded.ts` (#4668)

The interface of `encryptAndSendToDevices` changes because `DeviceInfo` is from the legacy crypto. In fact `encryptAndSendToDevices` only need pairs of userId and deviceId.

* Remove legacy crypto files (#4672)

* fix(legacy store): fix legacy store typing

In #4663, the storeXXX methods were removed of the CryptoStore interface but they are used internally by IndexedDBCryptoStore.

* feat(legacy crypto)!: remove content of `crypto/*` except legacy stores

* test(legacy crypto): remove `spec/unit/crypto/*` except legacy store tests

* refactor: remove unused types

* doc: fix broken link

* doc: remove link tag when typedoc is unable to find the CryptoApi

* Clean up integ test after legacy crypto removal (#4682)

* test(crypto): remove `newBackendOnly` test closure

* test(crypto): fix duplicate test name

* test(crypto): remove `oldBackendOnly` test closure

* test(crypto): remove `rust-sdk` comparison

* test(crypto): remove iteration on `CRYPTO_BACKEND`

* test(crypto): remove old legacy comments and tests

* test(crypto): fix documentations and removed unused expect

* Restore broken link to `CryptoApi` (#4692)

* chore: fix linting and formatting due to merge

* Remove unused crypto type and missing doc (#4696)

* chore(crypto): remove unused types

* doc(crypto): add missing link

* test(call): add test when crypto is enabled
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants