From 0c04d5b2abaeb8d30dc81aace31cc41136918d21 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Sun, 8 Dec 2019 23:13:33 +0100 Subject: [PATCH 01/12] add a failing test that creates 4 accounts and a group chat with remove/add see also #985 --- python/tests/conftest.py | 15 ++++++---- python/tests/test_account.py | 58 ++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/python/tests/conftest.py b/python/tests/conftest.py index f0fec7c7c..05896cb8c 100644 --- a/python/tests/conftest.py +++ b/python/tests/conftest.py @@ -85,16 +85,21 @@ class SessionLiveConfigFromFile: class SessionLiveConfigFromURL: def __init__(self, url, create_token): self.configlist = [] - for i in range(2): - res = requests.post(url, json={"token_create_user": int(create_token)}) + self.url = url + self.create_token = create_token + + def get(self, index): + try: + return self.configlist[index] + except IndexError: + assert index == len(self.configlist), index + res = requests.post(self.url, json={"token_create_user": int(self.create_token)}) if res.status_code != 200: pytest.skip("creating newtmpuser failed {!r}".format(res)) d = res.json() config = dict(addr=d["email"], mail_pw=d["password"]) self.configlist.append(config) - - def get(self, index): - return self.configlist[index] + return config def exists(self): return bool(self.configlist) diff --git a/python/tests/test_account.py b/python/tests/test_account.py index cc6db441f..352661501 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -382,6 +382,22 @@ class TestOfflineChat: assert not res.is_ask_verifygroup() assert res.contact_id == 10 + def test_group_chat_many_members_add_remove(self, ac1, lp): + lp.sec("ac1: creating group chat with 10 other members") + chat = ac1.create_group_chat(name="title1") + contacts = [] + for i in range(10): + contact = ac1.create_contact("some{}@example.org".format(i)) + contacts.append(contact) + chat.add_contact(contact) + + num_contacts = len(chat.get_contacts()) + assert num_contacts == 11 + + lp.sec("ac1: removing two contacts and checking things are right") + chat.remove_contact(contacts[9]) + chat.remove_contact(contacts[3]) + assert len(chat.get_contacts()) == 9 class TestOnlineAccount: def get_chat(self, ac1, ac2, both_created=False): @@ -1057,6 +1073,48 @@ class TestOnlineAccount: assert not locations3 +class TestGroupStressTests: + def test_group_many_members_add_leave_remove(self, acfactory, lp): + lp.sec("creating and configuring five accounts") + ac1 = acfactory.get_online_configuring_account() + accounts = [acfactory.get_online_configuring_account() for i in range(3)] + wait_configuration_progress(ac1, 1000) + for acc in accounts: + wait_configuration_progress(acc, 1000) + + lp.sec("ac1: creating group chat with 3 other members") + chat = ac1.create_group_chat("title1") + contacts = [] + chars = list("äöüsr") + for acc in accounts: + contact = ac1.create_contact(acc.get_config("addr"), name=chars.pop()) + contacts.append(contact) + chat.add_contact(contact) + # make sure the other side accepts our messages + c1 = acc.create_contact(ac1.get_config("addr"), "ä member") + acc.create_chat_by_contact(c1) + + assert not chat.is_promoted() + + lp.sec("ac1: send mesage to new group chat") + chat.send_text("hello") + assert chat.is_promoted() + + num_contacts = len(chat.get_contacts()) + assert num_contacts == 3 + 1 + + lp.sec("ac2: checking that the chat arrived correctly") + ac2 = accounts[0] + msg = ac2.wait_next_incoming_message() + assert msg.text == "hello" + print("chat is", msg.chat) + 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 + + class TestOnlineConfigureFails: def test_invalid_password(self, acfactory): ac1, configdict = acfactory.get_online_config() From 6edb52554078634c638ae0ca2564fbe42b6c4563 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 09:55:19 +0100 Subject: [PATCH 02/12] snap --- src/dc_receive_imf.rs | 6 ++++++ src/mimefactory.rs | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 072732c4a..9accd07dd 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -1086,6 +1086,7 @@ fn create_or_lookup_group( chat::add_to_chat_contacts_table(context, chat_id, from_id as u32); } for &to_id in to_ids.iter() { + info!(context, "adding to={:?} to chat id={}", to_id, chat_id); if !Contact::addr_equals_contact(context, &self_addr, to_id) && (skip.is_none() || !Contact::addr_equals_contact(context, skip.unwrap(), to_id)) { @@ -1590,6 +1591,8 @@ fn dc_add_or_lookup_contacts_by_address_list( 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() { match addr { mailparse::MailAddr::Single(info) => { @@ -1636,9 +1639,11 @@ fn add_or_lookup_contact_by_addr( *check_self = true; } + /* if *check_self { return; } + */ // add addr_spec if missing, update otherwise let display_name_normalized = display_name @@ -1647,6 +1652,7 @@ 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(); diff --git a/src/mimefactory.rs b/src/mimefactory.rs index d047d9a40..32799ae04 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -382,7 +382,7 @@ impl<'a, 'b> MimeFactory<'a, 'b> { let mut unprotected_headers: Vec
= Vec::new(); let from = Address::new_mailbox_with_name( - encode_words(&self.from_displayname), + self.from_displayname.to_string(), self.from_addr.clone(), ); @@ -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( - encode_words(name), + name.to_string(), addr.clone(), )); } From 5f916f5a9c795f1d6ddd53b47d61cfc492b6de16 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 14:16:21 +0100 Subject: [PATCH 03/12] - create and use a ContactIds type as an ordered set instead of "Vec". - recreate the group list more carefully, fixes #985 - resultify a few functions in the dc_receive pipeline - don't quote displaynames in email-addresses, use utf8, preliminrarily addresses #976 --- Cargo.lock | 1 + Cargo.toml | 1 + examples/repl/cmdline.rs | 4 +- python/tests/test_account.py | 9 +- src/contact.rs | 35 +++++-- src/dc_receive_imf.rs | 179 ++++++++++++++++++++--------------- src/dc_tools.rs | 24 ----- src/error.rs | 4 +- src/imap/mod.rs | 12 ++- src/mimefactory.rs | 2 +- src/mimeparser.rs | 4 + src/securejoin.rs | 2 +- 12 files changed, 159 insertions(+), 118 deletions(-) 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); } From f242b40d0a58c2b3e138d69e5d852db6661d503c Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 18:36:26 +0100 Subject: [PATCH 04/12] nicer print of imap capabilities --- src/imap/mod.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/imap/mod.rs b/src/imap/mod.rs index db52fcb60..62f36cf00 100644 --- a/src/imap/mod.rs +++ b/src/imap/mod.rs @@ -7,7 +7,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use async_imap::{ error::Result as ImapResult, - types::{Fetch, Flag, Mailbox, Name, NameAttribute}, + types::{Capability, Fetch, Flag, Mailbox, Name, NameAttribute}, }; use async_std::sync::{Mutex, RwLock}; use async_std::task; @@ -389,9 +389,14 @@ impl Imap { } else { let can_idle = caps.has_str("IDLE"); let has_xlist = caps.has_str("XLIST"); - let caps_list = caps - .iter() - .fold(String::new(), |s, c| s + &format!(" {:?}", c)); + let caps_list = caps.iter().fold(String::new(), |s, c| { + if let Capability::Atom(x) = c { + s + &format!(" {}", x) + } else { + s + &format!(" {:?}", c) + } + }); + self.config.write().await.can_idle = can_idle; self.config.write().await.has_xlist = has_xlist; *self.connected.lock().await = true; From c33797ff84c82c95edcba40710430fac6726e061 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 19:41:58 +0100 Subject: [PATCH 05/12] remove reverse "check_self" return --- src/dc_receive_imf.rs | 112 ++++++++++++++++++++---------------------- src/mimeparser.rs | 21 +------- 2 files changed, 53 insertions(+), 80 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 8217b20e8..f0775ffc0 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -64,9 +64,6 @@ pub fn dc_receive_imf( // the function returns the number of created messages in the database let mut incoming = true; let mut incoming_origin = Origin::Unknown; - let mut to_self = false; - let mut from_id = 0u32; - let mut from_id_blocked = false; let mut to_id = 0u32; let mut chat_id = 0; let mut hidden = false; @@ -114,34 +111,6 @@ pub fn dc_receive_imf( sent_timestamp = mailparse::dateparse(value).unwrap_or_default(); } - // get From: and check if it is known (for known From:'s we add the other To:/Cc: in the 3rd pass) - // or if From: is equal to SELF (in this case, it is any outgoing messages, - // 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_contact_ids = ContactIds::new(); - dc_add_or_lookup_contacts_by_address_list( - context, - &field_from, - Origin::IncomingUnknownFrom, - &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_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 [**] - } - } - // Make sure, to_ids starts with the first To:-address (Cc: is added in the loop below pass) if let Some(field) = mime_parser.get(HeaderDef::To) { dc_add_or_lookup_contacts_by_address_list( @@ -155,10 +124,44 @@ pub fn dc_receive_imf( Origin::IncomingUnknownTo }, &mut to_ids, - &mut to_self, )?; } + // get From: (it can be an address list!) and check if it is known (for known From:'s we add + // the other To:/Cc: in the 3rd pass) + // or if From: is equal to SELF (in this case, it is any outgoing messages, + // we do not check Return-Path any more as this is unreliable, see issue #150) + let mut from_id = 0; + let mut from_id_blocked = false; + + if let Some(field_from) = mime_parser.get(HeaderDef::From_) { + let mut from_ids = ContactIds::new(); + dc_add_or_lookup_contacts_by_address_list( + context, + &field_from, + Origin::IncomingUnknownFrom, + &mut from_ids, + )?; + if from_ids.len() > 1 { + warn!(context, "mail has more than one address in From: {:?}", field_from); + } + if from_ids.contains(&DC_CONTACT_ID_SELF) { + incoming = false; + if to_ids.len() == 1 && to_ids.contains(&DC_CONTACT_ID_SELF) { + from_id = DC_CONTACT_ID_SELF; + } + } else if !from_ids.is_empty() { + from_id = from_ids.get_index(0).cloned().unwrap_or_default(); + incoming_origin = Contact::get_origin_by_id(context, from_id, &mut from_id_blocked) + } else { + warn!(context, "mail has an empty From header: {:?}", field_from); + // 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 [**] + } + } + + // Add parts let rfc724_mid = match mime_parser.get_rfc724_mid() { @@ -170,13 +173,7 @@ pub fn dc_receive_imf( match dc_create_incoming_rfc724_mid(sent_timestamp, from_id, &to_ids) { Some(x) => x, None => { - cleanup( - context, - &create_event_to_send, - &created_db_entries, - &rr_event_to_send, - ); - bail!("could not create incoming rfc724_mid"); + bail!("No Message-Id found and could not create incoming rfc724_mid"); } } } @@ -200,7 +197,6 @@ pub fn dc_receive_imf( &mut to_id, flags, &mut needs_delete_job, - to_self, &mut insert_msg_id, &mut created_db_entries, &mut create_event_to_send, @@ -286,7 +282,6 @@ fn add_parts( to_id: &mut u32, flags: u32, needs_delete_job: &mut bool, - to_self: bool, insert_msg_id: &mut MsgId, created_db_entries: &mut Vec<(usize, MsgId)>, create_event_to_send: &mut Option, @@ -314,7 +309,6 @@ fn add_parts( Origin::IncomingUnknownCc }, to_ids, - &mut false, )?; } @@ -539,7 +533,11 @@ fn add_parts( } } } - if *chat_id == 0 && to_ids.is_empty() && to_self { + let self_sent = *from_id == DC_CONTACT_ID_SELF + && to_ids.len() == 1 + && to_ids.contains(&DC_CONTACT_ID_SELF); + + if *chat_id == 0 && self_sent { // from_id==to_id==DC_CONTACT_ID_SELF - this is a self-sent messages, // maybe an Autocrypt Setup Messag let (id, bl) = @@ -1579,7 +1577,6 @@ fn dc_add_or_lookup_contacts_by_address_list( addr_list_raw: &str, origin: Origin, to_ids: &mut ContactIds, - check_self: &mut bool, ) -> Result<()> { let addrs = match mailparse::addrparse(addr_list_raw) { Ok(addrs) => addrs, @@ -1595,25 +1592,20 @@ fn dc_add_or_lookup_contacts_by_address_list( for addr in addrs.iter() { match addr { mailparse::MailAddr::Single(info) => { - 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; - } + to_ids.insert( + add_or_lookup_contact_by_addr(context, &info.display_name, &info.addr, origin)? + ); } mailparse::MailAddr::Group(infos) => { for info in &infos.addrs { - 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; - } + to_ids.insert( + add_or_lookup_contact_by_addr( + context, + &info.display_name, + &info.addr, + origin, + )? + ); } } } diff --git a/src/mimeparser.rs b/src/mimeparser.rs index f356a24d1..ab3073cee 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -527,7 +527,7 @@ impl<'a> MimeParser<'a> { let filename = get_attachment_filename(mail); info!( self.context, - "add_single_part_if_known {:?} {:?} {:?}", mime_type, msg_type, filename + "add_single_part_if_known {:?} {:?}", mime_type, msg_type ); let old_part_count = self.parts.len(); @@ -665,25 +665,6 @@ impl<'a> MimeParser<'a> { } } - pub fn sender_equals_recipient(&self) -> bool { - /* get From: and check there is exactly one sender */ - if let Some(field) = self.get(HeaderDef::From_) { - if let Ok(addrs) = mailparse::addrparse(field) { - if addrs.len() != 1 { - return false; - } - if let mailparse::MailAddr::Single(ref info) = addrs[0] { - let from_addr_norm = addr_normalize(&info.addr); - let recipients = get_recipients(self.header.iter()); - if recipients.len() == 1 && recipients.contains(from_addr_norm) { - return true; - } - } - } - } - false - } - pub fn repl_msg_by_error(&mut self, error_msg: impl AsRef) { if self.parts.is_empty() { return; From 4dc5e0378f8ba22d908351c81cba32c429df69eb Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 20:25:46 +0100 Subject: [PATCH 06/12] fix final problem, tests pass now --- src/dc_receive_imf.rs | 57 ++++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index f0775ffc0..479f79bc6 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -127,7 +127,7 @@ pub fn dc_receive_imf( )?; } - // get From: (it can be an address list!) and check if it is known (for known From:'s we add + // get From: (it can be an address list!) and check if it is known (for known From:'s we add // the other To:/Cc: in the 3rd pass) // or if From: is equal to SELF (in this case, it is any outgoing messages, // we do not check Return-Path any more as this is unreliable, see issue #150) @@ -143,7 +143,10 @@ pub fn dc_receive_imf( &mut from_ids, )?; if from_ids.len() > 1 { - warn!(context, "mail has more than one address in From: {:?}", field_from); + warn!( + context, + "mail has more than one address in From: {:?}", field_from + ); } if from_ids.contains(&DC_CONTACT_ID_SELF) { incoming = false; @@ -161,7 +164,6 @@ pub fn dc_receive_imf( } } - // Add parts let rfc724_mid = match mime_parser.get_rfc724_mid() { @@ -450,7 +452,7 @@ fn add_parts( chat::unblock(context, *chat_id); chat_id_blocked = Blocked::Not; } else if is_reply_to_known_message(context, mime_parser) { - // we do not want any chat to be created implicitly. Because of the origin-scale-up, + // we do not want any chat to be created implicitly. Because of the origin-scale-up, // the contact requests will pop up and this should be just fine. Contact::scaleup_origin_by_id(context, *from_id, Origin::IncomingReplyTo); info!( @@ -533,9 +535,9 @@ fn add_parts( } } } - let self_sent = *from_id == DC_CONTACT_ID_SELF - && to_ids.len() == 1 - && to_ids.contains(&DC_CONTACT_ID_SELF); + let self_sent = *from_id == DC_CONTACT_ID_SELF + && to_ids.len() == 1 + && to_ids.contains(&DC_CONTACT_ID_SELF); if *chat_id == 0 && self_sent { // from_id==to_id==DC_CONTACT_ID_SELF - this is a self-sent messages, @@ -1405,13 +1407,20 @@ fn check_verified_properties( } } + // we do not need to check if we are verified with ourself + let mut to_ids = to_ids.clone(); + to_ids.remove(&DC_CONTACT_ID_SELF); + + if to_ids.is_empty() { + return Ok(()); + } let to_ids_str = join(to_ids.iter().map(|x| x.to_string()), ","); let rows = context.sql.query_map( format!( "SELECT c.addr, LENGTH(ps.verified_key_fingerprint) FROM contacts c \ LEFT JOIN acpeerstates ps ON c.addr=ps.addr WHERE c.id IN({}) ", - to_ids_str, + to_ids_str ), params![], |row| Ok((row.get::<_, String>(0)?, row.get::<_, i32>(1).unwrap_or(0))), @@ -1422,6 +1431,12 @@ fn check_verified_properties( )?; for (to_addr, _is_verified) in rows.into_iter() { + info!( + context, + "check_verified_properties: {:?} self={:?}", + to_addr, + context.is_self_addr(&to_addr) + ); let mut is_verified = _is_verified != 0; let mut peerstate = Peerstate::from_addr(context, &context.sql, &to_addr); @@ -1592,20 +1607,21 @@ fn dc_add_or_lookup_contacts_by_address_list( for addr in addrs.iter() { match addr { mailparse::MailAddr::Single(info) => { - to_ids.insert( - add_or_lookup_contact_by_addr(context, &info.display_name, &info.addr, origin)? - ); + to_ids.insert(add_or_lookup_contact_by_addr( + context, + &info.display_name, + &info.addr, + origin, + )?); } mailparse::MailAddr::Group(infos) => { for info in &infos.addrs { - to_ids.insert( - add_or_lookup_contact_by_addr( - context, - &info.display_name, - &info.addr, - origin, - )? - ); + to_ids.insert(add_or_lookup_contact_by_addr( + context, + &info.display_name, + &info.addr, + origin, + )?); } } } @@ -1624,14 +1640,11 @@ fn add_or_lookup_contact_by_addr( if context.is_self_addr(addr)? { return Ok(DC_CONTACT_ID_SELF); } - - // add addr_spec if missing, update otherwise let display_name_normalized = display_name .as_ref() .map(normalize_name) .unwrap_or_default(); - // can be NULL info!( context, "looking up addr={:?} display_name={:?}", addr, display_name_normalized From 17ce02a87c1e759c68a67a1c8704cbbbac1b0fea Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 21:45:01 +0100 Subject: [PATCH 07/12] add some comment and remove some code after quick a/v with @r10s --- src/dc_receive_imf.rs | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 479f79bc6..2b5e90393 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -792,7 +792,6 @@ fn create_or_lookup_group( to_ids: &ContactIds, ) -> Result<(u32, Blocked)> { let mut chat_id_blocked = Blocked::Not; - let to_ids_cnt = to_ids.len(); let mut recreate_member_list = false; let mut send_EVENT_CHAT_MODIFIED = false; let mut X_MrRemoveFromGrp = None; @@ -803,9 +802,9 @@ fn create_or_lookup_group( if mime_parser.is_system_message == SystemMessage::LocationStreamingEnabled { better_msg = - context.stock_system_msg(StockMessage::MsgLocationEnabled, "", "", from_id as u32) + context.stock_system_msg(StockMessage::MsgLocationEnabled, "", "", from_id as u32); + set_better_msg(mime_parser, &better_msg); } - set_better_msg(mime_parser, &better_msg); let mut grpid = "".to_string(); if let Some(optional_field) = mime_parser.get(HeaderDef::ChatGroupId) { @@ -840,6 +839,8 @@ fn create_or_lookup_group( } } } + // now we have a grpid that is non-empty + // but we might not know about this group let grpname = mime_parser.get(HeaderDef::ChatGroupName).cloned(); @@ -993,6 +994,18 @@ fn create_or_lookup_group( }; } + // We have a valid chat_id > DC_CHAT_ID_LAST_SPECIAL. + // + // However, it's possible that we got a non-DC message + // and the user hit "reply" instead of "reply-all". + // We heuristically detect this case and show + // a placeholder-system-message to warn about this + // and refer to "message-info" to see the message. + // This is similar to how we show messages arriving + // in verified chat using an un-verified key or cleartext. + + // XXX insert code in a different PR :) + // execute group commands if X_MrAddToGrp.is_some() || X_MrRemoveFromGrp.is_some() { recreate_member_list = true; @@ -1094,29 +1107,6 @@ fn create_or_lookup_group( if send_EVENT_CHAT_MODIFIED { context.call_cb(Event::ChatModified(chat_id)); } - - // check the number of receivers - - // the only critical situation is if the user hits "Reply" instead - // of "Reply all" in a non-messenger-client */ - if to_ids_cnt == 1 - && !mime_parser.has_chat_version() - && chat::get_chat_contact_cnt(context, chat_id) > 3 - { - // to_ids_cnt==1 may be "From: A, To: B, SELF" as SELF is not counted in to_ids_cnt. - // So everything up to 3 is no error. - create_or_lookup_adhoc_group( - context, - mime_parser, - allow_creation, - create_blocked, - from_id, - to_ids, - ) - .map_err(|err| { - warn!(context, "could not create ad-hoc group: {:?}", err); - err - })?; - } Ok((chat_id, chat_id_blocked)) } From a95fbfe2716543f269513dbeac3cf7618048c118 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Mon, 9 Dec 2019 22:28:29 +0100 Subject: [PATCH 08/12] add a test --- python/tests/test_account.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 1b8080db2..193e68eda 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -632,6 +632,9 @@ class TestOnlineAccount: def test_send_and_receive_message_markseen(self, acfactory, lp): ac1, ac2 = acfactory.get_two_online_accounts() + # make DC's life harder wrt to encodings + ac1.set_config("displayname", "ä name") + lp.sec("ac1: create chat with ac2") chat = self.get_chat(ac1, ac2) From 054cf987548de59c96c8caff3a8f1efe26b535e0 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Tue, 10 Dec 2019 00:26:46 +0100 Subject: [PATCH 09/12] try to work around mailparser not decoding rfc2047 displaynames this pulls in changes in our fork of rust-email to also correctly generate rfc2047 encoding --- Cargo.lock | 4 +++- Cargo.toml | 1 + src/dc_receive_imf.rs | 12 ++++++++++-- src/mimefactory.rs | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0ce738a5c..c864a0f09 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -656,6 +656,7 @@ dependencies = [ "chrono 0.4.10 (registry+https://github.com/rust-lang/crates.io-index)", "debug_stub_derive 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "deltachat_derive 0.1.0", + "email 0.0.21 (git+https://github.com/deltachat/rust-email)", "encoded-words 0.1.0 (git+https://github.com/async-email/encoded-words)", "escaper 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "failure 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)", @@ -827,10 +828,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index" [[package]] name = "email" version = "0.0.21" -source = "git+https://github.com/deltachat/rust-email#265a54a8c31355c506610c032c81112dc0953afb" +source = "git+https://github.com/deltachat/rust-email#b71c13d7d9a599ebc811a8e2ca350e2793fe58d3" dependencies = [ "base64 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)", "chrono 0.4.10 (registry+https://github.com/rust-lang/crates.io-index)", + "encoded-words 0.1.0 (git+https://github.com/async-email/encoded-words)", "encoding 0.2.33 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "rand 0.7.2 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/Cargo.toml b/Cargo.toml index 119d6b92a..deb1bd7b8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ reqwest = { version = "0.9.15", default-features = false, features = ["rustls-tl num-derive = "0.2.5" num-traits = "0.2.6" async-smtp = { git = "https://github.com/async-email/async-smtp" } +email = { git = "https://github.com/deltachat/rust-email" } lettre_email = { git = "https://github.com/deltachat/lettre", branch = "feat/mail" } async-imap = { git = "https://github.com/async-email/async-imap", branch="master" } async-tls = "0.6" diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index 2b5e90393..e4d2524c1 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -1583,6 +1583,10 @@ fn dc_add_or_lookup_contacts_by_address_list( origin: Origin, to_ids: &mut ContactIds, ) -> Result<()> { + // XXX we use manual decoding + // https://github.com/staktrace/mailparse/issues/50 + use email::rfc2047::decode_rfc2047; + let addrs = match mailparse::addrparse(addr_list_raw) { Ok(addrs) => addrs, Err(err) => { @@ -1597,18 +1601,22 @@ fn dc_add_or_lookup_contacts_by_address_list( for addr in addrs.iter() { match addr { mailparse::MailAddr::Single(info) => { + // mailparse does not give us decoded vals + let display_name = decode_rfc2047(&info.display_name.clone().unwrap_or_default()); to_ids.insert(add_or_lookup_contact_by_addr( context, - &info.display_name, + &display_name, &info.addr, origin, )?); } mailparse::MailAddr::Group(infos) => { for info in &infos.addrs { + let display_name = + decode_rfc2047(&info.display_name.clone().unwrap_or_default()); to_ids.insert(add_or_lookup_contact_by_addr( context, - &info.display_name, + &display_name, &info.addr, origin, )?); diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 833f7eda4..e7ee8b40e 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1051,3 +1051,40 @@ pub fn needs_encoding(to_check: impl AsRef) -> bool { !c.is_ascii_alphanumeric() && c != '-' && c != '_' && c != '.' && c != '~' && c != '%' }) } + +#[cfg(test)] +mod tests { + use super::*; + use email::rfc2047::decode_rfc2047; + use mailparse::{addrparse, MailAddr}; + + #[test] + fn test_render_email_address() { + let display_name = "ä space"; + let addr = "x@y.org"; + + assert!(!display_name.is_ascii()); + + let s = format!( + "{}", + Address::new_mailbox_with_name(display_name.to_string(), addr.to_string()) + ); + + println!("{}", s); + assert!(s.is_ascii()); + + assert_eq!(s, "=?utf-8?q?=C3=A4_space?= "); + + match &addrparse(&s).unwrap()[0] { + MailAddr::Single(info) => { + // XXX addrparse should not return rfc2047 encoding + // but the decoded string, see + // https://github.com/staktrace/mailparse/issues/50 + let s = decode_rfc2047(&info.display_name.clone().unwrap()); + assert_eq!(s, Some(display_name.to_string())); + assert_eq!(info.addr, addr.to_string()); + } + _ => panic!(), + } + } +} From 2cbf287998c72a27136820596ed5b8b6810dea64 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Tue, 10 Dec 2019 00:39:13 +0100 Subject: [PATCH 10/12] don't cargo-update on CI jobs but use the Cargo.lock file we manually maintain/update --- .circleci/config.yml | 2 -- appveyor.yml | 1 - 2 files changed, 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 82d3fd361..e4704159b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -49,7 +49,6 @@ jobs: - run: rustup default $(cat rust-toolchain) - run: rustup component add --toolchain $(cat rust-toolchain) rustfmt - run: rustup component add --toolchain $(cat rust-toolchain) clippy-preview - - run: cargo update - run: cargo fetch - run: rustc +stable --version - run: rustc +$(cat rust-toolchain) --version @@ -91,7 +90,6 @@ jobs: curl https://sh.rustup.rs -sSf | sh -s -- -y - run: rustup install $(cat rust-toolchain) - run: rustup default $(cat rust-toolchain) - - run: cargo update - run: cargo fetch - run: name: Test diff --git a/appveyor.yml b/appveyor.yml index 132892d49..7276acac1 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -8,7 +8,6 @@ install: - set PATH=%PATH%;%USERPROFILE%\.cargo\bin - rustc -vV - cargo -vV - - cargo update build: false From d0a04be825ca5b3f9d517e5444881b9db074ec80 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Tue, 10 Dec 2019 01:21:28 +0100 Subject: [PATCH 11/12] remove hack for decoding, and add a test that encoding/decoding works now --- .circleci/config.yml | 2 +- python/tests/test_account.py | 1 + src/dc_receive_imf.rs | 12 ++---------- src/mimefactory.rs | 15 --------------- 4 files changed, 4 insertions(+), 26 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index e4704159b..1ebd757ff 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -185,7 +185,7 @@ jobs: - *restore-cache - run: name: Run cargo clippy - command: cargo clippy --all + command: cargo clippy workflows: diff --git a/python/tests/test_account.py b/python/tests/test_account.py index 193e68eda..d036c772f 100644 --- a/python/tests/test_account.py +++ b/python/tests/test_account.py @@ -652,6 +652,7 @@ class TestOnlineAccount: msg_in = ac2.get_message_by_id(msg_out.id) assert msg_in.text == "message1" assert not msg_in.is_forwarded() + assert msg_in.get_sender_contact().display_name == ac1.get_config("displayname") lp.sec("check the message arrived in contact-requets/deaddrop") chat2 = msg_in.chat diff --git a/src/dc_receive_imf.rs b/src/dc_receive_imf.rs index e4d2524c1..2b5e90393 100644 --- a/src/dc_receive_imf.rs +++ b/src/dc_receive_imf.rs @@ -1583,10 +1583,6 @@ fn dc_add_or_lookup_contacts_by_address_list( origin: Origin, to_ids: &mut ContactIds, ) -> Result<()> { - // XXX we use manual decoding - // https://github.com/staktrace/mailparse/issues/50 - use email::rfc2047::decode_rfc2047; - let addrs = match mailparse::addrparse(addr_list_raw) { Ok(addrs) => addrs, Err(err) => { @@ -1601,22 +1597,18 @@ fn dc_add_or_lookup_contacts_by_address_list( for addr in addrs.iter() { match addr { mailparse::MailAddr::Single(info) => { - // mailparse does not give us decoded vals - let display_name = decode_rfc2047(&info.display_name.clone().unwrap_or_default()); to_ids.insert(add_or_lookup_contact_by_addr( context, - &display_name, + &info.display_name, &info.addr, origin, )?); } mailparse::MailAddr::Group(infos) => { for info in &infos.addrs { - let display_name = - decode_rfc2047(&info.display_name.clone().unwrap_or_default()); to_ids.insert(add_or_lookup_contact_by_addr( context, - &display_name, + &info.display_name, &info.addr, origin, )?); diff --git a/src/mimefactory.rs b/src/mimefactory.rs index e7ee8b40e..fd441ccbd 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -1055,8 +1055,6 @@ pub fn needs_encoding(to_check: impl AsRef) -> bool { #[cfg(test)] mod tests { use super::*; - use email::rfc2047::decode_rfc2047; - use mailparse::{addrparse, MailAddr}; #[test] fn test_render_email_address() { @@ -1071,20 +1069,7 @@ mod tests { ); println!("{}", s); - assert!(s.is_ascii()); assert_eq!(s, "=?utf-8?q?=C3=A4_space?= "); - - match &addrparse(&s).unwrap()[0] { - MailAddr::Single(info) => { - // XXX addrparse should not return rfc2047 encoding - // but the decoded string, see - // https://github.com/staktrace/mailparse/issues/50 - let s = decode_rfc2047(&info.display_name.clone().unwrap()); - assert_eq!(s, Some(display_name.to_string())); - assert_eq!(info.addr, addr.to_string()); - } - _ => panic!(), - } } } From 56ee7a0abde3167788e2763576c9506e774807d5 Mon Sep 17 00:00:00 2001 From: holger krekel Date: Tue, 10 Dec 2019 02:33:13 +0100 Subject: [PATCH 12/12] try to remove updates to lock file --- .circleci/config.yml | 6 ------ 1 file changed, 6 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 1ebd757ff..7b02209b8 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -36,12 +36,6 @@ jobs: executor: default steps: - checkout - - run: - name: Update submodules - command: git submodule update --init --recursive - - run: - name: Calculate dependencies - command: cargo generate-lockfile - restore_cache: keys: - cargo-v3-{{ checksum "rust-toolchain" }}-{{ checksum "Cargo.toml" }}-{{ checksum "Cargo.lock" }}-{{ arch }}