From 144418f47cd83c2f4e7ed0cee2a7c63a2e0dd7a3 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Tue, 25 Jun 2024 21:07:43 +0000 Subject: [PATCH] chore(gui-client): show an error dialog if the IPC service isn't running (#5454) Closes #5377 Windows image KDE image ```[tasklist] ### Tasks - [x] Fix broken smoke tests #5464 - [x] Linux manual sign-in - [x] Linux auto sign-in - [x] Windows manual sign-in - [x] Window auto sign-in - [x] Open for review - [ ] Apply feedback - [ ] Merge ``` --------- Signed-off-by: Reactor Scram --- rust/gui-client/src-tauri/src/client.rs | 29 +----- rust/gui-client/src-tauri/src/client/gui.rs | 94 +++++++++---------- .../src-tauri/src/client/gui/errors.rs | 58 ++++++++++++ rust/gui-client/src-tauri/src/client/ipc.rs | 12 ++- rust/headless-client/Cargo.toml | 1 + rust/headless-client/src/ipc.rs | 38 ++++++-- rust/headless-client/src/ipc/linux.rs | 33 ++++--- rust/headless-client/src/ipc/windows.rs | 15 ++- 8 files changed, 174 insertions(+), 106 deletions(-) create mode 100644 rust/gui-client/src-tauri/src/client/gui/errors.rs diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index 7d55f802c..1ca43018a 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -1,7 +1,6 @@ use anyhow::{bail, Context as _, Result}; use clap::{Args, Parser}; use connlib_client_shared::file_logger; -use firezone_headless_client::FIREZONE_GROUP; use std::path::PathBuf; use tracing::instrument; use tracing_subscriber::EnvFilter; @@ -74,7 +73,7 @@ pub(crate) fn run() -> Result<()> { Ok(true) => run_gui(cli), Ok(false) => bail!("The GUI should run as a normal user, not elevated"), Err(error) => { - show_error_dialog(&error)?; + gui::show_error_dialog(&error)?; Err(error.into()) } } @@ -125,7 +124,7 @@ fn run_gui(cli: Cli) -> Result<()> { // Make sure errors get logged, at least to stderr if let Err(error) = &result { tracing::error!(?error, error_msg = %error); - show_error_dialog(error)?; + gui::show_error_dialog(error)?; } Ok(result?) @@ -158,30 +157,6 @@ fn start_logging(directives: &str) -> Result { Ok(logging_handles.logger) } -#[instrument(skip_all)] -fn show_error_dialog(error: &gui::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 { - // TODO: Update this URL - gui::Error::WebViewNotInstalled => "Firezone cannot start because WebView2 is not installed. Follow the instructions at .".to_string(), - gui::Error::DeepLink(deep_link::Error::CantListen) => "Firezone is already running. If it's not responding, force-stop it.".to_string(), - gui::Error::DeepLink(deep_link::Error::Other(error)) => error.to_string(), - gui::Error::Logging(_) => "Logging error".to_string(), - gui::Error::UserNotInFirezoneGroup => format!("You are not a member of the group `{FIREZONE_GROUP}`. Try `sudo usermod -aG {FIREZONE_GROUP} $USER` and then reboot"), - gui::Error::Other(error) => error.to_string(), - }; - tracing::error!(?user_friendly_error_msg); - - native_dialog::MessageDialog::new() - .set_title("Firezone Error") - .set_text(&user_friendly_error_msg) - .set_type(native_dialog::MessageType::Error) - .show_alert()?; - Ok(()) -} - /// The debug / test flags like `crash_on_purpose` and `test_update_notification` /// don't propagate when we use `RunAs` to elevate ourselves. So those must be run /// from an admin terminal, or with "Run as administrator" in the right-click menu. diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 69dfd1e48..d86ae0f62 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -10,7 +10,7 @@ use crate::client::{ settings::{self, AdvancedSettings}, Failure, }; -use anyhow::{bail, Context, Result}; +use anyhow::{anyhow, bail, Context, Result}; use secrecy::{ExposeSecret, SecretString}; use std::{path::PathBuf, str::FromStr, sync::Arc, time::Duration}; use system_tray_menu::Event as TrayMenuEvent; @@ -21,6 +21,7 @@ use url::Url; use ControllerRequest as Req; +mod errors; mod ran_before; pub(crate) mod system_tray_menu; @@ -40,6 +41,7 @@ mod os; #[allow(clippy::unnecessary_wraps)] mod os; +pub(crate) use errors::{show_error_dialog, Error}; pub(crate) use os::set_autostart; pub(crate) type CtlrTx = mpsc::Sender; @@ -55,24 +57,6 @@ pub(crate) struct Managed { pub inject_faults: bool, } -// TODO: Replace with `anyhow` gradually per -#[allow(dead_code)] -#[derive(Debug, thiserror::Error)] -pub(crate) enum Error { - #[error("Deep-link module error: {0}")] - DeepLink(#[from] deep_link::Error), - #[error("Logging module error: {0}")] - Logging(#[from] logging::Error), - - // `client.rs` provides a more user-friendly message when showing the error dialog box for certain variants - #[error("UserNotInFirezoneGroup")] - UserNotInFirezoneGroup, - #[error("WebViewNotInstalled")] - WebViewNotInstalled, - #[error(transparent)] - Other(#[from] anyhow::Error), -} - /// Runs the Tauri GUI and returns on exit or unrecoverable error /// /// Still uses `thiserror` so we can catch the deep_link `CantListen` error @@ -236,6 +220,7 @@ pub(crate) fn run( } Ok(Err(error)) => { tracing::error!(?error, "run_controller returned an error"); + errors::show_error_dialog(&error).unwrap(); 1 } Ok(Ok(_)) => 0, @@ -454,9 +439,9 @@ struct Session { impl Controller { // TODO: Figure out how re-starting sessions automatically will work /// Pre-req: the auth module must be signed in - async fn start_session(&mut self, token: SecretString) -> Result<()> { + async fn start_session(&mut self, token: SecretString) -> Result<(), Error> { if self.session.is_some() { - bail!("can't start session, we're already in a session"); + Err(anyhow!("can't start session, we're already in a session"))?; } let callback_handler = CallbackHandler { @@ -486,20 +471,21 @@ impl Controller { Ok(()) } - async fn handle_deep_link(&mut self, url: &SecretString) -> Result<()> { + async fn handle_deep_link(&mut self, url: &SecretString) -> Result<(), Error> { let auth_response = client::deep_link::parse_auth_callback(url).context("Couldn't parse scheme request")?; tracing::info!("Received deep link over IPC"); // Uses `std::fs` - let token = self.auth.handle_response(auth_response)?; - self.start_session(token) - .await - .context("Couldn't start connlib session")?; + let token = self + .auth + .handle_response(auth_response) + .context("Couldn't handle auth response")?; + self.start_session(token).await?; Ok(()) } - async fn handle_request(&mut self, req: ControllerRequest) -> Result<()> { + async fn handle_request(&mut self, req: ControllerRequest) -> Result<(), Error> { match req { Req::ApplySettings(settings) => { self.advanced_settings = settings; @@ -529,36 +515,43 @@ impl Controller { .set_title("Firezone Error") .set_text(&error_msg) .set_type(native_dialog::MessageType::Error) - .show_alert()?; + .show_alert() + .context("Couldn't show Disconnected alert")?; } } Req::ExportLogs { path, stem } => logging::export_logs_to(path, stem) .await .context("Failed to export logs to zip")?, - Req::Fail(_) => bail!("Impossible error: `Fail` should be handled before this"), + Req::Fail(_) => Err(anyhow!( + "Impossible error: `Fail` should be handled before this" + ))?, Req::GetAdvancedSettings(tx) => { tx.send(self.advanced_settings.clone()).ok(); } - Req::SchemeRequest(url) => self - .handle_deep_link(&url) - .await - .context("Couldn't handle deep link")?, + Req::SchemeRequest(url) => self.handle_deep_link(&url).await?, Req::SignIn | Req::SystemTrayMenu(TrayMenuEvent::SignIn) => { - if let Some(req) = self.auth.start_sign_in()? { + if let Some(req) = self + .auth + .start_sign_in() + .context("Couldn't start sign-in flow")? + { let url = req.to_url(&self.advanced_settings.auth_base_url); self.refresh_system_tray_menu()?; - tauri::api::shell::open(&self.app.shell_scope(), url.expose_secret(), None)?; + tauri::api::shell::open(&self.app.shell_scope(), url.expose_secret(), None) + .context("Couldn't open auth page")?; self.app .get_window("welcome") .context("Couldn't get handle to Welcome window")? - .hide()?; + .hide() + .context("Couldn't hide Welcome window")?; } } Req::SystemTrayMenu(TrayMenuEvent::AdminPortal) => tauri::api::shell::open( &self.app.shell_scope(), &self.advanced_settings.auth_base_url, None, - )?, + ) + .context("Couldn't open auth page")?, Req::SystemTrayMenu(TrayMenuEvent::Copy(s)) => arboard::Clipboard::new() .context("Couldn't access clipboard")? .set_text(s) @@ -595,11 +588,12 @@ impl Controller { self.sign_out().await?; } Req::SystemTrayMenu(TrayMenuEvent::Url(url)) => { - tauri::api::shell::open(&self.app.shell_scope(), url, None)? - } - Req::SystemTrayMenu(TrayMenuEvent::Quit) => { - bail!("Impossible error: `Quit` should be handled before this") + tauri::api::shell::open(&self.app.shell_scope(), url, None) + .context("Couldn't open URL from system tray")? } + Req::SystemTrayMenu(TrayMenuEvent::Quit) => Err(anyhow!( + "Impossible error: `Quit` should be handled before this" + ))?, Req::TunnelReady => { if !self.tunnel_ready { os::show_notification( @@ -620,7 +614,8 @@ impl Controller { } Req::UpdateNotificationClicked(download_url) => { tracing::info!("UpdateNotificationClicked in run_controller!"); - tauri::api::shell::open(&self.app.shell_scope(), download_url, None)?; + tauri::api::shell::open(&self.app.shell_scope(), download_url, None) + .context("Couldn't open update page")?; } } Ok(()) @@ -703,7 +698,7 @@ async fn run_controller( ctlr_tx: CtlrTx, mut rx: mpsc::Receiver, advanced_settings: AdvancedSettings, -) -> Result<()> { +) -> Result<(), Error> { tracing::info!("Entered `run_controller`"); let mut controller = Controller { advanced_settings, @@ -721,10 +716,7 @@ async fn run_controller( .token() .context("Failed to load token from disk during app start")? { - controller - .start_session(token) - .await - .context("Failed to restart session during app start")?; + controller.start_session(token).await?; } else { tracing::info!("No token / actor_name on disk, starting in signed-out state"); } @@ -733,7 +725,7 @@ async fn run_controller( let win = app .get_window("welcome") .context("Couldn't get handle to Welcome window")?; - win.show()?; + win.show().context("Couldn't show Welcome window")?; } let mut have_internet = @@ -782,15 +774,13 @@ async fn run_controller( tracing::error!("Crashing on purpose"); unsafe { sadness_generator::raise_segfault() } }, - Req::Fail(Failure::Error) => bail!("Test error"), + Req::Fail(Failure::Error) => Err(anyhow!("Test error"))?, Req::Fail(Failure::Panic) => panic!("Test panic"), Req::SystemTrayMenu(TrayMenuEvent::Quit) => { tracing::info!("User clicked Quit in the menu"); break } - req => if let Err(error) = controller.handle_request(req).await { - tracing::error!(?error, "Failed to handle a ControllerRequest"); - } + req => controller.handle_request(req).await?, } }, } diff --git a/rust/gui-client/src-tauri/src/client/gui/errors.rs b/rust/gui-client/src-tauri/src/client/gui/errors.rs new file mode 100644 index 000000000..6fca851eb --- /dev/null +++ b/rust/gui-client/src-tauri/src/client/gui/errors.rs @@ -0,0 +1,58 @@ +use super::{deep_link, logging}; +use anyhow::Result; +use firezone_headless_client::{ipc, FIREZONE_GROUP}; + +// TODO: Replace with `anyhow` gradually per +#[allow(dead_code)] +#[derive(Debug, thiserror::Error)] +pub(crate) enum Error { + #[error("Deep-link module error: {0}")] + DeepLink(#[from] deep_link::Error), + #[error("Logging module error: {0}")] + Logging(#[from] logging::Error), + + // `client.rs` provides a more user-friendly message when showing the error dialog box for certain variants + #[error("IPC")] + Ipc(#[from] ipc::Error), + #[error("UserNotInFirezoneGroup")] + UserNotInFirezoneGroup, + #[error("WebViewNotInstalled")] + WebViewNotInstalled, + #[error(transparent)] + Other(#[from] anyhow::Error), +} + +/// 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(crate) 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 { + // TODO: Update this URL + 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::Logging(_) => "Logging error".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(), + }; + tracing::error!(?user_friendly_error_msg); + + // 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_type(native_dialog::MessageType::Error) + .show_alert()?; + Ok(()) +} diff --git a/rust/gui-client/src-tauri/src/client/ipc.rs b/rust/gui-client/src-tauri/src/client/ipc.rs index 1bc015986..94735c871 100644 --- a/rust/gui-client/src-tauri/src/client/ipc.rs +++ b/rust/gui-client/src-tauri/src/client/ipc.rs @@ -2,7 +2,10 @@ use crate::client::gui::{ControllerRequest, CtlrTx}; use anyhow::{Context as _, Result}; use arc_swap::ArcSwap; use connlib_client_shared::callbacks::ResourceDescription; -use firezone_headless_client::{ipc, IpcClientMsg, IpcServerMsg}; +use firezone_headless_client::{ + ipc::{self, Error}, + IpcClientMsg, IpcServerMsg, +}; use futures::{SinkExt, StreamExt}; use secrecy::{ExposeSecret, SecretString}; use std::{net::IpAddr, sync::Arc}; @@ -43,7 +46,7 @@ impl CallbackHandler { pub(crate) struct Client { task: tokio::task::JoinHandle>, // Needed temporarily to avoid a big refactor. We can remove this in the future. - tx: firezone_headless_client::ipc::ClientWrite, + tx: ipc::ClientWrite, } impl Client { @@ -69,7 +72,7 @@ impl Client { token: SecretString, callback_handler: CallbackHandler, tokio_handle: tokio::runtime::Handle, - ) -> Result { + ) -> Result { tracing::info!( client_pid = std::process::id(), "Connecting to IPC service..." @@ -99,7 +102,8 @@ impl Client { token, }) .await - .context("Couldn't send Connect message")?; + .context("Couldn't send Connect message") + .map_err(Error::Other)?; Ok(client) } diff --git a/rust/headless-client/Cargo.toml b/rust/headless-client/Cargo.toml index d54383de2..afc7f3ba9 100644 --- a/rust/headless-client/Cargo.toml +++ b/rust/headless-client/Cargo.toml @@ -21,6 +21,7 @@ ip_network = { version = "0.4", default-features = false } secrecy = { workspace = true } serde = { version = "1.0.203", features = ["derive"] } serde_json = "1.0.117" +thiserror = { version = "1.0", default-features = false } # This actually relies on many other features in Tokio, so this will probably # fail to build outside the workspace. tokio = { version = "1.38.0", features = ["macros", "signal"] } diff --git a/rust/headless-client/src/ipc.rs b/rust/headless-client/src/ipc.rs index c7f6257be..7b10d636f 100644 --- a/rust/headless-client/src/ipc.rs +++ b/rust/headless-client/src/ipc.rs @@ -1,5 +1,5 @@ use crate::{IpcClientMsg, IpcServerMsg}; -use anyhow::{anyhow, Context as _, Result}; +use anyhow::{Context as _, Result}; use tokio::io::{ReadHalf, WriteHalf}; use tokio_util::{ bytes::BytesMut, @@ -24,6 +24,18 @@ pub type ClientWrite = FramedWrite, Encoder, Decoder>; pub(crate) type ServerWrite = FramedWrite, Encoder>; +// pub so that the GUI can display a human-friendly message +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("Couldn't find IPC service `{0}`")] + NotFound(String), + #[error("Permission denied")] + PermissionDenied, + + #[error(transparent)] + Other(anyhow::Error), +} + /// A name that both the server and client can use to find each other /// /// In the platform-specific code, this is translated to a Unix Domain Socket @@ -118,7 +130,11 @@ impl tokio_util::codec::Encoder<&E> for Encoder { /// Connect to the IPC service /// /// Public because the GUI Client will need it -pub async fn connect_to_service(id: ServiceId) -> Result<(ClientRead, ClientWrite)> { +pub async fn connect_to_service(id: ServiceId) -> Result<(ClientRead, ClientWrite), Error> { + // This is how ChatGPT recommended, and I couldn't think of any more clever + // way before I asked it. + let mut last_err = None; + for _ in 0..10 { match platform::connect_to_service(id).await { Ok(stream) => { @@ -132,15 +148,14 @@ pub async fn connect_to_service(id: ServiceId) -> Result<(ClientRead, ClientWrit ?error, "Couldn't connect to IPC service, will sleep and try again" ); + last_err = Some(error); // This won't come up much for humans but it helps the automated // tests pass tokio::time::sleep(std::time::Duration::from_millis(100)).await; } } } - Err(anyhow!( - "Failed to connect to IPC server after multiple attempts" - )) + Err(last_err.expect("Impossible - Exhausted all retries but didn't get any errors")) } impl platform::Server { @@ -156,11 +171,22 @@ impl platform::Server { mod tests { use super::{platform::Server, ServiceId}; use crate::{IpcClientMsg, IpcServerMsg}; - use anyhow::{ensure, Context as _, Result}; + use anyhow::{bail, ensure, Context as _, Result}; use futures::{SinkExt, StreamExt}; use std::time::Duration; use tokio::{task::JoinHandle, time::timeout}; + #[tokio::test] + async fn no_such_service() -> Result<()> { + let _ = tracing_subscriber::fmt().with_test_writer().try_init(); + const ID: ServiceId = ServiceId::Test("H56FRXVH"); + + if super::connect_to_service(ID).await.is_ok() { + bail!("`connect_to_service` should have failed for a non-existent service"); + } + Ok(()) + } + /// Make sure the IPC client and server can exchange messages #[tokio::test] async fn smoke() -> Result<()> { diff --git a/rust/headless-client/src/ipc/linux.rs b/rust/headless-client/src/ipc/linux.rs index df10acc56..f334dfbc6 100644 --- a/rust/headless-client/src/ipc/linux.rs +++ b/rust/headless-client/src/ipc/linux.rs @@ -1,6 +1,6 @@ -use super::ServiceId; -use anyhow::{Context as _, Result}; -use std::{os::unix::fs::PermissionsExt, path::PathBuf}; +use super::{Error, ServiceId}; +use anyhow::{anyhow, Context as _, Result}; +use std::{io::ErrorKind, os::unix::fs::PermissionsExt, path::PathBuf}; use tokio::net::{UnixListener, UnixStream}; pub(crate) struct Server { @@ -16,16 +16,25 @@ pub type ClientStream = UnixStream; pub(crate) type ServerStream = UnixStream; /// Connect to the IPC service -#[allow(clippy::unused_async)] -pub async fn connect_to_service(id: ServiceId) -> Result { +#[allow(clippy::wildcard_enum_match_arm)] +pub async fn connect_to_service(id: ServiceId) -> Result { let path = ipc_path(id); - let stream = UnixStream::connect(&path).await.with_context(|| { - format!( - "Couldn't connect to Unix domain socket at `{}`", - path.display() - ) - })?; - let cred = stream.peer_cred()?; + let stream = UnixStream::connect(&path) + .await + .map_err(|error| match error.kind() { + ErrorKind::ConnectionRefused => { + Error::Other(anyhow!("ConnectionRefused by Unix domain socket")) + } + ErrorKind::NotFound => Error::NotFound(path.display().to_string()), + ErrorKind::PermissionDenied => Error::PermissionDenied, + _ => Error::Other( + anyhow!(error.to_string()).context("Couldn't connect to Unix domain socket"), + ), + })?; + let cred = stream + .peer_cred() + .context("Couldn't get PID of UDS server") + .map_err(Error::Other)?; tracing::info!( uid = cred.uid(), gid = cred.gid(), diff --git a/rust/headless-client/src/ipc/windows.rs b/rust/headless-client/src/ipc/windows.rs index c6d47c71b..12dbc2834 100644 --- a/rust/headless-client/src/ipc/windows.rs +++ b/rust/headless-client/src/ipc/windows.rs @@ -1,7 +1,7 @@ -use super::ServiceId; +use super::{Error, ServiceId}; use anyhow::{bail, Context as _, Result}; use connlib_shared::BUNDLE_ID; -use std::{ffi::c_void, os::windows::io::AsRawHandle, time::Duration}; +use std::{ffi::c_void, io::ErrorKind, os::windows::io::AsRawHandle, time::Duration}; use tokio::net::windows::named_pipe; use windows::Win32::{ Foundation::HANDLE, @@ -23,17 +23,22 @@ pub(crate) type ServerStream = named_pipe::NamedPipeServer; /// /// This is async on Linux #[allow(clippy::unused_async)] -pub(crate) async fn connect_to_service(id: ServiceId) -> Result { +#[allow(clippy::wildcard_enum_match_arm)] +pub(crate) async fn connect_to_service(id: ServiceId) -> Result { let path = ipc_path(id); let stream = named_pipe::ClientOptions::new() .open(&path) - .with_context(|| format!("Couldn't connect to named pipe server at `{path}`"))?; + .map_err(|error| match error.kind() { + ErrorKind::NotFound => Error::NotFound(path), + _ => Error::Other(error.into()), + })?; let handle = HANDLE(stream.as_raw_handle() as isize); let mut server_pid: u32 = 0; // SAFETY: Windows doesn't store this pointer or handle, and we just got the handle // from Tokio, so it should be valid. unsafe { GetNamedPipeServerProcessId(handle, &mut server_pid) } - .context("Couldn't get PID of named pipe server")?; + .context("Couldn't get PID of named pipe server") + .map_err(Error::Other)?; tracing::info!(?server_pid, "Made IPC connection"); Ok(stream) }