refactor(crypto): Move session_id from EncryptionInfo to AlgorithmInfo as it is megolm specific

BillCarsonFr · web-flow · commit ff328403879b · 2025-04-29T08:07:03.000Z
This patch moves the `session_id` field from EncryptionInfo to
AlgorithmInfo::MegolmV1AesSha2 as it is specific to Megolm. We provide
transparent migration of the serialized data from one format to the other.

In the future we plan to reuse `EncryptionInfo` for to_device decryption
(using olm not megolm). So megolm session_id should move to algorithm
specific data.
diff --git a/bindings/matrix-sdk-crypto-ffi/src/machine.rs b/bindings/matrix-sdk-crypto-ffi/src/machine.rs
@@ -917,21 +917,21 @@ impl OlmMachine {
         let event_json: Event<'_> = serde_json::from_str(decrypted.event.json().get())?;
 
         Ok(match &encryption_info.algorithm_info {
-            AlgorithmInfo::MegolmV1AesSha2 { curve25519_key, sender_claimed_keys } => {
-                DecryptedEvent {
-                    clear_event: serde_json::to_string(&event_json)?,
-                    sender_curve25519_key: curve25519_key.to_owned(),
-                    claimed_ed25519_key: sender_claimed_keys
-                        .get(&DeviceKeyAlgorithm::Ed25519)
-                        .cloned(),
-                    forwarding_curve25519_chain: vec![],
-                    shield_state: if strict_shields {
-                        encryption_info.verification_state.to_shield_state_strict().into()
-                    } else {
-                        encryption_info.verification_state.to_shield_state_lax().into()
-                    },
-                }
-            }
+            AlgorithmInfo::MegolmV1AesSha2 {
+                curve25519_key,
+                sender_claimed_keys,
+                session_id: _,
+            } => DecryptedEvent {
+                clear_event: serde_json::to_string(&event_json)?,
+                sender_curve25519_key: curve25519_key.to_owned(),
+                claimed_ed25519_key: sender_claimed_keys.get(&DeviceKeyAlgorithm::Ed25519).cloned(),
+                forwarding_curve25519_chain: vec![],
+                shield_state: if strict_shields {
+                    encryption_info.verification_state.to_shield_state_strict().into()
+                } else {
+                    encryption_info.verification_state.to_shield_state_lax().into()
+                },
+            },
         })
     }
 
diff --git a/crates/matrix-sdk-base/src/event_cache/store/integration_tests.rs b/crates/matrix-sdk-base/src/event_cache/store/integration_tests.rs
@@ -62,9 +62,9 @@ pub fn make_test_event_with_event_id(
         algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
             curve25519_key: "1337".to_owned(),
             sender_claimed_keys: Default::default(),
+            session_id: Some("mysessionid9".to_owned()),
         },
         verification_state: VerificationState::Verified,
-        session_id: Some("mysessionid9".to_owned()),
     };
 
     let mut builder = EventFactory::new().text_msg(content).room(room_id).sender(*ALICE);
diff --git a/crates/matrix-sdk-common/src/deserialized_responses.rs b/crates/matrix-sdk-common/src/deserialized_responses.rs
@@ -281,11 +281,16 @@ pub enum AlgorithmInfo {
         /// decrypt this session. This map will usually contain a single ed25519
         /// key.
         sender_claimed_keys: BTreeMap<DeviceKeyAlgorithm, String>,
+
+        /// The Megolm session ID that was used to encrypt this event, or None
+        /// if this info was stored before we collected this data.
+        #[serde(default, skip_serializing_if = "Option::is_none")]
+        session_id: Option<String>,
     },
 }
 
 /// Struct containing information on how an event was decrypted.
