From 9837a089297a68d0f4eef49ec2571f4408cfb144 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp <19+brynary@users.noreply.github.com> Date: Thu, 21 May 2026 08:04:04 -0400 Subject: [PATCH] refactor(install): share persistence pipeline (#332) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Unifies installer persistence so CLI and web install paths share the same file/env/vault primitives, while preserving the CLI's server-API secret persistence and auth bootstrap behavior. ## What Changed - Added shared `fabro-install` config writers for installer-owned tagged enum tables, replacing `server.listen` and `cli.target` atomically so stale variant fields cannot survive. - Added `InstallPersistencePlan` for disk-backed settings, server env, and vault writes/removals with the existing rollback semantics for settings and vault failures. - Refactored `fabro install`, `fabro install github`, and `/install/finish` to use the shared persistence plan where their disk behavior overlaps. - Preserved full-install ordering: settings/env first, workflow-visible secrets through the server API second, and CLI `auth.json` only after API secret persistence succeeds. - Preserved web installer failure response fields for leftover and removed env keys, plus the post-success finish hook/shutdown behavior. ## Test Plan - `cargo nextest run -p fabro-install` - `cargo nextest run -p fabro-cli commands::install::tests` - `cargo nextest run -p fabro-cli --test it cmd::install` - `cargo nextest run -p fabro-server --features test-support --test it api::install` - `cargo +nightly-2026-04-14 fmt --check --all` - `git diff --check` --- [![Compound Engineering](https://img.shields.io/badge/Compound_Engineering-6366f1)](https://github.com/EveryInc/compound-engineering-plugin) 🤖 Generated with GPT-5 via [Codex](https://openai.com/codex) --- Cargo.lock | 1 + lib/crates/fabro-cli/src/commands/install.rs | 381 ++++++++--- lib/crates/fabro-cli/src/server_client.rs | 30 +- lib/crates/fabro-cli/tests/it/cmd/install.rs | 482 ++++++++++++- lib/crates/fabro-install/Cargo.toml | 1 + lib/crates/fabro-install/src/lib.rs | 639 ++++++++++++++++-- lib/crates/fabro-server/src/install.rs | 37 +- .../fabro-server/tests/it/api/install.rs | 52 +- lib/crates/fabro-util/src/dev_token.rs | 21 +- 9 files changed, 1405 insertions(+), 239 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 694945f00..84669e8da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2002,6 +2002,7 @@ dependencies = [ "fabro-config", "fabro-static", "fabro-types", + "fabro-util", "fabro-vault", "ring", "tempfile", diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 5782a93f0..c3442f96f 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -26,10 +26,12 @@ use fabro_client::{AuthEntry, AuthStore, DevTokenEntry, ServerTarget}; use fabro_config::bind::Bind; use fabro_config::daemon::ServerDaemon; use fabro_config::user::{SETTINGS_CONFIG_FILENAME, default_storage_dir}; -use fabro_config::{Storage, envfile}; +use fabro_config::{Storage, UserSettingsBuilder, envfile}; use fabro_install::{ - InstallListenConfig, PendingSettingsWrite, merge_server_settings as merge_server_settings_impl, - persist_install_outputs_direct, write_github_app_settings, write_token_settings, + InstallListenConfig, InstallPersistencePlan, PendingDevTokenWrite, PendingSettingsWrite, + VaultSecretWrite, merge_server_settings as merge_server_settings_impl, + prepare_dev_token_write_for_install, restore_optional_file, rollback_dev_token_write, + write_github_app_settings, write_token_settings, }; use fabro_model::catalog::CatalogProvider; use fabro_model::{Catalog, CredentialRef, ProviderId}; @@ -42,7 +44,7 @@ use fabro_util::printer::Printer; use fabro_util::terminal::Styles; use fabro_util::version::FABRO_VERSION; use fabro_util::{browser, dev_token, path, session_secret}; -use fabro_vault::{SecretType as VaultSecretType, Vault}; +use fabro_vault::SecretType as VaultSecretType; use futures::future::BoxFuture; use rand::Rng; use tokio::net::TcpListener; @@ -65,12 +67,12 @@ use crate::shared::provider_auth::{ ApiKeySource, authenticate_provider, authenticate_provider_with_api_key_source, authenticate_provider_with_method, prompt_confirm, prompt_password, provider_display_name, }; -use crate::{local_server, server_client}; +use crate::{local_server, server_client, user_config}; -const GITHUB_TOKEN_SECRET_KEY: &str = "GITHUB_TOKEN"; -const GITHUB_APP_PRIVATE_KEY_KEY: &str = "GITHUB_APP_PRIVATE_KEY"; -const GITHUB_APP_CLIENT_SECRET_KEY: &str = "GITHUB_APP_CLIENT_SECRET"; -const GITHUB_APP_WEBHOOK_SECRET_KEY: &str = "GITHUB_APP_WEBHOOK_SECRET"; +const GITHUB_TOKEN_SECRET_KEY: &str = fabro_static::EnvVars::GITHUB_TOKEN; +const GITHUB_APP_PRIVATE_KEY_KEY: &str = fabro_static::EnvVars::GITHUB_APP_PRIVATE_KEY; +const GITHUB_APP_CLIENT_SECRET_KEY: &str = fabro_static::EnvVars::GITHUB_APP_CLIENT_SECRET; +const GITHUB_APP_WEBHOOK_SECRET_KEY: &str = fabro_static::EnvVars::GITHUB_APP_WEBHOOK_SECRET; static INSTALL_CATALOG: LazyLock = LazyLock::new(|| { Catalog::from_builtin().expect("embedded install model catalog should be valid") @@ -1261,15 +1263,32 @@ async fn persist_install_outputs( server_env_secrets: &[(String, String)], vault_secrets: &[CreateSecretRequest], settings_write: Option>, + dev_token_write: Option, server_was_running: bool, + bootstrap_dev_token: Option<&str>, ) -> Result<()> { - persist_install_outputs_with_settings( + let bootstrap_dev_token = if server_was_running { + None + } else { + bootstrap_dev_token.map(str::to_string) + }; + persist_cli_install_outputs_with( storage_dir, - server_env_secrets, + server_env_updates(server_env_secrets), + Vec::new(), vault_secrets, settings_write, + dev_token_write, server_was_running, - |path| Box::pin(server_client::connect_server(path)), + move |path| { + let bootstrap_dev_token = bootstrap_dev_token.clone(); + Box::pin(async move { + match bootstrap_dev_token { + Some(token) => server_client::connect_server_with_dev_token(path, &token).await, + None => server_client::connect_server(path).await, + } + }) + }, |path, timeout| { Box::pin(async move { stop::stop_server(path, timeout).await.unwrap_or(false) }) }, @@ -1285,76 +1304,48 @@ struct PendingGitHubInstallWrite<'a> { vault_remove: Vec<&'static str>, } -fn restore_optional_file(path: &Path, previous_contents: Option<&str>) -> Result<()> { - match previous_contents { - Some(contents) => { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("creating directory {}", parent.display()))?; - } - std::fs::write(path, contents) - .with_context(|| format!("restoring {}", path.display()))?; - } - None => match std::fs::remove_file(path) { - Ok(()) => {} - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => { - return Err(anyhow::Error::new(err).context(format!("removing {}", path.display()))); - } - }, - } - - Ok(()) -} - fn persist_github_install_changes( storage_dir: &Path, writes: &PendingGitHubInstallWrite<'_>, ) -> Result<()> { - let storage = Storage::new(storage_dir); - let server_env_path = storage.runtime_directory().env_path(); - let vault_path = storage.secrets_path(); + let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); let previous_server_env = std::fs::read_to_string(&server_env_path).ok(); - let previous_vault = std::fs::read_to_string(&vault_path).ok(); - let result = (|| -> Result<()> { - let server_env_writes = server_env_updates(&writes.server_env_set); - let server_env_removals = server_env_removals(&writes.server_env_remove); - persist_install_outputs_direct( - storage_dir, - &server_env_writes, - &server_env_removals, - &[], - Some(&writes.settings_write), - )?; - - let mut vault = Vault::load(vault_path.clone()).map_err(anyhow::Error::from)?; - for key in &writes.vault_remove { - match vault.remove(key) { - Ok(()) | Err(fabro_vault::Error::NotFound(_)) => {} - Err(err) => return Err(err.into()), + match (InstallPersistencePlan { + storage_dir, + settings_write: Some(writes.settings_write), + server_env_writes: server_env_updates(&writes.server_env_set), + server_env_removals: server_env_removals(&writes.server_env_remove), + dev_token_write: None, + vault_writes: writes + .vault_set + .iter() + .map(|(key, value)| VaultSecretWrite { + name: key.clone(), + value: value.clone(), + secret_type: VaultSecretType::Token, + description: None, + }) + .collect(), + vault_removals: writes + .vault_remove + .iter() + .map(|key| (*key).to_string()) + .collect(), + } + .persist_direct()) + { + Ok(()) => Ok(()), + Err(err) => { + let err = anyhow::Error::from(err); + match restore_optional_file(&server_env_path, previous_server_env.as_deref()) { + Ok(()) => Err(err), + Err(restore_err) => { + Err(err.context(format!("server env rollback failure: {restore_err}"))) + } } } - for (key, value) in &writes.vault_set { - vault - .set(key, value, VaultSecretType::Token, None) - .map_err(anyhow::Error::from)?; - } - - Ok(()) - })(); - - if let Err(err) = result { - restore_optional_file( - writes.settings_write.path, - writes.settings_write.previous_contents, - )?; - restore_optional_file(&server_env_path, previous_server_env.as_deref())?; - restore_optional_file(&vault_path, previous_vault.as_deref())?; - return Err(err); } - - Ok(()) } async fn write_artifact_store_metadata( @@ -1367,25 +1358,30 @@ async fn write_artifact_store_metadata( Ok(()) } -async fn persist_install_outputs_with_settings( +async fn persist_cli_install_outputs_with( storage_dir: &Path, - server_env_secrets: &[(String, String)], + server_env_writes: Vec, + server_env_removals: Vec, vault_secrets: &[CreateSecretRequest], settings_write: Option>, + dev_token_write: Option, server_was_running: bool, connect_server: impl for<'a> Fn(&'a Path) -> BoxFuture<'a, Result>, stop_server: impl for<'a> Fn(&'a Path, Duration) -> BoxFuture<'a, bool>, ) -> Result<()> { let server_env_path = Storage::new(storage_dir).runtime_directory().env_path(); let previous_server_env = std::fs::read_to_string(&server_env_path).ok(); - let settings_write_ref = settings_write.as_ref(); - persist_install_outputs_direct( + let dev_token_write_for_rollback = dev_token_write.clone(); + InstallPersistencePlan { storage_dir, - &server_env_updates(server_env_secrets), - &[], - &[], - settings_write_ref, - )?; + settings_write, + server_env_writes, + server_env_removals, + dev_token_write, + vault_writes: Vec::new(), + vault_removals: Vec::new(), + } + .persist_direct()?; let persist_result = persist_vault_secrets_with( storage_dir, @@ -1397,17 +1393,29 @@ async fn persist_install_outputs_with_settings( .await; if let Err(err) = persist_result { - restore_optional_file(&server_env_path, previous_server_env.as_deref())?; + let mut rollback_failures = Vec::new(); + if let Err(restore_err) = + restore_optional_file(&server_env_path, previous_server_env.as_deref()) + { + rollback_failures.push(restore_err.to_string()); + } if let Some(write) = settings_write { - match write.previous_contents { - Some(previous) => std::fs::write(write.path, previous) - .with_context(|| format!("restoring settings file {}", write.path.display()))?, - None if write.path.exists() => std::fs::remove_file(write.path) - .with_context(|| format!("removing settings file {}", write.path.display()))?, - None => {} + if let Err(restore_err) = restore_optional_file(write.path, write.previous_contents) { + rollback_failures.push(restore_err.to_string()); } } - return Err(err); + if let Some(write) = dev_token_write_for_rollback.as_ref() { + if let Err(restore_err) = rollback_dev_token_write(write) { + rollback_failures.push(restore_err.to_string()); + } + } + if rollback_failures.is_empty() { + return Err(err); + } + return Err(err.context(format!( + "rollback failures: {}", + rollback_failures.join("; ") + ))); } Ok(()) @@ -1870,6 +1878,7 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( // Secrets and auth material let mut dev_token_for_auth_store = None; + let mut dev_token_write = None; { let session_secret = session_secret::generate_session_secret(); fabro_util::printerr!( @@ -1887,7 +1896,9 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( let dev_token_path = Storage::new(&storage_dir) .runtime_directory() .dev_token_path(); - let token = dev_token::read_or_mint_dev_token_for_install(&dev_token_path)?; + let prepared = prepare_dev_token_write_for_install(&dev_token_path)?; + let token = prepared.token; + dev_token_write = prepared.write; dev_token_for_auth_store = Some(token.clone()); fabro_util::printerr!( printer, @@ -1899,9 +1910,13 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( None }; - let mut generated_server_env_pairs = vec![("SESSION_SECRET".to_string(), session_secret)]; + let mut generated_server_env_pairs = vec![( + fabro_static::EnvVars::SESSION_SECRET.to_string(), + session_secret, + )]; if let Some(token) = dev_token { - generated_server_env_pairs.push(("FABRO_DEV_TOKEN".to_string(), token)); + generated_server_env_pairs + .push((fabro_static::EnvVars::FABRO_DEV_TOKEN.to_string(), token)); } server_env_pairs.extend(generated_server_env_pairs); } @@ -1915,11 +1930,20 @@ async fn run_install_inner(args: &InstallArgs, ctx: &CommandContext) -> Result<( contents: settings_toml.as_str(), previous_contents: existing_config_contents.as_deref(), }), + dev_token_write, server_was_running, + dev_token_for_auth_store.as_deref(), ) .await?; if let Some(token) = dev_token_for_auth_store { - let target = ServerTarget::http_url(&web_url)?; + let user_settings = UserSettingsBuilder::from_toml(&settings_toml)?; + let target = match user_config::resolve_nondefault_server_target( + &ServerTargetArgs::default(), + &user_settings, + )? { + Some(target) => target, + None => ServerTarget::http_url(&web_url)?, + }; if let Err(err) = AuthStore::default().put( &target, AuthEntry::DevToken(DevTokenEntry { @@ -2048,6 +2072,7 @@ mod tests { use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; + use fabro_vault::Vault; use httpmock::Method::POST; use httpmock::MockServer; @@ -2829,7 +2854,7 @@ client_id = "client-id" } #[tokio::test] - async fn persist_install_outputs_with_settings_rolls_back_new_files_on_secret_failure() { + async fn persist_cli_install_outputs_rolls_back_new_files_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; let vault_secrets = [CreateSecretRequest { @@ -2841,15 +2866,17 @@ client_id = "client-id" let settings_path = dir.path().join(SETTINGS_CONFIG_FILENAME); let stop_called = Arc::new(AtomicBool::new(false)); - let result = persist_install_outputs_with_settings( + let result = persist_cli_install_outputs_with( dir.path(), - &server_env_pairs, + server_env_updates(&server_env_pairs), + Vec::new(), &vault_secrets, Some(PendingSettingsWrite { path: &settings_path, contents: "_version = 1\n", previous_contents: None, }), + None, false, |_| Box::pin(async move { Err(anyhow::anyhow!("boom")) }), { @@ -2877,7 +2904,99 @@ client_id = "client-id" } #[tokio::test] - async fn persist_install_outputs_with_settings_restores_previous_contents_on_secret_failure() { + async fn persist_cli_install_outputs_rolls_back_staged_dev_token_on_secret_failure() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let dev_token_path = storage.runtime_directory().dev_token_path(); + let prepared = fabro_install::prepare_dev_token_write_for_install(&dev_token_path).unwrap(); + let server_env_pairs = [ + ("SESSION_SECRET".to_string(), "session".to_string()), + ("FABRO_DEV_TOKEN".to_string(), prepared.token.clone()), + ]; + let vault_secrets = [CreateSecretRequest { + name: "GITHUB_CLI_TOKEN".to_string(), + value: "gh-token".to_string(), + type_: ApiSecretType::Token, + description: None, + }]; + let settings_path = dir.path().join(SETTINGS_CONFIG_FILENAME); + + let result = persist_cli_install_outputs_with( + dir.path(), + server_env_updates(&server_env_pairs), + Vec::new(), + &vault_secrets, + Some(PendingSettingsWrite { + path: &settings_path, + contents: "_version = 1\n", + previous_contents: None, + }), + prepared.write, + false, + |_| Box::pin(async move { Err(anyhow::anyhow!("boom")) }), + |_, _| Box::pin(async move { true }), + ) + .await; + + assert!(result.is_err()); + assert!(!settings_path.exists()); + assert!(!storage.runtime_directory().env_path().exists()); + assert!( + !dev_token_path.exists(), + "failed install should not leave a staged dev token" + ); + } + + #[tokio::test] + async fn persist_cli_install_outputs_preserves_existing_dev_token_on_secret_failure() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let dev_token_path = storage.runtime_directory().dev_token_path(); + let token = fabro_util::dev_token::generate_dev_token(); + fabro_util::dev_token::write_dev_token(&dev_token_path, &token).unwrap(); + let prepared = fabro_install::prepare_dev_token_write_for_install(&dev_token_path).unwrap(); + assert_eq!(prepared.token, token); + assert!(prepared.write.is_none()); + let server_env_pairs = [ + ("SESSION_SECRET".to_string(), "session".to_string()), + ("FABRO_DEV_TOKEN".to_string(), token.clone()), + ]; + let vault_secrets = [CreateSecretRequest { + name: "GITHUB_CLI_TOKEN".to_string(), + value: "gh-token".to_string(), + type_: ApiSecretType::Token, + description: None, + }]; + let settings_path = dir.path().join(SETTINGS_CONFIG_FILENAME); + + let result = persist_cli_install_outputs_with( + dir.path(), + server_env_updates(&server_env_pairs), + Vec::new(), + &vault_secrets, + Some(PendingSettingsWrite { + path: &settings_path, + contents: "_version = 1\n", + previous_contents: None, + }), + prepared.write, + false, + |_| Box::pin(async move { Err(anyhow::anyhow!("boom")) }), + |_, _| Box::pin(async move { true }), + ) + .await; + + assert!(result.is_err()); + assert_eq!( + fabro_util::dev_token::read_dev_token_file(&dev_token_path).as_deref(), + Some(token.as_str()) + ); + assert!(!settings_path.exists()); + assert!(!storage.runtime_directory().env_path().exists()); + } + + #[tokio::test] + async fn persist_cli_install_outputs_restores_previous_contents_on_secret_failure() { let dir = tempfile::tempdir().unwrap(); let server_env_pairs = [("SESSION_SECRET".to_string(), "session".to_string())]; let vault_secrets = [CreateSecretRequest { @@ -2889,15 +3008,17 @@ client_id = "client-id" let settings_path = dir.path().join(SETTINGS_CONFIG_FILENAME); std::fs::write(&settings_path, "_version = 1\n[server]\n").unwrap(); - let result = persist_install_outputs_with_settings( + let result = persist_cli_install_outputs_with( dir.path(), - &server_env_pairs, + server_env_updates(&server_env_pairs), + Vec::new(), &vault_secrets, Some(PendingSettingsWrite { path: &settings_path, contents: "_version = 1\n[server]\nfoo = \"bar\"\n", previous_contents: Some("_version = 1\n[server]\n"), }), + None, false, |_| Box::pin(async move { Err(anyhow::anyhow!("boom")) }), |_, _| Box::pin(async move { true }), @@ -3044,6 +3165,66 @@ client_id = "client-id" assert_eq!(std::fs::read_to_string(&settings_path).unwrap(), "after"); } + #[test] + fn persist_github_install_changes_restores_server_env_on_vault_failure() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let server_env_path = storage.runtime_directory().env_path(); + envfile::write_env_file( + &server_env_path, + &std::collections::HashMap::from([ + ( + GITHUB_APP_PRIVATE_KEY_KEY.to_string(), + "private".to_string(), + ), + ( + GITHUB_APP_CLIENT_SECRET_KEY.to_string(), + "client".to_string(), + ), + ("KEEP_ME".to_string(), "1".to_string()), + ]), + ) + .unwrap(); + + let settings_path = dir.path().join(SETTINGS_CONFIG_FILENAME); + std::fs::write(&settings_path, "before").unwrap(); + + let result = persist_github_install_changes(dir.path(), &PendingGitHubInstallWrite { + settings_write: PendingSettingsWrite { + path: &settings_path, + contents: "after", + previous_contents: Some("before"), + }, + server_env_set: Vec::new(), + server_env_remove: vec![GITHUB_APP_PRIVATE_KEY_KEY, GITHUB_APP_CLIENT_SECRET_KEY], + vault_set: vec![("bad-secret-name".to_string(), "token".to_string())], + vault_remove: Vec::new(), + }); + + assert!(result.is_err()); + let server_env = envfile::read_env_file(&server_env_path).unwrap(); + assert_eq!( + server_env + .get(GITHUB_APP_PRIVATE_KEY_KEY) + .map(String::as_str), + Some("private") + ); + assert_eq!( + server_env + .get(GITHUB_APP_CLIENT_SECRET_KEY) + .map(String::as_str), + Some("client") + ); + assert_eq!(server_env.get("KEEP_ME").map(String::as_str), Some("1")); + assert_eq!(std::fs::read_to_string(&settings_path).unwrap(), "before"); + assert_eq!( + Vault::load(storage.secrets_path()) + .unwrap() + .get("bad-secret-name"), + None + ); + } + #[tokio::test] async fn write_artifact_store_metadata_creates_marker_in_resolved_store() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-cli/src/server_client.rs b/lib/crates/fabro-cli/src/server_client.rs index 952957a68..22cca5647 100644 --- a/lib/crates/fabro-cli/src/server_client.rs +++ b/lib/crates/fabro-cli/src/server_client.rs @@ -36,6 +36,18 @@ pub(crate) async fn connect_server(storage_dir: &Path) -> Result { connect_local_api_client_bundle(storage_dir, &user_config::active_settings_path(None)).await } +pub(crate) async fn connect_server_with_dev_token( + storage_dir: &Path, + dev_token: &str, +) -> Result { + connect_local_api_client_bundle_with_dev_token( + storage_dir, + &user_config::active_settings_path(None), + Some(dev_token), + ) + .await +} + pub(crate) async fn connect_server_target(target: &ServerTarget) -> Result { connect_target_api_client_bundle(target).await } @@ -121,6 +133,14 @@ async fn connect_managed_unix_socket_api_client_bundle( async fn connect_local_api_client_bundle( storage_dir: &Path, active_config_path: &Path, +) -> Result { + connect_local_api_client_bundle_with_dev_token(storage_dir, active_config_path, None).await +} + +async fn connect_local_api_client_bundle_with_dev_token( + storage_dir: &Path, + active_config_path: &Path, + bootstrap_dev_token: Option<&str>, ) -> Result { let bind = start::ensure_server_running_for_storage(storage_dir, active_config_path) .await @@ -130,7 +150,10 @@ async fn connect_local_api_client_bundle( let runtime_token_path = Storage::new(storage_dir) .runtime_directory() .dev_token_path(); - let token = wait_for_runtime_dev_token(&runtime_token_path).await?; + let token = match bootstrap_dev_token { + Some(token) => token.to_string(), + None => wait_for_runtime_dev_token(&runtime_token_path).await?, + }; let http_client = connect_unix_socket_http_client(&path, Some(&token)).await?; Ok(Client::builder() .transport("http://fabro", http_client) @@ -140,7 +163,10 @@ async fn connect_local_api_client_bundle( } Bind::Tcp(addr) => { let target = ServerTarget::http_url(format!("http://{addr}"))?; - let credential = resolve_target_credential(&target)?; + let credential = match bootstrap_dev_token { + Some(token) => Some(Credential::DevToken(token.to_string())), + None => resolve_target_credential(&target)?, + }; let oauth_session = refreshable_oauth(&target, credential.as_ref()); build_client(target, credential, oauth_session, None).await } diff --git a/lib/crates/fabro-cli/tests/it/cmd/install.rs b/lib/crates/fabro-cli/tests/it/cmd/install.rs index 5a3ed6fdb..3d8ec88c1 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/install.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/install.rs @@ -1,12 +1,18 @@ #![expect( clippy::disallowed_methods, - reason = "integration tests stage fixtures and subprocess env with sync test infrastructure" + clippy::disallowed_types, + reason = "integration tests stage fixtures, reserve ports, and subprocess env with sync test infrastructure" )] +use std::net::TcpListener; +use std::time::Duration; + use fabro_config::{Storage, envfile}; use fabro_test::{EnvVars, fabro_snapshot, test_context}; use fabro_vault::{SecretType, Vault}; +const INSTALL_COMMAND_TIMEOUT: Duration = Duration::from_secs(30); + #[test] fn help() { let context = test_context!(); @@ -252,6 +258,312 @@ fn skip_llm_conflicts_with_llm_credential_flags() { ); } +#[test] +fn non_interactive_token_install_bootstraps_server_auth_for_secret_persistence() { + let mut context = test_context!(); + std::fs::remove_file(context.home_dir.join(".fabro/settings.toml")).unwrap(); + let storage_dir = context.temp_dir.join("install-storage"); + context.manage_storage_dir(&storage_dir); + + let path = fake_gh_path(&context, "ghp_install_bootstrap"); + let web_url = unused_loopback_web_url(); + + let output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .env(EnvVars::PATH, &path) + .args([ + "install", + "--storage-dir", + storage_dir.to_str().unwrap(), + "--web-url", + &web_url, + "--non-interactive", + "--skip-llm", + "--github-strategy", + "token", + "--github-username", + "octocat", + "--overwrite-settings", + ]) + .output() + .expect("install command should run"); + + assert!( + output.status.success(), + "install should bootstrap auth before persisting secrets\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !stderr.contains("Authentication required"), + "install should not require a separate auth login while bootstrapping: {stderr}" + ); + + let list_output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .args(["--json", "secret", "list"]) + .output() + .expect("secret list command should run"); + + assert!( + list_output.status.success(), + "CLI auth saved by install should authenticate follow-up secret commands\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&list_output.stdout), + String::from_utf8_lossy(&list_output.stderr) + ); + let secrets: serde_json::Value = + serde_json::from_slice(&list_output.stdout).expect("secret list JSON should parse"); + assert!( + secrets + .as_array() + .expect("secret list should return an array") + .iter() + .any(|secret| secret["name"] == "GITHUB_TOKEN"), + "installed GitHub token should be persisted as a server-owned secret: {secrets}" + ); +} + +#[test] +fn keep_existing_settings_persists_secrets_without_rewriting_server_target() { + let mut context = test_context!(); + let storage_dir = context.temp_dir.join("install-storage"); + context.manage_storage_dir(&storage_dir); + let existing_web_url = unused_loopback_web_url(); + let requested_web_url = unused_loopback_web_url(); + write_http_install_settings(&context, &storage_dir, &existing_web_url, "keep-me"); + login_with_storage_dev_token(&context, &storage_dir, &existing_web_url); + + let path = fake_gh_path(&context, "ghp_keep_existing"); + let output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .env(EnvVars::PATH, path) + .args([ + "install", + "--storage-dir", + storage_dir.to_str().unwrap(), + "--web-url", + &requested_web_url, + "--non-interactive", + "--skip-llm", + "--github-strategy", + "token", + "--keep-existing-settings", + ]) + .output() + .expect("install command should run"); + + assert!( + output.status.success(), + "install should keep existing server settings while persisting secrets\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let settings = read_home_settings(&context); + let parsed: toml::Value = toml::from_str(&settings).unwrap(); + assert_eq!( + parsed + .get("server") + .and_then(toml::Value::as_table) + .and_then(|server| server.get("web")) + .and_then(toml::Value::as_table) + .and_then(|web| web.get("url")) + .and_then(toml::Value::as_str), + Some(existing_web_url.as_str()) + ); + assert_eq!( + parsed + .get("cli") + .and_then(toml::Value::as_table) + .and_then(|cli| cli.get("target")) + .and_then(toml::Value::as_table) + .and_then(|target| target.get("url")) + .and_then(toml::Value::as_str), + Some(existing_web_url.as_str()) + ); + assert!( + !settings.contains(&requested_web_url), + "--keep-existing-settings should not rewrite settings to the requested web URL" + ); + assert_secret_list_contains(&context, &["GITHUB_TOKEN"]); +} + +#[test] +fn install_against_running_authenticated_server_persists_secrets_and_leaves_server_running() { + let mut context = test_context!(); + let storage_dir = context.temp_dir.join("install-storage"); + context.manage_storage_dir(&storage_dir); + let web_url = unused_loopback_web_url(); + write_http_install_settings(&context, &storage_dir, &web_url, "running-server"); + login_with_storage_dev_token(&context, &storage_dir, &web_url); + + let start_output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .args([ + "server", + "start", + "--storage-dir", + storage_dir.to_str().unwrap(), + ]) + .output() + .expect("server start command should run"); + assert!( + start_output.status.success(), + "server start should succeed before install\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&start_output.stdout), + String::from_utf8_lossy(&start_output.stderr) + ); + + let path = fake_gh_path(&context, "ghp_running_server"); + let output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .env(EnvVars::PATH, path) + .args([ + "install", + "--storage-dir", + storage_dir.to_str().unwrap(), + "--web-url", + &web_url, + "--non-interactive", + "--skip-llm", + "--github-strategy", + "token", + "--github-username", + "octocat", + "--overwrite-settings", + ]) + .output() + .expect("install command should run"); + + assert!( + output.status.success(), + "install should persist secrets through the already-running server\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + + let status_output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .args([ + "server", + "status", + "--json", + "--storage-dir", + storage_dir.to_str().unwrap(), + ]) + .output() + .expect("server status command should run"); + assert!( + status_output.status.success(), + "server should still be running after install\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&status_output.stdout), + String::from_utf8_lossy(&status_output.stderr) + ); + let status: serde_json::Value = + serde_json::from_slice(&status_output.stdout).expect("server status JSON should parse"); + assert_eq!(status["status"].as_str(), Some("running")); + assert_secret_list_contains(&context, &["GITHUB_TOKEN"]); +} + +#[test] +fn install_json_non_interactive_success_emits_complete_event() { + let mut context = test_context!(); + std::fs::remove_file(context.home_dir.join(".fabro/settings.toml")).unwrap(); + let storage_dir = context.temp_dir.join("install-storage"); + context.manage_storage_dir(&storage_dir); + let web_url = unused_loopback_web_url(); + login_with_storage_dev_token(&context, &storage_dir, &web_url); + + let path = fake_gh_path(&context, "ghp_json_success"); + let output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .env(EnvVars::PATH, path) + .args([ + "--json", + "install", + "--storage-dir", + storage_dir.to_str().unwrap(), + "--web-url", + &web_url, + "--non-interactive", + "--skip-llm", + "--github-strategy", + "token", + "--github-username", + "octocat", + "--overwrite-settings", + ]) + .output() + .expect("install command should run"); + + assert!( + output.status.success(), + "JSON install should succeed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + let stdout = String::from_utf8(output.stdout).unwrap(); + let events = stdout + .lines() + .map(|line| serde_json::from_str::(line).unwrap()) + .collect::>(); + assert_eq!(events, vec![serde_json::json!({ + "event": "install_complete", + "status": "success" + })]); +} + +#[fabro_macros::e2e_test(live("ANTHROPIC_API_KEY"))] +fn install_with_anthropic_api_key_persists_llm_and_github_secrets() { + let mut context = test_context!(); + std::fs::remove_file(context.home_dir.join(".fabro/settings.toml")).unwrap(); + let storage_dir = context.temp_dir.join("install-storage"); + context.manage_storage_dir(&storage_dir); + let web_url = unused_loopback_web_url(); + login_with_storage_dev_token(&context, &storage_dir, &web_url); + + let path = fake_gh_path(&context, "ghp_anthropic"); + let output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .env(EnvVars::PATH, path) + .args([ + "install", + "--storage-dir", + storage_dir.to_str().unwrap(), + "--web-url", + &web_url, + "--non-interactive", + "--llm-provider", + "anthropic", + "--llm-api-key-env", + "ANTHROPIC_API_KEY", + "--github-strategy", + "token", + "--github-username", + "octocat", + "--overwrite-settings", + ]) + .output() + .expect("install command should run"); + + assert!( + output.status.success(), + "install should validate and persist the scripted LLM API key\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_secret_list_contains(&context, &["GITHUB_TOKEN", "ANTHROPIC_API_KEY"]); +} + #[test] fn github_requires_prior_install() { let context = test_context!(); @@ -350,26 +662,7 @@ mode = "keep-me" ) .unwrap(); - let fake_bin = context.temp_dir.join("fake-bin"); - std::fs::create_dir_all(&fake_bin).unwrap(); - let fake_gh = fake_bin.join("gh"); - std::fs::write( - &fake_gh, - "#!/bin/sh\nif [ \"$1\" = \"auth\" ] && [ \"$2\" = \"token\" ]; then\n printf 'token-from-gh\\n'\n exit 0\nfi\nexit 1\n", - ) - .unwrap(); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - - std::fs::set_permissions(&fake_gh, std::fs::Permissions::from_mode(0o755)).unwrap(); - } - - let path = format!( - "{}:{}", - fake_bin.display(), - std::env::var(EnvVars::PATH).unwrap() - ); + let path = fake_gh_path(&context, "token-from-gh"); let output = context .command() .env(EnvVars::PATH, path) @@ -454,3 +747,150 @@ mode = "keep-me" Some(SecretType::Token) ); } + +fn unused_loopback_web_url() -> String { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind unused loopback port"); + let port = listener.local_addr().expect("read loopback addr").port(); + drop(listener); + format!("http://127.0.0.1:{port}") +} + +fn fake_gh_path(context: &fabro_test::TestContext, token: &str) -> String { + let fake_bin = context.temp_dir.join(format!("fake-bin-{token}")); + std::fs::create_dir_all(&fake_bin).expect("fake gh bin directory should be created"); + let fake_gh = fake_bin.join("gh"); + std::fs::write( + &fake_gh, + format!("#!/bin/sh\nif [ \"$1\" = \"auth\" ] && [ \"$2\" = \"token\" ]; then\n printf '{token}\\n'\n exit 0\nfi\nexit 1\n"), + ) + .expect("fake gh script should be written"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + + std::fs::set_permissions(&fake_gh, std::fs::Permissions::from_mode(0o755)) + .expect("fake gh script should be executable"); + } + + format!( + "{}:{}", + fake_bin.display(), + std::env::var(EnvVars::PATH).expect("PATH should be set for install tests") + ) +} + +fn write_raw_home_settings(context: &fabro_test::TestContext, settings: &str) { + let settings_path = context.home_dir.join(".fabro/settings.toml"); + std::fs::create_dir_all( + settings_path + .parent() + .expect("settings path should have a parent directory"), + ) + .expect("settings directory should be created"); + std::fs::write(settings_path, settings).expect("settings file should be written"); +} + +fn read_home_settings(context: &fabro_test::TestContext) -> String { + std::fs::read_to_string(context.home_dir.join(".fabro/settings.toml")) + .expect("settings file should be readable") +} + +fn write_http_install_settings( + context: &fabro_test::TestContext, + storage_dir: &std::path::Path, + web_url: &str, + metadata_mode: &str, +) { + let address = web_url + .strip_prefix("http://") + .expect("test web URL should be an http URL"); + write_raw_home_settings( + context, + &format!( + r#" +_version = 1 + +[server.storage] +root = "{}" + +[server.api] +url = "{}/api/v1" + +[server.web] +enabled = true +url = "{}" + +[server.auth] +methods = ["dev-token"] + +[server.listen] +type = "tcp" +address = "{}" + +[cli.target] +type = "http" +url = "{}" + +[project.metadata] +mode = "{}" +"#, + storage_dir.display(), + web_url, + web_url, + address, + web_url, + metadata_mode + ), + ); +} + +fn login_with_storage_dev_token( + context: &fabro_test::TestContext, + storage_dir: &std::path::Path, + web_url: &str, +) { + let token = fabro_util::dev_token::read_dev_token_file( + &Storage::new(storage_dir) + .runtime_directory() + .dev_token_path(), + ) + .expect("storage dev token should be valid"); + let output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .args(["auth", "login", "--server", web_url, "--dev-token", &token]) + .output() + .expect("auth login command should run"); + assert!( + output.status.success(), + "auth login should seed CLI auth for {web_url}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +fn assert_secret_list_contains(context: &fabro_test::TestContext, expected_names: &[&str]) { + let list_output = context + .command() + .timeout(INSTALL_COMMAND_TIMEOUT) + .args(["--json", "secret", "list"]) + .output() + .expect("secret list command should run"); + assert!( + list_output.status.success(), + "secret list should succeed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&list_output.stdout), + String::from_utf8_lossy(&list_output.stderr) + ); + let secrets: serde_json::Value = + serde_json::from_slice(&list_output.stdout).expect("secret list JSON should parse"); + let array = secrets + .as_array() + .expect("secret list should return an array"); + for expected_name in expected_names { + assert!( + array.iter().any(|secret| secret["name"] == *expected_name), + "secret list should include {expected_name}: {secrets}" + ); + } +} diff --git a/lib/crates/fabro-install/Cargo.toml b/lib/crates/fabro-install/Cargo.toml index 025a87fb4..608e5ef6e 100644 --- a/lib/crates/fabro-install/Cargo.toml +++ b/lib/crates/fabro-install/Cargo.toml @@ -17,6 +17,7 @@ toml.workspace = true fabro-config = { path = "../fabro-config" } fabro-static.workspace = true fabro-types = { path = "../fabro-types" } +fabro-util = { path = "../fabro-util" } fabro-vault = { path = "../fabro-vault" } [dev-dependencies] diff --git a/lib/crates/fabro-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index 4e10bde87..2a625ff76 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -8,14 +8,28 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; use fabro_config::{Storage, envfile}; use fabro_static::EnvVars; +use fabro_util::dev_token; use fabro_vault::{SecretType as VaultSecretType, Vault}; +#[derive(Debug, Clone, Copy)] pub struct PendingSettingsWrite<'a> { pub path: &'a Path, pub contents: &'a str, pub previous_contents: Option<&'a str>, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PendingDevTokenWrite { + path: PathBuf, + token: String, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct PreparedInstallDevToken { + pub token: String, + pub write: Option, +} + pub const OBJECT_STORE_MANAGED_COMMENT: &str = "managed by fabro-install: object-store"; pub const OBJECT_STORE_ACCESS_KEY_ID_ENV: &str = EnvVars::AWS_ACCESS_KEY_ID; pub const OBJECT_STORE_SECRET_ACCESS_KEY_ENV: &str = EnvVars::AWS_SECRET_ACCESS_KEY; @@ -28,6 +42,16 @@ pub struct VaultSecretWrite { pub description: Option, } +pub struct InstallPersistencePlan<'a> { + pub storage_dir: &'a Path, + pub settings_write: Option>, + pub server_env_writes: Vec, + pub server_env_removals: Vec, + pub dev_token_write: Option, + pub vault_writes: Vec, + pub vault_removals: Vec, +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum InstallListenConfig { Tcp(String), @@ -99,6 +123,21 @@ pub fn default_web_url() -> String { "http://127.0.0.1:32276".to_string() } +pub fn prepare_dev_token_write_for_install(path: &Path) -> Result { + if let Some(token) = dev_token::read_dev_token_for_install(path)? { + return Ok(PreparedInstallDevToken { token, write: None }); + } + + let token = dev_token::generate_dev_token(); + Ok(PreparedInstallDevToken { + token: token.clone(), + write: Some(PendingDevTokenWrite { + path: path.to_path_buf(), + token, + }), + }) +} + fn root_table_mut(doc: &mut toml::Value) -> Result<&mut toml::Table> { doc.as_table_mut() .context("settings.toml root is not a table") @@ -136,28 +175,14 @@ fn github_integration_table(doc: &mut toml::Value) -> Result<&mut toml::Table> { .context("settings.toml [server.integrations.github] is not a table") } -pub fn merge_server_settings( - doc: &mut toml::Value, - web_url: &str, - listen_config: &InstallListenConfig, -) -> Result<()> { +fn set_server_listen(doc: &mut toml::Value, listen_config: &InstallListenConfig) -> Result<()> { let root = root_table_mut(doc)?; - root.insert("_version".to_string(), toml::Value::Integer(1)); - let server = ensure_table(root, "server")?; - - let api = ensure_table(server, "api")?; - api.insert( - "url".to_string(), - toml::Value::String(format!("{web_url}/api/v1")), - ); - - let listen = ensure_table(server, "listen")?; + let mut listen = toml::Table::default(); match listen_config { InstallListenConfig::Tcp(address) => { listen.insert("type".to_string(), toml::Value::String("tcp".to_string())); listen.insert("address".to_string(), toml::Value::String(address.clone())); - listen.remove("path"); } InstallListenConfig::Unix(path) => { listen.insert("type".to_string(), toml::Value::String("unix".to_string())); @@ -165,9 +190,44 @@ pub fn merge_server_settings( "path".to_string(), toml::Value::String(path.display().to_string()), ); - listen.remove("address"); } } + server.insert("listen".to_string(), toml::Value::Table(listen)); + Ok(()) +} + +fn set_cli_target_http(doc: &mut toml::Value, web_url: &str) -> Result<()> { + let root = root_table_mut(doc)?; + let cli = ensure_table(root, "cli")?; + let mut target = toml::Table::default(); + target.insert("type".to_string(), toml::Value::String("http".to_string())); + target.insert("url".to_string(), toml::Value::String(web_url.to_string())); + cli.insert("target".to_string(), toml::Value::Table(target)); + Ok(()) +} + +pub fn merge_server_settings( + doc: &mut toml::Value, + web_url: &str, + listen_config: &InstallListenConfig, +) -> Result<()> { + { + let root = root_table_mut(doc)?; + root.insert("_version".to_string(), toml::Value::Integer(1)); + + let server = ensure_table(root, "server")?; + + let api = ensure_table(server, "api")?; + api.insert( + "url".to_string(), + toml::Value::String(format!("{web_url}/api/v1")), + ); + } + + set_server_listen(doc, listen_config)?; + + let root = root_table_mut(doc)?; + let server = ensure_table(root, "server")?; let web = ensure_table(server, "web")?; web.insert("enabled".to_string(), toml::Value::Boolean(true)); @@ -179,10 +239,7 @@ pub fn merge_server_settings( toml::Value::Array(vec![toml::Value::String("dev-token".to_string())]), ); - let cli = ensure_table(root, "cli")?; - let target = ensure_table(cli, "target")?; - target.insert("type".to_string(), toml::Value::String("http".to_string())); - target.insert("url".to_string(), toml::Value::String(web_url.to_string())); + set_cli_target_http(doc, web_url)?; Ok(()) } @@ -403,7 +460,7 @@ pub fn write_sandbox_settings( Ok(()) } -fn restore_optional_file(path: &Path, previous_contents: Option<&str>) -> Result<()> { +pub fn restore_optional_file(path: &Path, previous_contents: Option<&str>) -> Result<()> { match previous_contents { Some(contents) => { if let Some(parent) = path.parent() { @@ -425,6 +482,24 @@ fn restore_optional_file(path: &Path, previous_contents: Option<&str>) -> Result Ok(()) } +pub fn rollback_dev_token_write(write: &PendingDevTokenWrite) -> Result<()> { + match std::fs::remove_file(&write.path) { + Ok(()) => {} + Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} + Err(err) => { + return Err(anyhow::Error::new(err) + .context(format!("removing dev token {}", write.path.display()))); + } + } + + Ok(()) +} + +fn write_pending_dev_token(write: &PendingDevTokenWrite) -> Result<()> { + dev_token::write_dev_token(&write.path, &write.token) + .with_context(|| format!("writing dev token {}", write.path.display())) +} + fn persist_server_env_secrets( storage_dir: &Path, writes: &[envfile::EnvFileUpdate], @@ -446,13 +521,23 @@ fn persist_server_env_secrets( .with_context(|| format!("updating server env file {}", env_path.display())) } -fn persist_vault_secrets_direct(storage_dir: &Path, secrets: &[VaultSecretWrite]) -> Result<()> { - if secrets.is_empty() { +fn persist_vault_secrets_direct( + storage_dir: &Path, + secrets: &[VaultSecretWrite], + removals: &[String], +) -> Result<()> { + if secrets.is_empty() && removals.is_empty() { return Ok(()); } let vault_path = Storage::new(storage_dir).secrets_path(); let mut vault = Vault::load(vault_path).map_err(anyhow::Error::from)?; + for name in removals { + match vault.remove(name) { + Ok(()) | Err(fabro_vault::Error::NotFound(_)) => {} + Err(err) => return Err(err.into()), + } + } for secret in secrets { vault .set( @@ -466,6 +551,108 @@ fn persist_vault_secrets_direct(storage_dir: &Path, secrets: &[VaultSecretWrite] Ok(()) } +fn direct_persistence_error(err: anyhow::Error, rollback_failures: &[String]) -> anyhow::Error { + if rollback_failures.is_empty() { + err.context("persisting install outputs directly") + } else { + err.context(format!( + "persisting install outputs directly; rollback failures: {}", + rollback_failures.join("; ") + )) + } +} + +fn rollback_direct_persistence( + settings_write: Option<&PendingSettingsWrite<'_>>, + vault_path: &Path, + previous_vault: Option<&str>, + dev_token_write: Option<&PendingDevTokenWrite>, +) -> Vec { + let mut failures = Vec::new(); + + if let Some(write) = settings_write { + if let Err(err) = restore_optional_file(write.path, write.previous_contents) { + failures.push(err.to_string()); + } + } + if let Err(err) = restore_optional_file(vault_path, previous_vault) { + failures.push(err.to_string()); + } + if let Some(write) = dev_token_write { + if let Err(err) = rollback_dev_token_write(write) { + failures.push(err.to_string()); + } + } + + failures +} + +impl InstallPersistencePlan<'_> { + pub fn persist_direct(&self) -> std::result::Result<(), PersistInstallOutputsError> { + let server_env_report = persist_server_env_secrets( + self.storage_dir, + &self.server_env_writes, + &self.server_env_removals, + ) + .map_err(|err| PersistInstallOutputsError::new(err, false, Vec::new()))?; + let removed_env_keys = server_env_report.removed_keys; + + if let Some(write) = self.settings_write.as_ref() { + if let Some(parent) = write.path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("creating settings directory {}", parent.display())) + .map_err(|err| { + PersistInstallOutputsError::new(err, true, removed_env_keys.clone()) + })?; + } + std::fs::write(write.path, write.contents) + .with_context(|| format!("writing settings file {}", write.path.display())) + .map_err(|err| { + PersistInstallOutputsError::new(err, true, removed_env_keys.clone()) + })?; + } + + let vault_path = Storage::new(self.storage_dir).secrets_path(); + let previous_vault = std::fs::read_to_string(&vault_path).ok(); + + if let Err(err) = + persist_vault_secrets_direct(self.storage_dir, &self.vault_writes, &self.vault_removals) + { + let rollback_failures = rollback_direct_persistence( + self.settings_write.as_ref(), + &vault_path, + previous_vault.as_deref(), + self.dev_token_write.as_ref(), + ); + let error = direct_persistence_error(err, &rollback_failures); + return Err(PersistInstallOutputsError::new( + error, + true, + removed_env_keys, + )); + } + + if let Some(write) = self.dev_token_write.as_ref() { + if let Err(err) = write_pending_dev_token(write) { + let rollback_failures = rollback_direct_persistence( + self.settings_write.as_ref(), + &vault_path, + previous_vault.as_deref(), + Some(write), + ); + let error = direct_persistence_error(err, &rollback_failures); + return Err(PersistInstallOutputsError::new( + error, + true, + removed_env_keys, + )); + } + } + + Ok(()) + } +} + pub fn persist_install_outputs_direct( storage_dir: &Path, server_env_writes: &[envfile::EnvFileUpdate], @@ -473,65 +660,35 @@ pub fn persist_install_outputs_direct( vault_secrets: &[VaultSecretWrite], settings_write: Option<&PendingSettingsWrite<'_>>, ) -> std::result::Result<(), PersistInstallOutputsError> { - let server_env_report = - persist_server_env_secrets(storage_dir, server_env_writes, server_env_removals) - .map_err(|err| PersistInstallOutputsError::new(err, false, Vec::new()))?; - let removed_env_keys = server_env_report.removed_keys; - - if let Some(write) = settings_write { - if let Some(parent) = write.path.parent() { - std::fs::create_dir_all(parent) - .with_context(|| format!("creating settings directory {}", parent.display())) - .map_err(|err| { - PersistInstallOutputsError::new(err, true, removed_env_keys.clone()) - })?; - } - std::fs::write(write.path, write.contents) - .with_context(|| format!("writing settings file {}", write.path.display())) - .map_err(|err| PersistInstallOutputsError::new(err, true, removed_env_keys.clone()))?; + InstallPersistencePlan { + storage_dir, + settings_write: settings_write.copied(), + server_env_writes: server_env_writes.to_vec(), + server_env_removals: server_env_removals.to_vec(), + dev_token_write: None, + vault_writes: vault_secrets.to_vec(), + vault_removals: Vec::new(), } - - let vault_path = Storage::new(storage_dir).secrets_path(); - let previous_vault = std::fs::read_to_string(&vault_path).ok(); - - if let Err(err) = persist_vault_secrets_direct(storage_dir, vault_secrets) { - let mut rollback_failures = Vec::new(); - if let Some(write) = settings_write { - if let Err(restore_err) = restore_optional_file(write.path, write.previous_contents) { - rollback_failures.push(restore_err.to_string()); - } - } - if let Err(restore_err) = restore_optional_file(&vault_path, previous_vault.as_deref()) { - rollback_failures.push(restore_err.to_string()); - } - let error = if rollback_failures.is_empty() { - err.context("persisting install outputs directly") - } else { - err.context(format!( - "persisting install outputs directly; rollback failures: {}", - rollback_failures.join("; ") - )) - }; - return Err(PersistInstallOutputsError::new( - error, - true, - removed_env_keys, - )); - } - - Ok(()) + .persist_direct() } #[cfg(test)] mod tests { - use fabro_config::{ServerSettingsBuilder, Storage, envfile}; + use std::path::PathBuf; + + use fabro_config::{ServerSettingsBuilder, Storage, UserSettingsBuilder, envfile}; + use fabro_types::settings::cli::CliTargetSettings; + use fabro_util::dev_token::{ + generate_dev_token, read_dev_token_file, validate_dev_token_format, write_dev_token, + }; use fabro_vault::{SecretType as VaultSecretType, Vault}; use super::{ InstallListenConfig, InstallObjectStoreCredentialMode, InstallObjectStoreSelection, - InstallSandboxSelection, OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_MANAGED_COMMENT, - OBJECT_STORE_SECRET_ACCESS_KEY_ENV, PendingSettingsWrite, VaultSecretWrite, - default_web_url, merge_server_settings, persist_install_outputs_direct, + InstallPersistencePlan, InstallSandboxSelection, OBJECT_STORE_ACCESS_KEY_ID_ENV, + OBJECT_STORE_MANAGED_COMMENT, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, PendingSettingsWrite, + VaultSecretWrite, default_web_url, merge_server_settings, persist_install_outputs_direct, + prepare_dev_token_write_for_install, set_cli_target_http, set_server_listen, write_github_app_settings, write_object_store_settings, write_sandbox_settings, }; @@ -598,6 +755,141 @@ name = "custom" ); } + #[test] + fn merge_server_settings_replaces_stale_unix_cli_target_fields() { + let mut doc: toml::Value = toml::from_str( + r#" +_version = 1 + +[cli.target] +type = "unix" +path = "/tmp/fabro.sock" +"#, + ) + .unwrap(); + + merge_server_settings( + &mut doc, + &default_web_url(), + &InstallListenConfig::Tcp("127.0.0.1:32276".to_string()), + ) + .unwrap(); + + let target = doc + .get("cli") + .and_then(toml::Value::as_table) + .and_then(|cli| cli.get("target")) + .and_then(toml::Value::as_table) + .expect("cli.target should be a table"); + assert_eq!( + target.get("type").and_then(toml::Value::as_str), + Some("http") + ); + assert_eq!( + target.get("url").and_then(toml::Value::as_str), + Some(default_web_url().as_str()) + ); + assert!(!target.contains_key("path")); + + let toml_str = toml::to_string_pretty(&doc).expect("settings should serialize"); + let settings = UserSettingsBuilder::from_toml(&toml_str).expect("settings should resolve"); + assert!(matches!( + settings.cli.target, + Some(CliTargetSettings::Http { .. }) + )); + } + + #[test] + fn set_cli_target_http_replaces_the_full_tagged_enum_table() { + let mut doc: toml::Value = toml::from_str( + r#" +_version = 1 + +[cli.target] +type = "unix" +path = "/tmp/fabro.sock" +stale = "remove-me" +"#, + ) + .unwrap(); + + set_cli_target_http(&mut doc, &default_web_url()).unwrap(); + + let target = doc + .get("cli") + .and_then(toml::Value::as_table) + .and_then(|cli| cli.get("target")) + .and_then(toml::Value::as_table) + .expect("cli.target should be a table"); + assert_eq!(target.len(), 2); + assert_eq!( + target.get("type").and_then(toml::Value::as_str), + Some("http") + ); + assert_eq!( + target.get("url").and_then(toml::Value::as_str), + Some(default_web_url().as_str()) + ); + } + + #[test] + fn set_server_listen_replaces_stale_variant_fields_in_both_directions() { + let mut doc: toml::Value = toml::from_str( + r#" +_version = 1 + +[server.listen] +type = "tcp" +address = "127.0.0.1:32276" +path = "/tmp/stale.sock" +stale = "remove-me" +"#, + ) + .unwrap(); + + set_server_listen( + &mut doc, + &InstallListenConfig::Unix(PathBuf::from("/tmp/fabro.sock")), + ) + .unwrap(); + let listen = doc + .get("server") + .and_then(toml::Value::as_table) + .and_then(|server| server.get("listen")) + .and_then(toml::Value::as_table) + .expect("server.listen should be a table"); + assert_eq!(listen.len(), 2); + assert_eq!( + listen.get("type").and_then(toml::Value::as_str), + Some("unix") + ); + assert_eq!( + listen.get("path").and_then(toml::Value::as_str), + Some("/tmp/fabro.sock") + ); + + set_server_listen( + &mut doc, + &InstallListenConfig::Tcp("0.0.0.0:32276".to_string()), + ) + .unwrap(); + let listen = doc + .get("server") + .and_then(toml::Value::as_table) + .and_then(|server| server.get("listen")) + .and_then(toml::Value::as_table) + .expect("server.listen should be a table"); + assert_eq!(listen.len(), 2); + assert_eq!( + listen.get("type").and_then(toml::Value::as_str), + Some("tcp") + ); + assert_eq!( + listen.get("address").and_then(toml::Value::as_str), + Some("0.0.0.0:32276") + ); + } + #[test] fn write_github_app_settings_uses_server_integrations_github() { let mut doc = toml::Value::Table(toml::Table::default()); @@ -706,6 +998,205 @@ name = "custom" ); } + #[test] + fn install_persistence_plan_direct_writes_and_removes_vault_secrets() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let mut vault = Vault::load(storage.secrets_path()).unwrap(); + vault + .set("REMOVE_ME", "old", VaultSecretType::Token, None) + .unwrap(); + vault + .set("KEEP_ME", "keep", VaultSecretType::Token, None) + .unwrap(); + + InstallPersistencePlan { + storage_dir: dir.path(), + settings_write: None, + server_env_writes: Vec::new(), + server_env_removals: Vec::new(), + dev_token_write: None, + vault_writes: vec![VaultSecretWrite { + name: "NEW_SECRET".to_string(), + value: "new".to_string(), + secret_type: VaultSecretType::Token, + description: None, + }], + vault_removals: vec!["REMOVE_ME".to_string()], + } + .persist_direct() + .unwrap(); + + let vault = Vault::load(storage.secrets_path()).unwrap(); + assert_eq!(vault.get("REMOVE_ME"), None); + assert_eq!(vault.get("KEEP_ME"), Some("keep")); + assert_eq!(vault.get("NEW_SECRET"), Some("new")); + } + + #[test] + fn install_persistence_plan_direct_restores_settings_and_vault_on_secret_failure() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let settings_path = dir.path().join("settings.toml"); + std::fs::write(&settings_path, "_version = 1\n[server]\n").unwrap(); + let vault_path = storage.secrets_path(); + let mut vault = Vault::load(vault_path.clone()).unwrap(); + vault + .set("REMOVE_ME", "old", VaultSecretType::Token, None) + .unwrap(); + + let result = InstallPersistencePlan { + storage_dir: dir.path(), + settings_write: Some(PendingSettingsWrite { + path: &settings_path, + contents: "_version = 1\n[server]\nfoo = \"bar\"\n", + previous_contents: Some("_version = 1\n[server]\n"), + }), + server_env_writes: vec![envfile::EnvFileUpdate { + key: "SESSION_SECRET".to_string(), + value: "session".to_string(), + comment: None, + }], + server_env_removals: Vec::new(), + dev_token_write: None, + vault_writes: vec![VaultSecretWrite { + name: "bad-secret-name".to_string(), + value: "boom".to_string(), + secret_type: VaultSecretType::Token, + description: None, + }], + vault_removals: vec!["REMOVE_ME".to_string()], + } + .persist_direct(); + + assert!(result.is_err()); + assert_eq!( + std::fs::read_to_string(&settings_path).unwrap(), + "_version = 1\n[server]\n" + ); + let vault = Vault::load(vault_path).unwrap(); + assert_eq!(vault.get("REMOVE_ME"), Some("old")); + assert_eq!(vault.get("bad-secret-name"), None); + let server_env = envfile::read_env_file(&storage.runtime_directory().env_path()).unwrap(); + assert_eq!( + server_env.get("SESSION_SECRET").map(String::as_str), + Some("session") + ); + } + + #[test] + fn prepare_dev_token_write_for_install_missing_file_stages_without_writing() { + let dir = tempfile::tempdir().unwrap(); + let path = Storage::new(dir.path()) + .runtime_directory() + .dev_token_path(); + + let prepared = prepare_dev_token_write_for_install(&path).unwrap(); + + assert!(validate_dev_token_format(&prepared.token)); + assert!( + prepared.write.is_some(), + "missing token file should stage a write" + ); + assert!( + !path.exists(), + "preparing a token must not create the token file" + ); + } + + #[test] + fn prepare_dev_token_write_for_install_reuses_existing_file() { + let dir = tempfile::tempdir().unwrap(); + let path = Storage::new(dir.path()) + .runtime_directory() + .dev_token_path(); + let token = generate_dev_token(); + write_dev_token(&path, &token).unwrap(); + + let prepared = prepare_dev_token_write_for_install(&path).unwrap(); + + assert_eq!(prepared.token, token); + assert!( + prepared.write.is_none(), + "existing valid token should not be rewritten" + ); + } + + #[test] + fn prepare_dev_token_write_for_install_rejects_invalid_existing_file() { + let dir = tempfile::tempdir().unwrap(); + let path = Storage::new(dir.path()) + .runtime_directory() + .dev_token_path(); + std::fs::create_dir_all(path.parent().unwrap()).unwrap(); + std::fs::write(&path, "not-a-valid-token").unwrap(); + + let err = prepare_dev_token_write_for_install(&path).unwrap_err(); + + assert!(err.to_string().contains("invalid dev token format")); + } + + #[test] + fn install_persistence_plan_direct_writes_staged_dev_token_on_success() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let path = storage.runtime_directory().dev_token_path(); + let prepared = prepare_dev_token_write_for_install(&path).unwrap(); + let token = prepared.token.clone(); + + InstallPersistencePlan { + storage_dir: dir.path(), + settings_write: None, + server_env_writes: Vec::new(), + server_env_removals: Vec::new(), + dev_token_write: prepared.write, + vault_writes: Vec::new(), + vault_removals: Vec::new(), + } + .persist_direct() + .unwrap(); + + assert_eq!(read_dev_token_file(&path).as_deref(), Some(token.as_str())); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + } + } + + #[test] + fn install_persistence_plan_direct_does_not_leave_staged_dev_token_on_vault_failure() { + let dir = tempfile::tempdir().unwrap(); + let storage = Storage::new(dir.path()); + let path = storage.runtime_directory().dev_token_path(); + let prepared = prepare_dev_token_write_for_install(&path).unwrap(); + + let result = InstallPersistencePlan { + storage_dir: dir.path(), + settings_write: None, + server_env_writes: Vec::new(), + server_env_removals: Vec::new(), + dev_token_write: prepared.write, + vault_writes: vec![VaultSecretWrite { + name: "bad-secret-name".to_string(), + value: "boom".to_string(), + secret_type: VaultSecretType::Token, + description: None, + }], + vault_removals: Vec::new(), + } + .persist_direct(); + + assert!(result.is_err()); + assert!( + !path.exists(), + "failed persistence should not leave a newly staged dev token file" + ); + } + #[test] fn merge_server_settings_keeps_tcp_bind_separate_from_public_web_url() { use fabro_types::settings::server::ServerListenSettings; diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index df6696732..3dd61b937 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -17,10 +17,11 @@ use fabro_config::Storage; use fabro_config::bind::{Bind, BindRequest}; use fabro_config::envfile::EnvFileUpdate; use fabro_install::{ - InstallListenConfig, InstallSandboxSelection, OBJECT_STORE_ACCESS_KEY_ID_ENV, - OBJECT_STORE_SECRET_ACCESS_KEY_ENV, PendingSettingsWrite, VaultSecretWrite, - merge_server_settings, persist_install_outputs_direct, write_github_app_settings, - write_object_store_settings, write_sandbox_settings, write_token_settings, + InstallListenConfig, InstallPersistencePlan, InstallSandboxSelection, + OBJECT_STORE_ACCESS_KEY_ID_ENV, OBJECT_STORE_SECRET_ACCESS_KEY_ENV, PendingSettingsWrite, + VaultSecretWrite, merge_server_settings, prepare_dev_token_write_for_install, + write_github_app_settings, write_object_store_settings, write_sandbox_settings, + write_token_settings, }; use fabro_llm::client::Client as LlmClient; use fabro_llm::generate::{GenerateParams, generate}; @@ -34,7 +35,7 @@ use fabro_types::settings::interp::InterpString; use fabro_types::settings::server::ObjectStoreSettings; use fabro_types::settings::{is_wildcard_host, validate_public_url_with_label}; use fabro_util::version::FABRO_VERSION; -use fabro_util::{Home, dev_token, session_secret}; +use fabro_util::{Home, session_secret}; use fabro_vault::SecretType as VaultSecretType; use object_store::aws::resolve_bucket_region; use object_store::path::Path as ObjectStorePath; @@ -1562,6 +1563,7 @@ async fn post_install_finish( let mut server_env_writes = object_store_env_plan.writes; let server_env_removals = object_store_env_plan.removals; let mut dev_token: Option = None; + let mut dev_token_write = None; match github { GithubInstallState::Token(github) => { if let Err(err) = write_token_settings(&mut settings_doc) { @@ -1576,7 +1578,7 @@ async fn post_install_finish( let dev_token_path = Storage::new(state.storage_dir.as_ref()) .runtime_directory() .dev_token_path(); - let token = match dev_token::read_or_mint_dev_token_for_install(&dev_token_path) { + let prepared = match prepare_dev_token_write_for_install(&dev_token_path) { Ok(value) => value, Err(err) => { return install_error_response( @@ -1585,7 +1587,8 @@ async fn post_install_finish( ); } }; - dev_token = Some(token); + dev_token_write = prepared.write; + dev_token = Some(prepared.token); } GithubInstallState::App(github) => { if let Err(err) = write_github_app_settings( @@ -1631,23 +1634,27 @@ async fn post_install_finish( )] let previous_settings = std::fs::read_to_string(state.config_path.as_ref()).ok(); - if let Err(err) = persist_install_outputs_direct( - state.storage_dir.as_ref(), - &server_env_writes, - &server_env_removals, - &vault_secrets, - Some(&PendingSettingsWrite { + let persistence_plan = InstallPersistencePlan { + storage_dir: state.storage_dir.as_ref(), + settings_write: Some(PendingSettingsWrite { path: state.config_path.as_ref(), contents: &settings_toml, previous_contents: previous_settings.as_deref(), }), - ) { + server_env_writes, + server_env_removals, + dev_token_write, + vault_writes: vault_secrets, + vault_removals: Vec::new(), + }; + if let Err(err) = persistence_plan.persist_direct() { error!(error = %err, "install persistence failed"); let status = StatusCode::INTERNAL_SERVER_ERROR; let detail = err.to_string(); let title = status.canonical_reason().unwrap_or("Unknown").to_string(); let leftover_env_keys: Vec = if err.server_env_applied { - server_env_writes + persistence_plan + .server_env_writes .iter() .map(|write| write.key.clone()) .collect() diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index b2fe734ab..be92a986d 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -11,13 +11,13 @@ use std::time::Duration; use axum::body::Body; use axum::http::{Request, StatusCode}; -use fabro_config::{ServerSettingsBuilder, Storage}; +use fabro_config::{ServerSettingsBuilder, Storage, envfile}; use fabro_install::OBJECT_STORE_MANAGED_COMMENT; use fabro_model::ProviderId; use fabro_server::install::{ InstallAppState, InstallFinishHook, InstallFinishInfo, build_install_router, }; -use fabro_util::{Home, dev_token}; +use fabro_util::Home; use fabro_vault::Vault; use httpmock::Method::GET; use httpmock::MockServer; @@ -929,18 +929,27 @@ async fn token_install_finish_persists_settings_env_and_vault() { "127.0.0.1:32276" ); - let server_env = std::fs::read_to_string( - fabro_config::Storage::new(temp_dir.path()) - .runtime_directory() - .env_path(), - ) - .unwrap(); - assert!(server_env.contains("SESSION_SECRET=")); - assert!(server_env.contains("FABRO_DEV_TOKEN=")); - assert!(!server_env.contains("AWS_ACCESS_KEY_ID=")); - assert!(!server_env.contains("AWS_SECRET_ACCESS_KEY=")); + let storage = fabro_config::Storage::new(temp_dir.path()); + let server_env = envfile::read_env_file(&storage.runtime_directory().env_path()).unwrap(); + assert!(server_env.contains_key(fabro_static::EnvVars::SESSION_SECRET)); + assert!(server_env.contains_key(fabro_static::EnvVars::FABRO_DEV_TOKEN)); + assert!(!server_env.contains_key(fabro_static::EnvVars::AWS_ACCESS_KEY_ID)); + assert!(!server_env.contains_key(fabro_static::EnvVars::AWS_SECRET_ACCESS_KEY)); + let finish_dev_token = finish_body["dev_token"] + .as_str() + .expect("token install should return a dev token"); + let storage_dev_token = + fabro_util::dev_token::read_dev_token_file(&storage.runtime_directory().dev_token_path()) + .expect("token install should write the storage dev token"); + assert_eq!(storage_dev_token, finish_dev_token); + assert_eq!( + server_env + .get(fabro_static::EnvVars::FABRO_DEV_TOKEN) + .map(String::as_str), + Some(finish_dev_token) + ); - let vault = Vault::load(fabro_config::Storage::new(temp_dir.path()).secrets_path()).unwrap(); + let vault = Vault::load(storage.secrets_path()).unwrap(); assert!(vault.get("ANTHROPIC_API_KEY").is_some()); assert_eq!(vault.get("GITHUB_TOKEN"), Some("ghp_test_token")); } @@ -2278,7 +2287,7 @@ async fn install_finish_failure_reports_only_env_keys_actually_removed() { } #[tokio::test] -async fn install_finish_failure_does_not_create_home_dev_token() { +async fn install_finish_failure_does_not_create_dev_token_files() { let temp_dir = tempfile::tempdir().unwrap(); let home_root = tempfile::tempdir().unwrap(); let home = Home::new(home_root.path().join(".fabro")); @@ -2319,12 +2328,17 @@ async fn install_finish_failure_does_not_create_home_dev_token() { !home.root().join("dev-token").exists(), "home dev token file should not be created" ); - let storage_dev_token = - dev_token::read_dev_token_file(&storage.runtime_directory().dev_token_path()) - .expect("storage dev token should exist"); + assert!( + !storage.runtime_directory().dev_token_path().exists(), + "storage dev token file should not be created when persistence fails" + ); - let server_env = std::fs::read_to_string(storage.runtime_directory().env_path()).unwrap(); - assert!(server_env.contains(&format!("FABRO_DEV_TOKEN={storage_dev_token}"))); + let server_env = envfile::read_env_file(&storage.runtime_directory().env_path()).unwrap(); + assert!( + server_env + .get(fabro_static::EnvVars::FABRO_DEV_TOKEN) + .is_some_and(|value| !value.is_empty()) + ); } #[tokio::test] diff --git a/lib/crates/fabro-util/src/dev_token.rs b/lib/crates/fabro-util/src/dev_token.rs index b169d020c..f9486a9a7 100644 --- a/lib/crates/fabro-util/src/dev_token.rs +++ b/lib/crates/fabro-util/src/dev_token.rs @@ -63,20 +63,25 @@ pub fn read_dev_token_or_err(path: &Path) -> Result { } } -pub fn read_or_mint_dev_token_for_install(path: &Path) -> Result { +pub fn read_dev_token_for_install(path: &Path) -> Result> { match fs::read_to_string(path) { Ok(contents) => { let token = contents.trim().to_string(); if validate_dev_token_format(&token) { - return Ok(token); + Ok(Some(token)) + } else { + Err(anyhow!("invalid dev token format in {}", path.display())) } - return Err(anyhow!("invalid dev token format in {}", path.display())); - } - Err(err) if err.kind() == std::io::ErrorKind::NotFound => {} - Err(err) => { - return Err(anyhow::Error::from(err)) - .with_context(|| format!("read dev token {}", path.display())); } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(err) => Err(anyhow::Error::from(err)) + .with_context(|| format!("read dev token {}", path.display())), + } +} + +pub fn read_or_mint_dev_token_for_install(path: &Path) -> Result { + if let Some(token) = read_dev_token_for_install(path)? { + return Ok(token); } let token = generate_dev_token();