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 legacy crypto #4653

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

* Remove `MatrixClient.initLegacyCrypto`

* Remove `MatrixClient.initLegacyCrypto` in README.md

* Remove tests using `MatrixClient.initLegacyCrypto`
* chore(legacy call): Remove `DeviceInfo` usage

* refactor(legacy call): throw `GroupCallUnknownDeviceError` at the end of `initOpponentCrypto`
* 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.
* feat(legacy crypto!): remove legacy crypto usage in `event.ts`

* test(legacy crypto): update event.spec.ts to not use legacy crypto types
* feat(legacy crypto!): remove legacy crypto export in `matrix.ts`

* test(legacy crypto): update `megolm-backup.spec.ts` to import directly `CryptoApi`
* 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
* feat(CryptoBackend)!: remove deprecated methods

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

* test(rust-crypto): remove tests of deprecated methods of `CryptoBackend`
The interface of `encryptAndSendToDevices` changes because `DeviceInfo` is from the legacy crypto. In fact `encryptAndSendToDevices` only need pairs of userId and deviceId.
* 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
…-legacy-crypto

# Conflicts:
#	src/crypto/index.ts
florianduros and others added 2 commits February 4, 2025 15:56
* 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
…-legacy-crypto

# Conflicts:
#	src/client.ts
#	src/crypto/EncryptionSetup.ts
#	src/crypto/OutgoingRoomKeyRequestManager.ts
@@ -30,7 +30,7 @@ interface Extensible {

/* eslint-disable camelcase */

/** The result of a call to {@link MatrixClient.exportRoomKeys} */
/** The result of a call to CryptoApi.exportRoomKeys */
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it breaks docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #4672

Copy link
Member

Choose a reason for hiding this comment

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

image

It works as per the docs I linked you to when you asked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh perfect, I didn't understood first the documentation. I tried multiple times different without success. I'll make a PR for that

Copy link
Member

@t3chguy t3chguy Feb 5, 2025

Choose a reason for hiding this comment

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

Also just noticed in the screenshot I included that it looks like OlmGroupSessionExtraData is now unused that /crypto/ has been deleted so seems like the checks for dead code were not exhaustive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I relied on knip to catch the unused type, this one wasn't raised unfortunately.

src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/models/event.ts Show resolved Hide resolved
*
* @returns a promise that resolves when the request is queued
*/
public cancelAndResendKeyRequest(crypto: Crypto, userId: string): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Event.cancelAndResendKeyRequest is only used by

matrix-js-sdk/src/client.ts

Lines 3211 to 3225 in 7d68753

/**
* Cancel a room key request for this event if one is ongoing and resend the
* request.
* @param event - event of which to cancel and resend the room
* key request.
* @returns A promise that will resolve when the key request is queued
*
* @deprecated Not supported for Rust Cryptography.
*/
public cancelAndResendEventRoomKeyRequest(event: MatrixEvent): Promise<void> {
if (!this.crypto) {
throw new Error("End-to-End encryption disabled");
}
return event.cancelAndResendKeyRequest(this.crypto, this.getUserId()!);
}

Which was deprecated and deleted in this PR. Cryptois also deprecated, this function can't live with Crypto.

Copy link
Member

Choose a reason for hiding this comment

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

Right, but it should be deprecated before removal, just because another method with a similar name was deprecated does not mean this was deprecated.

@@ -1721,10 +1691,6 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
};
}

public setVerificationRequest(request: VerificationRequest): void {
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't deprecated?

src/rust-crypto/rust-crypto.ts Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Show resolved Hide resolved
src/common-crypto/CryptoBackend.ts Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Feb 4, 2025

@t3chguy the strategy we agreed was that we'd review PRs on the way into the florianduros/rip-out-legacy-crypto/remove-legacy-crypto branch rather than attempt to give meaningful review to the branch as a whole. I don't think we should put another set of roadblocks on merging this branch now.

@t3chguy
Copy link
Member

t3chguy commented Feb 5, 2025

@t3chguy the strategy we agreed was that we'd review PRs on the way into the florianduros/rip-out-legacy-crypto/remove-legacy-crypto branch rather than attempt to give meaningful review to the branch as a whole. I don't think we should put another set of roadblocks on merging this branch now.

Where was the list of component parts which are to be PR'd individually reviewed? What's to say all the PRs have been made now and there doesn't need to be further PRs made onto this feature branch? If we don't review this then what's to stop the component sum of all the PRs not being acceptable?

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.

Rip out legacy crypto code
4 participants