-#[derive(Clone, Debug, Deserialize, Serialize)]
+#[derive(Clone, Debug, Serialize)]
 pub struct EncryptionInfo {
     /// The user ID of the event sender, note this is untrusted data unless the
     /// `verification_state` is `Verified` as well.
@@ -302,9 +307,54 @@ pub struct EncryptionInfo {
     /// Callers that persist this should mark the state as dirty when a device
     /// change is received down the sync.
     pub verification_state: VerificationState,
-    /// The Megolm session ID that was used to encrypt this event, or None if
-    /// this info was stored before we collected this data.
-    pub session_id: Option<String>,
+}
+
+impl EncryptionInfo {
+    /// Helper to get the megolm session id used to encrypt.
+    pub fn session_id(&self) -> Option<&str> {
+        let AlgorithmInfo::MegolmV1AesSha2 { session_id, .. } = &self.algorithm_info;
+        session_id.as_deref()
+    }
+}
+
+impl<'de> Deserialize<'de> for EncryptionInfo {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        // Backwards compatibility: Capture session_id at root if exists. In legacy
+        // EncryptionInfo the session_id was not in AlgorithmInfo
+        #[derive(Deserialize)]
+        struct Helper {
+            pub sender: OwnedUserId,
+            pub sender_device: Option<OwnedDeviceId>,
+            pub algorithm_info: AlgorithmInfo,
+            pub verification_state: VerificationState,
+            #[serde(rename = "session_id")]
+            pub old_session_id: Option<String>,
+        }
+
+        let Helper {
+            sender,
+            sender_device,
+            algorithm_info:
+                AlgorithmInfo::MegolmV1AesSha2 { curve25519_key, sender_claimed_keys, session_id },
+            verification_state,
+            old_session_id,
+        } = Helper::deserialize(deserializer)?;
+
+        Ok(EncryptionInfo {
+            sender,
+            sender_device,
+            algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
+                // Migration, merge the old_session_id in algorithm_info
+                session_id: session_id.or(old_session_id),
+                curve25519_key,
+                sender_claimed_keys,
+            },
+            verification_state,
+        })
+    }
 }
 
 /// Represents a matrix room event that has been returned from `/sync`,
@@ -549,12 +599,11 @@ impl TimelineEventKind {
     pub fn session_id(&self) -> Option<&str> {
         match self {
             TimelineEventKind::Decrypted(decrypted_room_event) => {
-                decrypted_room_event.encryption_info.session_id.as_ref()
+                decrypted_room_event.encryption_info.session_id()
             }
-            TimelineEventKind::UnableToDecrypt { utd_info, .. } => utd_info.session_id.as_ref(),
+            TimelineEventKind::UnableToDecrypt { utd_info, .. } => utd_info.session_id.as_deref(),
             TimelineEventKind::PlainText { .. } => None,
         }
-        .map(String::as_str)
     }
 }
 
@@ -1056,9 +1105,9 @@ mod tests {
                     algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
                         curve25519_key: "xxx".to_owned(),
                         sender_claimed_keys: Default::default(),
+                        session_id: Some("xyz".to_owned()),
                     },
                     verification_state: VerificationState::Verified,
