MatrixRTC: ToDevice distribution for media stream keys #4785

BillCarsonFr · 2025-04-02T13:35:10Z

Require changes on StopGapWidget to properly pass to device events without stripping things

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

- use correct value for: `onEncryptionKeysChanged` - only update `latestGeneratedKeyIndex` for "this user" key

src/client.ts

src/embedded.ts

toger5 · 2025-04-09T10:14:38Z

src/matrixrtc/EncryptionManager.ts

+    // TODO: remove this value since I think this is not helpful to use
+    // it represents what index we actually sent over to ElementCall (which we do in a delayed manner)
+    // but it has no releavnt information for the encryption manager.
+    private mediaTrailerKeyIndexInUse = -1;
+    private latestGeneratedKeyIndex = -1;


We intentionally want the transparency here. This class EncryptionManager will be deprecated soon
and replaced by a refactored version.
It might be worth renaming this to LegacyEncryptionManager already?

toger5 · 2025-04-09T10:15:06Z

src/matrixrtc/EncryptionManager.ts

            logger.warn("Tried to send encryption keys event but no current key index found!");
            return;
        }

-        const keyIndexToSend = indexToSend ?? this.currentEncryptionKeyIndex;
+        const keyIndexToSend = indexToSend ?? this.latestGeneratedKeyIndex;
+        // TODO remove this debug log. it just shows then when sending mediaTrailerKeyIndexInUse contained the wrong index.


this TODO comment is also expected and will help building the refactored manager.

src/matrixrtc/MatrixRTCSession.ts

src/matrixrtc/ToDeviceKeyTransport.ts

dbkr

A quick PR description saying what this is doing might have been helpful, although I think I've worked it out now apart from maybe some of the changes to EncryptionManager which don't seem all that relevant.

src/client.ts

dbkr · 2025-04-09T12:36:54Z

src/client.ts

