Merge branch 'fix-verification-double-start'

Author: poljar

crates/matrix-sdk-crypto/src/verification/requests.rs

@@ -44,7 +44,7 @@ use super::{
     event_enums::{
         CancelContent, DoneContent, OutgoingContent, ReadyContent, RequestContent, StartContent,
     },
-    CancelInfo, Cancelled, FlowId, VerificationStore,
+    CancelInfo, Cancelled, FlowId, Verification, VerificationStore,
 };
 #[cfg(feature = "qrcode")]
 use super::{
@@ -1126,11 +1126,10 @@ impl RequestState<Ready> {
         };
 
         let identity = self.store.get_user_identity(sender).await?;
-        let own_identity = self
-            .store
-            .get_user_identity(self.store.account.user_id())
-            .await?
-            .and_then(|i| i.into_own());
+        let own_user_id = self.store.account.user_id();
+        let own_device_id = self.store.account.device_id();
+        let own_identity =
+            self.store.get_user_identity(own_user_id).await?.and_then(|i| i.into_own());
 
         match content.method() {
             StartMethod::SasV1(_) => {
@@ -1142,13 +1141,27 @@ impl RequestState<Ready> {
                     we_started,
                     request_handle,
                 ) {
-                    // TODO check if there is already a SAS verification, i.e. we
-                    // already started one before the other side tried to do the
-                    // same; ignore it if we did and we're the lexicographically
-                    // smaller user ID, otherwise auto-accept the newly started one.
                     Ok(s) => {
-                        info!("Started a new SAS verification.");
-                        self.verification_cache.insert_sas(s);
+                        let start_new = if let Some(Verification::SasV1(_sas)) =
+                            self.verification_cache.get(sender, self.flow_id.as_str())
+                        {
+                            // If there is already a SAS verification, i.e. we already started one
+                            // before the other side tried to do the same; ignore it if we did and
+                            // we're the lexicographically smaller user ID (or device ID if equal).
+                            use std::cmp::Ordering;
+                            match (sender.cmp(own_user_id), device.device_id().cmp(own_device_id)) {
+                                (Ordering::Greater, _) | (Ordering::Equal, Ordering::Greater) => {
+                                    false
+                                }
+                                _ => true,
+                            }
+                        } else {
+                            true
+                        };
+                        if start_new {
+                            info!("Started a new SAS verification.");
+                            self.verification_cache.insert_sas(s);
+                        }
                     }
                     Err(c) => {
                         warn!(

crates/matrix-sdk-crypto/src/verification/sas/inner_sas.rs

@@ -181,7 +181,7 @@ impl InnerSas {
         methods: Vec<ShortAuthenticationString>,
     ) -> Option<(InnerSas, OwnedAcceptContent)> {
         if let InnerSas::Started(s) = self {
-            let sas = s.into_accepted(methods);
+            let sas = s.into_we_accepted(methods);
             let content = sas.as_content();
             Some((InnerSas::WeAccepted(sas), content))
         } else {
@@ -258,22 +258,29 @@ impl InnerSas {
         content: &AnyVerificationContent,
     ) -> (Self, Option<OutgoingContent>) {
         match content {
-            AnyVerificationContent::Accept(c) => {
-                if let InnerSas::Created(s) = self {
-                    match s.into_accepted(sender, c) {
-                        Ok(s) => {
-                            let content = s.as_content();
-                            (InnerSas::Accepted(s), Some(content))
-                        }
-                        Err(s) => {
-                            let content = s.as_content();
-                            (InnerSas::Cancelled(s), Some(content))
-                        }
+            AnyVerificationContent::Accept(c) => match self {
+                InnerSas::Created(s) => match s.into_accepted(sender, c) {
+                    Ok(s) => {
+                        let content = s.as_content();
+                        (InnerSas::Accepted(s), Some(content))
                     }
-                } else {
-                    (self, None)
-                }
-            }
+                    Err(s) => {
+                        let content = s.as_content();
+                        (InnerSas::Cancelled(s), Some(content))
+                    }
+                },
+                InnerSas::Started(s) => match s.into_accepted(sender, c) {
+                    Ok(s) => {
+                        let content = s.as_content();
+                        (InnerSas::Accepted(s), Some(content))
+                    }
+                    Err(s) => {
+                        let content = s.as_content();
+                        (InnerSas::Cancelled(s), Some(content))
+                    }
+                },
+                _ => (self, None),
+            },
             AnyVerificationContent::Cancel(c) => {
                 let (sas, _) = self.cancel(false, c.cancel_code().to_owned());
                 (sas, None)

crates/matrix-sdk-crypto/src/verification/sas/sas_state.rs

@@ -70,6 +70,17 @@ const MACS: &[MessageAuthenticationCode] = &[MessageAuthenticationCode::HkdfHmac
 const STRINGS: &[ShortAuthenticationString] =
     &[ShortAuthenticationString::Decimal, ShortAuthenticationString::Emoji];
 
+fn the_protocol_definitions() -> SasV1Content {
+    SasV1ContentInit {
+        short_authentication_string: STRINGS.to_vec(),
+        key_agreement_protocols: KEY_AGREEMENT_PROTOCOLS.to_vec(),
+        message_authentication_codes: MACS.to_vec(),
+        hashes: HASHES.to_vec(),
+    }
+    .try_into()
+    .expect("Invalid protocol definition.")
+}
+
 // The max time a SAS flow can take from start to done.
 const MAX_AGE: Duration = Duration::from_secs(60 * 5);
 
@@ -420,16 +431,7 @@ impl SasState<Created> {
             last_event_time: Arc::new(Instant::now()),
             started_from_request,
 
-            state: Arc::new(Created {
-                protocol_definitions: SasV1ContentInit {
-                    short_authentication_string: STRINGS.to_vec(),
-                    key_agreement_protocols: KEY_AGREEMENT_PROTOCOLS.to_vec(),
-                    message_authentication_codes: MACS.to_vec(),
-                    hashes: HASHES.to_vec(),
-                }
-                .try_into()
-                .expect("Invalid protocol definition."),
-            }),
+            state: Arc::new(Created { protocol_definitions: the_protocol_definitions() }),
         }
     }
 
@@ -571,7 +573,7 @@ impl SasState<Started> {
         }
     }
 
-    pub fn into_accepted(self, methods: Vec<ShortAuthenticationString>) -> SasState<WeAccepted> {
+    pub fn into_we_accepted(self, methods: Vec<ShortAuthenticationString>) -> SasState<WeAccepted> {
         let mut accepted_protocols = self.state.accepted_protocols.as_ref().to_owned();
         accepted_protocols.short_auth_string = methods;
 
@@ -594,6 +596,69 @@ impl SasState<Started> {
             }),
         }
     }
+
+    fn as_content(&self) -> OwnedStartContent {
+        match self.verification_flow_id.as_ref() {
+            FlowId::ToDevice(s) => {
+                OwnedStartContent::ToDevice(ToDeviceKeyVerificationStartEventContent::new(
+                    self.device_id().into(),
+                    s.to_string(),
+                    StartMethod::SasV1(the_protocol_definitions()),
+                ))
+            }
+            FlowId::InRoom(r, e) => OwnedStartContent::Room(
+                r.clone(),
+                KeyVerificationStartEventContent::new(
+                    self.device_id().into(),
+                    StartMethod::SasV1(the_protocol_definitions()),
+                    Relation::new(e.clone()),
+                ),
+            ),
+        }
+    }
+
+    /// Receive a m.key.verification.accept event, changing the state into
+    /// an Accepted one.
+    ///
+    /// Note: Even though the other side has started the (or rather "a") sas
+    /// verification, it can still accept one, if we have sent one
+    /// simultaneously. In this case we just go on with the verification
+    /// that *we* started.
+    ///
+    /// # Arguments
+    ///
+    /// * `event` - The m.key.verification.accept event that was sent to us by
+    /// the other side.
+    pub fn into_accepted(
+        self,
+        sender: &UserId,
+        content: &AcceptContent,
+    ) -> Result<SasState<Accepted>, SasState<Cancelled>> {
+        self.check_event(sender, content.flow_id()).map_err(|c| self.clone().cancel(true, c))?;
+
+        if let AcceptMethod::SasV1(content) = content.method() {
+            let accepted_protocols = AcceptedProtocols::try_from(content.clone())
+                .map_err(|c| self.clone().cancel(true, c))?;
+
+            let start_content = self.as_content().into();
+
+            Ok(SasState {
+                inner: self.inner,
+                ids: self.ids,
+                verification_flow_id: self.verification_flow_id,
+                creation_time: self.creation_time,
+                last_event_time: Instant::now().into(),
+                started_from_request: self.started_from_request,
+                state: Arc::new(Accepted {
+                    start_content,
+                    commitment: content.commitment.clone(),
+                    accepted_protocols: accepted_protocols.into(),
+                }),
+            })
+        } else {
+            Err(self.cancel(true, CancelCode::UnknownMethod))
+        }
+    }
 }
 
 impl SasState<WeAccepted> {
@@ -1191,7 +1256,7 @@ mod test {
             &start_content.as_start_content(),
             false,
         );
-        let bob_sas = bob_sas.unwrap().into_accepted(vec![ShortAuthenticationString::Emoji]);
+        let bob_sas = bob_sas.unwrap().into_we_accepted(vec![ShortAuthenticationString::Emoji]);
 
         (alice_sas, bob_sas)
     }