diff --git a/src/securejoin/bobstate.rs b/src/securejoin/bobstate.rs index fc266fcc2..c26820e3a 100644 --- a/src/securejoin/bobstate.rs +++ b/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, 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>) -> Option { - match *guard { - Some(_) => Some(Self { + pub fn from_guard(mut guard: MutexGuard<'a, Option>) -> Option { + match guard.take() { + 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 { - Some(ref bobstate) => bobstate.chat_id, - None => unreachable!(), - } + self.bobstate.chat_id } /// 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 { - Some(ref bobstate) => &bobstate.invite, - None => unreachable!(), - } + &self.bobstate.invite } /// Handles the given message for the securejoin handshake for Bob. @@ -105,24 +91,23 @@ impl<'a> BobStateHandle<'a> { mime_message: &MimeMessage, ) -> Option { info!(context, "Handling securejoin message for BobStateHandle"); - match *self.guard { - Some(ref mut bobstate) => match bobstate.handle_message(context, mime_message).await { - Ok(Some(stage)) => { - if matches!(stage, - BobHandshakeStage::Completed - | BobHandshakeStage::Terminated(_)) - { - self.finish_protocol(context).await; - } - Some(stage) - } - Ok(None) => None, - Err(_) => { + match self.bobstate.handle_message(context, mime_message).await { + Ok(Some(stage)) => { + if matches!(stage, BobHandshakeStage::Completed | BobHandshakeStage::Terminated(_)) + { self.finish_protocol(context).await; - None } - }, - None => None, // also unreachable!() + Some(stage) + } + 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 { .. } = bobstate.invite { - context.stop_ongoing().await; - } + if let QrInvite::Group { .. } = self.bobstate.invite { + 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 -/// state and explicitly avoid doing performing any user interactions required by -/// securejoin. This simplifies the concerns and logic required in both the callers and in -/// the state management. The return values can be used to understand what user -/// interactions need to happen. +/// securejoin handshake for Bob. The methods only concern themselves with the protocol +/// state and explicitly avoid performing any user interactions required by securejoin. +/// This simplifies the concerns and logic required in both the callers and in the state +/// management. The return values can be used to understand what user interactions need to +/// 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. /// diff --git a/src/securejoin/mod.rs b/src/securejoin/mod.rs index 47e5af638..6c73ebe2e 100644 --- a/src/securejoin/mod.rs +++ b/src/securejoin/mod.rs @@ -83,13 +83,15 @@ impl Bob { if guard.is_some() { return Err(JoinError::AlreadyRunning); } - let mut did_alloc_ongoing = false; - if let QrInvite::Group { .. } = invite { - if context.alloc_ongoing().await.is_err() { - return Err(JoinError::OngoingRunning); + let did_alloc_ongoing = match invite { + QrInvite::Group { .. } => { + if context.alloc_ongoing().await.is_err() { + 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 { @@ -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), diff --git a/src/securejoin/qrinvite.rs b/src/securejoin/qrinvite.rs index 268d9215a..8c1fe6316 100644 --- a/src/securejoin/qrinvite.rs +++ b/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 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, } diff --git a/src/test_utils.rs b/src/test_utils.rs index 467d7872e..8041903b2 100644 --- a/src/test_utils.rs +++ b/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 {