From 7540770dec0c73bc34acc0ec21b0109f8e05a136 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Mon, 13 Jan 2020 23:27:04 +0100 Subject: [PATCH] Resultify get_chat_id_by_grpid I want to avoid having to be able to represent a chat_id of 0 in order to more nicely turn chat_id into a ChatId newtype. --- src/chat.rs | 28 ++++++++++++++-------------- src/dc_receive_imf.rs | 20 +++++++++++++++++--- src/dc_tools.rs | 1 - src/securejoin.rs | 42 +++++++++++++++++++++++++++++------------- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 157f18fd5..6f223866c 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -2111,21 +2111,21 @@ pub fn get_chat_cnt(context: &Context) -> usize { } } -pub fn get_chat_id_by_grpid(context: &Context, grpid: impl AsRef) -> (u32, bool, Blocked) { - context - .sql - .query_row( - "SELECT id, blocked, type FROM chats WHERE grpid=?;", - params![grpid.as_ref()], - |row| { - let chat_id = row.get(0)?; +pub(crate) fn get_chat_id_by_grpid( + context: &Context, + grpid: impl AsRef, +) -> Result<(u32, bool, Blocked), sql::Error> { + context.sql.query_row( + "SELECT id, blocked, type FROM chats WHERE grpid=?;", + params![grpid.as_ref()], + |row| { + let chat_id = row.get(0)?; - let b = row.get::<_, Option>(1)?.unwrap_or_default(); - let v = row.get::<_, Option>(2)?.unwrap_or_default(); - Ok((chat_id, v == Chattype::VerifiedGroup, b)) - }, - ) - .unwrap_or((0, false, Blocked::Not)) + let b = row.get::<_, Option>(1)?.unwrap_or_default(); + let v = row.get::<_, Option>(2)?.unwrap_or_default(); + Ok((chat_id, v == Chattype::VerifiedGroup, b)) + }, + ) } /// Adds a message to device chat. diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index ceb65f874..bae57b07e 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -867,7 +867,8 @@ fn create_or_lookup_group( set_better_msg(mime_parser, &better_msg); // check, if we have a chat with this group ID - let (mut chat_id, chat_id_verified, _blocked) = chat::get_chat_id_by_grpid(context, &grpid); + let (mut chat_id, chat_id_verified, _blocked) = + chat::get_chat_id_by_grpid(context, &grpid).unwrap_or((0, false, Blocked::Not)); if chat_id != 0 { if chat_id_verified { if let Err(err) = @@ -1179,10 +1180,23 @@ fn create_group_record( ) .is_err() { + warn!( + context, + "Failed to create group '{}' for grpid={}", + grpname.as_ref(), + grpid.as_ref() + ); return 0; } - - sql::get_rowid(context, &context.sql, "chats", "grpid", grpid.as_ref()) + let chat_id = sql::get_rowid(context, &context.sql, "chats", "grpid", grpid.as_ref()); + info!( + context, + "Created group '{}' grpid={} as chat #{}", + grpname.as_ref(), + grpid.as_ref(), + chat_id + ); + chat_id } fn create_adhoc_grp_id(context: &Context, member_ids: &[u32]) -> String { diff --git a/src/dc_tools.rs b/src/dc_tools.rs index 878b08ca4..c5ce00cc0 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -653,7 +653,6 @@ mod tests { ); } - #[test] #[test] fn test_dc_extract_grpid_from_rfc724_mid() { // Should return None if we pass invalid mid diff --git a/src/securejoin.rs b/src/securejoin.rs index f8aeb3b70..6a77a843e 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -154,6 +154,7 @@ pub fn dc_join_securejoin(context: &Context, qr: &str) -> u32 { context, bob.qr_scan.as_ref().unwrap().text2.as_ref().unwrap(), ) + .unwrap_or((0, false, Blocked::Not)) .0 } else { contact_chat_id @@ -593,17 +594,20 @@ pub(crate) fn handle_securejoin_handshake( return Ok(HandshakeMessage::Ignore); } }; - let (group_chat_id, _, _) = chat::get_chat_id_by_grpid(context, field_grpid); - if group_chat_id == 0 { - error!(context, "Chat {} not found.", &field_grpid); - return Err(HandshakeError::ChatNotFound { - group: field_grpid.to_string(), - }); - } else if let Err(err) = - // Alice -> Bob and all members - chat::add_contact_to_chat_ex(context, group_chat_id, contact_id, true) - { - error!(context, "failed to add contact: {}", err); + match chat::get_chat_id_by_grpid(context, field_grpid) { + Ok((group_chat_id, _, _)) => { + if let Err(err) = + chat::add_contact_to_chat_ex(context, group_chat_id, contact_id, true) + { + error!(context, "failed to add contact: {}", err); + } + } + Err(err) => { + error!(context, "Chat {} not found: {}", &field_grpid, err); + return Err(HandshakeError::ChatNotFound { + group: field_grpid.to_string(), + }); + } } } else { // Alice -> Bob @@ -638,7 +642,13 @@ pub(crate) fn handle_securejoin_handshake( let vg_expect_encrypted = if join_vg { let group_id = get_qr_attr!(context, text2).to_string(); - let (_, is_verified_group, _) = chat::get_chat_id_by_grpid(context, group_id); + // This is buggy, is_verified_group will always be + // false since the group is created by receive_imf by + // the very handshake message we're handling now. But + // only after we have returned. It does not impact + // the security invariants of secure-join however. + let (_, is_verified_group, _) = chat::get_chat_id_by_grpid(context, &group_id) + .unwrap_or((0, false, Blocked::Not)); // when joining a non-verified group // the vg-member-added message may be unencrypted // when not all group members have keys or prefer encryption. @@ -716,7 +726,13 @@ pub(crate) fn handle_securejoin_handshake( .get(HeaderDef::SecureJoinGroup) .map(|s| s.as_str()) .unwrap_or_else(|| ""); - let (group_chat_id, _, _) = chat::get_chat_id_by_grpid(context, &field_grpid); + let (group_chat_id, _, _) = chat::get_chat_id_by_grpid(context, &field_grpid) + .map_err(|err| { + warn!(context, "Failed to lookup chat_id from grpid: {}", err); + HandshakeError::ChatNotFound { + group: field_grpid.to_string(), + } + })?; context.call_cb(Event::SecurejoinMemberAdded { chat_id: group_chat_id, contact_id,