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 (341ab9c) with main (a527439)}

Summary

✅ 50 untouched

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

Codecov Report

❌ Patch coverage is 84.84848% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.48%. Comparing base (2522a36) to head (341ab9c).
⚠️ Report is 16 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-crypto/src/store/mod.rs	77.02%	12 Missing and 5 partials ⚠️
...atrix-sdk-crypto/src/olm/group_sessions/inbound.rs	93.47%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5943      +/-   ##
==========================================
- Coverage   88.51%   88.48%   -0.03%     
==========================================
  Files         363      363              
  Lines      103432   103527      +95     
  Branches   103432   103527      +95     
==========================================
+ Hits        91548    91609      +61     
- Misses       7531     7562      +31     
- Partials     4353     4356       +3

☔ 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

Addressed in 741a442

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.

Addressed in 4ffef64

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

Addressed in 741a442

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

Left-over from before we decided to use InboundGroupSession, removed in 741a442

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.

Addressed in 741a442

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.

Addressed in 1b951ab

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>,


Addressed in 741a442

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?

I presume impl TryFrom<(&HistoricRoomKey, SenderData)> for InboundGroupSession is too hacky? I do like the polymorphism you point out in your final paragraph, and it'd be nice to keep that, I think.

impl TryFrom<(&HistoricRoomKey, SenderData)> for InboundGroupSession is too hacky?

That does feel pretty hacky. Once you get that specific, it seems like a dedicated method is preferable to trying to implement TryFrom or TryInto. (HistoricRoomKey::try_into_inbound_group_session(self, sender_data:: &SenderData) or sth.)

I do like the polymorphism you point out in your final paragraph, and it'd be nice to keep that, I think.

Ok, up to you.

I think we can exploit TryFrom<T> for T being implemented in the standard library - I am cooking something up...

Nevermind, opted to just rewrite Store::Import_sessions_impl

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

kaylendog added 2 commits December 15, 2025 15:52

chore: Rename sender_data to forwarder_data for clarity

741a442

refactor: Use matches! over match statement.

4ffef64

kaylendog added 5 commits December 15, 2025 16:22

refactor: Remove argument with always None value.

1b951ab

fix: Negate bundle verification import condition

8327b1d

refactor: Use helper method to construct IGS from HistoricRoomKey.

2e45d17

chore: Fix clippy lints.

9d921d8

tests: Check if forwarder_data is added to store.

341ab9c

kaylendog force-pushed the kaylendog/history_sharing/store_history_sender_details branch from fdfaf0e to 341ab9c Compare December 15, 2025 18:13

kaylendog requested a review from richvdh December 15, 2025 18:14

		/// * `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!

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!

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