diff --git a/CHANGELOG.md b/CHANGELOG.md index a1f003c4c..cd07e0a15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - Make sure malformed messsages will never block receiving further messages anymore #3771 - strip leading/trailing whitespace from "Chat-Group-Name{,-Changed}:" headers content #3650 - Assume all Thunderbird users prefer encryption #3774 +- refactor peerstate handling to ensure no duplicate peerstates #3776 ## 1.102.0 diff --git a/src/decrypt.rs b/src/decrypt.rs index 4f76a2786..8c57cdd36 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -306,7 +306,7 @@ pub(crate) async fn get_autocrypt_peerstate( if addr_cmp(&peerstate.addr, from) { if allow_change { peerstate.apply_header(header, message_time); - peerstate.save_to_db(&context.sql, false).await?; + peerstate.save_to_db(&context.sql).await?; } else { info!( context, @@ -322,7 +322,7 @@ pub(crate) async fn get_autocrypt_peerstate( // to the database. } else { let p = Peerstate::from_header(header, message_time); - p.save_to_db(&context.sql, true).await?; + p.save_to_db(&context.sql).await?; peerstate = Some(p); } } else { diff --git a/src/e2ee.rs b/src/e2ee.rs index cd92c5a3d..311894a04 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -147,7 +147,6 @@ mod tests { use crate::chat; use crate::message::{Message, Viewtype}; use crate::param::Param; - use crate::peerstate::ToSave; use crate::test_utils::{bob_keypair, TestContext}; use super::*; @@ -297,7 +296,6 @@ Sent with my Delta Chat Messenger: https://delta.chat"; gossip_key_fingerprint: Some(pub_key.fingerprint()), verified_key: Some(pub_key.clone()), verified_key_fingerprint: Some(pub_key.fingerprint()), - to_save: Some(ToSave::All), fingerprint_changed: false, }; vec![(Some(peerstate), addr)] diff --git a/src/mimeparser.rs b/src/mimeparser.rs index 04a4b4d52..2e811ebb9 100644 --- a/src/mimeparser.rs +++ b/src/mimeparser.rs @@ -310,7 +310,7 @@ impl MimeMessage { // && decryption_info.dkim_results.allow_keychange { peerstate.degrade_encryption(message_time); - peerstate.save_to_db(&context.sql, false).await?; + peerstate.save_to_db(&context.sql).await?; } } (Ok(mail), HashSet::new(), false) @@ -1586,11 +1586,11 @@ async fn update_gossip_peerstates( let peerstate; if let Some(mut p) = Peerstate::from_addr(context, &header.addr).await? { p.apply_gossip(&header, message_time); - p.save_to_db(&context.sql, false).await?; + p.save_to_db(&context.sql).await?; peerstate = p; } else { let p = Peerstate::from_gossip(&header, message_time); - p.save_to_db(&context.sql, true).await?; + p.save_to_db(&context.sql).await?; peerstate = p; }; peerstate diff --git a/src/peerstate.rs b/src/peerstate.rs index 165fd05fc..d64667306 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -46,7 +46,6 @@ pub struct Peerstate { pub gossip_key_fingerprint: Option, pub verified_key: Option, pub verified_key_fingerprint: Option, - pub to_save: Option, pub fingerprint_changed: bool, } @@ -63,7 +62,6 @@ impl PartialEq for Peerstate { && self.gossip_key_fingerprint == other.gossip_key_fingerprint && self.verified_key == other.verified_key && self.verified_key_fingerprint == other.verified_key_fingerprint - && self.to_save == other.to_save && self.fingerprint_changed == other.fingerprint_changed } } @@ -84,19 +82,11 @@ impl fmt::Debug for Peerstate { .field("gossip_key_fingerprint", &self.gossip_key_fingerprint) .field("verified_key", &self.verified_key) .field("verified_key_fingerprint", &self.verified_key_fingerprint) - .field("to_save", &self.to_save) .field("fingerprint_changed", &self.fingerprint_changed) .finish() } } -#[derive(Debug, Copy, Clone, PartialEq, Eq, FromPrimitive, ToPrimitive)] -#[repr(u8)] -pub enum ToSave { - Timestamps = 0x01, - All = 0x02, -} - impl Peerstate { pub fn from_header(header: &Aheader, message_time: i64) -> Self { Peerstate { @@ -111,7 +101,6 @@ impl Peerstate { gossip_timestamp: 0, verified_key: None, verified_key_fingerprint: None, - to_save: Some(ToSave::All), fingerprint_changed: false, } } @@ -137,7 +126,6 @@ impl Peerstate { gossip_timestamp: message_time, verified_key: None, verified_key_fingerprint: None, - to_save: Some(ToSave::All), fingerprint_changed: false, } } @@ -229,7 +217,6 @@ impl Peerstate { .map(|s| s.parse::()) .transpose() .unwrap_or_default(), - to_save: None, fingerprint_changed: false, }; @@ -248,14 +235,10 @@ impl Peerstate { let old_public_fingerprint = self.public_key_fingerprint.take(); self.public_key_fingerprint = Some(public_key.fingerprint()); - if old_public_fingerprint.is_none() - || self.public_key_fingerprint.is_none() - || old_public_fingerprint != self.public_key_fingerprint + if old_public_fingerprint.is_some() + && old_public_fingerprint != self.public_key_fingerprint { - self.to_save = Some(ToSave::All); - if old_public_fingerprint.is_some() { - self.fingerprint_changed = true; - } + self.fingerprint_changed = true; } } @@ -267,8 +250,6 @@ impl Peerstate { || self.gossip_key_fingerprint.is_none() || old_gossip_fingerprint != self.gossip_key_fingerprint { - self.to_save = Some(ToSave::All); - // Warn about gossip key change only if there is no public key obtained from // Autocrypt header, which overrides gossip key. if old_gossip_fingerprint.is_some() && self.public_key_fingerprint.is_none() { @@ -281,7 +262,6 @@ impl Peerstate { pub fn degrade_encryption(&mut self, message_time: i64) { self.prefer_encrypt = EncryptPreference::Reset; self.last_seen = message_time; - self.to_save = Some(ToSave::All); } pub fn apply_header(&mut self, header: &Aheader, message_time: i64) { @@ -292,19 +272,16 @@ impl Peerstate { if message_time > self.last_seen { self.last_seen = message_time; self.last_seen_autocrypt = message_time; - self.to_save = Some(ToSave::Timestamps); if (header.prefer_encrypt == EncryptPreference::Mutual || header.prefer_encrypt == EncryptPreference::NoPreference) && header.prefer_encrypt != self.prefer_encrypt { self.prefer_encrypt = header.prefer_encrypt; - self.to_save = Some(ToSave::All) } if self.public_key.as_ref() != Some(&header.public_key) { self.public_key = Some(header.public_key.clone()); self.recalc_fingerprint(); - self.to_save = Some(ToSave::All); } } } @@ -316,11 +293,9 @@ impl Peerstate { if message_time > self.gossip_timestamp { self.gossip_timestamp = message_time; - self.to_save = Some(ToSave::Timestamps); if self.gossip_key.as_ref() != Some(&gossip_header.public_key) { self.gossip_key = Some(gossip_header.public_key.clone()); self.recalc_fingerprint(); - self.to_save = Some(ToSave::All) } // This is non-standard. @@ -339,7 +314,6 @@ impl Peerstate { && gossip_header.prefer_encrypt == EncryptPreference::Mutual { self.prefer_encrypt = EncryptPreference::Mutual; - self.to_save = Some(ToSave::All); } }; } @@ -395,7 +369,6 @@ impl Peerstate { if self.public_key_fingerprint.is_some() && self.public_key_fingerprint.as_ref().unwrap() == fingerprint { - self.to_save = Some(ToSave::All); self.verified_key = self.public_key.clone(); self.verified_key_fingerprint = self.public_key_fingerprint.clone(); true @@ -407,7 +380,6 @@ impl Peerstate { if self.gossip_key_fingerprint.is_some() && self.gossip_key_fingerprint.as_ref().unwrap() == fingerprint { - self.to_save = Some(ToSave::All); self.verified_key = self.gossip_key.clone(); self.verified_key_fingerprint = self.gossip_key_fingerprint.clone(); true @@ -421,66 +393,48 @@ impl Peerstate { } } - pub async fn save_to_db(&self, sql: &Sql, create: bool) -> Result<()> { - if self.to_save == Some(ToSave::All) || create { - sql.execute( - if create { - "INSERT INTO acpeerstates ( \ - last_seen, \ - last_seen_autocrypt, \ - prefer_encrypted, \ - public_key, \ - gossip_timestamp, \ - gossip_key, \ - public_key_fingerprint, \ - gossip_key_fingerprint, \ - verified_key, \ - verified_key_fingerprint, \ - addr \ - ) VALUES(?,?,?,?,?,?,?,?,?,?,?)" - } else { - "UPDATE acpeerstates \ - SET last_seen=?, \ - last_seen_autocrypt=?, \ - prefer_encrypted=?, \ - public_key=?, \ - gossip_timestamp=?, \ - gossip_key=?, \ - public_key_fingerprint=?, \ - gossip_key_fingerprint=?, \ - verified_key=?, \ - verified_key_fingerprint=? \ - WHERE addr=?" - }, - paramsv![ - self.last_seen, - self.last_seen_autocrypt, - self.prefer_encrypt as i64, - self.public_key.as_ref().map(|k| k.to_bytes()), - self.gossip_timestamp, - self.gossip_key.as_ref().map(|k| k.to_bytes()), - self.public_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.verified_key.as_ref().map(|k| k.to_bytes()), - self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.addr, - ], - ) - .await?; - } else if self.to_save == Some(ToSave::Timestamps) { - sql.execute( - "UPDATE acpeerstates SET last_seen=?, last_seen_autocrypt=?, gossip_timestamp=? \ - WHERE addr=?;", - paramsv![ - self.last_seen, - self.last_seen_autocrypt, - self.gossip_timestamp, - self.addr - ], - ) - .await?; - } - + pub async fn save_to_db(&self, sql: &Sql) -> Result<()> { + sql.execute( + "INSERT INTO acpeerstates ( + last_seen, + last_seen_autocrypt, + prefer_encrypted, + public_key, + gossip_timestamp, + gossip_key, + public_key_fingerprint, + gossip_key_fingerprint, + verified_key, + verified_key_fingerprint, + addr) + VALUES (?,?,?,?,?,?,?,?,?,?,?) + ON CONFLICT (addr) + DO UPDATE SET + last_seen = excluded.last_seen, + last_seen_autocrypt = excluded.last_seen_autocrypt, + prefer_encrypted = excluded.prefer_encrypted, + public_key = excluded.public_key, + gossip_timestamp = excluded.gossip_timestamp, + gossip_key = excluded.gossip_key, + public_key_fingerprint = excluded.public_key_fingerprint, + gossip_key_fingerprint = excluded.gossip_key_fingerprint, + verified_key = excluded.verified_key, + verified_key_fingerprint = excluded.verified_key_fingerprint", + paramsv![ + self.last_seen, + self.last_seen_autocrypt, + self.prefer_encrypt as i64, + self.public_key.as_ref().map(|k| k.to_bytes()), + self.gossip_timestamp, + self.gossip_key.as_ref().map(|k| k.to_bytes()), + self.public_key_fingerprint.as_ref().map(|fp| fp.hex()), + self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()), + self.verified_key.as_ref().map(|k| k.to_bytes()), + self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()), + self.addr, + ], + ) + .await?; Ok(()) } @@ -651,14 +605,8 @@ pub async fn maybe_do_aeap_transition( "Internal error: Tried to do an AEAP transition without an autocrypt header??", )?; peerstate.apply_header(header, info.message_time); - peerstate.to_save = Some(ToSave::All); - // We don't know whether a peerstate with this address already existed, or a - // new one should be created, so just try both create=false and create=true, - // and if this fails, create=true, one will succeed (this is a very cold path, - // so performance doesn't really matter). - peerstate.save_to_db(&context.sql, true).await?; - peerstate.save_to_db(&context.sql, false).await?; + peerstate.save_to_db(&context.sql).await?; } } @@ -710,7 +658,7 @@ mod tests { let pub_key = alice_keypair().public; - let mut peerstate = Peerstate { + let peerstate = Peerstate { addr: addr.into(), last_seen: 10, last_seen_autocrypt: 11, @@ -722,12 +670,11 @@ mod tests { gossip_key_fingerprint: Some(pub_key.fingerprint()), verified_key: Some(pub_key.clone()), verified_key_fingerprint: Some(pub_key.fingerprint()), - to_save: Some(ToSave::All), fingerprint_changed: false, }; assert!( - peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(), + peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(), "failed to save to db" ); @@ -736,8 +683,6 @@ mod tests { .expect("failed to load peerstate from db") .expect("no peerstate found in the database"); - // clear to_save, as that is not persissted - peerstate.to_save = None; assert_eq!(peerstate, peerstate_new); let peerstate_new2 = Peerstate::from_fingerprint(&ctx.ctx, &pub_key.fingerprint()) .await @@ -764,16 +709,15 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - to_save: Some(ToSave::All), fingerprint_changed: false, }; assert!( - peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(), + peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(), "failed to save" ); assert!( - peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(), + peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(), "double-call with create failed" ); } @@ -785,7 +729,7 @@ mod tests { let pub_key = alice_keypair().public; - let mut peerstate = Peerstate { + let peerstate = Peerstate { addr: addr.into(), last_seen: 10, last_seen_autocrypt: 11, @@ -797,12 +741,11 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - to_save: Some(ToSave::All), fingerprint_changed: false, }; assert!( - peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(), + peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(), "failed to save" ); @@ -810,8 +753,6 @@ mod tests { .await .expect("failed to load peerstate from db"); - // clear to_save, as that is not persissted - peerstate.to_save = None; assert_eq!(Some(peerstate), peerstate_new); } @@ -862,7 +803,6 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - to_save: None, fingerprint_changed: false, }; assert_eq!(peerstate.prefer_encrypt, EncryptPreference::NoPreference); diff --git a/src/qr.rs b/src/qr.rs index 800eb4872..c12ce8045 100644 --- a/src/qr.rs +++ b/src/qr.rs @@ -641,7 +641,6 @@ mod tests { use crate::aheader::EncryptPreference; use crate::chat::{create_group_chat, ProtectionStatus}; use crate::key::DcKey; - use crate::peerstate::ToSave; use crate::securejoin::get_securejoin_qr; use crate::test_utils::{alice_keypair, TestContext}; use anyhow::Result; @@ -894,11 +893,10 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - to_save: Some(ToSave::All), fingerprint_changed: false, }; assert!( - peerstate.save_to_db(&ctx.ctx.sql, true).await.is_ok(), + peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(), "failed to save peerstate" ); diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 13241072d..6fbfdddf0 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -2129,7 +2129,7 @@ async fn check_verified_properties( &fp, PeerstateVerifiedStatus::BidirectVerified, ); - peerstate.save_to_db(&context.sql, false).await?; + peerstate.save_to_db(&context.sql).await?; is_verified = true; } } diff --git a/src/securejoin.rs b/src/securejoin.rs index 81d0bd78d..08822a5a1 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -18,7 +18,7 @@ use crate::key::{DcKey, Fingerprint, SignedPublicKey}; use crate::message::{Message, Viewtype}; use crate::mimeparser::{MimeMessage, SystemMessage}; use crate::param::Param; -use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus, ToSave}; +use crate::peerstate::{Peerstate, PeerstateKeyType, PeerstateVerifiedStatus}; use crate::qr::check_qr; use crate::stock_str; use crate::token; @@ -640,11 +640,7 @@ async fn mark_peer_as_verified(context: &Context, fingerprint: &Fingerprint) -> PeerstateVerifiedStatus::BidirectVerified, ) { peerstate.prefer_encrypt = EncryptPreference::Mutual; - peerstate.to_save = Some(ToSave::All); - peerstate - .save_to_db(&context.sql, false) - .await - .unwrap_or_default(); + peerstate.save_to_db(&context.sql).await.unwrap_or_default(); return Ok(()); } } @@ -932,10 +928,9 @@ mod tests { gossip_key_fingerprint: Some(alice_pubkey.fingerprint()), verified_key: None, verified_key_fingerprint: None, - to_save: Some(ToSave::All), fingerprint_changed: false, }; - peerstate.save_to_db(&bob.ctx.sql, true).await?; + peerstate.save_to_db(&bob.ctx.sql).await?; // Step 1: Generate QR-code, ChatId(0) indicates setup-contact let qr = get_securejoin_qr(&alice.ctx, None).await?; diff --git a/src/sql.rs b/src/sql.rs index ce31b5995..3c2d1418d 100644 --- a/src/sql.rs +++ b/src/sql.rs @@ -280,7 +280,7 @@ impl Sql { for addr in &addrs { if let Some(ref mut peerstate) = Peerstate::from_addr(context, addr).await? { peerstate.recalc_fingerprint(); - peerstate.save_to_db(self, false).await?; + peerstate.save_to_db(self).await?; } } } diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index a32bd0abd..a02c36ba5 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -616,6 +616,39 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid); ) .await?; } + if dbversion < 94 { + sql.execute_migration( + // Create new `acpeerstates` table, same as before but with unique constraint. + // + // This allows to use `UPSERT` to update existing or insert a new peerstate + // depending on whether one exists already. + "CREATE TABLE new_acpeerstates ( + id INTEGER PRIMARY KEY, + addr TEXT DEFAULT '' COLLATE NOCASE, + last_seen INTEGER DEFAULT 0, + last_seen_autocrypt INTEGER DEFAULT 0, + public_key, + prefer_encrypted INTEGER DEFAULT 0, + gossip_timestamp INTEGER DEFAULT 0, + gossip_key, + public_key_fingerprint TEXT DEFAULT '', + gossip_key_fingerprint TEXT DEFAULT '', + verified_key, + verified_key_fingerprint TEXT DEFAULT '', + UNIQUE (addr) -- Only one peerstate per address + ); + INSERT OR IGNORE INTO new_acpeerstates SELECT * FROM acpeerstates; + DROP TABLE acpeerstates; + ALTER TABLE new_acpeerstates RENAME TO acpeerstates; + CREATE INDEX acpeerstates_index1 ON acpeerstates (addr); + CREATE INDEX acpeerstates_index3 ON acpeerstates (public_key_fingerprint); + CREATE INDEX acpeerstates_index4 ON acpeerstates (gossip_key_fingerprint); + CREATE INDEX acpeerstates_index5 ON acpeerstates (verified_key_fingerprint); + ", + 94, + ) + .await?; + } let new_version = sql .get_raw_config_int(VERSION_CFG) diff --git a/src/tests/aeap.rs b/src/tests/aeap.rs index 7391e87b1..fbccd401a 100644 --- a/src/tests/aeap.rs +++ b/src/tests/aeap.rs @@ -341,9 +341,8 @@ async fn mark_as_verified(this: &TestContext, other: &TestContext) { peerstate.verified_key = peerstate.public_key.clone(); peerstate.verified_key_fingerprint = peerstate.public_key_fingerprint.clone(); - peerstate.to_save = Some(peerstate::ToSave::All); - peerstate.save_to_db(&this.sql, false).await.unwrap(); + peerstate.save_to_db(&this.sql).await.unwrap(); } async fn get_last_info_msg(t: &TestContext, chat_id: ChatId) -> Option {