diff --git a/deltachat-jsonrpc/src/api/types/calls.rs b/deltachat-jsonrpc/src/api/types/calls.rs index 0c555dc2e..e779f1c89 100644 --- a/deltachat-jsonrpc/src/api/types/calls.rs +++ b/deltachat-jsonrpc/src/api/types/calls.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Context as _, Result}; use deltachat::calls::{call_state, sdp_has_video, CallState}; use deltachat::context::Context; @@ -26,7 +26,9 @@ pub struct JsonrpcCallInfo { impl JsonrpcCallInfo { pub async fn from_msg_id(context: &Context, msg_id: MsgId) -> Result { - let call_info = context.load_call_by_id(msg_id).await?; + let call_info = context.load_call_by_id(msg_id).await?.with_context(|| { + format!("Attempting to get call state of non-call message {msg_id}") + })?; let sdp_offer = call_info.place_call_info.clone(); let has_video = sdp_has_video(&sdp_offer).unwrap_or_default(); let state = JsonrpcCallState::from_msg_id(context, msg_id).await?; diff --git a/src/calls.rs b/src/calls.rs index c988ad5b2..2daf0e8ec 100644 --- a/src/calls.rs +++ b/src/calls.rs @@ -213,7 +213,9 @@ impl Context { call_id: MsgId, accept_call_info: String, ) -> Result<()> { - let mut call: CallInfo = self.load_call_by_id(call_id).await?; + let mut call: CallInfo = self.load_call_by_id(call_id).await?.with_context(|| { + format!("accept_incoming_call is called with {call_id} which does not refer to a call") + })?; ensure!(call.is_incoming()); if call.is_accepted() || call.is_ended() { info!(self, "Call already accepted/ended"); @@ -248,7 +250,9 @@ impl Context { /// Cancel, decline or hangup an incoming or outgoing call. pub async fn end_call(&self, call_id: MsgId) -> Result<()> { - let mut call: CallInfo = self.load_call_by_id(call_id).await?; + let mut call: CallInfo = self.load_call_by_id(call_id).await?.with_context(|| { + format!("end_call is called with {call_id} which does not refer to a call") + })?; if call.is_ended() { info!(self, "Call already ended"); return Ok(()); @@ -291,7 +295,13 @@ impl Context { call_id: MsgId, ) -> Result<()> { sleep(Duration::from_secs(wait)).await; - let mut call = context.load_call_by_id(call_id).await?; + let Some(mut call) = context.load_call_by_id(call_id).await? else { + warn!( + context, + "emit_end_call_if_unaccepted is called with {call_id} which does not refer to a call." + ); + return Ok(()); + }; if !call.is_accepted() && !call.is_ended() { if call.is_incoming() { call.mark_as_canceled(&context).await?; @@ -316,7 +326,10 @@ impl Context { from_id: ContactId, ) -> Result<()> { if mime_message.is_call() { - let call = self.load_call_by_id(call_id).await?; + let Some(call) = self.load_call_by_id(call_id).await? else { + warn!(self, "{call_id} does not refer to a call message"); + return Ok(()); + }; if call.is_incoming() { if call.is_stale() { @@ -352,7 +365,11 @@ impl Context { } else { match mime_message.is_system_message { SystemMessage::CallAccepted => { - let mut call = self.load_call_by_id(call_id).await?; + let Some(mut call) = self.load_call_by_id(call_id).await? else { + warn!(self, "{call_id} does not refer to a call message"); + return Ok(()); + }; + if call.is_ended() || call.is_accepted() { info!(self, "CallAccepted received for accepted/ended call"); return Ok(()); @@ -377,7 +394,11 @@ impl Context { } } SystemMessage::CallEnded => { - let mut call = self.load_call_by_id(call_id).await?; + let Some(mut call) = self.load_call_by_id(call_id).await? else { + warn!(self, "{call_id} does not refer to a call message"); + return Ok(()); + }; + if call.is_ended() { // may happen eg. if a a message is missed info!(self, "CallEnded received for ended call"); @@ -421,15 +442,26 @@ impl Context { } /// Loads information about the call given its ID. - pub async fn load_call_by_id(&self, call_id: MsgId) -> Result { + /// + /// If the message referred to by ID is + /// not a call message, returns `None`. + pub async fn load_call_by_id(&self, call_id: MsgId) -> Result> { let call = Message::load_from_db(self, call_id).await?; - self.load_call_by_message(call) + Ok(self.load_call_by_message(call)) } - fn load_call_by_message(&self, call: Message) -> Result { - ensure!(call.viewtype == Viewtype::Call); + // Loads information about the call given the `Message`. + // + // If the `Message` is not a call message, returns `None` + fn load_call_by_message(&self, call: Message) -> Option { + if call.viewtype != Viewtype::Call { + // This can happen e.g. if a "call accepted" + // or "call ended" message is received + // with `In-Reply-To` referring to non-call message. + return None; + } - Ok(CallInfo { + Some(CallInfo { place_call_info: call .param .get(Param::WebrtcRoom) @@ -497,8 +529,13 @@ pub enum CallState { } /// Returns call state given the message ID. +/// +/// Returns an error if the message is not a call message. pub async fn call_state(context: &Context, msg_id: MsgId) -> Result { - let call = context.load_call_by_id(msg_id).await?; + let call = context + .load_call_by_id(msg_id) + .await? + .with_context(|| format!("{msg_id} is not a call message"))?; let state = if call.is_incoming() { if call.is_accepted() { if call.is_ended() { diff --git a/src/calls/calls_tests.rs b/src/calls/calls_tests.rs index bbd382734..34fe7e720 100644 --- a/src/calls/calls_tests.rs +++ b/src/calls/calls_tests.rs @@ -1,6 +1,8 @@ use super::*; use crate::chat::forward_msgs; use crate::config::Config; +use crate::constants::DC_CHAT_ID_TRASH; +use crate::receive_imf::receive_imf; use crate::test_utils::{TestContext, TestContextManager}; struct CallSetup { @@ -53,7 +55,10 @@ async fn setup_call() -> Result { for (t, m) in [(&alice, &alice_call), (&alice2, &alice2_call)] { assert!(!m.is_info()); assert_eq!(m.viewtype, Viewtype::Call); - let info = t.load_call_by_id(m.id).await?; + let info = t + .load_call_by_id(m.id) + .await? + .expect("m should be a call message"); assert!(!info.is_incoming()); assert!(!info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); @@ -71,7 +76,10 @@ async fn setup_call() -> Result { t.evtracker .get_matching(|evt| matches!(evt, EventType::IncomingCall { .. })) .await; - let info = t.load_call_by_id(m.id).await?; + let info = t + .load_call_by_id(m.id) + .await? + .expect("IncomingCall event should refer to a call message"); assert!(info.is_incoming()); assert!(!info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); @@ -111,7 +119,10 @@ async fn accept_call() -> Result { .get_matching(|evt| matches!(evt, EventType::IncomingCallAccepted { .. })) .await; let sent2 = bob.pop_sent_msg().await; - let info = bob.load_call_by_id(bob_call.id).await?; + let info = bob + .load_call_by_id(bob_call.id) + .await? + .expect("bob_call should be a call message"); assert!(info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); assert_eq!(call_state(&bob, bob_call.id).await?, CallState::Active); @@ -121,7 +132,10 @@ async fn accept_call() -> Result { bob2.evtracker .get_matching(|evt| matches!(evt, EventType::IncomingCallAccepted { .. })) .await; - let info = bob2.load_call_by_id(bob2_call.id).await?; + let info = bob2 + .load_call_by_id(bob2_call.id) + .await? + .expect("bob2_call should be a call message"); assert!(info.is_accepted()); assert_eq!(call_state(&bob2, bob2_call.id).await?, CallState::Active); @@ -140,7 +154,10 @@ async fn accept_call() -> Result { accept_call_info: ACCEPT_INFO.to_string() } ); - let info = alice.load_call_by_id(alice_call.id).await?; + let info = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); assert!(info.is_accepted()); assert_eq!(info.place_call_info, PLACE_INFO); assert_eq!(call_state(&alice, alice_call.id).await?, CallState::Active); @@ -460,14 +477,20 @@ async fn test_mark_calls() -> Result<()> { alice, alice_call, .. } = setup_call().await?; - let mut call_info: CallInfo = alice.load_call_by_id(alice_call.id).await?; + let mut call_info: CallInfo = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); assert!(!call_info.is_accepted()); assert!(!call_info.is_ended()); call_info.mark_as_accepted(&alice).await?; assert!(call_info.is_accepted()); assert!(!call_info.is_ended()); - let mut call_info: CallInfo = alice.load_call_by_id(alice_call.id).await?; + let mut call_info: CallInfo = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); assert!(call_info.is_accepted()); assert!(!call_info.is_ended()); @@ -484,7 +507,10 @@ async fn test_update_call_text() -> Result<()> { alice, alice_call, .. } = setup_call().await?; - let call_info = alice.load_call_by_id(alice_call.id).await?; + let call_info = alice + .load_call_by_id(alice_call.id) + .await? + .expect("alice_call should be a call message"); call_info.update_text(&alice, "foo bar").await?; let alice_call = Message::load_from_db(&alice, alice_call.id).await?; @@ -529,3 +555,52 @@ async fn test_forward_call() -> Result<()> { Ok(()) } + +/// Tests that "end call" message referring +/// to a text message does not make receive_imf fail. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_end_text_call() -> Result<()> { + let mut tcm = TestContextManager::new(); + let alice = &tcm.alice().await; + + let received1 = receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + Date: Sun, 22 Mar 2020 22:37:57 +0000\n\ + Chat-Version: 1.0\n\ + \n\ + Hello\n", + false, + ) + .await? + .unwrap(); + assert_eq!(received1.msg_ids.len(), 1); + let msg = Message::load_from_db(alice, received1.msg_ids[0]) + .await + .unwrap(); + assert_eq!(msg.viewtype, Viewtype::Text); + + // Receiving "Call ended" message that refers + // to the text message does not result in an error. + let received2 = receive_imf( + alice, + b"From: bob@example.net\n\ + To: alice@example.org\n\ + Message-ID: \n\ + Date: Sun, 22 Mar 2020 23:37:57 +0000\n\ + In-Reply-To: \n\ + Chat-Version: 1.0\n\ + Chat-Content: call-ended\n\ + \n\ + Call ended\n", + false, + ) + .await? + .unwrap(); + assert_eq!(received2.msg_ids.len(), 1); + assert_eq!(received2.chat_id, DC_CHAT_ID_TRASH); + + Ok(()) +}