+     * @param payload - The payload to send. This will be encrypted.
+     * @returns Promise which resolves once queued.
+     */
+    public async encryptAndSendToDevice(


If this is actually calling queueToDevice maybe it should be called encryptAndQueueToDevice to be less confusing.

I think since it will send the event eventually calling it send it okay?
Also we are considering to allow error handling in the widget so this should not resolve once queued but once send and return the error. This is a tracked issue we will tackle as a second iteration on both rust sdk ec and js-sdk.

Well, so does queueToDevice, but fair enough - as long as we don't end up wanting to add another one that doesn't queue.

I created: #4793
So we can tackle this in a better way and get rid of the TODO comment. (not yet ready to review needs to wait until next week i suspect.)

src/matrixrtc/IKeyTransport.ts

src/matrixrtc/ToDeviceKeyTransport.ts

dbkr · 2025-04-09T12:42:46Z

src/matrixrtc/ToDeviceKeyTransport.ts

+    }
+
+    public stop(): void {
+        this.client.off(ClientEvent.ToDeviceEvent, this.onToDeviceEvent);


This won't work as you've wrapped it in an anonymous function when adding.

This is a very good catch thx. I dont know why the wrapper was added (maybe some params were different eventually.)

src/matrixrtc/ToDeviceKeyTransport.ts

spec/unit/matrixrtc/MatrixRTCSession.spec.ts

spec/unit/matrixrtc/ToDeviceKeyTransport.spec.ts

dbkr · 2025-04-09T13:11:27Z

src/matrixrtc/EncryptionManager.ts

+    // TODO: remove this value since I think this is not helpful to use
+    // it represents what index we actually sent over to ElementCall (which we do in a delayed manner)
+    // but it has no releavnt information for the encryption manager.
+    private mediaTrailerKeyIndexInUse = -1;
+    private latestGeneratedKeyIndex = -1;


Not sure why you're adding something and then claiming in a comment that it should be removed...

Also, why the rename?

Mostly because of: #4785 (comment)

But you might be right. we did the testing where we needed the logging so probably its best to just remove it.

* MatrixRTC: ToDevice distribution for media stream keys * test: Add RTC to device transport test * lint * fix key indexing * fix indexing take two - use correct value for: `onEncryptionKeysChanged` - only update `latestGeneratedKeyIndex` for "this user" key * test: add test for join config `useExperimentalToDeviceTransport` * update test to fail without the fixed encryption key index * review * review (dave) --------- Co-authored-by: Timo <[email protected]>

toger5 mentioned this pull request Apr 3, 2025

[ER-59] E2EE: Send media keys via to_device message element-hq/element-call#1281

Open

42 tasks

toger5 force-pushed the valere/matrix_rtc_key_transport branch from 538ad63 to bd6dda1 Compare April 3, 2025 13:18

BillCarsonFr force-pushed the valere/matrix_rtc_to_device_keys branch from d3733d7 to ee6747f Compare April 4, 2025 09:41

toger5 added the T-Feature Request to add a new feature which does not exist right now label Apr 7, 2025

Base automatically changed from valere/matrix_rtc_key_transport to develop April 7, 2025 08:50

toger5 added the T-Enhancement label Apr 7, 2025

toger5 force-pushed the valere/matrix_rtc_to_device_keys branch 2 times, most recently from 29c5d75 to 499c56e Compare April 7, 2025 09:35

github-actions bot deployed to PR Documentation Preview April 7, 2025 09:36 View deployment

MatrixRTC: ToDevice distribution for media stream keys

0a587e9

BillCarsonFr force-pushed the valere/matrix_rtc_to_device_keys branch from 499c56e to 0a587e9 Compare April 7, 2025 12:55

github-actions bot deployed to PR Documentation Preview April 7, 2025 12:56 View deployment

test: Add RTC to device transport test

233fd85

BillCarsonFr changed the title ~~DRAFT DRAFT TO DEVICE KEYS~~ MatrixRTC: ToDevice distribution for media stream keys Apr 7, 2025

lint

b3b5347

BillCarsonFr force-pushed the valere/matrix_rtc_to_device_keys branch from 297e635 to b3b5347 Compare April 7, 2025 17:51

github-actions bot deployed to PR Documentation Preview April 7, 2025 17:53 View deployment

fix key indexing

598a31c

github-actions bot deployed to PR Documentation Preview April 8, 2025 15:37 View deployment

fix indexing take two

3dd55e4

- use correct value for: `onEncryptionKeysChanged` - only update `latestGeneratedKeyIndex` for "this user" key

github-actions bot deployed to PR Documentation Preview April 8, 2025 16:18 View deployment

test: add test for join config useExperimentalToDeviceTransport

96382ca

github-actions bot deployed to PR Documentation Preview April 9, 2025 09:23 View deployment

update test to fail without the fixed encryption key index

5c01a2d

github-actions bot deployed to PR Documentation Preview April 9, 2025 10:12 View deployment

toger5 requested changes Apr 9, 2025

View reviewed changes

toger5 marked this pull request as ready for review April 9, 2025 10:18

toger5 requested review from a team as code owners April 9, 2025 10:18

toger5 requested a review from robintown April 9, 2025 10:18

toger5 requested a review from dbkr April 9, 2025 10:18

toger5 force-pushed the valere/matrix_rtc_to_device_keys branch from d93b1db to 3178269 Compare April 9, 2025 11:01

toger5 approved these changes Apr 9, 2025

View reviewed changes

review

778db8c

toger5 force-pushed the valere/matrix_rtc_to_device_keys branch from 3178269 to 778db8c Compare April 9, 2025 11:09

github-actions bot deployed to PR Documentation Preview April 9, 2025 11:10 View deployment

dbkr reviewed Apr 9, 2025

View reviewed changes

toger5 mentioned this pull request Apr 9, 2025

Add option to enable to-device-encryption element-hq/element-call#3167

Merged

review (dave)

a9aa68d

github-actions bot deployed to PR Documentation Preview April 9, 2025 13:53 View deployment

toger5 requested a review from dbkr April 9, 2025 14:43

dbkr approved these changes Apr 9, 2025

View reviewed changes

toger5 added this pull request to the merge queue Apr 10, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 10, 2025

toger5 added this pull request to the merge queue Apr 10, 2025

Merged via the queue into develop with commit 3f03c1d Apr 10, 2025
30 checks passed

toger5 deleted the valere/matrix_rtc_to_device_keys branch April 10, 2025 08:41

This was referenced Apr 10, 2025

Flaky test "should log and return to welcome page with correct error when login state is not found" #4795

Closed

Flaky Playright Test: should log and return to welcome page with correct error when login state is not found element-hq/element-web#29708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MatrixRTC: ToDevice distribution for media stream keys #4785

MatrixRTC: ToDevice distribution for media stream keys #4785

BillCarsonFr commented Apr 2, 2025 •

edited by toger5

Loading

toger5 Apr 9, 2025

toger5 Apr 9, 2025

dbkr left a comment

dbkr Apr 9, 2025

toger5 Apr 9, 2025

dbkr Apr 9, 2025

toger5 Apr 10, 2025

dbkr Apr 9, 2025

toger5 Apr 9, 2025

dbkr Apr 9, 2025

toger5 Apr 9, 2025

MatrixRTC: ToDevice distribution for media stream keys #4785

MatrixRTC: ToDevice distribution for media stream keys #4785

Conversation

BillCarsonFr commented Apr 2, 2025 • edited by toger5 Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillCarsonFr commented Apr 2, 2025 •

edited by toger5

Loading