From e83b07093f3aefc331d129467444672121212e39 Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Fri, 19 Jan 2024 11:17:26 -0600 Subject: [PATCH] refactor(windows): clean up and simplify subcommands (#3306) Specifically deep linking had some problems: - Passing PIPE_ID when there was no other possible valid argument - Production use case accidentally flowed through a debug subcommand - Unused subcommands that were ready to be removed --- rust/windows-client/src-tauri/src/client.rs | 21 +++++++++--- .../src-tauri/src/client/crash_handling.rs | 3 +- .../src-tauri/src/client/debug_commands.rs | 34 ------------------- .../src-tauri/src/client/deep_link.rs | 19 +++++++---- .../src-tauri/src/client/gui.rs | 32 ++++++----------- 5 files changed, 40 insertions(+), 69 deletions(-) diff --git a/rust/windows-client/src-tauri/src/client.rs b/rust/windows-client/src-tauri/src/client.rs index 9f71919ce..924ec055d 100644 --- a/rust/windows-client/src-tauri/src/client.rs +++ b/rust/windows-client/src-tauri/src/client.rs @@ -15,6 +15,17 @@ mod resolvers; mod settings; mod wintun_install; +/// Bundle ID / App ID that we use to distinguish ourself from other programs on the system +/// +/// e.g. In ProgramData and AppData we use this to name our subdirectories for configs and data, +/// and Windows may use it to track things like the MSI installer, notification titles, +/// deep link registration, etc. +/// +/// This should be identical to the `tauri.bundle.identifier` over in `tauri.conf.json`, +/// but sometimes I need to use this before Tauri has booted up, or in a place where +/// getting the Tauri app handle would be awkward. +pub const BUNDLE_ID: &str = "dev.firezone.client"; + /// Output of `git describe` at compile time /// e.g. `1.0.0-pre.4-20-ged5437c88-modified` where: /// @@ -106,7 +117,6 @@ pub(crate) fn run() -> Result<()> { Some(Cmd::DebugCrash) => debug_commands::crash(), Some(Cmd::DebugHostname) => debug_commands::hostname(), Some(Cmd::DebugNetworkChanges) => debug_commands::network_changes(), - Some(Cmd::DebugPipeServer) => debug_commands::pipe_server(), Some(Cmd::DebugWintun) => debug_commands::wintun(cli), // If we already tried to elevate ourselves, don't try again Some(Cmd::Elevated) => gui::run(GuiParams { @@ -114,8 +124,11 @@ pub(crate) fn run() -> Result<()> { flag_elevated: true, inject_faults: cli.inject_faults, }), - Some(Cmd::OpenDeepLink(deep_link)) => debug_commands::open_deep_link(&deep_link.url), - Some(Cmd::RegisterDeepLink) => debug_commands::register_deep_link(), + Some(Cmd::OpenDeepLink(deep_link)) => { + let rt = tokio::runtime::Runtime::new()?; + rt.block_on(deep_link::open(&deep_link.url))?; + Ok(()) + } } } @@ -137,11 +150,9 @@ pub enum Cmd { DebugCrash, DebugHostname, DebugNetworkChanges, - DebugPipeServer, DebugWintun, Elevated, OpenDeepLink(DeepLink), - RegisterDeepLink, } #[derive(Args)] diff --git a/rust/windows-client/src-tauri/src/client/crash_handling.rs b/rust/windows-client/src-tauri/src/client/crash_handling.rs index 6c414bbea..8c1231368 100755 --- a/rust/windows-client/src-tauri/src/client/crash_handling.rs +++ b/rust/windows-client/src-tauri/src/client/crash_handling.rs @@ -4,6 +4,7 @@ //! //! TODO: Capture crash dumps on panic. +use crate::client::BUNDLE_ID; use anyhow::{anyhow, bail, Context}; use known_folders::{get_known_folder_path, KnownFolder}; use std::{fs::File, io::Write, path::PathBuf}; @@ -105,7 +106,7 @@ impl minidumper::ServerHandler for Handler { fn create_minidump_file(&self) -> Result<(File, PathBuf), std::io::Error> { let dump_path = get_known_folder_path(KnownFolder::ProgramData) .expect("should be able to find C:/ProgramData") - .join(crate::client::gui::BUNDLE_ID) + .join(BUNDLE_ID) .join("dumps") .join("last_crash.dmp"); diff --git a/rust/windows-client/src-tauri/src/client/debug_commands.rs b/rust/windows-client/src-tauri/src/client/debug_commands.rs index 7275a08e1..53aee527a 100644 --- a/rust/windows-client/src-tauri/src/client/debug_commands.rs +++ b/rust/windows-client/src-tauri/src/client/debug_commands.rs @@ -3,12 +3,8 @@ use crate::client::Cli; use anyhow::Result; -use tokio::runtime::Runtime; use windows::Win32::System::Com::{CoInitializeEx, CoUninitialize, COINIT_MULTITHREADED}; -// TODO: In tauri-plugin-deep-link, this is the identifier in tauri.conf.json -const PIPE_NAME: &str = "dev.firezone.client"; - pub fn crash() -> Result<()> { // `_` doesn't seem to work here, the log files end up empty let _handles = crate::client::logging::setup("debug")?; @@ -47,36 +43,6 @@ pub fn network_changes() -> Result<()> { Ok(()) } -pub fn open_deep_link(path: &url::Url) -> Result<()> { - tracing_subscriber::fmt::init(); - - let rt = Runtime::new()?; - rt.block_on(crate::client::deep_link::open(PIPE_NAME, path))?; - Ok(()) -} - -// Copied the named pipe idea from `interprocess` and `tauri-plugin-deep-link`, -// although I believe it's considered best practice on Windows to use named pipes for -// single-instance apps. -pub fn pipe_server() -> Result<()> { - tracing_subscriber::fmt::init(); - - let rt = Runtime::new()?; - rt.block_on(async { - loop { - let server = crate::client::deep_link::Server::new(PIPE_NAME)?; - server.accept().await?; - } - }) -} - -// This is copied almost verbatim from tauri-plugin-deep-link's `register` fn, with an improvement -// that we send the deep link to a subcommand so the URL won't confuse `clap` -pub fn register_deep_link() -> Result<()> { - crate::client::deep_link::register(PIPE_NAME)?; - Ok(()) -} - pub fn wintun(_: Cli) -> Result<()> { tracing_subscriber::fmt::init(); diff --git a/rust/windows-client/src-tauri/src/client/deep_link.rs b/rust/windows-client/src-tauri/src/client/deep_link.rs index 7552ea19e..022aac313 100644 --- a/rust/windows-client/src-tauri/src/client/deep_link.rs +++ b/rust/windows-client/src-tauri/src/client/deep_link.rs @@ -1,7 +1,7 @@ //! A module for registering, catching, and parsing deep links that are sent over to the app's already-running instance //! Based on reading some of the Windows code from , which is licensed "MIT OR Apache-2.0" -use crate::client::auth::Response as AuthResponse; +use crate::client::{auth::Response as AuthResponse, BUNDLE_ID}; use secrecy::SecretString; use std::{ffi::c_void, io, path::Path}; use tokio::{io::AsyncReadExt, io::AsyncWriteExt, net::windows::named_pipe}; @@ -92,7 +92,7 @@ impl Server { /// Construct a server, but don't await client connections yet /// /// Panics if there is no Tokio runtime - pub fn new(id: &str) -> Result { + pub fn new() -> Result { // This isn't air-tight - We recreate the whole server on each loop, // rather than binding 1 socket and accepting many streams like a normal socket API. // I can only assume Tokio is following Windows' underlying API. @@ -125,7 +125,7 @@ impl Server { bInheritHandle: false.into(), }; - let path = named_pipe_path(id); + let path = named_pipe_path(BUNDLE_ID); let server = unsafe { server_options .create_with_security_attributes_raw(path, &mut sa as *mut _ as *mut c_void) @@ -169,8 +169,8 @@ impl Server { } /// Open a deep link by sending it to the already-running instance of the app -pub async fn open(id: &str, url: &url::Url) -> Result<(), Error> { - let path = named_pipe_path(id); +pub async fn open(url: &url::Url) -> Result<(), Error> { + let path = named_pipe_path(BUNDLE_ID); let mut client = named_pipe::ClientOptions::new() .open(path) .map_err(Error::Connect)?; @@ -187,14 +187,14 @@ pub async fn open(id: &str, url: &url::Url) -> Result<(), Error> { /// that we send the deep link to a subcommand so the URL won't confuse `clap` /// /// * `id` A unique ID for the app, e.g. "com.contoso.todo-list" or "dev.firezone.client" -pub fn register(id: &str) -> Result<(), Error> { +pub fn register() -> Result<(), Error> { let exe = tauri_utils::platform::current_exe() .map_err(Error::CurrentExe)? .display() .to_string() .replace("\\\\?\\", ""); - set_registry_values(id, &exe).map_err(Error::WindowsRegistry)?; + set_registry_values(BUNDLE_ID, &exe).map_err(Error::WindowsRegistry)?; Ok(()) } @@ -219,6 +219,11 @@ fn set_registry_values(id: &str, exe: &str) -> Result<(), io::Error> { Ok(()) } +/// Returns a valid name for a Windows named pipe +/// +/// # Arguments +/// +/// * `id` - BUNDLE_ID, e.g. `dev.firezone.client` fn named_pipe_path(id: &str) -> String { format!(r"\\.\pipe\{}", id) } diff --git a/rust/windows-client/src-tauri/src/client/gui.rs b/rust/windows-client/src-tauri/src/client/gui.rs index da4fdcb47..2140a9f9b 100644 --- a/rust/windows-client/src-tauri/src/client/gui.rs +++ b/rust/windows-client/src-tauri/src/client/gui.rs @@ -3,7 +3,7 @@ // TODO: `git grep` for unwraps before 1.0, especially this gui module -use crate::client::{self, deep_link, AppLocalDataDir}; +use crate::client::{self, deep_link, AppLocalDataDir, BUNDLE_ID}; use anyhow::{anyhow, bail, Context, Result}; use arc_swap::ArcSwap; use client::{ @@ -54,17 +54,6 @@ impl Managed { pub async fn fault_msleep(&self, _millis: u64) {} } -/// Bundle ID / App ID that we use to distinguish ourself from other programs on the system -/// -/// e.g. In ProgramData and AppData we use this to name our subdirectories for configs and data, -/// and Windows may use it to track things like the MSI installer, notification titles, -/// deep link registration, etc. -/// -/// This should be identical to the `tauri.bundle.identifier` over in `tauri.conf.json`, -/// but sometimes I need to use this before Tauri has booted up, or in a place where -/// getting the Tauri app handle would be awkward. -pub const BUNDLE_ID: &str = "dev.firezone.client"; - /// Runs the Tauri GUI and returns on exit or unrecoverable error pub(crate) fn run(params: client::GuiParams) -> Result<()> { let client::GuiParams { @@ -98,18 +87,17 @@ pub(crate) fn run(params: client::GuiParams) -> Result<()> { }); } - // Make sure we're single-instance - // We register our deep links to call the `open-deep-link` subcommand, - // so if we're at this point, we know we've been launched manually - let server = deep_link::Server::new(BUNDLE_ID)?; - - // We know now we're the only instance on the computer, so register our exe - // to handle deep links - deep_link::register(BUNDLE_ID)?; - let (ctlr_tx, ctlr_rx) = mpsc::channel(5); let notify_controller = Arc::new(Notify::new()); + // Make sure we're single-instance + // We register our deep links to call the `open-deep-link` subcommand, + // so if we're at this point, we know we've been launched manually + let server = deep_link::Server::new()?; + + // We know now we're the only instance on the computer, so register our exe + // to handle deep links + deep_link::register()?; tokio::spawn(accept_deep_links(server, ctlr_tx.clone())); let managed = Managed { @@ -229,7 +217,7 @@ async fn accept_deep_links(mut server: deep_link::Server, ctlr_tx: CtlrTx) -> Re .ok(); } // We re-create the named pipe server every time we get a link, because of an oddity in the Windows API. - server = deep_link::Server::new(BUNDLE_ID)?; + server = deep_link::Server::new()?; } }