From e2117dd2201fabfeb43516b26c86a5b2d2cee579 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 14 Nov 2024 05:20:05 +0000 Subject: [PATCH] refactor(gui-client): don't double log errors (#7330) Currently, some errors are double-logged when we show them to the user because of the `tracing::error!` statements within the generation of the user-friendly error message for the error dialog. To get rid of these, we generalise the `show_error_dialog` function to take just the message and move the generation of the message to a function on the `Error` itself. This also allows us to split out a separate error type that is only used for the elevation check, thereby reducing the complexity of the other error enum. --- rust/gui-client/src-common/src/errors.rs | 57 +++++++++---------- rust/gui-client/src-tauri/Cargo.toml | 2 +- rust/gui-client/src-tauri/src/client.rs | 5 +- .../src-tauri/src/client/elevation.rs | 26 +++++++-- rust/gui-client/src-tauri/src/client/gui.rs | 2 +- 5 files changed, 53 insertions(+), 39 deletions(-) diff --git a/rust/gui-client/src-common/src/errors.rs b/rust/gui-client/src-common/src/errors.rs index a42fc5985..41fb4a188 100644 --- a/rust/gui-client/src-common/src/errors.rs +++ b/rust/gui-client/src-common/src/errors.rs @@ -1,6 +1,6 @@ use crate::{self as common, deep_link}; use anyhow::Result; -use firezone_headless_client::{ipc, FIREZONE_GROUP}; +use firezone_headless_client::ipc; // TODO: Replace with `anyhow` gradually per #[derive(Debug, thiserror::Error)] @@ -23,50 +23,45 @@ pub enum Error { IpcServiceTerminating, #[error("Failed to connect to portal")] PortalConnection(String), - #[error("UserNotInFirezoneGroup")] - UserNotInFirezoneGroup, #[error("WebViewNotInstalled")] WebViewNotInstalled, #[error(transparent)] Other(#[from] anyhow::Error), } +impl Error { + // Decision to put the error strings here: + // This message gets shown to users in the GUI and could be localized, unlike + // messages in the log which only need to be used for `git grep`. + pub fn user_friendly_msg(&self) -> String { + match self { + Error::ConnectToFirezoneFailed(_) => self.to_string(), + Error::WebViewNotInstalled => "Firezone cannot start because WebView2 is not installed. Follow the instructions at .".to_string(), + Error::DeepLink(deep_link::Error::CantListen) => "Firezone is already running. If it's not responding, force-stop it.".to_string(), + Error::DeepLink(deep_link::Error::Other(error)) => error.to_string(), + Error::Ipc(ipc::Error::NotFound(_)) => "Couldn't find Firezone IPC service. Is the service running?".to_string(), + Error::Ipc(ipc::Error::PermissionDenied) => "Permission denied for Firezone IPC service. This should only happen on dev systems.".to_string(), + Error::Ipc(ipc::Error::Other(error)) => error.to_string(), + Error::IpcClosed => "IPC connection closed".to_string(), + Error::IpcRead => "IPC read failure".to_string(), + Error::IpcServiceTerminating => "The Firezone IPC service is terminating. Please restart the GUI Client.".to_string(), + Error::Logging(_) => "Logging error".to_string(), + Error::PortalConnection(_) => "Couldn't connect to the Firezone Portal. Are you connected to the Internet?".to_string(), + Error::Other(error) => error.to_string(), + } + } +} + /// Blocks the thread and shows an error dialog /// /// Doesn't play well with async, only use this if we're bailing out of the /// entire process. -pub fn show_error_dialog(error: &Error) -> Result<()> { - // Decision to put the error strings here: - // This message gets shown to users in the GUI and could be localized, unlike - // messages in the log which only need to be used for `git grep`. - let user_friendly_error_msg = match error { - Error::ConnectToFirezoneFailed(_) => error.to_string(), - Error::WebViewNotInstalled => "Firezone cannot start because WebView2 is not installed. Follow the instructions at .".to_string(), - Error::DeepLink(deep_link::Error::CantListen) => "Firezone is already running. If it's not responding, force-stop it.".to_string(), - Error::DeepLink(deep_link::Error::Other(error)) => error.to_string(), - Error::Ipc(ipc::Error::NotFound(path)) => { - tracing::error!(?path, "Couldn't find Firezone IPC service"); - "Couldn't find Firezone IPC service. Is the service running?".to_string() - } - Error::Ipc(ipc::Error::PermissionDenied) => "Permission denied for Firezone IPC service. This should only happen on dev systems.".to_string(), - Error::Ipc(ipc::Error::Other(error)) => error.to_string(), - Error::IpcClosed => "IPC connection closed".to_string(), - Error::IpcRead => "IPC read failure".to_string(), - Error::IpcServiceTerminating => "The Firezone IPC service is terminating. Please restart the GUI Client.".to_string(), - Error::Logging(_) => "Logging error".to_string(), - Error::PortalConnection(error) => { - tracing::error!(%error, "Couldn't connect to the Portal"); - "Couldn't connect to the Firezone Portal. Are you connected to the Internet?".to_string() - } - Error::UserNotInFirezoneGroup => format!("You are not a member of the group `{FIREZONE_GROUP}`. Try `sudo usermod -aG {FIREZONE_GROUP} $USER` and then reboot"), - Error::Other(error) => error.to_string(), - }; - +pub fn show_error_dialog(msg: String) -> Result<()> { // I tried the Tauri dialogs and for some reason they don't show our // app icon. native_dialog::MessageDialog::new() .set_title("Firezone Error") - .set_text(&user_friendly_error_msg) + .set_text(&msg) .set_type(native_dialog::MessageType::Error) .show_alert()?; Ok(()) diff --git a/rust/gui-client/src-tauri/Cargo.toml b/rust/gui-client/src-tauri/Cargo.toml index 268b1a8eb..7b480c59b 100644 --- a/rust/gui-client/src-tauri/Cargo.toml +++ b/rust/gui-client/src-tauri/Cargo.toml @@ -38,7 +38,7 @@ tauri-plugin-notification = "2.0.1" tauri-plugin-shell = "2.0.2" tauri-runtime = "2.1.0" tauri-utils = "2.0.1" -thiserror = { version = "1.0.68", default-features = false } +thiserror = "1.0.68" tokio = { workspace = true, features = ["signal", "time", "macros", "rt", "rt-multi-thread"] } tokio-util = { version = "0.7.11", features = ["codec"] } tracing = { workspace = true } diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index e1c19ff95..99b8ba4fb 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -41,8 +41,9 @@ pub(crate) fn run() -> Result<()> { // Our elevation is correct (not elevated), just run the GUI Ok(true) => run_gui(cli), Ok(false) => bail!("The GUI should run as a normal user, not elevated"), + #[cfg(not(target_os = "windows"))] // Windows elevation check never fails. Err(error) => { - common::errors::show_error_dialog(&error)?; + common::errors::show_error_dialog(error.user_friendly_msg())?; Err(error.into()) } } @@ -113,7 +114,7 @@ fn run_gui(cli: Cli) -> Result<()> { // Make sure errors get logged, at least to stderr if let Err(error) = &result { tracing::error!(error = std_dyn_err(error), error_msg = %error); - common::errors::show_error_dialog(error)?; + common::errors::show_error_dialog(error.user_friendly_msg())?; } Ok(result?) diff --git a/rust/gui-client/src-tauri/src/client/elevation.rs b/rust/gui-client/src-tauri/src/client/elevation.rs index 0f5b7644f..1e6a0b8cd 100644 --- a/rust/gui-client/src-tauri/src/client/elevation.rs +++ b/rust/gui-client/src-tauri/src/client/elevation.rs @@ -3,7 +3,6 @@ pub(crate) use platform::gui_check; #[cfg(target_os = "linux")] mod platform { use anyhow::{Context as _, Result}; - use firezone_gui_client_common::errors::Error; use firezone_headless_client::FIREZONE_GROUP; /// Returns true if all permissions are correct for the GUI to run @@ -12,13 +11,13 @@ mod platform { /// so for security and practicality reasons the GUIs must be non-root. /// (In Linux by default a root GUI app barely works at all) pub(crate) fn gui_check() -> Result { - let user = std::env::var("USER").context("USER env var should be set")?; + let user = std::env::var("USER").context("Unable to determine current user")?; if user == "root" { return Ok(false); } let fz_gid = firezone_group()?.gid; - let groups = nix::unistd::getgroups().context("`nix::unistd::getgroups`")?; + let groups = nix::unistd::getgroups().context("Unable to read groups of current user")?; if !groups.contains(&fz_gid) { return Err(Error::UserNotInFirezoneGroup); } @@ -32,12 +31,28 @@ mod platform { .with_context(|| format!("`{FIREZONE_GROUP}` group must exist on the system"))?; Ok(group) } + + #[derive(Debug, thiserror::Error)] + pub(crate) enum Error { + #[error("User is not part of {FIREZONE_GROUP} group")] + UserNotInFirezoneGroup, + #[error(transparent)] + Other(#[from] anyhow::Error), + } + + impl Error { + pub(crate) fn user_friendly_msg(&self) -> String { + match self { + Error::UserNotInFirezoneGroup => format!("You are not a member of the group `{FIREZONE_GROUP}`. Try `sudo usermod -aG {FIREZONE_GROUP} $USER` and then reboot"), + Error::Other(e) => format!("Failed to determine group ownership: {e:#}"), + } + } + } } #[cfg(target_os = "windows")] mod platform { use anyhow::Result; - use firezone_gui_client_common::errors::Error; // Returns true on Windows /// @@ -47,6 +62,9 @@ mod platform { pub(crate) fn gui_check() -> Result { Ok(true) } + + #[derive(Debug, Clone, Copy, thiserror::Error)] + pub(crate) enum Error {} } #[cfg(test)] diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index e173a0aa2..47db53190 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -262,7 +262,7 @@ pub(crate) fn run( } Ok(Err(error)) => { tracing::error!(error = std_dyn_err(&error), "run_controller returned an error"); - if let Err(e) = errors::show_error_dialog(&error) { + if let Err(e) = errors::show_error_dialog(error.user_friendly_msg()) { tracing::error!(error = anyhow_dyn_err(&e), "Failed to show error dialog"); } telemetry.stop_on_crash().await;