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

@@ -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)]

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");
 

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();
 

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 <https://github.com/FabianLars/tauri-plugin-deep-link>, 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<Self, Error> {
+    pub fn new() -> Result<Self, Error> {
         // 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)
 }

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()?;
     }
 }