From a47fec7f6c7a99f2efa2c7c7abced65616bdff2e Mon Sep 17 00:00:00 2001 From: iequidoo Date: Thu, 16 Nov 2023 21:42:20 -0300 Subject: [PATCH] feat: Sync `Config::{MdnsEnabled,ShowEmails}` across devices (#4954) Motivation: Syncing these options will improve UX in very most cases and should be done. Other candidates are less clear or are advanced options, we can reconsider that at some point later. Approach: - Sync options one-by-one when the corresponding option is set (even if to the same value). - Don't sync when an option is reset to a default as defaults may differ across client versions. - Check on both sides that the option should be synced so that if there are different client versions, the synchronisation of the option is either done or not done in both directions. Moreover, receivers of a config value need to check if a key can be synced because some settings (e.g. Avatar path) could otherwise lead to exfiltration of files from a receiver's device if we assume an attacker to have control of a device in a multi-device setting or if multiple users are sharing an account. - Don't sync `SyncMsgs` itself. --- src/config.rs | 113 +++++++++++++++++++++++++++++++++++++++++++++++ src/configure.rs | 5 ++- src/sync.rs | 19 +++++++- 3 files changed, 135 insertions(+), 2 deletions(-) diff --git a/src/config.rs b/src/config.rs index 1c4d0b0ba..192b4e25c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -5,6 +5,7 @@ use std::path::Path; use std::str::FromStr; use anyhow::{ensure, Context as _, Result}; +use serde::{Deserialize, Serialize}; use strum::{EnumProperty, IntoEnumIterator}; use strum_macros::{AsRefStr, Display, EnumIter, EnumProperty, EnumString}; @@ -13,8 +14,10 @@ use crate::constants::DC_VERSION_STR; use crate::contact::addr_cmp; use crate::context::Context; use crate::events::EventType; +use crate::log::LogExt; use crate::mimefactory::RECOMMENDED_FILE_SIZE; use crate::provider::{get_provider_by_id, Provider}; +use crate::sync::{self, Sync::*, SyncData}; use crate::tools::{get_abs_path, improve_single_line_input, EmailAddress}; /// The available configuration keys. @@ -31,6 +34,8 @@ use crate::tools::{get_abs_path, improve_single_line_input, EmailAddress}; EnumProperty, PartialOrd, Ord, + Serialize, + Deserialize, )] #[strum(serialize_all = "snake_case")] pub enum Config { @@ -340,6 +345,21 @@ pub enum Config { VerifiedOneOnOneChats, } +impl Config { + /// Whether the config option is synced across devices. + /// + /// This must be checked on both sides so that if there are different client versions, the + /// synchronisation of a particular option is either done or not done in both directions. + /// Moreover, receivers of a config value need to check if a key can be synced because some + /// settings (e.g. Avatar path) could otherwise lead to exfiltration of files from a receiver's + /// device if we assume an attacker to have control of a device in a multi-device setting or if + /// multiple users are sharing an account. Another example is `Self::SyncMsgs` itself which + /// mustn't be controlled by other devices. + pub(crate) fn is_synced(&self) -> bool { + matches!(self, Self::MdnsEnabled | Self::ShowEmails) + } +} + impl Context { /// Returns true if configuration value is set for the given key. pub async fn config_exists(&self, key: Config) -> Result { @@ -460,6 +480,15 @@ impl Context { /// Set the given config key. /// If `None` is passed as a value the value is cleared and set to the default if there is one. pub async fn set_config(&self, key: Config, value: Option<&str>) -> Result<()> { + self.set_config_ex(key.is_synced().into(), key, value).await + } + + pub(crate) async fn set_config_ex( + &self, + sync: sync::Sync, + key: Config, + value: Option<&str>, + ) -> Result<()> { match key { Config::Selfavatar => { self.sql @@ -522,6 +551,23 @@ impl Context { self.sql.set_raw_config(key.as_ref(), value).await?; } } + + if sync != Sync { + return Ok(()); + } + let Some(val) = value else { + return Ok(()); + }; + let val = val.to_string(); + if self + .add_sync_item(SyncData::Config { key, val }) + .await + .log_err(self) + .is_err() + { + return Ok(()); + } + self.send_sync_msg().await.log_err(self).ok(); Ok(()) } @@ -797,4 +843,71 @@ mod tests { Ok(()) } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_sync() -> Result<()> { + let alice0 = TestContext::new_alice().await; + let alice1 = TestContext::new_alice().await; + for a in [&alice0, &alice1] { + a.set_config_bool(Config::SyncMsgs, true).await?; + } + + async fn sync(alice0: &TestContext, alice1: &TestContext) -> Result<()> { + let sync_msg = alice0.pop_sent_msg().await; + alice1.recv_msg(&sync_msg).await; + Ok(()) + } + + let mdns_enabled = alice0.get_config_bool(Config::MdnsEnabled).await?; + // Alice1 has a different config value. + alice1 + .set_config_bool(Config::MdnsEnabled, !mdns_enabled) + .await?; + // This changes nothing, but still sends a sync message. + alice0 + .set_config_bool(Config::MdnsEnabled, mdns_enabled) + .await?; + sync(&alice0, &alice1).await?; + assert_eq!( + alice1.get_config_bool(Config::MdnsEnabled).await?, + mdns_enabled + ); + + // Usual sync scenario. + alice0 + .set_config_bool(Config::MdnsEnabled, !mdns_enabled) + .await?; + sync(&alice0, &alice1).await?; + assert_eq!( + alice1.get_config_bool(Config::MdnsEnabled).await?, + !mdns_enabled + ); + + // Reset to default. Test that it's not synced because defaults may differ across client + // versions. + alice0.set_config(Config::MdnsEnabled, None).await?; + assert!(alice0.get_config_bool(Config::MdnsEnabled).await?); + alice0.set_config_bool(Config::MdnsEnabled, false).await?; + sync(&alice0, &alice1).await?; + assert!(!alice1.get_config_bool(Config::MdnsEnabled).await?); + + let show_emails = alice0.get_config_bool(Config::ShowEmails).await?; + alice0 + .set_config_bool(Config::ShowEmails, !show_emails) + .await?; + sync(&alice0, &alice1).await?; + assert_eq!( + alice1.get_config_bool(Config::ShowEmails).await?, + !show_emails + ); + + // `Config::SyncMsgs` mustn't be synced. + alice0.set_config_bool(Config::SyncMsgs, false).await?; + alice0.set_config_bool(Config::SyncMsgs, true).await?; + alice0.set_config_bool(Config::MdnsEnabled, true).await?; + sync(&alice0, &alice1).await?; + assert!(alice1.get_config_bool(Config::MdnsEnabled).await?); + + Ok(()) + } } diff --git a/src/configure.rs b/src/configure.rs index 8d128a835..7f85552c4 100644 --- a/src/configure.rs +++ b/src/configure.rs @@ -34,6 +34,7 @@ use crate::provider::{Protocol, Socket, UsernamePattern}; use crate::smtp::Smtp; use crate::socks::Socks5Config; use crate::stock_str; +use crate::sync::Sync::*; use crate::tools::{time, EmailAddress}; use crate::{chat, e2ee, provider}; @@ -132,7 +133,9 @@ async fn on_configure_completed( for def in config_defaults { if !context.config_exists(def.key).await? { info!(context, "apply config_defaults {}={}", def.key, def.value); - context.set_config(def.key, Some(def.value)).await?; + context + .set_config_ex(Nosync, def.key, Some(def.value)) + .await?; } else { info!( context, diff --git a/src/sync.rs b/src/sync.rs index 8021359a5..4162f57e5 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -20,7 +20,7 @@ use crate::tools::time; use crate::{stock_str, token}; /// Whether to send device sync messages. Aimed for usage in the internal API. -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub(crate) enum Sync { Nosync, Sync, @@ -35,6 +35,15 @@ impl From for bool { } } +impl From for Sync { + fn from(sync: bool) -> Sync { + match sync { + false => Sync::Nosync, + true => Sync::Sync, + } + } +} + #[derive(Debug, Serialize, Deserialize)] pub(crate) struct QrTokenData { pub(crate) invitenumber: String, @@ -50,6 +59,10 @@ pub(crate) enum SyncData { id: chat::SyncId, action: chat::SyncAction, }, + Config { + key: Config, + val: String, + }, } #[derive(Debug, Serialize, Deserialize)] @@ -263,6 +276,10 @@ impl Context { AddQrToken(token) => self.add_qr_token(token).await, DeleteQrToken(token) => self.delete_qr_token(token).await, AlterChat { id, action } => self.sync_alter_chat(id, action).await, + SyncData::Config { key, val } => match key.is_synced() { + true => self.set_config_ex(Sync::Nosync, *key, Some(val)).await, + false => Ok(()), + }, }, SyncDataOrUnknown::Unknown(data) => { warn!(self, "Ignored unknown sync item: {data}.");