PR review feedback

- doc fixes
- make BobStateHandle safer by moving the state out of the handle.
- handle more match cases explicit in BobState returns
- fewer mutable variables

src/securejoin/bobstate.rs

@@ -2,7 +2,7 @@
 //!
 //! This module contains the state machine to run the Secure-Join handshake for Bob and does
 //! not do any user interaction required by the protocol.  Instead the state machine
-//! provides all the information to the driver of them to perform the correct interactions.
+//! provides all the information to its driver so it can perform the correct interactions.
 //!
 //! The [`BobState`] is only directly used to initially create it when starting the
 //! protocol.  Afterwards it must be stored in a mutex and the [`BobStateHandle`] should be
@@ -53,15 +53,17 @@ pub enum BobHandshakeStage {
 /// to return the lock.
 pub struct BobStateHandle<'a> {
     guard: MutexGuard<'a, Option<BobState>>,
+    bobstate: BobState,
     clear_state_on_drop: bool,
 }
 
 impl<'a> BobStateHandle<'a> {
     /// Creates a new instance, upholding the guarantee that [`BobState`] must exist.
-    pub fn from_guard(guard: MutexGuard<'a, Option<BobState>>) -> Option<Self> {
+    pub fn from_guard(mut guard: MutexGuard<'a, Option<BobState>>) -> Option<Self> {
-        match *guard {
+        match guard.take() {
-            Some(_) => Some(Self {
+            Some(bobstate) => Some(Self {
                 guard,
+                bobstate,
                 clear_state_on_drop: false,
             }),
             None => None,
@@ -69,29 +71,13 @@ impl<'a> BobStateHandle<'a> {
     }
 
     /// Returns the [`ChatId`] of the 1:1 chat with the inviter (Alice).
-    ///
-    /// # Safety
-    ///
-    /// [`BobStateHandle::from_guard`] guarantees this struct is only created with a valid
-    /// state available.
     pub fn chat_id(&self) -> ChatId {
-        match *self.guard {
+        self.bobstate.chat_id
-            Some(ref bobstate) => bobstate.chat_id,
-            None => unreachable!(),
-        }
     }
 
     /// Returns a reference to the [`QrInvite`] of the joiner process.
-    ///
-    /// # Safety
-    ///
-    /// [`BobStateHandle::from_guard`] guarantees this struct is only created with a valid
-    /// state available.
     pub fn invite(&self) -> &QrInvite {
-        match *self.guard {
+        &self.bobstate.invite
-            Some(ref bobstate) => &bobstate.invite,
-            None => unreachable!(),
-        }
     }
 
     /// Handles the given message for the securejoin handshake for Bob.
@@ -105,24 +91,23 @@ impl<'a> BobStateHandle<'a> {
         mime_message: &MimeMessage,
     ) -> Option<BobHandshakeStage> {
         info!(context, "Handling securejoin message for BobStateHandle");
-        match *self.guard {
+        match self.bobstate.handle_message(context, mime_message).await {
-            Some(ref mut bobstate) => match bobstate.handle_message(context, mime_message).await {
+            Ok(Some(stage)) => {
-                Ok(Some(stage)) => {
+                if matches!(stage, BobHandshakeStage::Completed | BobHandshakeStage::Terminated(_))
-                    if matches!(stage,
+                {
-                                BobHandshakeStage::Completed
-                                | BobHandshakeStage::Terminated(_))
-                    {
-                        self.finish_protocol(context).await;
-                    }
-                    Some(stage)
-                }
-                Ok(None) => None,
-                Err(_) => {
                     self.finish_protocol(context).await;
-                    None
                 }
-            },
+                Some(stage)
-            None => None, // also unreachable!()
+            }
+            Ok(None) => None,
+            Err(err) => {
+                warn!(
+                    context,
+                    "Error handling handshake message, aborting handshake: {}", err
+                );
+                self.finish_protocol(context).await;
+                None
+            }
         }
     }
 
@@ -132,7 +117,7 @@ impl<'a> BobStateHandle<'a> {
     /// allowing a new handshake to be started from [`Bob`].
     ///
     /// Note that the state is only cleared on Drop since otherwise the invariant that the
-    /// state is always cosistent is violated.  However the "ongoing" prococess is released
+    /// state is always consistent is violated.  However the "ongoing" prococess is released
     /// here a little bit earlier as this requires access to the Context, which we do not
     /// have on Drop (Drop can not run asynchronous code).
     ///
@@ -141,10 +126,8 @@ impl<'a> BobStateHandle<'a> {
     async fn finish_protocol(&mut self, context: &Context) {
         info!(context, "Finishing securejoin handshake protocol for Bob");
         self.clear_state_on_drop = true;
-        if let Some(ref bobstate) = *self.guard {
+        if let QrInvite::Group { .. } = self.bobstate.invite {
-            if let QrInvite::Group { .. } = bobstate.invite {
+            context.stop_ongoing().await;
-                context.stop_ongoing().await;
-            }
         }
     }
 }
@@ -153,6 +136,10 @@ impl<'a> Drop for BobStateHandle<'a> {
     fn drop(&mut self) {
         if self.clear_state_on_drop {
             self.guard.take();
+        } else {
+            // Make sure to put back the BobState into the Option of the Mutex, it was taken
+            // out by the constructor.
+            self.guard.replace(self.bobstate.clone());
         }
     }
 }
@@ -165,18 +152,18 @@ impl<'a> Drop for BobStateHandle<'a> {
 /// This purposefully has nothing optional, the state is always fully valid.  See
 /// [`Bob::state`] to get access to this state.
 ///
-/// # Conducing the securejoin handshake
+/// # Conducting the securejoin handshake
 ///
 /// The methods on this struct allow you to interact with the state and thus conduct the
-/// securejoin handshake for Bob.  The methods **only concern themselves** with the protocol
+/// securejoin handshake for Bob.  The methods only concern themselves with the protocol
-/// state and explicitly avoid doing performing any user interactions required by
+/// state and explicitly avoid performing any user interactions required by securejoin.
-/// securejoin.  This simplifies the concerns and logic required in both the callers and in
+/// This simplifies the concerns and logic required in both the callers and in the state
-/// the state management.  The return values can be used to understand what user
+/// management.  The return values can be used to understand what user interactions need to
-/// interactions need to happen.
+/// happen.
 ///
 /// [`Bob`]: super::Bob
 /// [`Bob::state`]: super::Bob::state
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct BobState {
     /// The QR Invite code.
     invite: QrInvite,
@@ -223,7 +210,7 @@ impl BobState {
         }
     }
 
-    /// Returns the [`QrInvite`] use to create this [`BobState`].
+    /// Returns the [`QrInvite`] used to create this [`BobState`].
     pub fn invite(&self) -> &QrInvite {
         &self.invite
     }
@@ -489,7 +476,7 @@ impl BobHandshakeMsg {
 }
 
 /// The next message expected by [`BobState`] in the setup-contact/secure-join protocol.
-#[derive(Debug, PartialEq)]
+#[derive(Debug, Clone, PartialEq)]
 enum SecureJoinStep {
     /// Expecting the auth-required message.
     ///

src/securejoin/mod.rs

@@ -83,13 +83,15 @@ impl Bob {
         if guard.is_some() {
             return Err(JoinError::AlreadyRunning);
         }
-        let mut did_alloc_ongoing = false;
+        let did_alloc_ongoing = match invite {
-        if let QrInvite::Group { .. } = invite {
+            QrInvite::Group { .. } => {
-            if context.alloc_ongoing().await.is_err() {
+                if context.alloc_ongoing().await.is_err() {
-                return Err(JoinError::OngoingRunning);
+                    return Err(JoinError::OngoingRunning);
+                }
+                true
             }
-            did_alloc_ongoing = true;
+            _ => false,
-        }
+        };
         match BobState::start_protocol(context, invite).await {
             Ok((state, stage)) => {
                 if matches!(stage, BobHandshakeStage::RequestWithAuthSent) {
@@ -235,7 +237,7 @@ pub enum JoinError {
 /// This is the start of the process for the joiner.  See the module and ffi documentation
 /// for more details.
 ///
-/// When **joining a group** this will start an "ongoing" process and will block until the
+/// When joining a group this will start an "ongoing" process and will block until the
 /// process is completed, the [`ChatId`] for the new group is not known any sooner.  When
 /// verifying a contact this returns immediately.
 pub async fn dc_join_securejoin(context: &Context, qr: &str) -> Result<ChatId, JoinError> {
@@ -657,11 +659,18 @@ pub(crate) async fn handle_securejoin_handshake(
                             .await;
                         Ok(HandshakeMessage::Done)
                     }
-                    Some(_stage) => {
+                    Some(BobHandshakeStage::Completed) => {
                         // Can only be BobHandshakeStage::Completed
                         secure_connection_established(context, bobstate.chat_id()).await;
                         Ok(retval)
                     }
+                    Some(_) => {
+                        warn!(
+                            context,
+                            "Impossible state returned from handling handshake message"
+                        );
+                        Ok(retval)
+                    }
                     None => Ok(retval),
                 },
                 None => Ok(retval),

src/securejoin/qrinvite.rs

@@ -2,7 +2,7 @@
 //!
 //! QR-codes are decoded into a more general-purpose [`Lot`] struct normally, this struct is
 //! so general it is not even specific to QR-codes.  This makes working with it rather hard,
-//! so here we have a wrapper type that specifically deals weith Secure-Join QR-codes so
+//! so here we have a wrapper type that specifically deals with Secure-Join QR-codes so
 //! that the Secure-Join code can have many more guarantees when dealing with this.
 
 use std::convert::TryFrom;
@@ -37,7 +37,7 @@ impl QrInvite {
     /// The contact ID of the inviter.
     ///
     /// The actual QR-code contains a URL-encoded email address, but upon scanning this is
-    /// currently translated to a contact ID.
+    /// translated to a contact ID.
     pub fn contact_id(&self) -> u32 {
         match self {
             Self::Contact { contact_id, .. } | Self::Group { contact_id, .. } => *contact_id,
@@ -73,6 +73,9 @@ impl TryFrom<Lot> for QrInvite {
         if lot.state != LotState::QrAskVerifyContact && lot.state != LotState::QrAskVerifyGroup {
             return Err(QrError::UnsupportedProtocol);
         }
+        if lot.id == 0 {
+            return Err(QrError::MissingContactId);
+        }
         let fingerprint = lot.fingerprint.ok_or(QrError::MissingFingerprint)?;
         let invitenumber = lot.invitenumber.ok_or(QrError::MissingInviteNumber)?;
         let authcode = lot.auth.ok_or(QrError::MissingAuthCode)?;
@@ -112,4 +115,6 @@ pub enum QrError {
     MissingGroupName,
     #[error("Missing group id")]
     MissingGroupId,
+    #[error("Missing contact id")]
+    MissingContactId,
 }

src/test_utils.rs

@@ -116,9 +116,9 @@ impl TestContext {
         }
     }
 
-    /// Creates a new configured [TestContext].
+    /// Creates a new configured [`TestContext`].
     ///
-    /// This is a shortcut which automatically calls [TestContext::configure_alice] after
+    /// This is a shortcut which automatically calls [`TestContext::configure_alice`] after
     /// creating the context.
     pub async fn new_alice() -> Self {
         let t = Self::with_name("alice").await;
@@ -126,7 +126,7 @@ impl TestContext {
         t
     }
 
-    /// Creates a new configured [TestContext].
+    /// Creates a new configured [`TestContext`].
     ///
     /// This is a shortcut which configures bob@example.net with a fixed key.
     pub async fn new_bob() -> Self {