Mirrors/chatmail-core
1
0
Fork 0
mirror of https://github.com/chatmail/core.git synced 2026-04-13 11:40:41 +03:00
Code Issues Packages Projects Releases Wiki Activity

Fix contact name update rules

Browse Source 
The following rules apply now:
1. "name" column is only updated manually and never over the network
2. "authname" column is only updated over the network and never manually
3. Displayname is a "name" if it is non-empty, otherwise it is "authname".

This fixes a known (pytest.xfail) problem of "name" being changed over
the network when user has set it to non-empty string manually.

This also fixes the problem when "name" and "authname"
became unsynchronized accidentally, when they were equal and then
Origin::IncomingUnknownTo update arrived, setting "name" but not
"authname". Rust regression test is added for this case.
This commit is contained in:
link2xt
2021-02-08 22:46:53 +03:00
committed by link2xt
parent 6a4624be25
commit 3f9242a610
5 changed files with 170 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
python/tests/test_account.py
View File

@@ -435,11 +435,11 @@ class TestOfflineChat:
email = "hello <hello@example.org>"
contact1 = ac1.create_contact(email)
assert contact1.addr == "hello@example.org"
assert contact1.display_name == "hello"
assert contact1.name == "hello"
contact1 = ac1.create_contact(email, name="world")
assert contact1.display_name == "world"
assert contact1.name == "world"
contact2 = ac1.create_contact("display1 <x@example.org>", "real")
assert contact2.display_name == "real"
assert contact2.name == "real"
def test_create_chat_simple(self, acfactory):
ac1 = acfactory.get_configured_offline_account()
@@ -2131,7 +2131,9 @@ class TestOnlineAccount:
ac1, ac2 = acfactory.get_two_online_accounts()
ac1.set_config("displayname", "Account 1")
chat12 = acfactory.get_accepted_chat(ac1, ac2)
# Similar to acfactory.get_accepted_chat, but without setting the contact name.
ac2.create_contact(ac1.get_config("addr")).create_chat()
chat12 = ac1.create_contact(ac2.get_config("addr")).create_chat()
contact = None
def update_name():
@@ -2162,12 +2164,7 @@ class TestOnlineAccount:
# so it should not be changed.
ac1.set_config("displayname", "Renamed again")
updated_name = update_name()
if updated_name == "Renamed again":
# Known bug, mark as XFAIL
pytest.xfail("Contact was renamed after explicit rename")
else:
# No renames should happen after explicit rename
assert updated_name == "Renamed"
assert updated_name == "Renamed"
def test_group_quote(self, acfactory, lp):
"""Test quoting in a group with a new member who have not seen the quoted message."""

216
src/contact.rs
View File

