diff --git a/CHANGELOG.md b/CHANGELOG.md index 551259222..6e35613ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - handle drafts from mailto links in scanned QR #3492 - do not overflow ratelimiter leaky bucket #3496 - (AEAP) Add device message after you changed your address #3505 +- (AEAP) Revert #3491, instead only replace contacts in verified groups #3510 ### Fixes - don't squash text parts of NDN into attachments #3497 diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index f94224241..74c13edc6 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -6377,8 +6377,9 @@ void dc_event_unref(dc_event_t* event); /// Otherwise you might miss messages of contacts who did not get your new /// address yet." + the link to the AEAP blog post /// -/// The UIs have to add the link: +/// As soon as there is a post about AEAP, the UIs should add it: /// set_stock_translation(123, getString(aeap_explanation) + "\n\n" + AEAP_BLOG_LINK) +/// /// Used in a device message that explains AEAP. #define DC_STR_AEAP_EXPLANATION_AND_LINK 123 diff --git a/src/e2ee.rs b/src/e2ee.rs index 935876d02..ed7b27d11 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -149,11 +149,13 @@ pub(crate) async fn get_autocrypt_peerstate( // Apply Autocrypt header if let Some(header) = autocrypt_header { - // The "from_nongossiped_fingerprint" part is for AEAP: + // The "from_verified_fingerprint" part is for AEAP: // If we know this fingerprint from another addr, // we may want to do a transition from this other addr // (and keep its peerstate) - peerstate = Peerstate::from_nongossiped_fingerprint_or_addr( + // For security reasons, for now, we only do a transition + // if the fingerprint is verified. + peerstate = Peerstate::from_verified_fingerprint_or_addr( context, &header.public_key.fingerprint(), from, diff --git a/src/peerstate.rs b/src/peerstate.rs index 02126a8a8..cf319d0f8 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -4,13 +4,12 @@ use std::collections::HashSet; use std::fmt; use crate::aheader::{Aheader, EncryptPreference}; -use crate::chat::{self, is_contact_in_chat, Chat, ChatId}; +use crate::chat::{self, is_contact_in_chat, Chat}; use crate::chatlist::Chatlist; use crate::constants::Chattype; use crate::contact::{addr_cmp, Contact, Origin}; use crate::context::Context; use crate::events::EventType; -use crate::headerdef::HeaderDef; use crate::key::{DcKey, Fingerprint, SignedPublicKey}; use crate::message::Message; use crate::mimeparser::SystemMessage; @@ -166,7 +165,7 @@ impl Peerstate { Self::from_stmt(context, query, paramsv![fp, fp, fp]).await } - pub async fn from_nongossiped_fingerprint_or_addr( + pub async fn from_verified_fingerprint_or_addr( context: &Context, fingerprint: &Fingerprint, addr: &str, @@ -175,9 +174,9 @@ impl Peerstate { gossip_timestamp, gossip_key, public_key_fingerprint, gossip_key_fingerprint, \ verified_key, verified_key_fingerprint \ FROM acpeerstates \ - WHERE public_key_fingerprint=? \ + WHERE verified_key_fingerprint=? \ OR addr=? COLLATE NOCASE \ - ORDER BY public_key_fingerprint=? DESC, last_seen DESC LIMIT 1;"; + ORDER BY verified_key_fingerprint=? DESC, last_seen DESC LIMIT 1;"; let fp = fingerprint.hex(); Self::from_stmt(context, query, paramsv![fp, addr, fp]).await } @@ -517,12 +516,22 @@ impl Peerstate { .with_context(|| format!("contact with peerstate.addr {:?} not found", &self.addr))?; let chats = Chatlist::try_load(context, 0, None, Some(contact_id)).await?; + let msg = match &change { + PeerstateChange::FingerprintChange => { + stock_str::contact_setup_changed(context, self.addr.clone()).await + } + PeerstateChange::Aeap(new_addr) => { + let old_contact = Contact::load_from_db(context, contact_id).await?; + stock_str::aeap_addr_changed( + context, + old_contact.get_display_name(), + &self.addr, + new_addr, + ) + .await + } + }; for (chat_id, msg_id) in chats.iter() { - let msg = match &change { - PeerstateChange::FingerprintChange => { - stock_str::contact_setup_changed(context, self.addr.clone()).await - } - }; let timestamp_sort = if let Some(msg_id) = msg_id { let lastmsg = Message::load_from_db(context, *msg_id).await?; lastmsg.timestamp_sort @@ -536,6 +545,36 @@ impl Peerstate { .await? .unwrap_or(0) }; + + if let PeerstateChange::Aeap(new_addr) = &change { + let chat = Chat::load_from_db(context, *chat_id).await?; + + if chat.typ == Chattype::Group && !chat.is_protected() { + // Don't add an info_msg to the group, in order not to make the user think + // that the address was automatically replaced in the group. + continue; + } + + // For security reasons, for now, we only do the AEAP transition if the fingerprint + // is verified (that's what from_verified_fingerprint_or_addr() does). + // In order to not have inconsistent group membership state, we then only do the + // transition in verified groups and in broadcast lists. + if (chat.typ == Chattype::Group && chat.is_protected()) + || chat.typ == Chattype::Broadcast + { + chat::remove_from_chat_contacts_table(context, *chat_id, contact_id).await?; + + let (new_contact_id, _) = + Contact::add_or_lookup(context, "", new_addr, Origin::IncomingUnknownFrom) + .await?; + if !is_contact_in_chat(context, *chat_id, new_contact_id).await? { + chat::add_to_chat_contacts_table(context, *chat_id, new_contact_id).await?; + } + + context.emit_event(EventType::ChatModified(*chat_id)); + } + } + chat::add_info_msg_with_cmd( context, *chat_id, @@ -596,63 +635,15 @@ pub async fn maybe_do_aeap_transition( && mime_parser.from_is_signed && info.message_time > peerstate.last_seen { - // Add an info messages to the chat - let contact_id = context - .sql - .query_get_value( - "SELECT id FROM contacts WHERE addr=? COLLATE NOCASE;", - paramsv![peerstate.addr], + // Add info messages to chats with this (verified) contact + // + peerstate + .handle_setup_change( + context, + info.message_time, + PeerstateChange::Aeap(info.from.clone()), ) - .await? - .with_context(|| { - format!("contact with addr {:?} not found", &peerstate.addr) - })?; - - let old_contact = Contact::load_from_db(context, contact_id).await?; - let msg = stock_str::aeap_addr_changed( - context, - old_contact.get_display_name(), - &peerstate.addr, - &from.addr, - ) - .await; - - let grpid = match mime_parser.get_header(HeaderDef::ChatGroupId) { - Some(h) => h, - None => return Ok(()), - }; - let (chat_id, protected, _blocked) = - match chat::get_chat_id_by_grpid(context, grpid).await? { - Some(s) => s, - None => return Ok(()), - }; - - if protected && !peerstate.has_verified_key(&mime_parser.signatures) { - return Ok(()); - } - - chat::add_info_msg(context, chat_id, &msg, info.message_time).await?; - - let (new_contact_id, _) = - Contact::add_or_lookup(context, "", &from.addr, Origin::IncomingUnknownFrom) - .await?; - - let chat = Chat::load_from_db(context, chat_id).await?; - if chat.typ == Chattype::Group || chat.typ == Chattype::Broadcast { - chat::remove_from_chat_contacts_table(context, chat_id, contact_id).await?; - - if !is_contact_in_chat(context, chat_id, new_contact_id).await? { - chat::add_to_chat_contacts_table(context, chat_id, new_contact_id).await?; - } - - context.emit_event(EventType::ChatModified(chat_id)); - } - - // Create a chat with the new address with the same blocked-level as the old chat - let new_chat_id = - ChatId::create_for_contact_with_blocked(context, new_contact_id, chat.blocked) - .await?; - chat::add_info_msg(context, new_chat_id, &msg, info.message_time).await?; + .await?; peerstate.addr = info.from.clone(); let header = info.autocrypt_header.as_ref().context( @@ -678,9 +669,9 @@ enum PeerstateChange { /// The contact's public key fingerprint changed, likely because /// the contact uses a new device and didn't transfer their key. FingerprintChange, - // /// The contact changed their address to the given new address - // /// (Automatic Email Address Porting). - // Aeap(String), (currently unused) + /// The contact changed their address to the given new address + /// (Automatic Email Address Porting). + Aeap(String), } /// Removes duplicate peerstates from `acpeerstates` database table. diff --git a/src/stock_str.rs b/src/stock_str.rs index 38c881ca9..27033d696 100644 --- a/src/stock_str.rs +++ b/src/stock_str.rs @@ -337,7 +337,7 @@ pub enum StockMessage { AeapAddrChanged = 122, #[strum(props( - fallback = "You changed your email address from %1$s to %2$s.\n\nIf you now send a message to a group, contacts there will automatically replace the old with your new address.\n\nIt's highly advised to set up your old email provider to forward all emails to your new email address. Otherwise you might miss messages of contacts who did not get your new address yet." + fallback = "You changed your email address from %1$s to %2$s.\n\nIf you now send a message to a verified group, contacts there will automatically replace the old with your new address.\n\nIt's highly advised to set up your old email provider to forward all emails to your new email address. Otherwise you might miss messages of contacts who did not get your new address yet." ))] AeapExplanationAndLink = 123, } diff --git a/src/tests/aeap.rs b/src/tests/aeap.rs index 5eaee0169..5e6ddfc78 100644 --- a/src/tests/aeap.rs +++ b/src/tests/aeap.rs @@ -69,24 +69,24 @@ Message w/out In-Reply-To } enum ChatForTransition { - // OneToOne, + OneToOne, GroupChat, VerifiedGroup, } use ChatForTransition::*; -// #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -// async fn test_aeap_transition_0() { -// check_aeap_transition(OneToOne, false, false).await; -// } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_aeap_transition_0() { + check_aeap_transition(OneToOne, false, false).await; +} #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_aeap_transition_1() { check_aeap_transition(GroupChat, false, false).await; } -// #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -// async fn test_aeap_transition_0_verified() { -// check_aeap_transition(OneToOne, true, false).await; -// } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_aeap_transition_0_verified() { + check_aeap_transition(OneToOne, true, false).await; +} #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_aeap_transition_1_verified() { check_aeap_transition(GroupChat, true, false).await; @@ -96,18 +96,18 @@ async fn test_aeap_transition_2_verified() { check_aeap_transition(VerifiedGroup, true, false).await; } -// #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -// async fn test_aeap_transition_0_bob_knew_new_addr() { -// check_aeap_transition(OneToOne, false, true).await; -// } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_aeap_transition_0_bob_knew_new_addr() { + check_aeap_transition(OneToOne, false, true).await; +} #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_aeap_transition_1_bob_knew_new_addr() { check_aeap_transition(GroupChat, false, true).await; } -// #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -// async fn test_aeap_transition_0_verified_bob_knew_new_addr() { -// check_aeap_transition(OneToOne, true, true).await; -// } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn test_aeap_transition_0_verified_bob_knew_new_addr() { + check_aeap_transition(OneToOne, true, true).await; +} #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn test_aeap_transition_1_verified_bob_knew_new_addr() { check_aeap_transition(GroupChat, true, true).await; @@ -184,9 +184,11 @@ async fn check_aeap_transition( let already_new_contact = Contact::create(&bob, "Alice", "fiona@example.net") .await .unwrap(); - chat::add_contact_to_chat(&bob, groups[0], already_new_contact) - .await - .unwrap(); + if verified { + chat::add_contact_to_chat(&bob, groups[2], already_new_contact) + .await + .unwrap(); + } // groups 0 and 2 stay unpromoted (i.e. local // on Bob's device, Alice doesn't know about them) @@ -206,7 +208,7 @@ async fn check_aeap_transition( tcm.section("Alice sends another message to Bob, this time from her new addr"); // No matter which chat Alice sends to, the transition should be done in all groups let chat_to_send = match chat_for_transition { - // OneToOne => alice.create_chat(&bob).await.id, + OneToOne => alice.create_chat(&bob).await.id, GroupChat => group1_alice, VerifiedGroup => group3_alice.expect("No verified group"), }; @@ -219,8 +221,7 @@ async fn check_aeap_transition( tcm.section("Check that the AEAP transition worked"); check_that_transition_worked( - // The transition is only done in the chat where the message was sent for now: - &[recvd.chat_id], + &groups[2..], &alice, "alice@example.org", ALICE_NEW_ADDR, @@ -228,6 +229,7 @@ async fn check_aeap_transition( &bob, ) .await; + check_no_transition_done(&groups[0..2], "alice@example.org", &bob).await; // Assert that the autocrypt header is also applied to the peerstate // if the address changed @@ -247,8 +249,7 @@ async fn check_aeap_transition( assert_eq!(recvd.text.unwrap(), "Hello from my old addr!"); check_that_transition_worked( - // The transition is only done in the chat where the message was sent for now: - &[recvd.chat_id], + &groups[2..], &alice, // Note that "alice@example.org" and ALICE_NEW_ADDR are switched now: ALICE_NEW_ADDR, @@ -276,11 +277,19 @@ async fn check_that_transition_worked( let members = chat::get_chat_contacts(bob, *group).await.unwrap(); // In all the groups, exactly Bob and Alice's new number are members. // (and Alice's new number isn't in there twice) - assert_eq!(members.len(), 2); + assert_eq!( + members.len(), + 2, + "Group {} has members {:?}, but should have members {:?} and {:?}", + group, + &members, + new_contact, + ContactId::SELF + ); assert!(members.contains(&new_contact)); assert!(members.contains(&ContactId::SELF)); - let info_msg = get_last_info_msg(bob, *group).await; + let info_msg = get_last_info_msg(bob, *group).await.unwrap(); let expected_text = stock_str::aeap_addr_changed(bob, name, old_alice_addr, new_alice_addr).await; assert_eq!(info_msg.text.unwrap(), expected_text); @@ -293,6 +302,36 @@ async fn check_that_transition_worked( } } +async fn check_no_transition_done(groups: &[ChatId], old_alice_addr: &str, bob: &TestContext) { + let old_contact = Contact::lookup_id_by_addr(bob, old_alice_addr, contact::Origin::Unknown) + .await + .unwrap() + .unwrap(); + + for group in groups { + let members = chat::get_chat_contacts(bob, *group).await.unwrap(); + // In all the groups, exactly Bob and Alice's _old_ number are members. + assert_eq!( + members.len(), + 2, + "Group {} has members {:?}, but should have members {:?} and {:?}", + group, + &members, + old_contact, + ContactId::SELF + ); + assert!(members.contains(&old_contact)); + assert!(members.contains(&ContactId::SELF)); + + let last_info_msg = get_last_info_msg(bob, *group).await; + assert!( + last_info_msg.is_none(), + "{:?} shouldn't be there (or it's an unrelated info msg)", + last_info_msg + ); + } +} + async fn mark_as_verified(this: &TestContext, other: &TestContext) { let other_addr = other.get_primary_self_addr().await.unwrap(); let mut peerstate = peerstate::Peerstate::from_addr(this, &other_addr) @@ -307,16 +346,16 @@ async fn mark_as_verified(this: &TestContext, other: &TestContext) { peerstate.save_to_db(&this.sql, false).await.unwrap(); } -async fn get_last_info_msg(t: &TestContext, chat_id: ChatId) -> Message { +async fn get_last_info_msg(t: &TestContext, chat_id: ChatId) -> Option { let msgs = chat::get_chat_msgs(&t.ctx, chat_id, constants::DC_GCM_INFO_ONLY) .await .unwrap(); - let msg_id = if let chat::ChatItem::Message { msg_id } = msgs.last().unwrap() { + let msg_id = if let chat::ChatItem::Message { msg_id } = msgs.last()? { msg_id } else { - panic!("Wrong item type"); + return None; }; - Message::load_from_db(&t.ctx, *msg_id).await.unwrap() + Some(Message::load_from_db(&t.ctx, *msg_id).await.unwrap()) } /// Test that an attacker - here Fiona - can't replay a message sent by Alice