feat(crypto): Add `forwarder_data` to `InboundGroupSession` and pickle. #5943

kaylendog · 2025-12-10T12:23:11Z

Issue: #5109

Public API changes documented in changelogs (optional)

codspeed-hq · 2025-12-10T12:44:12Z

CodSpeed Performance Report

Merging #5943 will not alter performance

_{Comparing kaylendog/history_sharing/store_history_sender_details (f7c6e5f) with main (2522a36)}

Summary

✅ 50 untouched

codecov · 2025-12-10T13:59:09Z

Codecov Report

❌ Patch coverage is 92.68293% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.51%. Comparing base (2522a36) to head (f7c6e5f).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/store/mod.rs	88.23%	0 Missing and 2 partials ⚠️
...atrix-sdk-crypto/src/olm/group_sessions/inbound.rs	91.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5943   +/-   ##
=======================================
  Coverage   88.51%   88.51%           
=======================================
  Files         363      363           
  Lines      103432   103466   +34     
  Branches   103432   103466   +34     
=======================================
+ Hits        91548    91581   +33     
  Misses       7531     7531           
- Partials     4353     4354    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Issue: #5109 Signed-off-by: Skye Elliot <[email protected]>

richvdh

A few comments and thoughts.

There doesn't seem to be much in the way of tests that forwarder_data gets set correctly. I know that integration tests are coming later, but can we update test_receive_room_key_bundle?

richvdh · 2025-12-12T11:00:59Z

crates/matrix-sdk-crypto/src/store/traits.rs

+    /// * `sender_data` - If the sessions were received as part of an MSC4268
+    ///   key bundle, the information about the user who sent us the bundle.


this appears to be referring to a non-existent argument

richvdh · 2025-12-12T11:02:36Z

crates/matrix-sdk-crypto/src/store/mod.rs

-        if matches!(
-            &sender_data,
+        // The sender's device must be either `SenderData::SenderUnverified` (i.e.,
+        // TOFU-trusted) or `SenderData::SenderVerified` (i.e., fully verified
+        // via user verification and cross-signing).
+        let sender_data = match sender_data {
            SenderData::UnknownDevice { .. }
-                | SenderData::VerificationViolation(_)
-                | SenderData::DeviceInfo { .. }
-        ) {
-            warn!(
-                "Not accepting a historic room key bundle due to insufficient trust in the sender"
-            );
-            return Ok(());
-        }
+            | SenderData::VerificationViolation(_)
+            | SenderData::DeviceInfo { .. } => {
+                warn!(
+                    "Not accepting a historic room key bundle due to insufficient trust in the sender"
+                );
+                return Ok(());
+            }
+            SenderData::SenderUnverified(_) | SenderData::SenderVerified(_) => sender_data,
+        };


now that we pass the whole SenderData into import_room_key_bundle_sessions, I don't think the match statement is an improvement over the old matches! macro. Maybe keep the comment though.

richvdh · 2025-12-12T11:05:42Z

crates/matrix-sdk-crypto/src/store/crypto_store_wrapper.rs

+    /// * `sender_data` - If the sessions were received as part of an MSC4268
+    ///   key bundle, the information about the user who sent us the bundle.


another non-existent argument

richvdh · 2025-12-12T11:06:54Z

crates/matrix-sdk-crypto/src/store/types.rs

    /// store.
    pub received_room_key_bundles: Vec<StoredRoomKeyBundleData>,
+
+    pub sender_data: BTreeMap<OwnedRoomId, BTreeMap<String, KnownSenderData>>,


this needs documentation at the very least, but I don't understand why it is needed and how it relates to inbound_group_sessions

richvdh · 2025-12-12T11:08:05Z

crates/matrix-sdk-crypto/src/store/mod.rs

        &self,
        room_keys: Vec<T>,
        from_backup_version: Option<&str>,
+        sender_data: Option<&SenderData>,


given that this ends up as the forwarder_data rather than the sender_data, it should probably be named accordingly ... and that change could do with chasing up the call stack.

richvdh · 2025-12-12T11:19:56Z

bindings/matrix-sdk-crypto-ffi/src/machine.rs

        &self,
        keys: Vec<ExportedRoomKey>,
        from_backup_version: Option<&str>,