@@ -400,9 +400,16 @@ impl Contact {
}
}
// If the origin indicates that user entered the contact manually, from the address book or
// from the QR-code scan (potentially from the address book of their other phone), then name
// should go into the "name" column and never into "authname" column, to avoid leaking it
// into the network.
let manual = matches!(
origin,
Origin::ManuallyCreated | Origin::AddressBook | Origin::UnhandledQrScan
);
let mut update_addr = false;
let mut update_name = false;
let mut update_authname = false;
let mut row_id = 0;
if let Ok((id, row_name, row_addr, row_origin, row_authname)) = context.sql.query_row(
@@ -415,39 +422,22 @@ impl Contact {
let row_origin: Origin = row.get(3)?;
let row_authname: String = row.get(4)?;
if !name.is_empty() {
if !row_name.is_empty() {
if (origin >= row_origin || row_name == row_authname)
&& name != row_name
{
update_name = true;
}
} else {
update_name = true;
}
if origin == Origin::IncomingUnknownFrom && name != row_authname {
update_authname = true;
}
} else if origin == Origin::ManuallyCreated && !row_authname.is_empty() {
// no name given on manual edit, this will update the name to the authname
update_name = true;
}
Ok((row_id, row_name, row_addr, row_origin, row_authname))
},
)
.await {
let update_name = manual && name != row_name;
let update_authname =
!manual && name != row_authname && !name.is_empty() &&
(origin >= row_origin || origin == Origin::IncomingUnknownFrom || row_authname.is_empty());
row_id = id;
if origin as i32 >= row_origin as i32 && addr != row_addr {
update_addr = true;
}
if update_name || update_authname || update_addr || origin > row_origin {
let new_name = if update_name {
if !name.is_empty() {
name.to_string()
} else {
row_authname.clone()
}
name.to_string()
} else {
row_name
};
@@ -496,16 +486,15 @@ impl Contact {
sth_modified = Modifier::Modified;
}
} else {
if origin == Origin::IncomingUnknownFrom {
update_authname = true;
}
let update_name = manual;
let update_authname = !manual;
if context
.sql
.execute(
"INSERT INTO contacts (name, addr, origin, authname) VALUES(?, ?, ?, ?);",
paramsv![
name.to_string(),
if update_name { name.to_string() } else { "".to_string() },
addr,
origin,
if update_authname { name.to_string() } else { "".to_string() }
@@ -613,9 +602,9 @@ impl Contact {
AND c.id>?2 \
AND c.origin>=?3 \
AND c.blocked=0 \
AND (c.name LIKE ?4 OR c.addr LIKE ?5) \
AND (iif(c.name='',c.authname,c.name) LIKE ?4 OR c.addr LIKE ?5) \
AND (1=?6 OR LENGTH(ps.verified_key_fingerprint)!=0) \
ORDER BY LOWER(c.name||c.addr),c.id;",
ORDER BY LOWER(iif(c.name='',c.authname,c.name)||c.addr),c.id;",
paramsv![
self_addr,
DC_CONTACT_ID_LAST_SPECIAL as i32,
@@ -653,17 +642,25 @@ impl Contact {
} else {
add_self = true;
context.sql.query_map(
"SELECT id FROM contacts WHERE addr!=?1 AND id>?2 AND origin>=?3 AND blocked=0 ORDER BY LOWER(name||addr),id;",
paramsv![self_addr, DC_CONTACT_ID_LAST_SPECIAL as i32, 0x100],
|row| row.get::<_, i32>(0),
|ids| {
for id in ids {
ret.push(id? as u32);
}
Ok(())
}
).await?;
context
.sql
.query_map(
"SELECT id FROM contacts
WHERE addr!=?1
AND id>?2
AND origin>=?3
AND blocked=0
ORDER BY LOWER(iif(name='',authname,name)||addr),id;",
paramsv![self_addr, DC_CONTACT_ID_LAST_SPECIAL as i32, 0x100],
|row| row.get::<_, i32>(0),
|ids| {
for id in ids {
ret.push(id? as u32);
}
Ok(())
},
)
.await?;
}
if flag_add_self && add_self {
@@ -903,9 +900,12 @@ impl Contact {
/// The attached email address makes the question unique, eg. "Chat with Alan Miller (am@uniquedomain.com)?"
pub fn get_name_n_addr(&self) -> String {
if !self.name.is_empty() {
return format!("{} ({})", self.name, self.addr);
format!("{} ({})", self.name, self.addr)
} else if !self.authname.is_empty() {
format!("{} ({})", self.authname, self.addr)
} else {
(&self.addr).into()
}
(&self.addr).into()
}
/// Get the contact's profile image.
@@ -1300,27 +1300,65 @@ mod tests {
}
#[async_std::test]
async fn test_get_contacts() {
async fn test_get_contacts() -> Result<()> {
let context = TestContext::new().await;
let contacts = Contact::get_all(&context.ctx, 0, Some("some2"))
.await
.unwrap();
// Bob is not in the contacts yet.
let contacts = Contact::get_all(&context.ctx, 0, Some("bob")).await?;
assert_eq!(contacts.len(), 0);
let id = Contact::create(&context.ctx, "bob", "bob@mail.de")
.await
.unwrap();
let (id, _modified) = Contact::add_or_lookup(
&context.ctx,
"bob",
"user@example.org",
Origin::IncomingReplyTo,
)
.await?;
assert_ne!(id, 0);
let contacts = Contact::get_all(&context.ctx, 0, Some("bob"))
.await
.unwrap();
assert_eq!(contacts.len(), 1);
let contact = Contact::load_from_db(&context.ctx, id).await.unwrap();
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_authname(), "bob");
assert_eq!(contact.get_display_name(), "bob");
let contacts = Contact::get_all(&context.ctx, 0, Some("alice"))
.await
.unwrap();
// Search by name.
let contacts = Contact::get_all(&context.ctx, 0, Some("bob")).await?;
assert_eq!(contacts.len(), 1);
assert_eq!(contacts.get(0), Some(&id));
// Search by address.
let contacts = Contact::get_all(&context.ctx, 0, Some("user")).await?;
assert_eq!(contacts.len(), 1);
assert_eq!(contacts.get(0), Some(&id));
let contacts = Contact::get_all(&context.ctx, 0, Some("alice")).await?;
assert_eq!(contacts.len(), 0);
// Set Bob name to "someone" manually.
let (contact_bob_id, modified) = Contact::add_or_lookup(
&context.ctx,
"someone",
"user@example.org",
Origin::ManuallyCreated,
)
.await?;
assert_eq!(contact_bob_id, id);
assert_eq!(modified, Modifier::Modified);
let contact = Contact::load_from_db(&context.ctx, id).await.unwrap();
assert_eq!(contact.get_name(), "someone");
assert_eq!(contact.get_authname(), "bob");
assert_eq!(contact.get_display_name(), "someone");
// Not searchable by authname, because it is not displayed.
let contacts = Contact::get_all(&context.ctx, 0, Some("bob")).await?;
assert_eq!(contacts.len(), 0);
// Search by display name (same as manually set name).
let contacts = Contact::get_all(&context.ctx, 0, Some("someone")).await?;
assert_eq!(contacts.len(), 1);
assert_eq!(contacts.get(0), Some(&id));
Ok(())
}
#[async_std::test]
@@ -1349,16 +1387,17 @@ mod tests {
);
assert_eq!(Contact::add_address_book(&t, book).await.unwrap(), 4);
// check first added contact, this does not modify because of lower origin
// check first added contact, this modifies authname beacuse it is empty
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bla foo", "one@eins.org", Origin::IncomingUnknownTo)
.await
.unwrap();
assert!(contact_id > DC_CONTACT_ID_LAST_SPECIAL);
assert_eq!(sth_modified, Modifier::None);
assert_eq!(sth_modified, Modifier::Modified);
let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
assert_eq!(contact.get_id(), contact_id);
assert_eq!(contact.get_name(), "Name one");
assert_eq!(contact.get_authname(), "bla foo");
assert_eq!(contact.get_display_name(), "Name one");
assert_eq!(contact.get_addr(), "one@eins.org");
assert_eq!(contact.get_name_n_addr(), "Name one (one@eins.org)");
@@ -1440,7 +1479,7 @@ mod tests {
async fn test_remote_authnames() {
let t = TestContext::new().await;
// incoming mail `From: bob1 <bob@example.org>` - this should init authname and name
// incoming mail `From: bob1 <bob@example.org>` - this should init authname
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bob1", "bob@example.org", Origin::IncomingUnknownFrom)
.await
@@ -1449,10 +1488,10 @@ mod tests {
assert_eq!(sth_modified, Modifier::Created);
let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
assert_eq!(contact.get_authname(), "bob1");
assert_eq!(contact.get_name(), "bob1");
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "bob1");
// incoming mail `From: bob2 <bob@example.org>` - this should update authname and name
// incoming mail `From: bob2 <bob@example.org>` - this should update authname
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "bob2", "bob@example.org", Origin::IncomingUnknownFrom)
.await
@@ -1461,10 +1500,10 @@ mod tests {
assert_eq!(sth_modified, Modifier::Modified);
let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
assert_eq!(contact.get_authname(), "bob2");
assert_eq!(contact.get_name(), "bob2");
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "bob2");
// manually edit name to "bob3" - authname should be still be "bob2" a given in `From:` above
// manually edit name to "bob3" - authname should be still be "bob2" as given in `From:` above
let contact_id = Contact::create(&t, "bob3", "bob@example.org")
.await
.unwrap();
@@ -1499,7 +1538,7 @@ mod tests {
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "claire@example.org");
// incoming mail `From: claire1 <claire@example.org>` - this should update authname and name
// incoming mail `From: claire1 <claire@example.org>` - this should update authname
let (contact_id_same, sth_modified) = Contact::add_or_lookup(
&t,
"claire1",
@@ -1512,10 +1551,10 @@ mod tests {
assert_eq!(sth_modified, Modifier::Modified);
let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
assert_eq!(contact.get_authname(), "claire1");
assert_eq!(contact.get_name(), "claire1");
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "claire1");
// incoming mail `From: claire2 <claire@example.org>` - this should update authname and name
// incoming mail `From: claire2 <claire@example.org>` - this should update authname
let (contact_id_same, sth_modified) = Contact::add_or_lookup(
&t,
"claire2",
@@ -1528,10 +1567,47 @@ mod tests {
assert_eq!(sth_modified, Modifier::Modified);
let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
assert_eq!(contact.get_authname(), "claire2");
assert_eq!(contact.get_name(), "claire2");
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "claire2");
}
/// Regression test.
///
/// In the past, "Not Bob" name was stuck until "Bob" changed the name to "Not Bob" and back in
/// the "From:" field or user set the name to empty string manually.
#[async_std::test]
async fn test_remote_authnames_update_to() -> Result<()> {
let t = TestContext::new().await;
// Incoming message from Bob.
let (contact_id, sth_modified) =
Contact::add_or_lookup(&t, "Bob", "bob@example.org", Origin::IncomingUnknownFrom)
.await?;
assert_eq!(sth_modified, Modifier::Created);
let contact = Contact::load_from_db(&t, contact_id).await?;
assert_eq!(contact.get_display_name(), "Bob");
// 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?;
assert_eq!(contact_id, contact_id_same);
assert_eq!(sth_modified, Modifier::Modified);
let contact = Contact::load_from_db(&t, contact_id).await?;
assert_eq!(contact.get_display_name(), "Not Bob");
// 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?;
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?;
assert_eq!(contact.get_display_name(), "Bob");
Ok(())
}
#[async_std::test]
async fn test_remote_authnames_edit_empty() {
let t = TestContext::new().await;
@@ -1558,7 +1634,7 @@ mod tests {
Contact::create(&t, "", "dave@example.org").await.unwrap();
let contact = Contact::load_from_db(&t, contact_id).await.unwrap();
assert_eq!(contact.get_authname(), "dave2");
assert_eq!(contact.get_name(), "dave2");
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "dave2");
}

20
src/dc_receive_imf.rs
View File

@@ -2422,13 +2422,9 @@ mod tests {
)
.await
.unwrap();
assert_eq!(
Contact::load_from_db(&t, carl_contact_id)
.await
.unwrap()
.get_name(),
"h2"
);
let contact = Contact::load_from_db(&t, carl_contact_id).await.unwrap();
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "h2");
let chats = Chatlist::try_load(&t, 0, None, None).await.unwrap();
let msg = Message::load_from_db(&t, chats.get_msg_id(0).unwrap())
@@ -2471,13 +2467,9 @@ mod tests {
)
.await
.unwrap();
assert_eq!(
Contact::load_from_db(&t, carl_contact_id)
.await
.unwrap()
.get_name(),
"Carl"
);
let contact = Contact::load_from_db(&t, carl_contact_id).await.unwrap();
assert_eq!(contact.get_name(), "");
assert_eq!(contact.get_display_name(), "Carl");
}
#[async_std::test]

2
src/qr.rs
View File

@@ -490,6 +490,8 @@ mod tests {
let contact = Contact::get_by_id(&ctx.ctx, res.get_id()).await.unwrap();
assert_eq!(contact.get_addr(), "stress@test.local");
assert_eq!(contact.get_name(), "First Last");
assert_eq!(contact.get_authname(), "");
assert_eq!(contact.get_display_name(), "First Last");
}
#[async_std::test]

9
src/sql.rs
View File

@@ -1491,6 +1491,15 @@ CREATE INDEX devmsglabels_index1 ON devmsglabels (label);
}
sql.set_raw_config_int(context, "dbversion", 73).await?;
}
if dbversion < 74 {
info!(context, "[migration] v74");
sql.execute(
"UPDATE contacts SET name='' WHERE name=authname",
paramsv![],
)
.await?;
sql.set_raw_config_int(context, "dbversion", 74).await?;
}
// (2) updates that require high-level objects
// (the structure is complete now and all objects are usable)