Return Option from Contact::add_or_lookup()

This allows to distinguish exceptions,
such as database errors, from invalid user input.
For example, if the From: field of the message
does not look like an email address, the mail
should be ignored. But if there is a database
failure while writing a new contact for the address,
this error should be bubbled up.
This commit is contained in:
link2xt
2023-01-09 22:28:06 +00:00
parent 5ae6c25394
commit e215b4d919
13 changed files with 176 additions and 56 deletions

View File

@@ -4693,7 +4693,9 @@ mod tests {
assert!(!shall_attach_selfavatar(&t, chat_id).await?);
let (contact_id, _) =
Contact::add_or_lookup(&t, "", "foo@bar.org", Origin::IncomingUnknownTo).await?;
Contact::add_or_lookup(&t, "", "foo@bar.org", Origin::IncomingUnknownTo)
.await?
.unwrap();
add_contact_to_chat(&t, chat_id, contact_id).await?;
assert!(!shall_attach_selfavatar(&t, chat_id).await?);
t.set_config(Config::Selfavatar, None).await?; // setting to None also forces re-sending
@@ -4940,7 +4942,9 @@ mod tests {
bob.set_config(Config::ShowEmails, Some("2")).await?;
let (contact_id, _) =
Contact::add_or_lookup(&alice, "", "bob@example.net", Origin::ManuallyCreated).await?;
Contact::add_or_lookup(&alice, "", "bob@example.net", Origin::ManuallyCreated)
.await?
.unwrap();
let alice_chat_id = create_group_chat(&alice, ProtectionStatus::Unprotected, "grp").await?;
let alice_chat = Chat::load_from_db(&alice, alice_chat_id).await?;
@@ -5651,7 +5655,9 @@ mod tests {
async fn test_create_for_contact_with_blocked() -> Result<()> {
let t = TestContext::new().await;
let (contact_id, _) =
Contact::add_or_lookup(&t, "", "foo@bar.org", Origin::ManuallyCreated).await?;
Contact::add_or_lookup(&t, "", "foo@bar.org", Origin::ManuallyCreated)
.await?
.unwrap();
// create a blocked chat
let chat_id_orig =

View File

@@ -376,12 +376,14 @@ impl Contact {
/// May result in a `#DC_EVENT_CONTACTS_CHANGED` event.
pub async fn create(context: &Context, name: &str, addr: &str) -> Result<ContactId> {
let name = improve_single_line_input(name);
ensure!(!addr.is_empty(), "Cannot create contact with empty address");
let (name, addr) = sanitize_name_and_addr(&name, addr);
let (contact_id, sth_modified) =
Contact::add_or_lookup(context, &name, &addr, Origin::ManuallyCreated).await?;
Contact::add_or_lookup(context, &name, &addr, Origin::ManuallyCreated)
.await
.context("add_or_lookup")?
.with_context(|| format!("can't create a contact with address {:?}", addr))?;
let blocked = Contact::is_blocked_load(context, contact_id).await?;
match sth_modified {
Modifier::None => {}
@@ -466,12 +468,14 @@ impl Contact {
/// Depending on the origin, both, "row_name" and "row_authname" are updated from "name".
///
/// Returns the contact_id and a `Modifier` value indicating if a modification occurred.
///
/// Returns None if the contact with such address cannot exist.
pub(crate) async fn add_or_lookup(
context: &Context,
name: &str,
addr: &str,
mut origin: Origin,
) -> Result<(ContactId, Modifier)> {
) -> Result<Option<(ContactId, Modifier)>> {
let mut sth_modified = Modifier::None;
ensure!(!addr.is_empty(), "Can not add_or_lookup empty address");
@@ -480,7 +484,7 @@ impl Contact {
let addr = addr_normalize(addr).to_string();
if context.is_self_addr(&addr).await? {
return Ok((ContactId::SELF, sth_modified));
return Ok(Some((ContactId::SELF, sth_modified)));
}
if !may_be_valid_addr(&addr) {
@@ -490,7 +494,7 @@ impl Contact {
addr,
if !name.is_empty() { name } else { "<unset>" },
);
bail!("Bad address supplied: {:?}", addr);
return Ok(None);
}
let mut name = name;
@@ -653,7 +657,7 @@ impl Contact {
}
}
Ok((ContactId::new(row_id), sth_modified))
Ok(Some((ContactId::new(row_id), sth_modified)))
}
/// Add a number of contacts.
@@ -686,7 +690,10 @@ impl Contact {
"Failed to add address {} from address book: {}", addr, err
);
}
Ok((_, modified)) => {
Ok(None) => {
warn!(context, "Cannot create contact with address {}.", addr);
}
Ok(Some((_, modified))) => {
if modified != Modifier::None {
modify_cnt += 1
}
@@ -1697,7 +1704,8 @@ mod tests {
"user@example.org",
Origin::IncomingReplyTo,
)
.await?;
.await?
.unwrap();
assert_ne!(id, ContactId::UNDEFINED);
let contact = Contact::load_from_db(&context.ctx, id).await.unwrap();
@@ -1725,7 +1733,8 @@ mod tests {
"user@example.org",
Origin::ManuallyCreated,
)
.await?;
.await?
.unwrap();
assert_eq!(contact_bob_id, id);
assert_eq!(modified, Modifier::Modified);
let contact = Contact::load_from_db(&context.ctx, id).await.unwrap();
@@ -1775,6 +1784,7 @@ mod tests {
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bla foo", "one@eins.org", Origin::IncomingUnknownTo)
.await
.unwrap()
.unwrap();
assert!(!contact_id.is_special());
assert_eq!(sth_modified, Modifier::Modified);
@@ -1790,6 +1800,7 @@ mod tests {
let (contact_id_test, sth_modified) =
Contact::add_or_lookup(&t, "Real one", " one@eins.org ", Origin::ManuallyCreated)
.await
.unwrap()
.unwrap();
assert_eq!(contact_id, contact_id_test);
assert_eq!(sth_modified, Modifier::Modified);
@@ -1802,6 +1813,7 @@ mod tests {
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "", "three@drei.sam", Origin::IncomingUnknownTo)
.await
.unwrap()
.unwrap();
assert!(!contact_id.is_special());
assert_eq!(sth_modified, Modifier::None);
@@ -1819,6 +1831,7 @@ mod tests {
Origin::IncomingUnknownFrom,
)
.await
.unwrap()
.unwrap();
assert_eq!(contact_id, contact_id_test);
assert_eq!(sth_modified, Modifier::Modified);
@@ -1830,6 +1843,7 @@ mod tests {
let (contact_id_test, sth_modified) =
Contact::add_or_lookup(&t, "schnucki", "three@drei.sam", Origin::ManuallyCreated)
.await
.unwrap()
.unwrap();
assert_eq!(contact_id, contact_id_test);
assert_eq!(sth_modified, Modifier::Modified);
@@ -1842,6 +1856,7 @@ mod tests {
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "", "alice@w.de", Origin::IncomingUnknownTo)
.await
.unwrap()
.unwrap();
assert!(!contact_id.is_special());
assert_eq!(sth_modified, Modifier::None);
@@ -1979,7 +1994,8 @@ mod tests {
// Create Bob contact
let (contact_id, _) =
Contact::add_or_lookup(&alice, "Bob", "bob@example.net", Origin::ManuallyCreated)
.await?;
.await?
.unwrap();
let chat = alice
.create_chat_with_contact("Bob", "bob@example.net")
.await;
@@ -2055,6 +2071,7 @@ mod tests {
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bob1", "bob@example.org", Origin::IncomingUnknownFrom)
.await
.unwrap()
.unwrap();
assert!(!contact_id.is_special());
assert_eq!(sth_modified, Modifier::Created);
@@ -2067,6 +2084,7 @@ mod tests {
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bob2", "bob@example.org", Origin::IncomingUnknownFrom)
.await
.unwrap()
.unwrap();
assert!(!contact_id.is_special());
assert_eq!(sth_modified, Modifier::Modified);
@@ -2089,6 +2107,7 @@ mod tests {
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bob4", "bob@example.org", Origin::IncomingUnknownFrom)
.await
.unwrap()
.unwrap();
assert!(!contact_id.is_special());
assert_eq!(sth_modified, Modifier::Modified);
@@ -2118,6 +2137,7 @@ mod tests {
Origin::IncomingUnknownFrom,
)
.await
.unwrap()
.unwrap();
assert_eq!(contact_id, contact_id_same);
assert_eq!(sth_modified, Modifier::Modified);
@@ -2134,6 +2154,7 @@ mod tests {
Origin::IncomingUnknownFrom,
)
.await
.unwrap()
.unwrap();
assert_eq!(contact_id, contact_id_same);
assert_eq!(sth_modified, Modifier::Modified);
@@ -2154,7 +2175,8 @@ mod tests {
// Incoming message from Bob.
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "Bob", "bob@example.org", Origin::IncomingUnknownFrom)
.await?;
.await?
.unwrap();
assert_eq!(sth_modified, Modifier::Created);
let contact = Contact::load_from_db(&t, contact_id).await?;
assert_eq!(contact.get_display_name(), "Bob");
@@ -2162,7 +2184,8 @@ mod tests {
// Incoming message from someone else with "Not Bob" <bob@example.org> in the "To:" field.
let (contact_id_same, sth_modified) =
Contact::add_or_lookup(&t, "Not Bob", "bob@example.org", Origin::IncomingUnknownTo)
.await?;
.await?
.unwrap();
assert_eq!(contact_id, contact_id_same);
assert_eq!(sth_modified, Modifier::Modified);
let contact = Contact::load_from_db(&t, contact_id).await?;
@@ -2171,7 +2194,8 @@ mod tests {
// Incoming message from Bob, changing the name back.
let (contact_id_same, sth_modified) =
Contact::add_or_lookup(&t, "Bob", "bob@example.org", Origin::IncomingUnknownFrom)
.await?;
.await?
.unwrap();
assert_eq!(contact_id, contact_id_same);
assert_eq!(sth_modified, Modifier::Modified); // This was None until the bugfix
let contact = Contact::load_from_db(&t, contact_id).await?;
@@ -2312,7 +2336,8 @@ mod tests {
let (contact_bob_id, _modified) =
Contact::add_or_lookup(&alice, "Bob", "bob@example.net", Origin::ManuallyCreated)
.await?;
.await?
.unwrap();
let encrinfo = Contact::get_encrinfo(&alice, contact_bob_id).await?;
assert_eq!(encrinfo, "No encryption");
@@ -2471,7 +2496,8 @@ CCCB 5AA9 F6E1 141C 9431
let (contact_id, _) =
Contact::add_or_lookup(&alice, "Bob", "bob@example.net", Origin::ManuallyCreated)
.await?;
.await?
.unwrap();
let contact = Contact::load_from_db(&alice, contact_id).await?;
assert_eq!(contact.last_seen(), 0);

View File

@@ -1737,7 +1737,19 @@ async fn should_move_out_of_spam(
};
// No chat found.
let (from_id, blocked_contact, _origin) =
from_field_to_contact_id(context, &from, true).await?;
match from_field_to_contact_id(context, &from, true)
.await
.context("from_field_to_contact_id")?
{
Some(res) => res,
None => {
warn!(
context,
"Contact with From address {:?} cannot exist, not moving out of spam", from
);
return Ok(false);
}
};
if blocked_contact {
// Contact is blocked, leave the message in spam.
return Ok(false);
@@ -2027,7 +2039,10 @@ pub(crate) async fn prefetch_should_download(
None => return Ok(false),
};
let (_from_id, blocked_contact, origin) =
from_field_to_contact_id(context, &from, true).await?;
match from_field_to_contact_id(context, &from, true).await? {
Some(res) => res,
None => return Ok(false),
};
// prevent_rename=true as this might be a mailing list message and in this case it would be bad if we rename the contact.
// (prevent_rename is the last argument of from_field_to_contact_id())
@@ -2347,14 +2362,14 @@ async fn add_all_recipients_as_contacts(
.await
.with_context(|| format!("could not select {}", mailbox))?;
let contacts = imap
let recipients = imap
.get_all_recipients(context)
.await
.context("could not get recipients")?;
let mut any_modified = false;
for contact in contacts {
let display_name_normalized = contact
for recipient in recipients {
let display_name_normalized = recipient
.display_name
.as_ref()
.map(|s| normalize_name(s))
@@ -2363,17 +2378,20 @@ async fn add_all_recipients_as_contacts(
match Contact::add_or_lookup(
context,
&display_name_normalized,
&contact.addr,
&recipient.addr,
Origin::OutgoingTo,
)
.await
.await?
{
Ok((_, modified)) => {
Some((_, modified)) => {
if modified != Modifier::None {
any_modified = true;
}
}
Err(e) => warn!(context, "Could not add recipient: {}", e),
None => warn!(
context,
"Could not add contact for recipient with address {:?}", recipient.addr
),
}
}
if any_modified {

View File

@@ -1821,6 +1821,7 @@ mod tests {
Contact::add_or_lookup(&t, "Dave", "dave@example.com", Origin::ManuallyCreated)
.await
.unwrap()
.unwrap()
.0;
let chat_id = ChatId::create_for_contact(&t, contact_id).await.unwrap();

View File

@@ -542,14 +542,17 @@ impl Peerstate {
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, _) =
if let Some((new_contact_id, _)) =
Contact::add_or_lookup(context, "", new_addr, Origin::IncomingUnknownFrom)
.await?
{
chat::remove_from_chat_contacts_table(context, *chat_id, contact_id)
.await?;
chat::add_to_chat_contacts_table(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));
context.emit_event(EventType::ChatModified(*chat_id));
}
}
}

View File

@@ -221,10 +221,15 @@ async fn decode_openpgp(context: &Context, qr: &str) -> Result<Qr> {
.context("Can't load peerstate")?;
if let (Some(addr), Some(invitenumber), Some(authcode)) = (&addr, invitenumber, authcode) {
let contact_id = Contact::add_or_lookup(context, &name, addr, Origin::UnhandledQrScan)
let (contact_id, _) = Contact::add_or_lookup(context, &name, addr, Origin::UnhandledQrScan)
.await
.map(|(id, _)| id)
.with_context(|| format!("failed to add or lookup contact for address {:?}", addr))?;
.with_context(|| format!("failed to add or lookup contact for address {:?}", addr))?
.with_context(|| {
format!(
"do not want to lookup contact for invalid address {:?}",
addr
)
})?;
if let (Some(grpid), Some(grpname)) = (grpid, grpname) {
if context
@@ -287,10 +292,13 @@ async fn decode_openpgp(context: &Context, qr: &str) -> Result<Qr> {
}
} else if let Some(addr) = addr {
if let Some(peerstate) = peerstate {
let contact_id =
let (contact_id, _) =
Contact::add_or_lookup(context, &name, &peerstate.addr, Origin::UnhandledQrScan)
.await
.map(|(id, _)| id)?;
.context("add_or_lookup")?
.with_context(|| {
format!("Not looking up contact for address {:?}", &peerstate.addr)
})?;
let chat = ChatIdBlocked::get_for_contact(context, contact_id, Blocked::Request)
.await
.context("Failed to create (new) chat for contact")?;
@@ -620,7 +628,9 @@ impl Qr {
draft: Option<String>,
) -> Result<Self> {
let (contact_id, _) =
Contact::add_or_lookup(context, &name, &addr, Origin::UnhandledQrScan).await?;
Contact::add_or_lookup(context, &name, &addr, Origin::UnhandledQrScan)
.await?
.context("QR code does not contain a valid address")?;
Ok(Qr::Addr { contact_id, draft })
}
}

View File

@@ -368,6 +368,7 @@ Can we chat at 1pm pacific, today?"
let bob_id = Contact::add_or_lookup(&alice, "", "bob@example.net", Origin::ManuallyCreated)
.await?
.unwrap()
.0;
let bob_reaction = reactions.get(bob_id);
assert!(bob_reaction.is_empty()); // Bob has not reacted to message yet.

View File

@@ -170,7 +170,16 @@ pub(crate) async fn receive_imf_inner(
// If this is a mailing list email (i.e. list_id_header is some), don't change the displayname because in
// a mailing list the sender displayname sometimes does not belong to the sender email address.
let (from_id, _from_id_blocked, incoming_origin) =
from_field_to_contact_id(context, &mime_parser.from, prevent_rename).await?;
match from_field_to_contact_id(context, &mime_parser.from, prevent_rename).await? {
Some(contact_id_res) => contact_id_res,
None => {
warn!(
context,
"receive_imf: From field does not contain an acceptable address"
);
return Ok(None);
}
};
let incoming = from_id != ContactId::SELF;
@@ -373,22 +382,28 @@ pub async fn from_field_to_contact_id(
context: &Context,
from: &SingleInfo,
prevent_rename: bool,
) -> Result<(ContactId, bool, Origin)> {
) -> Result<Option<(ContactId, bool, Origin)>> {
let display_name = if prevent_rename {
Some("")
} else {
from.display_name.as_deref()
};
let from_id = add_or_lookup_contact_by_addr(
let from_id = if let Some(from_id) = add_or_lookup_contact_by_addr(
context,
display_name,
&from.addr,
Origin::IncomingUnknownFrom,
)
.await?;
.await
.context("add_or_lookup_contact_by_addr")?
{
from_id
} else {
return Ok(None);
};
if from_id == ContactId::SELF {
Ok((ContactId::SELF, false, Origin::OutgoingBcc))
Ok(Some((ContactId::SELF, false, Origin::OutgoingBcc)))
} else {
let mut from_id_blocked = false;
let mut incoming_origin = Origin::Unknown;
@@ -396,7 +411,7 @@ pub async fn from_field_to_contact_id(
from_id_blocked = contact.blocked;
incoming_origin = contact.origin;
}
Ok((from_id, from_id_blocked, incoming_origin))
Ok(Some((from_id, from_id_blocked, incoming_origin)))
}
}
@@ -1928,8 +1943,11 @@ async fn apply_mailinglist_changes(
}
let listid = &chat.grpid;
let (contact_id, _) =
Contact::add_or_lookup(context, "", list_post, Origin::Hidden).await?;
let contact_id =
match Contact::add_or_lookup(context, "", list_post, Origin::Hidden).await? {
Some((contact_id, _)) => contact_id,
None => return Ok(()),
};
let mut contact = Contact::load_from_db(context, contact_id).await?;
if contact.param.get(Param::ListId) != Some(listid) {
contact.param.set(Param::ListId, listid);
@@ -2268,28 +2286,39 @@ async fn add_or_lookup_contacts_by_address_list(
continue;
}
let display_name = info.display_name.as_deref();
contact_ids
.insert(add_or_lookup_contact_by_addr(context, display_name, addr, origin).await?);
if let Some(contact_id) =
add_or_lookup_contact_by_addr(context, display_name, addr, origin).await?
{
contact_ids.insert(contact_id);
} else {
warn!(context, "Contact with address {:?} cannot exist.", addr);
}
}
Ok(contact_ids.into_iter().collect::<Vec<ContactId>>())
}
/// Add contacts to database on receiving messages.
///
/// Returns `None` if the address can't be a valid email address.
async fn add_or_lookup_contact_by_addr(
context: &Context,
display_name: Option<&str>,
addr: &str,
origin: Origin,
) -> Result<ContactId> {
) -> Result<Option<ContactId>> {
if context.is_self_addr(addr).await? {
return Ok(ContactId::SELF);
return Ok(Some(ContactId::SELF));
}
let display_name_normalized = display_name.map(normalize_name).unwrap_or_default();
let (row_id, _modified) =
Contact::add_or_lookup(context, &display_name_normalized, addr, origin).await?;
Ok(row_id)
if let Some((contact_id, _modified)) =
Contact::add_or_lookup(context, &display_name_normalized, addr, origin).await?
{
Ok(Some(contact_id))
} else {
Ok(None)
}
}
#[cfg(test)]

View File

@@ -429,6 +429,7 @@ async fn test_escaped_recipients() {
Contact::add_or_lookup(&t, "Carl", "carl@host.tld", Origin::IncomingUnknownFrom)
.await
.unwrap()
.unwrap()
.0;
receive_imf(
@@ -471,6 +472,7 @@ async fn test_cc_to_contact() {
Contact::add_or_lookup(&t, "garabage", "carl@host.tld", Origin::IncomingUnknownFrom)
.await
.unwrap()
.unwrap()
.0;
receive_imf(
@@ -2058,6 +2060,7 @@ async fn test_duplicate_message() -> Result<()> {
Origin::IncomingUnknownFrom,
)
.await?
.unwrap()
.0;
let first_message = b"Received: from [127.0.0.1]
@@ -2111,6 +2114,7 @@ async fn test_ignore_footer_status_from_mailinglist() -> Result<()> {
t.set_config(Config::ShowEmails, Some("2")).await?;
let bob_id = Contact::add_or_lookup(&t, "", "bob@example.net", Origin::IncomingUnknownCc)
.await?
.unwrap()
.0;
let bob = Contact::load_from_db(&t, bob_id).await?;
assert_eq!(bob.get_status(), "");
@@ -2529,7 +2533,8 @@ Second thread."#;
"fiona@example.net",
Origin::IncomingUnknownTo,
)
.await?;
.await?
.unwrap();
chat::add_contact_to_chat(&alice, alice_first_msg.chat_id, alice_fiona_contact_id).await?;
let alice_first_invite = alice.pop_sent_msg().await;

View File

@@ -1006,7 +1006,8 @@ mod tests {
"bob@example.net",
Origin::ManuallyCreated,
)
.await?;
.await?
.unwrap();
let contact_bob = Contact::load_from_db(&alice.ctx, contact_bob_id).await?;
assert_eq!(
contact_bob.is_verified(&alice.ctx).await?,

View File

@@ -529,7 +529,11 @@ impl TestContext {
let (contact_id, modified) =
Contact::add_or_lookup(self, &name, &addr, Origin::MailinglistAddress)
.await
.unwrap();
.expect("add_or_lookup")
.expect(&format!(
"contact with address {:?} cannot be created",
&addr
));
match modified {
Modifier::None => (),
Modifier::Modified => warn!(&self.ctx, "Contact {} modified by TestContext", &addr),