From 67d11b1e01dad573941e1359255209461f05a064 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Sat, 24 May 2025 03:39:58 +1000 Subject: [PATCH] fix(gui-client): don't reset favourites when settings change (#9211) The GUI client currently has a bug that resets the favourites and the status of the Internet Resource every time the advanced settings are saved. This happens because those fields are annotated with `#[serde(default)]` and are thus initialised to their default value when the struct is deserialised from the frontend. To mitigate this, we introduce a new `GeneralSettings` struct that holds the status of the Internet Resource and the list of favourites. When a client starts up, it will try to migrate the existing advanced settings into the new split of general and advanced settings. --- .../src-tauri/src/bin/firezone-gui-client.rs | 14 ++- rust/gui-client/src-tauri/src/controller.rs | 29 ++--- rust/gui-client/src-tauri/src/gui.rs | 11 +- rust/gui-client/src-tauri/src/settings.rs | 106 +++++++++++++++++- website/src/components/Changelog/GUI.tsx | 7 +- 5 files changed, 138 insertions(+), 29 deletions(-) diff --git a/rust/gui-client/src-tauri/src/bin/firezone-gui-client.rs b/rust/gui-client/src-tauri/src/bin/firezone-gui-client.rs index 332969768..4d8708779 100644 --- a/rust/gui-client/src-tauri/src/bin/firezone-gui-client.rs +++ b/rust/gui-client/src-tauri/src/bin/firezone-gui-client.rs @@ -11,7 +11,7 @@ use clap::{Args, Parser}; use controller::Failure; use firezone_gui_client::{controller, deep_link, elevation, gui, logging, settings}; use firezone_telemetry::Telemetry; -use settings::AdvancedSettings; +use settings::AdvancedSettingsLegacy; use tracing_subscriber::EnvFilter; fn main() -> ExitCode { @@ -28,7 +28,7 @@ fn main() -> ExitCode { .install_default() .expect("Calling `install_default` only once per process should always succeed"); - let settings = settings::load_advanced_settings().unwrap_or_default(); + let settings = settings::load_advanced_settings::().unwrap_or_default(); let mut telemetry = Telemetry::default(); telemetry.start( @@ -61,7 +61,11 @@ fn main() -> ExitCode { } } -fn try_main(cli: Cli, rt: &tokio::runtime::Runtime, mut settings: AdvancedSettings) -> Result<()> { +fn try_main( + cli: Cli, + rt: &tokio::runtime::Runtime, + mut settings: AdvancedSettingsLegacy, +) -> Result<()> { let config = gui::RunConfig { inject_faults: cli.inject_faults, debug_update_check: cli.debug_update_check, @@ -186,11 +190,11 @@ fn try_main(cli: Cli, rt: &tokio::runtime::Runtime, mut settings: AdvancedSettin } /// Parse the log filter from settings, showing an error and fixing it if needed -fn fix_log_filter(settings: &mut AdvancedSettings) -> Result<()> { +fn fix_log_filter(settings: &mut AdvancedSettingsLegacy) -> Result<()> { if EnvFilter::try_new(&settings.log_filter).is_ok() { return Ok(()); } - settings.log_filter = AdvancedSettings::default().log_filter; + settings.log_filter = AdvancedSettingsLegacy::default().log_filter; native_dialog::MessageDialog::new() .set_title("Log filter error") diff --git a/rust/gui-client/src-tauri/src/controller.rs b/rust/gui-client/src-tauri/src/controller.rs index 33d6b29f8..fd14f1f27 100644 --- a/rust/gui-client/src-tauri/src/controller.rs +++ b/rust/gui-client/src-tauri/src/controller.rs @@ -3,7 +3,7 @@ use crate::{ gui::{self, system_tray}, ipc::{self, SocketId}, logging, service, - settings::{self, AdvancedSettings}, + settings::{self, AdvancedSettings, GeneralSettings}, updates, uptime, }; use anyhow::{Context, Result, anyhow, bail}; @@ -32,7 +32,7 @@ mod ran_before; pub type CtlrTx = mpsc::Sender; pub struct Controller { - /// Debugging-only settings like API URL, auth URL, log filter + general_settings: GeneralSettings, advanced_settings: AdvancedSettings, // Sign-in state with the portal / deep links auth: auth::Auth, @@ -205,10 +205,12 @@ enum EventloopTick { pub struct FailedToReceiveHello(anyhow::Error); impl Controller { + #[expect(clippy::too_many_arguments, reason = "We don't care.")] pub(crate) async fn start( ctlr_tx: CtlrTx, integration: I, rx: mpsc::Receiver, + general_settings: GeneralSettings, advanced_settings: AdvancedSettings, log_filter_reloader: FilterReloadHandle, updates_rx: mpsc::Receiver>, @@ -227,6 +229,7 @@ impl Controller { let network_notifier = new_network_notifier().await?.boxed(); let controller = Controller { + general_settings, advanced_settings, auth: auth::Auth::new()?, clear_logs_callback: None, @@ -470,7 +473,7 @@ impl Controller { self.advanced_settings = *settings; // Save to disk - settings::save(&self.advanced_settings).await?; + settings::save_advanced(&self.advanced_settings).await?; // Tell tunnel about new log level self.send_ipc(&service::ClientMsg::ApplyLogFilter { @@ -524,9 +527,7 @@ impl Controller { .set_welcome_window_visible(false, self.auth.session())?; } SystemTrayMenu(system_tray::Event::AddFavorite(resource_id)) => { - self.advanced_settings - .favorite_resources - .insert(resource_id); + self.general_settings.favorite_resources.insert(resource_id); self.refresh_favorite_resources().await?; } SystemTrayMenu(system_tray::Event::AdminPortal) => self @@ -556,7 +557,7 @@ impl Controller { } }, SystemTrayMenu(system_tray::Event::RemoveFavorite(resource_id)) => { - self.advanced_settings + self.general_settings .favorite_resources .remove(&resource_id); self.refresh_favorite_resources().await?; @@ -565,11 +566,11 @@ impl Controller { self.try_retry_connection().await? } SystemTrayMenu(system_tray::Event::EnableInternetResource) => { - self.advanced_settings.internet_resource_enabled = Some(true); + self.general_settings.internet_resource_enabled = Some(true); self.update_disabled_resources().await?; } SystemTrayMenu(system_tray::Event::DisableInternetResource) => { - self.advanced_settings.internet_resource_enabled = Some(false); + self.general_settings.internet_resource_enabled = Some(false); self.update_disabled_resources().await?; } SystemTrayMenu(system_tray::Event::ShowWindow(window)) => { @@ -804,7 +805,7 @@ impl Controller { } async fn update_disabled_resources(&mut self) -> Result<()> { - settings::save(&self.advanced_settings).await?; + settings::save_general(&self.general_settings).await?; let Some(internet_resource) = self.status.internet_resource() else { return Ok(()); @@ -812,7 +813,7 @@ impl Controller { let mut disabled_resources = BTreeSet::new(); - if !self.advanced_settings.internet_resource_enabled() { + if !self.general_settings.internet_resource_enabled() { disabled_resources.insert(internet_resource.id()); } @@ -827,7 +828,7 @@ impl Controller { /// Saves the current settings (including favorites) to disk and refreshes the tray menu async fn refresh_favorite_resources(&mut self) -> Result<()> { - settings::save(&self.advanced_settings).await?; + settings::save_general(&self.general_settings).await?; self.refresh_system_tray_menu(); Ok(()) } @@ -847,8 +848,8 @@ impl Controller { Status::TunnelReady { resources } => { system_tray::ConnlibState::SignedIn(system_tray::SignedIn { actor_name: auth_session.actor_name.clone(), - favorite_resources: self.advanced_settings.favorite_resources.clone(), - internet_resource_enabled: self.advanced_settings.internet_resource_enabled, + favorite_resources: self.general_settings.favorite_resources.clone(), + internet_resource_enabled: self.general_settings.internet_resource_enabled, resources: resources.clone(), }) } diff --git a/rust/gui-client/src-tauri/src/gui.rs b/rust/gui-client/src-tauri/src/gui.rs index 4ae5ee141..8746a3299 100644 --- a/rust/gui-client/src-tauri/src/gui.rs +++ b/rust/gui-client/src-tauri/src/gui.rs @@ -9,7 +9,7 @@ use crate::{ deep_link, ipc::{self, ClientRead, ClientWrite, SocketId}, logging, - settings::{self, AdvancedSettings}, + settings::{self, AdvancedSettings, AdvancedSettingsLegacy}, updates, }; use anyhow::{Context, Result, bail}; @@ -188,7 +188,7 @@ pub enum ServerMsg { pub fn run( rt: &tokio::runtime::Runtime, config: RunConfig, - advanced_settings: AdvancedSettings, + advanced_settings: AdvancedSettingsLegacy, reloader: firezone_logging::FilterReloadHandle, ) -> Result<()> { // Needed for the deep link server @@ -205,6 +205,9 @@ pub fn run( } }; + let (general_settings, advanced_settings) = + rt.block_on(settings::migrate_legacy_settings(advanced_settings)); + let (ctlr_tx, ctlr_rx) = mpsc::channel(5); let (ready_tx, mut ready_rx) = mpsc::channel::(1); @@ -329,11 +332,13 @@ pub fn run( tray, }; + // Spawn the controller let ctrl_task = tokio::spawn(Controller::start( ctlr_tx, integration, ctlr_rx, + general_settings, advanced_settings, reloader, updates_rx, @@ -431,7 +436,7 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> { tokio::time::sleep_until(quit_time).await; // Write the settings so we can check the path for those - settings::save(&AdvancedSettings::default()).await?; + settings::save_advanced(&AdvancedSettings::default()).await?; // Check results of tests let zip_len = tokio::fs::metadata(&path) diff --git a/rust/gui-client/src-tauri/src/settings.rs b/rust/gui-client/src-tauri/src/settings.rs index 99ac1cf9f..d9534a6e5 100644 --- a/rust/gui-client/src-tauri/src/settings.rs +++ b/rust/gui-client/src-tauri/src/settings.rs @@ -5,6 +5,7 @@ use crate::gui::Managed; use anyhow::{Context as _, Result}; use connlib_model::ResourceId; use firezone_bin_shared::known_dirs; +use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; use std::time::Duration; use std::{collections::HashSet, path::PathBuf}; @@ -40,7 +41,7 @@ pub(crate) async fn reset_advanced_settings( } #[derive(Clone, Deserialize, Serialize)] -pub struct AdvancedSettings { +pub struct AdvancedSettingsLegacy { pub auth_base_url: Url, pub api_url: Url, #[serde(default)] @@ -50,6 +51,21 @@ pub struct AdvancedSettings { pub log_filter: String, } +#[derive(Clone, Deserialize, Serialize)] +pub struct AdvancedSettings { + pub auth_base_url: Url, + pub api_url: Url, + pub log_filter: String, +} + +#[derive(Clone, Deserialize, Serialize)] +pub struct GeneralSettings { + #[serde(default)] + pub favorite_resources: HashSet, + #[serde(default)] + pub internet_resource_enabled: Option, +} + #[cfg(debug_assertions)] mod defaults { pub(crate) const AUTH_BASE_URL: &str = "https://app.firez.one"; @@ -64,7 +80,7 @@ mod defaults { pub(crate) const LOG_FILTER: &str = "info"; } -impl Default for AdvancedSettings { +impl Default for AdvancedSettingsLegacy { fn default() -> Self { Self { auth_base_url: Url::parse(defaults::AUTH_BASE_URL).expect("static URL is a valid URL"), @@ -76,20 +92,70 @@ impl Default for AdvancedSettings { } } -impl AdvancedSettings { +impl GeneralSettings { pub fn internet_resource_enabled(&self) -> bool { self.internet_resource_enabled.is_some_and(|v| v) } } +impl Default for AdvancedSettings { + fn default() -> Self { + Self { + auth_base_url: Url::parse(defaults::AUTH_BASE_URL).expect("static URL is a valid URL"), + api_url: Url::parse(defaults::API_URL).expect("static URL is a valid URL"), + log_filter: defaults::LOG_FILTER.to_string(), + } + } +} + pub fn advanced_settings_path() -> Result { Ok(known_dirs::settings() .context("`known_dirs::settings` failed")? .join("advanced_settings.json")) } -/// Saves the settings to disk -pub async fn save(settings: &AdvancedSettings) -> Result<()> { +pub fn general_settings_path() -> Result { + Ok(known_dirs::settings() + .context("`known_dirs::settings` failed")? + .join("general_settings.json")) +} + +pub async fn migrate_legacy_settings( + legacy: AdvancedSettingsLegacy, +) -> (GeneralSettings, AdvancedSettings) { + let general_settings = load_general_settings(); + + let advanced = AdvancedSettings { + auth_base_url: legacy.auth_base_url, + api_url: legacy.api_url, + log_filter: legacy.log_filter, + }; + + if let Ok(general) = general_settings { + return (general, advanced); + } + + let general = GeneralSettings { + favorite_resources: legacy.favorite_resources, + internet_resource_enabled: legacy.internet_resource_enabled, + }; + + if let Err(e) = save_general(&general).await { + tracing::error!("Failed to write new general settings: {e:#}"); + return (general, advanced); + } + if let Err(e) = save_advanced(&advanced).await { + tracing::error!("Failed to write new advanced settings: {e:#}"); + return (general, advanced); + } + + tracing::info!("Successfully migrate settings"); + + (general, advanced) +} + +/// Saves the advanced settings to disk +pub async fn save_advanced(settings: &AdvancedSettings) -> Result<()> { let path = advanced_settings_path()?; let dir = path .parent() @@ -103,16 +169,44 @@ pub async fn save(settings: &AdvancedSettings) -> Result<()> { Ok(()) } +/// Saves the general settings to disk +pub async fn save_general(settings: &GeneralSettings) -> Result<()> { + let path = general_settings_path()?; + let dir = path + .parent() + .context("settings path should have a parent")?; + + tokio::fs::create_dir_all(dir).await?; + tokio::fs::write(&path, serde_json::to_string(settings)?).await?; + + tracing::debug!(?path, "Saved settings"); + + Ok(()) +} + /// Return advanced settings if they're stored on disk /// /// Uses std::fs, so stick it in `spawn_blocking` for async contexts -pub fn load_advanced_settings() -> Result { +pub fn load_advanced_settings() -> Result +where + T: DeserializeOwned, +{ let path = advanced_settings_path()?; let text = std::fs::read_to_string(path)?; let settings = serde_json::from_str(&text)?; Ok(settings) } +/// Return general settings if they're stored on disk +/// +/// Uses std::fs, so stick it in `spawn_blocking` for async contexts +pub fn load_general_settings() -> Result { + let path = general_settings_path()?; + let text = std::fs::read_to_string(path)?; + let settings = serde_json::from_str(&text)?; + Ok(settings) +} + #[cfg(test)] mod tests { use super::*; diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index 8056a5c17..b0d3a4144 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -8,7 +8,12 @@ export default function GUI({ os }: { os: OS }) { return ( {/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */} - + + + Fixes an issue where changing the Advanced settings would reset + the favourited resources. + + Fixes an issue where connections failed to establish on machines