-                    session_id: Some("xyz".to_owned()),
                 },
                 unsigned_encryption_info: Some(BTreeMap::from([(
                     UnsignedEventLocation::RelationsReplace,
@@ -1093,11 +1142,11 @@ mod tests {
                             "algorithm_info": {
                                 "MegolmV1AesSha2": {
                                     "curve25519_key": "xxx",
-                                    "sender_claimed_keys": {}
+                                    "sender_claimed_keys": {},
+                                    "session_id": "xyz",
                                 }
                             },
                             "verification_state": "Verified",
-                            "session_id": "xyz",
                         },
                         "unsigned_encryption_info": {
                             "RelationsReplace": {"UnableToDecrypt": {
@@ -1144,9 +1193,8 @@ mod tests {
         assert_eq!(event.event_id(), Some(event_id!("$xxxxx:example.org").to_owned()));
         assert_matches!(
             event.encryption_info().unwrap().algorithm_info,
-            AlgorithmInfo::MegolmV1AesSha2 { .. }
+            AlgorithmInfo::MegolmV1AesSha2 { session_id: None, .. }
         );
-        assert_eq!(event.encryption_info().unwrap().session_id, None);
 
         // Test that the previous format, with an undecryptable unsigned event, can also
         // be deserialized.
@@ -1366,13 +1414,40 @@ mod tests {
                 (DeviceKeyAlgorithm::Curve25519, "claimedclaimedcurve25519".to_owned()),
                 (DeviceKeyAlgorithm::Ed25519, "claimedclaimeded25519".to_owned()),
             ]),
+            session_id: None,
         };
 
         with_settings!({ prepend_module_to_snapshot => false }, {
             assert_json_snapshot!(info)
         });
     }
 
+    #[test]
+    fn test_encryption_info_migration() {
+        // In the old format the session_id was in the EncryptionInfo, now
+        // it is moved to the `algorithm_info` struct.
+        let old_format = json!({
+          "sender": "@alice:localhost",
+          "sender_device": "ABCDEFGH",
+          "algorithm_info": {
+            "MegolmV1AesSha2": {
+              "curve25519_key": "curvecurvecurve",
+              "sender_claimed_keys": {}
+            }
+          },
+          "verification_state": "Verified",
+          "session_id": "mysessionid76"
+        });
+
+        let deserialized = serde_json::from_value::<EncryptionInfo>(old_format).unwrap();
+        let expected_session_id = Some("mysessionid76".to_owned());
+
+        let AlgorithmInfo::MegolmV1AesSha2 { session_id, .. } = deserialized.algorithm_info.clone();
+        assert_eq!(session_id, expected_session_id);
+
+        assert_json_snapshot!(deserialized);
+    }
+
     #[test]
     fn snapshot_test_encryption_info() {
         let info = EncryptionInfo {
@@ -1381,9 +1456,9 @@ mod tests {
             algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
                 curve25519_key: "curvecurvecurve".into(),
                 sender_claimed_keys: Default::default(),
+                session_id: Some("mysessionid76".to_owned()),
             },
             verification_state: VerificationState::Verified,
-            session_id: Some("mysessionid76".to_owned()),
         };
 
         with_settings!({ sort_maps => true, prepend_module_to_snapshot => false }, {
@@ -1411,9 +1486,9 @@ mod tests {
                                 "qzdW3F5IMPFl0HQgz5w/L5Oi/npKUFn8Um84acIHfPY".to_owned(),
                             ),
                         ]),
+                        session_id: Some("mysessionid112".to_owned()),
                     },
                     verification_state: VerificationState::Verified,
-                    session_id: Some("mysessionid112".to_owned()),
                 },
                 unsigned_encryption_info: Some(BTreeMap::from([(
                     UnsignedEventLocation::RelationsThreadLatestEvent,
diff --git a/crates/matrix-sdk-common/src/snapshots/matrix_sdk_common__deserialized_responses__tests__encryption_info_migration.snap b/crates/matrix-sdk-common/src/snapshots/matrix_sdk_common__deserialized_responses__tests__encryption_info_migration.snap
@@ -0,0 +1,16 @@
+---
+source: crates/matrix-sdk-common/src/deserialized_responses.rs
+expression: deserialized
+---
+{
+  "sender": "@alice:localhost",
+  "sender_device": "ABCDEFGH",
+  "algorithm_info": {
+    "MegolmV1AesSha2": {
+      "curve25519_key": "curvecurvecurve",
+      "sender_claimed_keys": {},
+      "session_id": "mysessionid76"
+    }
+  },
+  "verification_state": "Verified"
+}
diff --git a/crates/matrix-sdk-common/src/snapshots/snapshot_test_encryption_info.snap b/crates/matrix-sdk-common/src/snapshots/snapshot_test_encryption_info.snap
@@ -1,6 +1,5 @@
 ---
 source: crates/matrix-sdk-common/src/deserialized_responses.rs
-assertion_line: 1360
 expression: info
 ---
 {
@@ -9,9 +8,9 @@ expression: info
   "algorithm_info": {
     "MegolmV1AesSha2": {
       "curve25519_key": "curvecurvecurve",
-      "sender_claimed_keys": {}
+      "sender_claimed_keys": {},
+      "session_id": "mysessionid76"
     }
   },
-  "verification_state": "Verified",
-  "session_id": "mysessionid76"
+  "verification_state": "Verified"
 }
diff --git a/crates/matrix-sdk-common/src/snapshots/snapshot_test_sync_timeline_event.snap b/crates/matrix-sdk-common/src/snapshots/snapshot_test_sync_timeline_event.snap
@@ -12,12 +12,12 @@ expression: "serde_json::to_value(&room_event).unwrap()"
             "sender_claimed_keys": {
               "curve25519": "qzdW3F5IMPFl0HQgz5w/L5Oi/npKUFn8Um84acIHfPY",
               "ed25519": "I3YsPwqMZQXHkSQbjFNEs7b529uac2xBpI83eN3LUXo"
-            }
+            },
+            "session_id": "mysessionid112"
           }
         },
         "sender": "@sender:example.com",
         "sender_device": "ABCDEFGHIJ",
-        "session_id": "mysessionid112",
         "verification_state": "Verified"
       },
       "event": {
diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md
@@ -8,6 +8,9 @@ All notable changes to this project will be documented in this file.
 
 ### Features
 
+- [**breaking**] Move `session_id` from `EncryptionInfo` to `AlgorithmInfo` as it is megolm specific. 
+  Use `EncryptionInfo::session_id()` helper for quick access.
+
 - Send stable identifier `sender_device_keys` for MSC4147 (Including device
   keys with Olm-encrypted events).
   ([#4964](https://github.com/matrix-org/matrix-rust-sdk/pull/4964))
diff --git a/crates/matrix-sdk-crypto/src/machine/mod.rs b/crates/matrix-sdk-crypto/src/machine/mod.rs
@@ -1720,9 +1720,9 @@ impl OlmMachine {
                     .iter()
                     .map(|(k, v)| (k.to_owned(), v.to_base64()))
                     .collect(),
+                session_id: Some(session.session_id().to_owned()),
             },
             verification_state,
-            session_id: Some(session.session_id().to_owned()),
         })
     }
 
diff --git a/crates/matrix-sdk-ui/src/timeline/controller/decryption_retry_task.rs b/crates/matrix-sdk-ui/src/timeline/controller/decryption_retry_task.rs
@@ -174,10 +174,7 @@ fn compute_event_indices_to_retry_decryption(
         } else {
             // Non-UTDs only have a session ID if they are remote and have it in the
             // EncryptionInfo
-            event
-                .as_remote()
-                .and_then(|remote| remote.encryption_info.as_ref()?.session_id.as_ref())
-                .map(String::as_str)
+            event.as_remote().and_then(|remote| remote.encryption_info.as_ref()?.session_id())
         };
 
         if let Some(session_id) = session_id {
@@ -232,7 +229,7 @@ async fn make_replacement_for<P: RoomDataProvider>(
     let item = item?;
     let event = item.as_event()?;
     let remote = event.as_remote()?;
-    let session_id = remote.encryption_info.as_ref()?.session_id.as_deref()?;
+    let session_id = remote.encryption_info.as_ref()?.session_id()?;
 
     let new_encryption_info =
         room_data_provider.get_encryption_info(session_id, &event.sender).await;
@@ -515,9 +512,9 @@ mod tests {
                 algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
                     curve25519_key: "".to_owned(),
                     sender_claimed_keys: BTreeMap::new(),
+                    session_id: Some(session_id.to_owned()),
                 },
                 verification_state: VerificationState::Verified,
-                session_id: Some(session_id.to_owned()),
             }),
             original_json: None,
             latest_edit_json: None,
diff --git a/crates/matrix-sdk-ui/src/timeline/tests/edit.rs b/crates/matrix-sdk-ui/src/timeline/tests/edit.rs
@@ -173,9 +173,9 @@ async fn test_edit_updates_encryption_info() {
         algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
             curve25519_key: "123".to_owned(),
             sender_claimed_keys: BTreeMap::new(),
+            session_id: Some("mysessionid6333".to_owned()),
         },
         verification_state: VerificationState::Verified,
-        session_id: Some("mysessionid6333".to_owned()),
     };
 
     let original_event: TimelineEvent = DecryptedRoomEvent {
diff --git a/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs b/crates/matrix-sdk-ui/src/timeline/tests/encryption.rs
@@ -697,9 +697,9 @@ fn make_encryption_info(session_id: &str, verification_state: VerificationState)
         algorithm_info: AlgorithmInfo::MegolmV1AesSha2 {
             curve25519_key: Default::default(),
             sender_claimed_keys: Default::default(),
+            session_id: Some(session_id.to_owned()),
         },
         verification_state,
-        session_id: Some(session_id.to_owned()),
     }
 }
 

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,5 @@`
`1`	`1`	`---`
`2`	`2`	`source: crates/matrix-sdk-common/src/deserialized_responses.rs`
`3`		`-assertion_line: 1360`
`4`	`3`	`expression: info`
`5`	`4`	`---`
`6`	`5`	`{`
`@@ -9,9 +8,9 @@ expression: info`
`9`	`8`	`"algorithm_info": {`
`10`	`9`	`"MegolmV1AesSha2": {`
`11`	`10`	`"curve25519_key": "curvecurvecurve",`
`12`		`- "sender_claimed_keys": {}`
	`11`	`+ "sender_claimed_keys": {},`
	`12`	`+ "session_id": "mysessionid76"`
`13`	`13`	`}`
`14`	`14`	`},`
`15`		`- "verification_state": "Verified",`
`16`		`- "session_id": "mysessionid76"`
	`15`	`+ "verification_state": "Verified"`
`17`	`16`	`}`