+        sender_data: Option<&SenderData>,


since this is unused (and afaik we have no intention to use it), I'd suggest getting rid of it.

richvdh · 2025-12-12T11:23:39Z

crates/matrix-sdk-crypto/src/store/mod.rs

        &self,
        exported_keys: Vec<ExportedRoomKey>,
        from_backup_version: Option<&str>,
+        sender_data: Option<&SenderData>,


richvdh · 2025-12-12T11:36:10Z

crates/matrix-sdk-crypto/src/store/mod.rs

        let mut keys = BTreeMap::new();

        for (i, key) in room_keys.into_iter().enumerate() {
            match key.try_into() {


Low-confidence comment:

It feels like, ideally, we ought to be setting the InboundGroupSession::forwarder_data at the point we convert from HistoricRoomKey: it's not really meaningful to convert from HistoricRoomKey to InboundGroupSession without a forwarder_data.

In other words, we should replace InboundGroupSession::try_from(&HistoricRoomKey) with some other method that also takes a &SenderData.

On the flip side, import_sessions_impl is used for ExportedRoomKey as well as HistoricRoomKey, so this change would probably mean pushing the try_into call, and hence the error handling up the stack. It might give us more code and less clarity.

WDYT?

kaylendog assigned richvdh and kaylendog Dec 10, 2025

kaylendog requested review from a team as code owners December 10, 2025 12:23

kaylendog requested review from Hywan and poljar and removed request for a team December 10, 2025 12:23

kaylendog marked this pull request as draft December 10, 2025 12:23

kaylendog removed request for Hywan and poljar December 10, 2025 12:23

kaylendog force-pushed the kaylendog/history_sharing/store_history_sender_details branch 3 times, most recently from c322d5d to ceb0c71 Compare December 10, 2025 14:53

kaylendog marked this pull request as ready for review December 10, 2025 15:04

kaylendog mentioned this pull request Dec 10, 2025

feat: Add forwarder and forwarder_device to EncryptionInfo. #5945

Draft

1 task

kaylendog force-pushed the kaylendog/history_sharing/store_history_sender_details branch from ceb0c71 to a61c06d Compare December 10, 2025 17:08

richvdh and others added 2 commits December 11, 2025 09:51

feat: Add forwarder_data to InboundGroupSession and pickle.

c2fca6c

Issue: #5109 Signed-off-by: Skye Elliot <[email protected]>

tests(crypto): Doctests, update snapshots to include forwarder_data.

f7c6e5f

kaylendog force-pushed the kaylendog/history_sharing/store_history_sender_details branch from a61c06d to f7c6e5f Compare December 11, 2025 09:53

kaylendog mentioned this pull request Dec 11, 2025

History sharing: store details of the provenance of a shared megolm session #5109

Open

kaylendog requested review from poljar and richvdh and removed request for richvdh December 11, 2025 17:03

richvdh reviewed Dec 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(crypto): Add `forwarder_data` to `InboundGroupSession` and pickle. #5943

feat(crypto): Add `forwarder_data` to `InboundGroupSession` and pickle. #5943

Uh oh!

kaylendog commented Dec 10, 2025

Uh oh!

codspeed-hq bot commented Dec 10, 2025 •

edited

Loading

Uh oh!

codecov bot commented Dec 10, 2025 •

edited

Loading

Uh oh!

richvdh left a comment

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

richvdh Dec 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		/// * `sender_data` - If the sessions were received as part of an MSC4268
		/// key bundle, the information about the user who sent us the bundle.

feat(crypto): Add forwarder_data to InboundGroupSession and pickle. #5943

Are you sure you want to change the base?

feat(crypto): Add forwarder_data to InboundGroupSession and pickle. #5943

Uh oh!

Conversation

kaylendog commented Dec 10, 2025

Uh oh!

codspeed-hq bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #5943 will not alter performance

Summary

Uh oh!

codecov bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat(crypto): Add `forwarder_data` to `InboundGroupSession` and pickle. #5943

feat(crypto): Add `forwarder_data` to `InboundGroupSession` and pickle. #5943

codspeed-hq bot commented Dec 10, 2025 •

edited

Loading

codecov bot commented Dec 10, 2025 •

edited

Loading