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.
This commit is contained in:
iequidoo
2023-11-16 21:42:20 -03:00
committed by iequidoo
parent 084434d3b4
commit a47fec7f6c
3 changed files with 135 additions and 2 deletions

View File

@@ -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<bool> {
@@ -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(())
}
}