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

Add CryptoApi.encryptToDeviceMessages() and deprecate Crypto.encryptAndSendToDevices() #4380

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

Conversation

hughns
Copy link
Member

@hughns hughns commented Sep 2, 2024

Fixes #3304

  • Adds CryptoApi.encryptToDeviceMessages with rust and libolm implementations.
  • Deprecates Crypto.encryptAndSendToDevices.

Depends on matrix-org/matrix-rust-sdk-crypto-wasm#140 and #4396

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).

Deprecate Crypto. encryptAndSendToDevices and MatrixClient. encryptAndSendToDevices
@hughns hughns added T-Enhancement T-Deprecation A pull request that makes something deprecated labels Sep 2, 2024
@hughns hughns changed the title Add CryptoApi. encryptToDeviceMessages Implement MatrixClient.encryptAndSendToDevices for rust crypto and add CryptoApi.encryptToDeviceMessages Sep 2, 2024
@richvdh richvdh self-requested a review September 2, 2024 12:23
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 like the right general direction to me

src/client.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
@hughns hughns changed the base branch from develop to hughns/matrix-sdk-crypto-wasm-8 September 9, 2024 09:58
@hughns hughns changed the base branch from hughns/matrix-sdk-crypto-wasm-8 to develop September 18, 2024 08:55
src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
* set of devices.
*
* @param eventType the type of the event to send
* @param devices an array of (user ID, device ID) pairs to encrypt the payload for
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can/should impart any advice about the maximum number of entries in devices?

Copy link
Member Author

Choose a reason for hiding this comment

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

A cursory look suggests that the limiting factor would be the size of the claim request generated by KeyClaimManager.ensureSessionsForUsers().

Might it make sense to batch that at some sensible level? Similar to the batch size of 20 that is imposed for actually sending the to-device events to the server.

As a user of the higher-level CryptoApi I think any batching would be done for me automatically (which currently isn't the case).

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to add an integration test in spec/integ/crypto that exercises the encryption without mocking out bits of OlmMachine. I'm aware that such things can be fiddly to get to work, though, so I won't insist.

@richvdh
Copy link
Member

richvdh commented Oct 8, 2024

BTW it looks like the description in the PR is a bit outdated -- could you give it a refresh?

@hughns hughns changed the title Implement MatrixClient.encryptAndSendToDevices for rust crypto and add CryptoApi.encryptToDeviceMessages Add CryptoApi.encryptToDeviceMessages() and deprecate Crypto.encryptAndSendToDevices() Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Deprecation A pull request that makes something deprecated T-Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Element-R: wire up encryptAndSendToDevices
2 participants