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

@@ -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 <https://github.com/firezone/firezone/pull/3546#discussion_r1477114789>
 #[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: <https://github.com/firezone/firezone/pull/3464#discussion_r1473608415>
+    // 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 <https://www.firezone.dev/kb/client-apps/windows-client>.".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: <https://github.com/firezone/firezone/pull/3464#discussion_r1473608415>
-    // 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 <https://www.firezone.dev/kb/client-apps/windows-client>.".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(())

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 }

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?)

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<bool, Error> {
-        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<bool, Error> {
         Ok(true)
     }
+
+    #[derive(Debug, Clone, Copy, thiserror::Error)]
+    pub(crate) enum Error {}
 }
 
 #[cfg(test)]

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;