diff --git a/deltachat-ffi/deltachat.h b/deltachat-ffi/deltachat.h index 013737fa1..5ca8617ff 100644 --- a/deltachat-ffi/deltachat.h +++ b/deltachat-ffi/deltachat.h @@ -2089,9 +2089,19 @@ int dc_may_be_valid_addr (const char* addr); /** - * Check if an e-mail address belongs to a known and unblocked contact. + * Looks up a known and unblocked contact with a given e-mail address. * To get a list of all known and unblocked contacts, use dc_get_contacts(). * + * **POTENTIAL SECURITY ISSUE**: If there are multiple contacts with this address + * (e.g. an address-contact and a key-contact), + * this looks up the most recently seen contact, + * i.e. which contact is returned depends on which contact last sent a message. + * If the user just clicked on a mailto: link, then this is the best thing you can do. + * But **DO NOT** internally represent contacts by their email address + * and do not use this function to look them up; + * otherwise this function will sometimes look up the wrong contact. + * Instead, you should internally represent contacts by their ids. + * * To validate an e-mail address independently of the contact database * use dc_may_be_valid_addr(). * @@ -2113,6 +2123,13 @@ uint32_t dc_lookup_contact_id_by_addr (dc_context_t* context, const char* * To add a number of contacts, see dc_add_address_book() which is much faster for adding * a bunch of addresses. * + * This will always create or look up an address-contact, + * i.e. a contact identified by an email address, + * with all messages sent to and from this contact being unencrypted. + * If the user just clicked on an email address, + * you should first check `lookup_contact_id_by_addr`, + * and only if there is no contact yet, call this function here. + * * May result in a #DC_EVENT_CONTACTS_CHANGED event. * * @memberof dc_context_t diff --git a/deltachat-jsonrpc/src/api.rs b/deltachat-jsonrpc/src/api.rs index 166cb81af..a728526fa 100644 --- a/deltachat-jsonrpc/src/api.rs +++ b/deltachat-jsonrpc/src/api.rs @@ -1473,7 +1473,14 @@ impl CommandApi { /// Add a single contact as a result of an explicit user action. /// - /// Returns contact id of the created or existing contact + /// This will always create or look up an address-contact, + /// i.e. a contact identified by an email address, + /// with all messages sent to and from this contact being unencrypted. + /// If the user just clicked on an email address, + /// you should first check [`Self::lookup_contact_id_by_addr`]/`lookupContactIdByAddr.`, + /// and only if there is no contact yet, call this function here. + /// + /// Returns contact id of the created or existing contact. async fn create_contact( &self, account_id: u32, @@ -1623,9 +1630,19 @@ impl CommandApi { Contact::get_encrinfo(&ctx, ContactId::new(contact_id)).await } - /// Check if an e-mail address belongs to a known and unblocked contact. + /// Looks up a known and unblocked contact with a given e-mail address. /// To get a list of all known and unblocked contacts, use contacts_get_contacts(). /// + /// **POTENTIAL SECURITY ISSUE**: If there are multiple contacts with this address + /// (e.g. an address-contact and a key-contact), + /// this looks up the most recently seen contact, + /// i.e. which contact is returned depends on which contact last sent a message. + /// If the user just clicked on a mailto: link, then this is the best thing you can do. + /// But **DO NOT** internally represent contacts by their email address + /// and do not use this function to look them up; + /// otherwise this function will sometimes look up the wrong contact. + /// Instead, you should internally represent contacts by their ids. + /// /// To validate an e-mail address independently of the contact database /// use check_email_validity(). async fn lookup_contact_id_by_addr( diff --git a/deltachat-rpc-client/src/deltachat_rpc_client/account.py b/deltachat-rpc-client/src/deltachat_rpc_client/account.py index 174205714..6f919f71e 100644 --- a/deltachat-rpc-client/src/deltachat_rpc_client/account.py +++ b/deltachat-rpc-client/src/deltachat_rpc_client/account.py @@ -185,7 +185,21 @@ class Account: return Contact(self, contact_id) def get_contact_by_addr(self, address: str) -> Optional[Contact]: - """Check if an e-mail address belongs to a known and unblocked contact.""" + """Looks up a known and unblocked contact with a given e-mail address. + To get a list of all known and unblocked contacts, use contacts_get_contacts(). + + **POTENTIAL SECURITY ISSUE**: If there are multiple contacts with this address + (e.g. an address-contact and a key-contact), + this looks up the most recently seen contact, + i.e. which contact is returned depends on which contact last sent a message. + If the user just clicked on a mailto: link, then this is the best thing you can do. + But **DO NOT** internally represent contacts by their email address + and do not use this function to look them up; + otherwise this function will sometimes look up the wrong contact. + Instead, you should internally represent contacts by their ids. + + To validate an e-mail address independently of the contact database + use check_email_validity().""" contact_id = self._rpc.lookup_contact_id_by_addr(self.id, address) return contact_id and Contact(self, contact_id) diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index 45914a7b1..bd49c0eb8 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/deltachat-rpc-client/tests/test_something.py @@ -171,7 +171,10 @@ def test_account(acfactory) -> None: assert alice.get_size() assert alice.is_configured() assert not alice.get_avatar() - assert alice.get_contact_by_addr(bob_addr) is None # There is no address-contact, only key-contact + # get_contact_by_addr() can lookup a key contact by address: + bob_contact = alice.get_contact_by_addr(bob_addr).get_snapshot() + assert bob_contact.display_name == "Bob" + assert bob_contact.is_key_contact assert alice.get_contacts() assert alice.get_contacts(snapshot=True) assert alice.self_contact diff --git a/python/src/deltachat/account.py b/python/src/deltachat/account.py index c40382db6..647226fe2 100644 --- a/python/src/deltachat/account.py +++ b/python/src/deltachat/account.py @@ -330,7 +330,21 @@ class Account: return bool(lib.dc_delete_contact(self._dc_context, contact_id)) def get_contact_by_addr(self, email: str) -> Optional[Contact]: - """get a contact for the email address or None if it's blocked or doesn't exist.""" + """Looks up a known and unblocked contact with a given e-mail address. + To get a list of all known and unblocked contacts, use contacts_get_contacts(). + + **POTENTIAL SECURITY ISSUE**: If there are multiple contacts with this address + (e.g. an address-contact and a key-contact), + this looks up the most recently seen contact, + i.e. which contact is returned depends on which contact last sent a message. + If the user just clicked on a mailto: link, then this is the best thing you can do. + But **DO NOT** internally represent contacts by their email address + and do not use this function to look them up; + otherwise this function will sometimes look up the wrong contact. + Instead, you should internally represent contacts by their ids. + + To validate an e-mail address independently of the contact database + use check_email_validity().""" _, addr = parseaddr(email) addr = as_dc_charpointer(addr) contact_id = lib.dc_lookup_contact_id_by_addr(self._dc_context, addr) diff --git a/src/contact.rs b/src/contact.rs index ca73060a7..707a37297 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -755,7 +755,19 @@ impl Contact { self.is_bot } - /// Check if an e-mail address belongs to a known and unblocked contact. + /// Looks up a known and unblocked contact with a given e-mail address. + /// To get a list of all known and unblocked contacts, use contacts_get_contacts(). + /// + /// + /// **POTENTIAL SECURITY ISSUE**: If there are multiple contacts with this address + /// (e.g. an address-contact and a key-contact), + /// this looks up the most recently seen contact, + /// i.e. which contact is returned depends on which contact last sent a message. + /// If the user just clicked on a mailto: link, then this is the best thing you can do. + /// But **DO NOT** internally represent contacts by their email address + /// and do not use this function to look them up; + /// otherwise this function will sometimes look up the wrong contact. + /// Instead, you should internally represent contacts by their ids. /// /// Known and unblocked contacts will be returned by `get_contacts()`. /// @@ -795,8 +807,8 @@ impl Contact { .query_get_value( "SELECT id FROM contacts WHERE addr=?1 COLLATE NOCASE - AND fingerprint='' -- Do not lookup key-contacts - AND id>?2 AND origin>=?3 AND (? OR blocked=?)", + AND id>?2 AND origin>=?3 AND (? OR blocked=?) + ORDER BY last_seen DESC LIMIT 1", ( &addr_normalized, ContactId::LAST_SPECIAL, diff --git a/src/contact/contact_tests.rs b/src/contact/contact_tests.rs index 4f5fb4c41..00023c639 100644 --- a/src/contact/contact_tests.rs +++ b/src/contact/contact_tests.rs @@ -1072,6 +1072,7 @@ async fn test_sync_create() -> Result<()> { .unwrap(); let a1b_contact = Contact::get_by_id(alice1, a1b_contact_id).await?; assert_eq!(a1b_contact.name, "Bob"); + assert_eq!(a1b_contact.is_key_contact(), false); Contact::create(alice0, "Bob Renamed", "bob@example.net").await?; test_utils::sync(alice0, alice1).await; @@ -1081,6 +1082,7 @@ async fn test_sync_create() -> Result<()> { assert_eq!(id, a1b_contact_id); let a1b_contact = Contact::get_by_id(alice1, a1b_contact_id).await?; assert_eq!(a1b_contact.name, "Bob Renamed"); + assert_eq!(a1b_contact.is_key_contact(), false); Ok(()) } diff --git a/src/mimefactory/mimefactory_tests.rs b/src/mimefactory/mimefactory_tests.rs index 49fbdc228..d5e12840f 100644 --- a/src/mimefactory/mimefactory_tests.rs +++ b/src/mimefactory/mimefactory_tests.rs @@ -683,6 +683,7 @@ async fn test_selfavatar_unencrypted_signed() { .unwrap() .unwrap(); let alice_contact = Contact::get_by_id(&bob.ctx, alice_id).await.unwrap(); + assert_eq!(alice_contact.is_key_contact(), false); assert!( alice_contact .get_profile_image(&bob.ctx) diff --git a/src/receive_imf/receive_imf_tests.rs b/src/receive_imf/receive_imf_tests.rs index 1f0fa3b16..0fe1b4f16 100644 --- a/src/receive_imf/receive_imf_tests.rs +++ b/src/receive_imf/receive_imf_tests.rs @@ -991,6 +991,7 @@ async fn test_other_device_writes_to_mailinglist() -> Result<()> { .await? .unwrap(); let list_post_contact = Contact::get_by_id(&t, list_post_contact_id).await?; + assert_eq!(list_post_contact.is_key_contact(), false); assert_eq!( list_post_contact.param.get(Param::ListId).unwrap(), "delta.codespeak.net" @@ -1453,6 +1454,7 @@ async fn test_apply_mailinglist_changes_assigned_by_reply() { .unwrap() .unwrap(); let contact = Contact::get_by_id(&t, contact_id).await.unwrap(); + assert_eq!(contact.is_key_contact(), false); assert_eq!( contact.param.get(Param::ListId).unwrap(), "deltachat-core-rust.deltachat.github.com" @@ -3682,10 +3684,13 @@ async fn test_mua_user_adds_recipient_to_single_chat() -> Result<()> { chat::get_chat_contacts(&alice, group_chat.id).await?.len(), 4 ); - let fiona = Contact::lookup_id_by_addr(&alice, "fiona@example.net", Origin::IncomingTo) - .await? - .unwrap(); - assert!(chat::is_contact_in_chat(&alice, group_chat.id, fiona).await?); + let fiona_contact_id = + Contact::lookup_id_by_addr(&alice, "fiona@example.net", Origin::IncomingTo) + .await? + .unwrap(); + assert!(chat::is_contact_in_chat(&alice, group_chat.id, fiona_contact_id).await?); + let fiona_contact = Contact::get_by_id(&alice, fiona_contact_id).await?; + assert_eq!(fiona_contact.is_key_contact(), false); Ok(()) } diff --git a/src/sql/migrations/migrations_tests.rs b/src/sql/migrations/migrations_tests.rs index 8919dd35d..c96fc02d9 100644 --- a/src/sql/migrations/migrations_tests.rs +++ b/src/sql/migrations/migrations_tests.rs @@ -47,6 +47,7 @@ async fn test_key_contacts_migration_autocrypt() -> Result<()> { .await? .unwrap(); let email_bob = Contact::get_by_id(&t, email_bob_id).await?; + assert_eq!(email_bob.is_key_contact(), false); assert_eq!(email_bob.origin, Origin::Hidden); // Email bob is in no chats, so, contact is hidden assert_eq!(email_bob.e2ee_avail(&t).await?, false); assert_eq!(email_bob.fingerprint(), None); @@ -85,6 +86,7 @@ async fn test_key_contacts_migration_email1() -> Result<()> { .await? .unwrap(); let email_bob = Contact::get_by_id(&t, email_bob_id).await?; + assert_eq!(email_bob.is_key_contact(), false); assert_eq!(email_bob.origin, Origin::OutgoingTo); assert_eq!(email_bob.e2ee_avail(&t).await?, false); assert_eq!(email_bob.fingerprint(), None); @@ -114,6 +116,7 @@ async fn test_key_contacts_migration_email2() -> Result<()> { .await? .unwrap(); let email_bob = Contact::get_by_id(&t, email_bob_id).await?; + assert_eq!(email_bob.is_key_contact(), false); assert_eq!(email_bob.origin, Origin::OutgoingTo); // Email bob is in no chats, so, contact is hidden assert_eq!(email_bob.e2ee_avail(&t).await?, false); assert_eq!(email_bob.fingerprint(), None); @@ -148,6 +151,7 @@ async fn test_key_contacts_migration_verified() -> Result<()> { .unwrap(); let email_bob = Contact::get_by_id(&t, email_bob_id).await?; dbg!(&email_bob); + assert_eq!(email_bob.is_key_contact(), false); assert_eq!(email_bob.origin, Origin::Hidden); // Email bob is in no chats, so, contact is hidden assert_eq!(email_bob.e2ee_avail(&t).await?, false); assert_eq!(email_bob.fingerprint(), None);