From b5a67cd2d7e8d2c3645702cdbb210cf211348eec Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Tue, 27 Aug 2024 15:14:08 -0500 Subject: [PATCH] fix(rust/gui-client): when the Client starts with a token but no Internet, wait for Internet and then connect (#6414) Closes #6389 I added a retry button since the network change detection is flaky inside Parallels. On bare metal Windows it works fine. --------- Signed-off-by: Reactor Scram Co-authored-by: Jamil --- rust/gui-client/docs/manual_testing.md | 5 +- rust/gui-client/src-tauri/src/client/gui.rs | 199 +++++++++++++----- .../src-tauri/src/client/gui/system_tray.rs | 29 ++- .../src/client/gui/system_tray/builder.rs | 2 + rust/headless-client/src/ipc_service.rs | 20 +- website/src/components/Changelog/GUI.tsx | 4 +- 6 files changed, 182 insertions(+), 77 deletions(-) diff --git a/rust/gui-client/docs/manual_testing.md b/rust/gui-client/docs/manual_testing.md index c6b2bf47a..d61e5dd18 100644 --- a/rust/gui-client/docs/manual_testing.md +++ b/rust/gui-client/docs/manual_testing.md @@ -149,4 +149,7 @@ Moved to [`network_roaming.md`](network_roaming.md) # No Internet -Given Firezone is signed in and not running, when you disconnect from the Internet and start Firezone, then Firezone should show an error and quit. +1. Given Firezone is signed in and not running, when you disconnect from the Internet and start Firezone, then Firezone will wait for Internet and show the same icon as when it's signed out. +1. Given Firezone is waiting for Internet, when you click "Retry sign-in", then Firezone will retry sign-in immediately. +1. Given Firezone is waiting for Internet, when you gain Internet, then Firezone will automatically sign in. +1. Given Firezone is waiting for Internet, when you click "Cancel sign-in", then Firezone will sign out. diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 1287dede6..6eea11404 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -452,10 +452,25 @@ pub(crate) enum ControllerRequest { enum Status { /// Firezone is disconnected. Disconnected, - /// Firezone is signing in and raising the tunnel. - Connecting { start_instant: Instant }, + /// At least one connection request has failed, due to failing to reach the Portal, and we are waiting for a network change before we try again + RetryingConnection { + /// The token to log in to the Portal, for retrying the connection request. + token: SecretString, + }, /// Firezone is ready to use. TunnelReady { resources: Vec }, + /// Firezone is signing in to the Portal. + WaitingForPortal { + /// The instant when we sent our most recent connect request. + start_instant: Instant, + /// The token to log in to the Portal, in case we need to retry the connection request. + token: SecretString, + }, + /// Firezone has connected to the Portal and is raising the tunnel. + WaitingForTunnel { + /// The instant when we sent our most recent connect request. + start_instant: Instant, + }, } impl Default for Status { @@ -465,12 +480,13 @@ impl Default for Status { } impl Status { - /// Returns true if connlib has started, even if it's still signing in. - fn connlib_is_up(&self) -> bool { + /// Returns true if we want to hear about DNS and network changes. + fn needs_network_changes(&self) -> bool { match self { - Self::Disconnected => false, - Self::Connecting { .. } => true, - Self::TunnelReady { .. } => true, + Status::Disconnected | Status::RetryingConnection { .. } => false, + Status::TunnelReady { .. } + | Status::WaitingForPortal { .. } + | Status::WaitingForTunnel { .. } => true, } } } @@ -492,10 +508,14 @@ struct Controller { impl Controller { async fn start_session(&mut self, token: SecretString) -> Result<(), Error> { - if self.status.connlib_is_up() { - Err(anyhow::anyhow!( + match self.status { + Status::Disconnected | Status::RetryingConnection { .. } => {} + Status::TunnelReady { .. } => Err(anyhow!( "Can't connect to Firezone, we're already connected." - ))?; + ))?, + Status::WaitingForPortal { .. } | Status::WaitingForTunnel { .. } => Err(anyhow!( + "Can't connect to Firezone, we're already connecting." + ))?, } let api_url = self.advanced_settings.api_url.clone(); @@ -504,10 +524,13 @@ impl Controller { // Count the start instant from before we connect let start_instant = Instant::now(); self.ipc_client - .connect_to_firezone(api_url.as_str(), token) + .connect_to_firezone(api_url.as_str(), token.expose_secret().clone().into()) .await?; // Change the status after we begin connecting - self.status = Status::Connecting { start_instant }; + self.status = Status::WaitingForPortal { + start_instant, + token, + }; self.refresh_system_tray_menu()?; Ok(()) } @@ -606,23 +629,24 @@ impl Controller { .context("Couldn't copy resource URL or other text to clipboard")?, Req::SystemTrayMenu(TrayMenuEvent::CancelSignIn) => { match &self.status { - Status::Disconnected => { + Status::Disconnected | Status::RetryingConnection { .. } | Status::WaitingForPortal { .. } => { tracing::info!("Calling `sign_out` to cancel sign-in"); self.sign_out().await?; } - Status::Connecting { start_instant: _ } => { + Status::TunnelReady{..} => tracing::error!("Can't cancel sign-in, the tunnel is already up. This is a logic error in the code."), + Status::WaitingForTunnel { .. } => { tracing::warn!( "Connlib is already raising the tunnel, calling `sign_out` anyway" ); self.sign_out().await?; } - Status::TunnelReady{..} => tracing::error!("Can't cancel sign-in, the tunnel is already up. This is a logic error in the code."), } } Req::SystemTrayMenu(TrayMenuEvent::RemoveFavorite(resource_id)) => { self.advanced_settings.favorite_resources.remove(&resource_id); self.refresh_favorite_resources().await?; } + Req::SystemTrayMenu(TrayMenuEvent::RetryPortalConnection) => self.try_retry_connection().await?, Req::SystemTrayMenu(TrayMenuEvent::EnableResource(resource_id)) => { self.advanced_settings.disabled_resources.remove(&resource_id); self.update_disabled_resources().await?; @@ -683,16 +707,7 @@ impl Controller { })?; Ok(()) } - IpcServerMsg::ConnectResult(result) => match result { - Ok(()) => Ok(ran_before::set().await?), - Err(IpcServiceError::PortalConnection(s)) => Err(Error::PortalConnection(s)), - Err(error) => { - tracing::error!(?error, "Failed to connect to Firezone"); - Err(Error::Other(anyhow!( - "Failed to connect to Firezone for non-Portal-related reason" - ))) - } - }, + IpcServerMsg::ConnectResult(result) => self.handle_connect_result(result).await, IpcServerMsg::OnDisconnect { error_msg, is_authentication_error, @@ -716,25 +731,10 @@ impl Controller { Ok(()) } IpcServerMsg::OnUpdateResources(resources) => { - if self.auth.session().is_none() { - // This could happen if the user cancels the sign-in - // before it completes. This is because the state machine - // between the GUI, the IPC service, and connlib isn't perfectly synced. - tracing::error!("Got `UpdateResources` while signed out"); - return Ok(()); - } tracing::debug!(len = resources.len(), "Got new Resources"); - if let Status::Connecting { start_instant } = - std::mem::replace(&mut self.status, Status::TunnelReady { resources }) - { - tracing::info!(elapsed = ?start_instant.elapsed(), "Tunnel ready"); - os::show_notification( - "Firezone connected", - "You are now signed in and able to access resources.", - )?; - } + self.status = Status::TunnelReady { resources }; if let Err(error) = self.refresh_system_tray_menu() { - tracing::error!(?error, "Failed to refresh Resource list"); + tracing::error!(?error, "Failed to refresh menu"); } self.update_disabled_resources().await?; @@ -746,6 +746,76 @@ impl Controller { self.tray.set_icon(system_tray::Icon::SignedOut).ok(); Err(Error::IpcServiceTerminating) } + IpcServerMsg::TunnelReady => { + if self.auth.session().is_none() { + // This could maybe happen if the user cancels the sign-in + // before it completes. This is because the state machine + // between the GUI, the IPC service, and connlib isn't perfectly synced. + tracing::error!("Got `UpdateResources` while signed out"); + return Ok(()); + } + if let Status::WaitingForTunnel { start_instant } = + std::mem::replace(&mut self.status, Status::TunnelReady { resources: vec![] }) + { + tracing::info!(elapsed = ?start_instant.elapsed(), "Tunnel ready"); + os::show_notification( + "Firezone connected", + "You are now signed in and able to access resources.", + )?; + } + if let Err(error) = self.refresh_system_tray_menu() { + tracing::error!(?error, "Failed to refresh menu"); + } + + Ok(()) + } + } + } + + async fn handle_connect_result( + &mut self, + result: Result<(), IpcServiceError>, + ) -> Result<(), Error> { + let (start_instant, token) = match &self.status { + Status::Disconnected + | Status::RetryingConnection { .. } + | Status::TunnelReady { .. } + | Status::WaitingForTunnel { .. } => { + tracing::error!("Impossible logic error, received `ConnectResult` when we weren't waiting on the Portal connection."); + return Ok(()); + } + Status::WaitingForPortal { + start_instant, + token, + } => (*start_instant, token.expose_secret().clone().into()), + }; + + match result { + Ok(()) => { + ran_before::set().await?; + self.status = Status::WaitingForTunnel { start_instant }; + if let Err(error) = self.refresh_system_tray_menu() { + tracing::error!(?error, "Failed to refresh menu"); + } + Ok(()) + } + Err(IpcServiceError::PortalConnection(error)) => { + tracing::warn!( + ?error, + "Failed to connect to Firezone Portal, will try again when the network changes" + ); + self.status = Status::RetryingConnection { token }; + if let Err(error) = self.refresh_system_tray_menu() { + tracing::error!(?error, "Failed to refresh menu"); + } + Ok(()) + } + Err(error) => { + tracing::error!(?error, "Failed to connect to Firezone"); + Err(Error::Other(anyhow!( + "Failed to connect to Firezone for non-Portal-related reason" + ))) + } } } @@ -787,7 +857,7 @@ impl Controller { tracing::error!("We have an auth session but no connlib session"); system_tray::AppState::SignedOut } - Status::Connecting { start_instant: _ } => system_tray::AppState::WaitingForConnlib, + Status::RetryingConnection { .. } => system_tray::AppState::RetryingConnection, Status::TunnelReady { resources } => { system_tray::AppState::SignedIn(system_tray::SignedIn { actor_name: &auth_session.actor_name, @@ -796,6 +866,8 @@ impl Controller { resources, }) } + Status::WaitingForPortal { .. } => system_tray::AppState::WaitingForPortal, + Status::WaitingForTunnel { .. } => system_tray::AppState::WaitingForTunnel, } } else if self.auth.ongoing_request().is_ok() { // Signing in, waiting on deep link callback @@ -807,20 +879,29 @@ impl Controller { Ok(()) } + /// If we're in the `RetryingConnection` state, use the token to retry the Portal connection + async fn try_retry_connection(&mut self) -> Result<()> { + let token = match &self.status { + Status::Disconnected + | Status::TunnelReady { .. } + | Status::WaitingForPortal { .. } + | Status::WaitingForTunnel { .. } => return Ok(()), + Status::RetryingConnection { token } => token, + }; + tracing::debug!("Retrying Portal connection..."); + self.start_session(token.expose_secret().clone().into()) + .await?; + Ok(()) + } + /// Deletes the auth token, stops connlib, and refreshes the tray menu async fn sign_out(&mut self) -> Result<()> { self.auth.sign_out()?; - if self.status.connlib_is_up() { - self.status = Status::Disconnected; - tracing::debug!("disconnecting connlib"); - // This is redundant if the token is expired, in that case - // connlib already disconnected itself. - self.ipc_client.disconnect_from_firezone().await?; - } else { - // Might just be because we got a double sign-out or - // the user canceled the sign-in or something innocent. - tracing::info!("Tried to sign out but connlib is not up, cancelled sign-in"); - } + self.status = Status::Disconnected; + tracing::debug!("disconnecting connlib"); + // This is redundant if the token is expired, in that case + // connlib already disconnected itself. + self.ipc_client.disconnect_from_firezone().await?; self.refresh_system_tray_menu()?; Ok(()) } @@ -901,18 +982,20 @@ async fn run_controller( tokio::select! { result = network_notifier.notified() => { result?; - if controller.status.connlib_is_up() { - tracing::debug!("Internet up/down changed, calling `Session::reconnect`"); - controller.ipc_client.reset().await?; + if controller.status.needs_network_changes() { + tracing::debug!("Internet up/down changed, calling `Session::reset`"); + controller.ipc_client.reset().await? } + controller.try_retry_connection().await? }, result = dns_notifier.notified() => { result?; - if controller.status.connlib_is_up() { + if controller.status.needs_network_changes() { let resolvers = firezone_headless_client::dns_control::system_resolvers_for_gui()?; tracing::debug!(?resolvers, "New DNS resolvers, calling `Session::set_dns`"); controller.ipc_client.set_dns(resolvers).await?; } + controller.try_retry_connection().await? }, req = rx.recv() => { let Some(req) = req else { diff --git a/rust/gui-client/src-tauri/src/client/gui/system_tray.rs b/rust/gui-client/src-tauri/src/client/gui/system_tray.rs index c51ba8883..8d2a9b29f 100644 --- a/rust/gui-client/src-tauri/src/client/gui/system_tray.rs +++ b/rust/gui-client/src-tauri/src/client/gui/system_tray.rs @@ -55,10 +55,12 @@ pub(crate) struct Tray { pub(crate) enum AppState<'a> { Loading, + RetryingConnection, + SignedIn(SignedIn<'a>), SignedOut, WaitingForBrowser, - WaitingForConnlib, - SignedIn(SignedIn<'a>), + WaitingForPortal, + WaitingForTunnel, } pub(crate) struct SignedIn<'a> { @@ -146,11 +148,12 @@ impl Tray { pub(crate) fn update(&mut self, state: AppState) -> Result<()> { let new_icon = match &state { - AppState::Loading | AppState::WaitingForBrowser | AppState::WaitingForConnlib => { - Icon::Busy - } - AppState::SignedOut => Icon::SignedOut, + AppState::Loading + | AppState::RetryingConnection + | AppState::SignedOut + | AppState::WaitingForBrowser => Icon::SignedOut, AppState::SignedIn { .. } => Icon::SignedIn, + AppState::WaitingForPortal | AppState::WaitingForTunnel => Icon::Busy, }; self.handle.set_tooltip(TOOLTIP)?; @@ -193,12 +196,14 @@ impl<'a> AppState<'a> { fn into_menu(self) -> Menu { match self { Self::Loading => Menu::default().disabled("Loading..."), + Self::RetryingConnection => retrying_sign_in("Waiting for Internet access..."), + Self::SignedIn(x) => signed_in(&x), Self::SignedOut => Menu::default() .item(Event::SignIn, "Sign In") .add_bottom_section(QUIT_TEXT_SIGNED_OUT), Self::WaitingForBrowser => signing_in("Waiting for browser..."), - Self::WaitingForConnlib => signing_in("Signing In..."), - Self::SignedIn(x) => signed_in(&x), + Self::WaitingForPortal => signing_in("Connecting to Firezone Portal..."), + Self::WaitingForTunnel => signing_in("Raising tunnel..."), } } } @@ -259,6 +264,14 @@ fn signed_in(signed_in: &SignedIn) -> Menu { menu.add_bottom_section(DISCONNECT_AND_QUIT) } +fn retrying_sign_in(waiting_message: &str) -> Menu { + Menu::default() + .disabled(waiting_message) + .item(Event::RetryPortalConnection, "Retry sign-in") + .item(Event::CancelSignIn, "Cancel sign-in") + .add_bottom_section(QUIT_TEXT_SIGNED_OUT) +} + fn signing_in(waiting_message: &str) -> Menu { Menu::default() .disabled(waiting_message) diff --git a/rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs b/rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs index a120e0c1c..73d25f7a6 100644 --- a/rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs +++ b/rust/gui-client/src-tauri/src/client/gui/system_tray/builder.rs @@ -52,6 +52,8 @@ pub(crate) enum Event { Copy(String), /// Marks this Resource as non-favorite RemoveFavorite(ResourceId), + /// If a Portal connection has failed, try again immediately + RetryPortalConnection, /// Starts the sign-in flow SignIn, /// Signs the user out, without quitting the app diff --git a/rust/headless-client/src/ipc_service.rs b/rust/headless-client/src/ipc_service.rs index bba348e3e..ec9f03a1c 100644 --- a/rust/headless-client/src/ipc_service.rs +++ b/rust/headless-client/src/ipc_service.rs @@ -1,6 +1,6 @@ use crate::{ device_id, dns_control::DnsController, known_dirs, signals, CallbackHandler, CliCommon, - ConnlibMsg, IpcServerMsg, LogFilterReloader, + ConnlibMsg, LogFilterReloader, }; use anyhow::{bail, Context as _, Result}; use clap::Parser; @@ -100,6 +100,8 @@ pub enum ServerMsg { /// "Firezone is updating, please restart the GUI" instead of an error like, /// "IPC connection closed". TerminatingGracefully, + /// The interface and tunnel are ready for traffic. + TunnelReady, } // All variants are `String` because almost no error type implements `Serialize` @@ -327,7 +329,7 @@ impl<'a> Handler<'a> { "Caught SIGINT / SIGTERM / Ctrl+C while an IPC client is connected" ); self.ipc_tx - .send(&IpcServerMsg::TerminatingGracefully) + .send(&ServerMsg::TerminatingGracefully) .await .unwrap(); break HandlerOk::ServiceTerminating; @@ -370,7 +372,7 @@ impl<'a> Handler<'a> { is_authentication_error, } => self .ipc_tx - .send(&IpcServerMsg::OnDisconnect { + .send(&ServerMsg::OnDisconnect { error_msg, is_authentication_error, }) @@ -382,12 +384,16 @@ impl<'a> Handler<'a> { if let Some(instant) = self.last_connlib_start_instant.take() { tracing::info!(elapsed = ?instant.elapsed(), "Tunnel ready"); } + self.ipc_tx + .send(&ServerMsg::TunnelReady) + .await + .context("Error while sending IPC message `TunnelReady`")?; } ConnlibMsg::OnUpdateResources(resources) => { // On every resources update, flush DNS to mitigate self.dns_controller.flush()?; self.ipc_tx - .send(&IpcServerMsg::OnUpdateResources(resources)) + .send(&ServerMsg::OnUpdateResources(resources)) .await .context("Error while sending IPC message `OnUpdateResources`")?; } @@ -411,9 +417,7 @@ impl<'a> Handler<'a> { ) .await; self.ipc_tx - .send(&IpcServerMsg::ClearedLogs( - result.map_err(|e| e.to_string()), - )) + .send(&ServerMsg::ClearedLogs(result.map_err(|e| e.to_string()))) .await .context("Error while sending IPC message")? } @@ -421,7 +425,7 @@ impl<'a> Handler<'a> { let token = secrecy::SecretString::from(token); let result = self.connect_to_firezone(&api_url, token); self.ipc_tx - .send(&IpcServerMsg::ConnectResult(result)) + .send(&ServerMsg::ConnectResult(result)) .await .context("Failed to send `ConnectResult`")? } diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index 9ce69202a..937f29a76 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -16,8 +16,8 @@ export default function GUI({ title }: { title: string }) { {/*
    - - Shows an error if there's no Internet at startup + + Waits for Internet if there's no Internet at startup and you're already signed in