feat: Also lookup key contacts in lookup_id_by_addr() (#7073)

If there is both a key and an address contact, return the most recently
seen one.

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

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(

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)
 

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

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)

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,

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(())
 }

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)

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)
+    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).await?);
+    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(())
 }

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);