diff --git a/Cargo.lock b/Cargo.lock index bfc1f5489..0ce738a5c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -662,6 +662,7 @@ dependencies = [ "failure_derive 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", "hex 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "image-meta 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", + "indexmap 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "itertools 0.8.2 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "lettre_email 0.9.2 (git+https://github.com/deltachat/lettre?branch=feat/mail)", diff --git a/Cargo.toml b/Cargo.toml index 1d87af162..119d6b92a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ serde_json = "1.0" chrono = "0.4.6" failure = "0.1.5" failure_derive = "0.1.5" +indexmap = "1.3.0" # TODO: make optional rustyline = "4.1.0" lazy_static = "1.4.0" diff --git a/examples/repl/cmdline.rs b/examples/repl/cmdline.rs index dfdac887b..dc3a8b6f6 100644 --- a/examples/repl/cmdline.rs +++ b/examples/repl/cmdline.rs @@ -94,7 +94,9 @@ pub fn dc_reset_tables(context: &Context, bits: i32) -> i32 { fn dc_poke_eml_file(context: &Context, filename: impl AsRef) -> Result<(), Error> { let data = dc_read_file(context, filename)?; - dc_receive_imf(context, &data, "import", 0, 0); + if let Err(err) = dc_receive_imf(context, &data, "import", 0, 0) { + println!("dc_receive_imf errored: {:?}", err); + } Ok(()) } diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 352661501..1b8080db2 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -399,6 +399,7 @@ class TestOfflineChat: chat.remove_contact(contacts[3]) assert len(chat.get_contacts()) == 9 + class TestOnlineAccount: def get_chat(self, ac1, ac2, both_created=False): c2 = ac1.create_contact(email=ac2.get_config("addr")) @@ -1111,8 +1112,12 @@ class TestGroupStressTests: assert len(msg.chat.get_contacts()) == 4 lp.sec("ac1: removing one contacts and checking things are right") - msg.chat.remove_contact(msg.chat.get_contacts()[-1]) - assert 0 + to_remove = msg.chat.get_contacts()[-1] + msg.chat.remove_contact(to_remove) + + sysmsg = ac1.wait_next_incoming_message() + assert to_remove.addr in sysmsg.text + assert len(sysmsg.chat.get_contacts()) == 3 class TestOnlineConfigureFails: diff --git a/src/contact.rs b/src/contact.rs index 00f371ce7..4612b3f1b 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -10,7 +10,7 @@ use crate::constants::*; use crate::context::Context; use crate::dc_tools::*; use crate::e2ee; -use crate::error::Result; +use crate::error::{Error, Result}; use crate::events::Event; use crate::key::*; use crate::login_param::LoginParam; @@ -1021,6 +1021,18 @@ fn cat_fingerprint( } } +impl Context { + /// determine whether the specified addr maps to the/a self addr + pub fn is_self_addr(&self, addr: &str) -> Result { + let self_addr = match self.get_config(Config::ConfiguredAddr) { + Some(s) => s, + None => return Err(Error::NotConfigured), + }; + + Ok(addr_cmp(self_addr, addr)) + } +} + pub fn addr_cmp(addr1: impl AsRef, addr2: impl AsRef) -> bool { let norm1 = addr_normalize(addr1.as_ref()).to_lowercase(); let norm2 = addr_normalize(addr2.as_ref()).to_lowercase(); @@ -1028,15 +1040,6 @@ pub fn addr_cmp(addr1: impl AsRef, addr2: impl AsRef) -> bool { norm1 == norm2 } -pub fn addr_equals_self(context: &Context, addr: impl AsRef) -> bool { - if !addr.as_ref().is_empty() { - if let Some(self_addr) = context.get_config(Config::ConfiguredAddr) { - return addr_cmp(addr, self_addr); - } - } - false -} - fn split_address_book(book: &str) -> Vec<(&str, &str)> { book.lines() .chunks(2) @@ -1120,6 +1123,18 @@ mod tests { assert_eq!(contacts.len(), 0); } + #[test] + fn test_is_self_addr() -> Result<()> { + let t = test_context(None); + assert!(t.ctx.is_self_addr("me@me.org").is_err()); + + let addr = configure_alice_keypair(&t.ctx); + assert_eq!(t.ctx.is_self_addr("me@me.org")?, false); + assert_eq!(t.ctx.is_self_addr(&addr)?, true); + + Ok(()) + } + #[test] fn test_add_or_lookup() { // add some contacts, this also tests add_address_book() diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 9accd07dd..8217b20e8 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -23,6 +23,9 @@ use crate::securejoin::handle_securejoin_handshake; use crate::sql; use crate::stock::StockMessage; +// IndexSet is like HashSet but maintains order of insertion +type ContactIds = indexmap::IndexSet; + #[derive(Debug, PartialEq, Eq)] enum CreateEvent { MsgsChanged, @@ -36,7 +39,7 @@ pub fn dc_receive_imf( server_folder: impl AsRef, server_uid: u32, flags: u32, -) { +) -> Result<()> { info!( context, "Receiving message {}/{}...", @@ -53,19 +56,10 @@ pub fn dc_receive_imf( println!("{}", String::from_utf8_lossy(imf_raw)); } - let mime_parser = MimeParser::from_bytes(context, imf_raw); - let mut mime_parser = if let Err(err) = mime_parser { - warn!(context, "dc_receive_imf parse error: {}", err); - return; - } else { - mime_parser.unwrap() - }; + let mut mime_parser = MimeParser::from_bytes(context, imf_raw)?; - if mime_parser.get(HeaderDef::From_).is_none() { - // Error - even adding an empty record won't help as we do not know the sender - warn!(context, "No From header."); - return; - } + // we can not add even an empty record if we have no info whatsoever + ensure!(mime_parser.has_headers(), "No Headers Found"); // the function returns the number of created messages in the database let mut incoming = true; @@ -85,7 +79,7 @@ pub fn dc_receive_imf( let mut create_event_to_send = Some(CreateEvent::MsgsChanged); let mut rr_event_to_send = Vec::new(); - let mut to_ids = Vec::with_capacity(16); + let mut to_ids = ContactIds::new(); // helper method to handle early exit and memory cleanup let cleanup = |context: &Context, @@ -125,25 +119,26 @@ pub fn dc_receive_imf( // we do not check Return-Path any more as this is unreliable, see issue #150 if let Some(field_from) = mime_parser.get(HeaderDef::From_) { let mut check_self = false; - let mut from_list = Vec::with_capacity(16); + let mut from_contact_ids = ContactIds::new(); dc_add_or_lookup_contacts_by_address_list( context, &field_from, Origin::IncomingUnknownFrom, - &mut from_list, + &mut from_contact_ids, &mut check_self, - ); + )?; if check_self { incoming = false; if mime_parser.sender_equals_recipient() { from_id = DC_CONTACT_ID_SELF; } - } else if !from_list.is_empty() { + } else if !from_contact_ids.is_empty() { + from_id = from_contact_ids.get_index(0).cloned().unwrap_or_default(); + incoming_origin = Contact::get_origin_by_id(context, from_id, &mut from_id_blocked) + } else { // if there is no from given, from_id stays 0 which is just fine. These messages // are very rare, however, we have to add them to the database (they go to the // "deaddrop" chat) to avoid a re-download from the server. See also [**] - from_id = from_list[0]; - incoming_origin = Contact::get_origin_by_id(context, from_id, &mut from_id_blocked) } } @@ -161,7 +156,7 @@ pub fn dc_receive_imf( }, &mut to_ids, &mut to_self, - ); + )?; } // Add parts @@ -175,14 +170,13 @@ pub fn dc_receive_imf( match dc_create_incoming_rfc724_mid(sent_timestamp, from_id, &to_ids) { Some(x) => x, None => { - error!(context, "can not create incoming rfc724_mid"); cleanup( context, &create_event_to_send, &created_db_entries, &rr_event_to_send, ); - return; + bail!("could not create incoming rfc724_mid"); } } } @@ -211,15 +205,13 @@ pub fn dc_receive_imf( &mut created_db_entries, &mut create_event_to_send, ) { - warn!(context, "add_parts error: {:?}", err); - cleanup( context, &create_event_to_send, &created_db_entries, &rr_event_to_send, ); - return; + bail!("add_parts error: {:?}", err); } } else { // there are no non-meta data in message, do some basic calculations so that the varaiables @@ -272,6 +264,8 @@ pub fn dc_receive_imf( &created_db_entries, &rr_event_to_send, ); + + Ok(()) } fn add_parts( @@ -282,7 +276,7 @@ fn add_parts( incoming_origin: &mut Origin, server_folder: impl AsRef, server_uid: u32, - to_ids: &mut Vec, + to_ids: &mut ContactIds, rfc724_mid: &str, sent_timestamp: &mut i64, from_id: &mut u32, @@ -321,7 +315,7 @@ fn add_parts( }, to_ids, &mut false, - ); + )?; } // check, if the mail is already in our database - if so, just update the folder/uid @@ -506,7 +500,7 @@ fn add_parts( state = MessageState::OutDelivered; *from_id = DC_CONTACT_ID_SELF; if !to_ids.is_empty() { - *to_id = to_ids[0]; + *to_id = to_ids.get_index(0).cloned().unwrap_or_default(); if *chat_id == 0 { let (new_chat_id, new_chat_id_blocked) = create_or_lookup_group( context, @@ -795,7 +789,7 @@ fn create_or_lookup_group( allow_creation: bool, create_blocked: Blocked, from_id: u32, - to_ids: &[u32], + to_ids: &ContactIds, ) -> Result<(u32, Blocked)> { let mut chat_id_blocked = Blocked::Not; let to_ids_cnt = to_ids.len(); @@ -1147,7 +1141,7 @@ fn create_or_lookup_adhoc_group( allow_creation: bool, create_blocked: Blocked, from_id: u32, - to_ids: &[u32], + to_ids: &ContactIds, ) -> Result<(u32, Blocked)> { // if we're here, no grpid was found, check if there is an existing // ad-hoc group matching the to-list or if we should and can create one @@ -1163,7 +1157,7 @@ fn create_or_lookup_adhoc_group( return Ok((0, Blocked::Not)); } - let mut member_ids = to_ids.to_vec(); + let mut member_ids: Vec = to_ids.iter().copied().collect(); if !member_ids.contains(&from_id) { member_ids.push(from_id); } @@ -1382,7 +1376,7 @@ fn check_verified_properties( context: &Context, mimeparser: &MimeParser, from_id: u32, - to_ids: &[u32], + to_ids: &ContactIds, ) -> Result<()> { let contact = Contact::load_from_db(context, from_id)?; @@ -1584,41 +1578,48 @@ fn dc_add_or_lookup_contacts_by_address_list( context: &Context, addr_list_raw: &str, origin: Origin, - ids: &mut Vec, + to_ids: &mut ContactIds, check_self: &mut bool, -) { - let addrs = mailparse::addrparse(addr_list_raw); - if addrs.is_err() { - return; - } - info!(context, "dc_add_or_lookup_contacts_by_address-list={:?}", addr_list_raw); - info!(context, "addrs={:?}", addrs); - for addr in addrs.unwrap().iter() { +) -> Result<()> { + let addrs = match mailparse::addrparse(addr_list_raw) { + Ok(addrs) => addrs, + Err(err) => { + bail!("could not parse {:?}: {:?}", addr_list_raw, err); + } + }; + + info!( + context, + "dc_add_or_lookup_contacts_by_address raw={:?} addrs={:?}", addr_list_raw, addrs + ); + for addr in addrs.iter() { match addr { mailparse::MailAddr::Single(info) => { - add_or_lookup_contact_by_addr( - context, - &info.display_name, - &info.addr, - origin, - ids, - check_self, - ); + let contact_id = + add_or_lookup_contact_by_addr(context, &info.display_name, &info.addr, origin)?; + to_ids.insert(contact_id); + if contact_id == DC_CONTACT_ID_SELF { + *check_self = true; + } } mailparse::MailAddr::Group(infos) => { for info in &infos.addrs { - add_or_lookup_contact_by_addr( + let contact_id = add_or_lookup_contact_by_addr( context, &info.display_name, &info.addr, origin, - ids, - check_self, - ); + )?; + to_ids.insert(contact_id); + if contact_id == DC_CONTACT_ID_SELF { + *check_self = true; + } } } } } + + Ok(()) } /// Add contacts to database on receiving messages. @@ -1627,24 +1628,11 @@ fn add_or_lookup_contact_by_addr( display_name: &Option, addr: &str, origin: Origin, - ids: &mut Vec, - check_self: &mut bool, -) { - // is addr_spec equal to SELF? - let self_addr = context - .get_config(Config::ConfiguredAddr) - .unwrap_or_default(); - - if addr_cmp(self_addr, addr) { - *check_self = true; +) -> Result { + if context.is_self_addr(addr)? { + return Ok(DC_CONTACT_ID_SELF); } - /* - if *check_self { - return; - } - */ - // add addr_spec if missing, update otherwise let display_name_normalized = display_name .as_ref() @@ -1652,14 +1640,31 @@ fn add_or_lookup_contact_by_addr( .unwrap_or_default(); // can be NULL - info!(context, "looking up addr={:?} display_name={:?}", addr, display_name_normalized); - let row_id = Contact::add_or_lookup(context, display_name_normalized, addr, origin) - .map(|(id, _)| id) - .unwrap_or_default(); + info!( + context, + "looking up addr={:?} display_name={:?}", addr, display_name_normalized + ); + let (row_id, _modified) = + Contact::add_or_lookup(context, display_name_normalized, addr, origin)?; + ensure!(row_id > 0, "could not add contact: {:?}", addr); - if 0 != row_id && !ids.contains(&row_id) { - ids.push(row_id); - }; + Ok(row_id) +} + +fn dc_create_incoming_rfc724_mid( + message_timestamp: i64, + contact_id_from: u32, + contact_ids_to: &ContactIds, +) -> Option { + /* create a deterministic rfc724_mid from input such that + repeatedly calling it with the same input results in the same Message-id */ + + let largest_id_to = contact_ids_to.iter().max().copied().unwrap_or_default(); + let result = format!( + "{}-{}-{}@stub", + message_timestamp, contact_id_from, largest_id_to + ); + Some(result) } #[cfg(test)] @@ -1704,4 +1709,24 @@ mod tests { assert_eq!(extract_grpid(&mimeparser, HeaderDef::InReplyTo), grpid); assert_eq!(extract_grpid(&mimeparser, HeaderDef::References), grpid); } + + #[test] + fn test_dc_create_incoming_rfc724_mid() { + let mut members = ContactIds::new(); + assert_eq!( + dc_create_incoming_rfc724_mid(123, 45, &members), + Some("123-45-0@stub".into()) + ); + members.insert(7); + members.insert(3); + assert_eq!( + dc_create_incoming_rfc724_mid(123, 45, &members), + Some("123-45-7@stub".into()) + ); + members.insert(9); + assert_eq!( + dc_create_incoming_rfc724_mid(123, 45, &members), + Some("123-45-9@stub".into()) + ); + } } diff --git a/src/dc_tools.rs b/src/dc_tools.rs index 493821865..c6067ad7c 100644 --- a/src/dc_tools.rs +++ b/src/dc_tools.rs @@ -182,22 +182,6 @@ fn encode_66bits_as_base64(v1: u32, v2: u32, fill: u32) -> String { String::from_utf8(wrapped_writer).unwrap() } -pub(crate) fn dc_create_incoming_rfc724_mid( - message_timestamp: i64, - contact_id_from: u32, - contact_ids_to: &[u32], -) -> Option { - /* create a deterministic rfc724_mid from input such that - repeatedly calling it with the same input results in the same Message-id */ - - let largest_id_to = contact_ids_to.iter().max().copied().unwrap_or_default(); - let result = format!( - "{}-{}-{}@stub", - message_timestamp, contact_id_from, largest_id_to - ); - Some(result) -} - /// Function generates a Message-ID that can be used for a new outgoing message. /// - this function is called for all outgoing messages. /// - the message ID should be globally unique @@ -780,14 +764,6 @@ mod tests { } } - #[test] - fn test_dc_create_incoming_rfc724_mid() { - let res = dc_create_incoming_rfc724_mid(123, 45, &[6, 7]); - assert_eq!(res, Some("123-45-7@stub".into())); - let res = dc_create_incoming_rfc724_mid(123, 45, &[]); - assert_eq!(res, Some("123-45-0@stub".into())); - } - #[test] fn test_file_get_safe_basename() { assert_eq!(get_safe_basename("12312/hello"), "hello"); diff --git a/src/error.rs b/src/error.rs index 6f1139094..5f0558f48 100644 --- a/src/error.rs +++ b/src/error.rs @@ -28,12 +28,14 @@ pub enum Error { InvalidMsgId, #[fail(display = "Watch folder not found {:?}", _0)] WatchFolderNotFound(String), - #[fail(display = "Inalid Email: {:?}", _0)] + #[fail(display = "Invalid Email: {:?}", _0)] MailParseError(#[cause] mailparse::MailParseError), #[fail(display = "Building invalid Email: {:?}", _0)] LettreError(#[cause] lettre_email::error::Error), #[fail(display = "FromStr error: {:?}", _0)] FromStr(#[cause] mime::FromStrError), + #[fail(display = "Not Configured")] + NotConfigured, } pub type Result = std::result::Result; diff --git a/src/imap/mod.rs b/src/imap/mod.rs index eb7bf5f88..db52fcb60 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -713,7 +713,17 @@ impl Imap { if !is_deleted && msg.body().is_some() { let body = msg.body().unwrap_or_default(); - dc_receive_imf(context, &body, folder.as_ref(), server_uid, flags as u32); + if let Err(err) = + dc_receive_imf(context, &body, folder.as_ref(), server_uid, flags as u32) + { + warn!( + context, + "dc_receive_imf failed for imap-message {}/{}: {:?}", + folder.as_ref(), + server_uid, + err + ); + } } } diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 32799ae04..833f7eda4 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -394,7 +394,7 @@ impl<'a, 'b> MimeFactory<'a, 'b> { to.push(Address::new_mailbox(addr.clone())); } else { to.push(Address::new_mailbox_with_name( - name.to_string(), + name.to_string(), addr.clone(), )); } diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 171717073..f356a24d1 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -338,6 +338,10 @@ impl<'a> MimeParser<'a> { self.header.contains_key("chat-version") } + pub(crate) fn has_headers(&self) -> bool { + !self.header.is_empty() + } + pub(crate) fn get_subject(&self) -> Option { if let Some(s) = self.get(HeaderDef::Subject) { if s.is_empty() { diff --git a/src/securejoin.rs b/src/securejoin.rs index 26624958b..2c1bb89b2 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -604,7 +604,7 @@ pub(crate) fn handle_securejoin_handshake( .get(HeaderDef::ChatGroupMemberAdded) .map(|s| s.as_str()) .unwrap_or_else(|| ""); - if join_vg && !addr_equals_self(context, cg_member_added) { + if join_vg && !context.is_self_addr(cg_member_added)? { info!(context, "Message belongs to a different handshake (scaled up contact anyway to allow creation of group)."); return Ok(ret); }