crypto: replace uses of compare_group_session

... with `merge_received_group_session`.

`merge_received_group_session` expands the logic of `compare_group_session` to
handle the fact that there is more than one axis of "better" or "worse" and we
may need to take the best bits of two copies of the session.

Author: richvdh

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -8,6 +8,8 @@ All notable changes to this project will be documented in this file.
 
 ### Features
 
+- When we receive an inbound Megolm session from two different sources, merge the two copies together to get the best of both.
+  ([#5865](https://github.com/matrix-org/matrix-rust-sdk/pull/5865)
 - When constructing a key bundle for history sharing, if we had received a key bundle ourselves, in which one or more sessions was marked as "history not shared", pass that on to the new user.
   ([#5820](https://github.com/matrix-org/matrix-rust-sdk/pull/5820)
 - Expose new method `CryptoStore::get_withheld_sessions_by_room_id`.

crates/matrix-sdk-crypto/src/gossiping/machine.rs

@@ -39,7 +39,7 @@ use ruma::{
     TransactionId, UserId,
 };
 use tracing::{debug, field::debug, info, instrument, trace, warn, Span};
-use vodozemac::{megolm::SessionOrdering, Curve25519PublicKey};
+use vodozemac::Curve25519PublicKey;
 
 use super::{GossipRequest, GossippedSecret, RequestEvent, RequestInfo, SecretInfo, WaitQueue};
 use crate::{
@@ -964,6 +964,7 @@ impl GossipMachine {
         Ok(name)
     }
 
+    #[tracing::instrument(skip_all)]
     async fn accept_forwarded_room_key(
         &self,
         info: &GossipRequest,
@@ -972,33 +973,11 @@ impl GossipMachine {
     ) -> Result<Option<InboundGroupSession>, CryptoStoreError> {
         match InboundGroupSession::try_from(event) {
             Ok(session) => {
-                if self.inner.store.compare_group_session(&session).await?
-                    == SessionOrdering::Better
-                {
+                let new_session = self.inner.store.merge_received_group_session(session).await?;
+                if new_session.is_some() {
                     self.mark_as_done(info).await?;
-
-                    info!(
-                        ?sender_key,
-                        claimed_sender_key = ?session.sender_key(),
-                        room_id = ?session.room_id(),
-                        session_id = session.session_id(),
-                        algorithm = ?session.algorithm(),
-                        "Received a forwarded room key",
-                    );
-
-                    Ok(Some(session))
-                } else {
-                    info!(
-                        ?sender_key,
-                        claimed_sender_key = ?session.sender_key(),
-                        room_id = ?session.room_id(),
-                        session_id = session.session_id(),
-                        algorithm = ?session.algorithm(),
-                        "Received a forwarded room key but we already have a better version of it",
-                    );
-
-                    Ok(None)
                 }
+                Ok(new_session)
             }
             Err(e) => {
                 warn!(?sender_key, "Couldn't create a group session from a received room key");

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

@@ -62,10 +62,7 @@ use tracing::{
     field::{debug, display},
     info, instrument, trace, warn, Span,
 };
-use vodozemac::{
-    megolm::{DecryptionError, SessionOrdering},
-    Curve25519PublicKey, Ed25519Signature,
-};
+use vodozemac::{megolm::DecryptionError, Curve25519PublicKey, Ed25519Signature};
 
 #[cfg(feature = "experimental-send-custom-to-device")]
 use crate::session_manager::split_devices_for_share_strategy;
@@ -916,25 +913,9 @@ impl OlmMachine {
                 let sender_data =
                     SenderDataFinder::find_using_event(self.store(), sender_key, event, &session)
                         .await?;
-
                 session.sender_data = sender_data;
 
-                match self.store().compare_group_session(&session).await? {
-                    SessionOrdering::Better => {
-                        info!("Received a new megolm room key");
-
-                        Ok(Some(session))
-                    }
-                    comparison_result => {
-                        warn!(
-                            ?comparison_result,
-                            "Received a megolm room key that we already have a better version \
-                             of, discarding"
-                        );
-
-                        Ok(None)
-                    }
-                }
+                Ok(self.store().merge_received_group_session(session).await?)
             }
             Err(e) => {
                 Span::current().record("session_id", &content.session_id);

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

@@ -634,29 +634,6 @@ impl Store {
         self.inner.store.save_changes(changes).await
     }
 
-    /// Compare the given `InboundGroupSession` with an existing session we have
-    /// in the store.
-    ///
-    /// This method returns `SessionOrdering::Better` if the given session is
-    /// better than the one we already have or if we don't have such a
-    /// session in the store.
-    pub(crate) async fn compare_group_session(
-        &self,
-        session: &InboundGroupSession,
-    ) -> Result<SessionOrdering> {
-        let old_session = self
-            .inner
-            .store
-            .get_inbound_group_session(session.room_id(), session.session_id())
-            .await?;
-
-        Ok(if let Some(old_session) = old_session {
-            session.compare(&old_session).await
-        } else {
-            SessionOrdering::Better
-        })
-    }
-
     /// Given an `InboundGroupSession` which we have just received, see if we
     /// have a matching session already in the store, and determine how to
     /// handle it.
@@ -1526,43 +1503,26 @@ impl Store {
     {
         let mut sessions = Vec::new();
 
-        async fn new_session_better(
-            session: &InboundGroupSession,
-            old_session: Option<InboundGroupSession>,
-        ) -> bool {
-            if let Some(old_session) = &old_session {
-                session.compare(old_session).await == SessionOrdering::Better
-            } else {
-                true
-            }
-        }
-
         let total_count = room_keys.len();
         let mut keys = BTreeMap::new();
 
         for (i, key) in room_keys.into_iter().enumerate() {
             match key.try_into() {
                 Ok(session) => {
-                    let old_session = self
-                        .inner
-                        .store
-                        .get_inbound_group_session(session.room_id(), session.session_id())
-                        .await?;
-
                     // Only import the session if we didn't have this session or
                     // if it's a better version of the same session.
-                    if new_session_better(&session, old_session).await {
+                    if let Some(merged) = self.merge_received_group_session(session).await? {
                         if from_backup_version.is_some() {
-                            session.mark_as_backed_up();
+                            merged.mark_as_backed_up();
                         }
 
-                        keys.entry(session.room_id().to_owned())
+                        keys.entry(merged.room_id().to_owned())
                             .or_insert_with(BTreeMap::new)
-                            .entry(session.sender_key().to_base64())
+                            .entry(merged.sender_key().to_base64())
                             .or_insert_with(BTreeSet::new)
-                            .insert(session.session_id().to_owned());
+                            .insert(merged.session_id().to_owned());
 
-                        sessions.push(session);
+                        sessions.push(merged);
                     }
                 }
                 Err(e) => {