From 7825710a69846ac14d01409e30886a198a0e10ac Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Fri, 23 Feb 2024 11:31:35 -0600 Subject: [PATCH] refactor(GUI clients): extract known_dirs module (#3734) The CI tests aren't running for Linux just yet. This organizes the well-known directories used on Linux and Windows for logs, config, etc., and adds them to the (unused) Linux smoke test Waiting on #3727 --------- Signed-off-by: Reactor Scram --- rust/gui-client/src-tauri/src/client.rs | 1 + rust/gui-client/src-tauri/src/client/auth.rs | 14 +- .../src-tauri/src/client/crash_handling.rs | 13 +- .../src-tauri/src/client/device_id.rs | 12 +- rust/gui-client/src-tauri/src/client/gui.rs | 9 +- .../src-tauri/src/client/known_dirs.rs | 120 ++++++++++++++++++ .../src-tauri/src/client/logging.rs | 16 +-- .../src-tauri/src/client/settings.rs | 30 ++--- scripts/tests/smoke-test-gui-linux.sh | 12 +- 9 files changed, 170 insertions(+), 57 deletions(-) create mode 100644 rust/gui-client/src-tauri/src/client/known_dirs.rs diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index a0b49e29c..d695658f3 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -10,6 +10,7 @@ mod deep_link; mod device_id; mod elevation; mod gui; +mod known_dirs; mod logging; mod network_changes; mod resolvers; diff --git a/rust/gui-client/src-tauri/src/client/auth.rs b/rust/gui-client/src-tauri/src/client/auth.rs index 23144e031..b09f4e90b 100644 --- a/rust/gui-client/src-tauri/src/client/auth.rs +++ b/rust/gui-client/src-tauri/src/client/auth.rs @@ -1,6 +1,7 @@ //! Fulfills -use connlib_shared::{control::SecureUrl, windows::app_local_data_dir}; +use crate::client::known_dirs; +use connlib_shared::control::SecureUrl; use rand::{thread_rng, RngCore}; use secrecy::{ExposeSecret, Secret, SecretString}; use std::path::PathBuf; @@ -25,8 +26,8 @@ const NONCE_LENGTH: usize = 32; pub(crate) enum Error { #[error("`actor_name_path` has no parent, this should be impossible")] ActorNamePathWrong, - #[error("`app_local_data_dir` failed")] - CantFindLocalAppDataFolder, + #[error("`known_dirs` failed")] + CantFindKnownDir, #[error("`create_dir_all` failed while writing `actor_name_path`")] CreateDirAll(std::io::Error), #[error("Couldn't delete actor_name from disk: {0}")] @@ -233,11 +234,8 @@ impl Auth { /// /// Hopefully we don't need to save anything else, or there will be a migration step fn actor_name_path() -> Result { - // Would it be better to break out another error type for `app_local_data_dir` here? - // It can only return one kind of error, unrelated to the other errors connlib can return - Ok(app_local_data_dir() - .map_err(|_| Error::CantFindLocalAppDataFolder)? - .join("data") + Ok(known_dirs::session() + .ok_or(Error::CantFindKnownDir)? .join("actor_name.txt")) } diff --git a/rust/gui-client/src-tauri/src/client/crash_handling.rs b/rust/gui-client/src-tauri/src/client/crash_handling.rs index 139dcbc5c..8de13bfe3 100755 --- a/rust/gui-client/src-tauri/src/client/crash_handling.rs +++ b/rust/gui-client/src-tauri/src/client/crash_handling.rs @@ -13,8 +13,8 @@ //! - Compile `minidump-stackwalk` with PR 891 merged //! - `minidump-stackwalker --symbols-path firezone.syms crash.dmp` +use crate::client::known_dirs; use anyhow::{anyhow, bail, Context, Result}; -use connlib_shared::windows::app_local_data_dir; use crash_handler::CrashHandler; use std::{fs::File, io::Write, path::PathBuf}; @@ -66,9 +66,8 @@ fn start_server_and_connect() -> Result<(minidumper::Client, std::process::Child let exe = std::env::current_exe().context("unable to find our own exe path")?; // Path of a Unix domain socket for IPC with the crash handler server // - let socket_path = app_local_data_dir() - .context("couldn't compute crash handler socket path")? - .join("data") + let socket_path = known_dirs::runtime() + .context("`known_dirs::runtime` failed")? .join("crash_handler_pipe"); let mut server = None; @@ -115,10 +114,8 @@ impl minidumper::ServerHandler for Handler { /// Called when a crash has been received and a backing file needs to be /// created to store it. fn create_minidump_file(&self) -> Result<(File, PathBuf), std::io::Error> { - let dump_path = app_local_data_dir() - .expect("app_local_data_dir() failed") - .join("data") - .join("logs") + let dump_path = known_dirs::logs() + .expect("Should be able to find logs dir to put crash dump in") .join("last_crash.dmp"); if let Some(dir) = dump_path.parent() { diff --git a/rust/gui-client/src-tauri/src/client/device_id.rs b/rust/gui-client/src-tauri/src/client/device_id.rs index 1c17f6906..dd7d10607 100644 --- a/rust/gui-client/src-tauri/src/client/device_id.rs +++ b/rust/gui-client/src-tauri/src/client/device_id.rs @@ -1,4 +1,3 @@ -use known_folders::{get_known_folder_path, KnownFolder}; use tokio::fs; #[derive(thiserror::Error, Debug)] @@ -16,18 +15,11 @@ pub(crate) enum Error { /// Per and , /// clients must generate their own random IDs and persist them to disk, to handle situations like VMs where a hardware ID is not unique or not available. /// -/// # Arguments -/// -/// * `identifier` - Our Tauri bundle identifier, e.g. "dev.firezone.client" -/// /// Returns: The UUID as a String, suitable for sending verbatim to `connlib_client_shared::Session::connect`. /// /// Errors: If the disk is unwritable when initially generating the ID, or unwritable when re-generating an invalid ID. -pub(crate) async fn device_id(identifier: &str) -> Result { - let dir = get_known_folder_path(KnownFolder::ProgramData) - .ok_or(Error::KnownFolder)? - .join(identifier) - .join("config"); +pub(crate) async fn device_id() -> Result { + let dir = crate::client::known_dirs::device_id().ok_or(Error::KnownFolder)?; let path = dir.join("device_id.json"); // Try to read it back from disk diff --git a/rust/gui-client/src-tauri/src/client/gui.rs b/rust/gui-client/src-tauri/src/client/gui.rs index 7236f514b..0930d40d6 100644 --- a/rust/gui-client/src-tauri/src/client/gui.rs +++ b/rust/gui-client/src-tauri/src/client/gui.rs @@ -4,7 +4,7 @@ // TODO: `git grep` for unwraps before 1.0, especially this gui module use crate::client::{ - self, about, deep_link, logging, network_changes, + self, about, deep_link, known_dirs, logging, network_changes, settings::{self, AdvancedSettings}, Failure, }; @@ -301,9 +301,8 @@ async fn smoke_test(ctlr_tx: CtlrTx) -> Result<()> { settings::apply_advanced_settings_inner(&settings::AdvancedSettings::default()).await?; // Test log exporting - let path = connlib_shared::windows::app_local_data_dir() - .context("`app_local_data_dir` failed")? - .join("data") + let path = known_dirs::session() + .context("`known_dirs::session` failed during smoke test")? .join("smoke_test_log_export.zip"); let stem = "connlib-smoke-test".into(); match tokio::fs::remove_file(&path).await { @@ -742,7 +741,7 @@ async fn run_controller( advanced_settings: AdvancedSettings, notify_controller: Arc, ) -> Result<()> { - let device_id = client::device_id::device_id(&app.config().tauri.bundle.identifier) + let device_id = client::device_id::device_id() .await .context("Failed to read / create device ID")?; diff --git a/rust/gui-client/src-tauri/src/client/known_dirs.rs b/rust/gui-client/src-tauri/src/client/known_dirs.rs new file mode 100644 index 000000000..a3bdb054b --- /dev/null +++ b/rust/gui-client/src-tauri/src/client/known_dirs.rs @@ -0,0 +1,120 @@ +//! An abstraction over well-known dirs like AppData/Local on Windows and $HOME/.config on Linux +//! +//! On Linux it uses `dirs` which is a convenience wrapper for getting XDG environment vars +//! +//! On Windows it uses `known_folders` which calls into Windows for forwards-compatibility +//! We can't use `dirs` on Windows because we need to match connlib for when it opens wintun.dll. +//! +//! I wanted the ProgramData folder on Windows, which `dirs` alone doesn't provide. + +pub(crate) use imp::{device_id, logs, runtime, session, settings}; + +#[cfg(target_os = "linux")] +mod imp { + use connlib_shared::BUNDLE_ID; + use std::path::PathBuf; + + /// e.g. `/home/alice/.config/dev.firezone.client/config` + /// + /// Device ID is stored here until lands + /// + /// Linux has no direct equivalent to Window's `ProgramData` dir, `/var` doesn't seem + /// to be writable by normal users. + pub(crate) fn device_id() -> Option { + Some(dirs::config_local_dir()?.join(BUNDLE_ID).join("config")) + } + + /// e.g. `/home/alice/.cache/dev.firezone.client/data/logs` + /// + /// Logs are considered cache because they're not configs and it's technically okay + /// if the system / user deletes them to free up space + pub(crate) fn logs() -> Option { + Some(dirs::cache_dir()?.join(BUNDLE_ID).join("data").join("logs")) + } + + /// e.g. `/run/user/1000/dev.firezone.client/data` + /// + /// Crash handler socket and other temp files go here + pub(crate) fn runtime() -> Option { + Some(dirs::runtime_dir()?.join(BUNDLE_ID).join("data")) + } + + /// e.g. `/home/alice/.local/share/dev.firezone.client/data` + /// + /// Things like actor name are stored here because they're kind of config, + /// the system / user should not delete them to free up space, but they're not + /// really config since the program will rewrite them automatically to persist sessions. + pub(crate) fn session() -> Option { + Some(dirs::data_local_dir()?.join(BUNDLE_ID).join("data")) + } + + /// e.g. `/home/alice/.config/dev.firezone.client/config` + /// + /// See connlib docs for details + pub(crate) fn settings() -> Option { + Some(dirs::config_local_dir()?.join(BUNDLE_ID).join("config")) + } +} + +#[cfg(target_os = "windows")] +mod imp { + use connlib_shared::BUNDLE_ID; + use known_folders::{get_known_folder_path, KnownFolder}; + use std::path::PathBuf; + + /// e.g. `C:\ProgramData\dev.firezone.client\config` + /// + /// Device ID is stored here until lands + pub(crate) fn device_id() -> Option { + Some( + get_known_folder_path(KnownFolder::ProgramData)? + .join(BUNDLE_ID) + .join("config"), + ) + } + + /// e.g. `C:\Users\Alice\AppData\Local\dev.firezone.client\data\logs` + /// + /// See connlib docs for details + pub(crate) fn logs() -> Option { + Some( + connlib_shared::windows::app_local_data_dir() + .ok()? + .join("data") + .join("logs"), + ) + } + + /// e.g. `C:\Users\Alice\AppData\Local\dev.firezone.client\data` + /// + /// Crash handler socket and other temp files go here + pub(crate) fn runtime() -> Option { + Some( + connlib_shared::windows::app_local_data_dir() + .ok()? + .join("data"), + ) + } + + /// e.g. `C:\Users\Alice\AppData\Local\dev.firezone.client\data` + /// + /// Things like actor name go here + pub(crate) fn session() -> Option { + Some( + connlib_shared::windows::app_local_data_dir() + .ok()? + .join("data"), + ) + } + + /// e.g. `C:\Users\Alice\AppData\Local\dev.firezone.client\config` + /// + /// See connlib docs for details + pub(crate) fn settings() -> Option { + Some( + connlib_shared::windows::app_local_data_dir() + .ok()? + .join("config"), + ) + } +} diff --git a/rust/gui-client/src-tauri/src/client/logging.rs b/rust/gui-client/src-tauri/src/client/logging.rs index 6992ed514..5ca686e04 100644 --- a/rust/gui-client/src-tauri/src/client/logging.rs +++ b/rust/gui-client/src-tauri/src/client/logging.rs @@ -1,9 +1,11 @@ //! Everything for logging to files, zipping up the files for export, and counting the files -use crate::client::gui::{ControllerRequest, CtlrTx, Managed}; +use crate::client::{ + gui::{ControllerRequest, CtlrTx, Managed}, + known_dirs, +}; use anyhow::{bail, Context, Result}; use connlib_client_shared::file_logger; -use connlib_shared::windows::app_local_data_dir; use serde::Serialize; use std::{fs, io, path::PathBuf, result::Result as StdResult, str::FromStr}; use tokio::task::spawn_blocking; @@ -179,13 +181,7 @@ pub(crate) async fn count_logs_inner() -> Result { Ok(file_count) } -/// Returns the well-known log path -/// -/// e.g. %LOCALAPPDATA%/dev.firezone.client/data/logs/ +/// Wrapper around `known_dirs::logs` pub(crate) fn log_path() -> Result { - let path = app_local_data_dir() - .map_err(|_| Error::CantFindLocalAppDataFolder)? - .join("data") - .join("logs"); - Ok(path) + known_dirs::logs().ok_or(Error::CantFindLocalAppDataFolder) } diff --git a/rust/gui-client/src-tauri/src/client/settings.rs b/rust/gui-client/src-tauri/src/client/settings.rs index be1341010..1909bbd4a 100644 --- a/rust/gui-client/src-tauri/src/client/settings.rs +++ b/rust/gui-client/src-tauri/src/client/settings.rs @@ -1,9 +1,11 @@ //! Everything related to the Settings window, including //! advanced settings and code for manipulating diagnostic logs. -use crate::client::gui::{ControllerRequest, Managed}; -use anyhow::Result; -use connlib_shared::windows::app_local_data_dir; +use crate::client::{ + gui::{ControllerRequest, Managed}, + known_dirs, +}; +use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; use std::{path::PathBuf, time::Duration}; use tokio::sync::oneshot; @@ -38,15 +40,10 @@ impl Default for AdvancedSettings { } } -struct DirAndPath { - dir: PathBuf, - path: PathBuf, -} - -fn advanced_settings_path() -> Result { - let dir = app_local_data_dir()?.join("config"); - let path = dir.join("advanced_settings.json"); - Ok(DirAndPath { dir, path }) +fn advanced_settings_path() -> Result { + Ok(known_dirs::settings() + .context("`known_dirs::settings` failed")? + .join("advanced_settings.json")) } #[tauri::command] @@ -93,8 +90,11 @@ pub(crate) async fn get_advanced_settings( } pub(crate) async fn apply_advanced_settings_inner(settings: &AdvancedSettings) -> Result<()> { - let DirAndPath { dir, path } = advanced_settings_path()?; - tokio::fs::create_dir_all(&dir).await?; + let path = advanced_settings_path()?; + let dir = path + .parent() + .context("settings path should have a parent")?; + tokio::fs::create_dir_all(dir).await?; tokio::fs::write(path, serde_json::to_string(&settings)?).await?; Ok(()) } @@ -103,7 +103,7 @@ pub(crate) async fn apply_advanced_settings_inner(settings: &AdvancedSettings) - /// /// Uses std::fs, so stick it in `spawn_blocking` for async contexts pub(crate) fn load_advanced_settings() -> Result { - let DirAndPath { dir: _, path } = advanced_settings_path()?; + let path = advanced_settings_path()?; let text = std::fs::read_to_string(path)?; let settings = serde_json::from_str(&text)?; Ok(settings) diff --git a/scripts/tests/smoke-test-gui-linux.sh b/scripts/tests/smoke-test-gui-linux.sh index c94d90b26..c520368fa 100755 --- a/scripts/tests/smoke-test-gui-linux.sh +++ b/scripts/tests/smoke-test-gui-linux.sh @@ -3,15 +3,25 @@ set -euo pipefail BUNDLE_ID="dev.firezone.client" -DUMP_PATH="$BUNDLE_ID/data/logs/last_crash.dmp" +DUMP_PATH="$HOME/.cache/$BUNDLE_ID/data/logs/last_crash.dmp" export FIREZONE_DISABLE_SYSTRAY=true PACKAGE=firezone-gui-client export RUST_LOG=firezone_gui_client=debug,warn export WEBKIT_DISABLE_COMPOSITING_MODE=1 +# Make sure the files we want to check don't exist on the system yet +stat "$HOME/.cache/$BUNDLE_ID" && exit 1 +stat "$HOME/.config/$BUNDLE_ID" && exit 1 + # Run the smoke test normally xvfb-run --auto-servernum cargo run -p "$PACKAGE" -- smoke-test +# Make sure the files were written in the right paths +# TODO: Inject some bogus sign-in sequence to test the actor_name file +stat "$HOME/.config/$BUNDLE_ID/config/advanced_settings.json" +stat "$HOME/.cache/$BUNDLE_ID/data/logs/"connlib*log +stat "$HOME/.config/$BUNDLE_ID/config/device_id.json" + # Delete the crash file if present rm -f "$DUMP_PATH"