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

Merged
merged 27 commits into from
Oct 28, 2024

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
@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
@hughns
Copy link
Member Author

hughns commented Oct 22, 2024

@richvdh I've had a go at adding an integration test in a87a29f.

However, whilst it passes on libolm it doesn't pass on rust SDK. It appears that getMissingSessions() is always returning null. Any idea what might be happening or missing?

@richvdh
Copy link
Member

richvdh commented Oct 23, 2024

It appears that getMissingSessions() is always returning null. Any idea what might be happening or missing?

Well, that means that there are Olm Sessions already established for all the devices that the rust layer knows about. In this case, it probably means that the rust layer doesn't know about any of the target user's devices.

We only fetch the device list when we are told by the homeserver that our cached device list for a given user might be outdated. And that only happens if we share at least one room with the offending user. (Is that likely to be a problem in practice, for this application?)

Try something like:

        const syncResponder = new SyncResponder(homeserverUrl);
        syncResponder.sendOrQueueSyncResponse(getSyncResponse([testData.BOB_TEST_USER_ID]));
        await syncPromise(aliceClient);

I think that will probably do the job, though you might have to do something else to trigger the /keys/query request. (As long as there is a /keys/query request pending, getMissingSessions should block until it happens.)

@hughns
Copy link
Member Author

hughns commented Oct 23, 2024

Thanks, @richvdh. I think this is now ready for re-review.

@hughns hughns requested a review from richvdh October 23, 2024 13:40
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

spec/integ/crypto/to-device-messages.spec.ts Outdated Show resolved Hide resolved
@hughns hughns added this pull request to the merge queue Oct 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 28, 2024
@hughns hughns added this pull request to the merge queue Oct 28, 2024
Merged via the queue into develop with commit 31aeb30 Oct 28, 2024
26 checks passed
@hughns hughns deleted the hughns/rust-send-to-device branch October 28, 2024 11:48
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