From c6ff36d9fbb0906899b64c4d60bbdf0e521ccde5 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 19 Apr 2026 13:54:47 -0400 Subject: [PATCH] refactor(install): consolidate shared primitives in fabro-install crate The `fabro-install` crate was introduced for the web wizard but the CLI kept its own copies of the same JWT keypair generation, TOML merging, and GitHub auth settings helpers. Delete the duplicates and route the CLI through `fabro_install::*`. The CLI keeps a thin `merge_server_settings` wrapper because it only ever binds TCP and derives the authority from `--web-url`. Also tighten `persist_install_outputs_direct` to take its `PendingSettingsWrite` argument by reference (satisfies `needless_pass_by_value`) and pull the remaining absolute paths in the crate's test module into `use` statements, clearing the nightly clippy warnings that this branch was carrying. Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 1 + lib/crates/fabro-cli/Cargo.toml | 1 + lib/crates/fabro-cli/src/commands/install.rs | 198 +------------------ lib/crates/fabro-install/src/lib.rs | 26 ++- lib/crates/fabro-server/src/install.rs | 2 +- 5 files changed, 24 insertions(+), 204 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cc7af3041..038a547ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1590,6 +1590,7 @@ dependencies = [ "fabro-graphviz", "fabro-hooks", "fabro-http", + "fabro-install", "fabro-interview", "fabro-llm", "fabro-macros", diff --git a/lib/crates/fabro-cli/Cargo.toml b/lib/crates/fabro-cli/Cargo.toml index eb18dc17f..425708108 100644 --- a/lib/crates/fabro-cli/Cargo.toml +++ b/lib/crates/fabro-cli/Cargo.toml @@ -27,6 +27,7 @@ fabro-github = { path = "../fabro-github" } fabro-agent = { path = "../fabro-agent" } fabro-devcontainer = { path = "../fabro-devcontainer" } fabro-hooks = { path = "../fabro-hooks" } +fabro-install = { path = "../fabro-install" } fabro-interview = { path = "../fabro-interview" } fabro-mcp = { path = "../fabro-mcp" } fabro-proc = { path = "../fabro-proc" } diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index a5615dd58..5a6b44d19 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -18,6 +18,10 @@ use fabro_api::types::{CreateSecretRequest, SecretType as ApiSecretType}; use fabro_auth::{AuthCredential, AuthMethod, codex_oauth_config, credential_id_for}; use fabro_config::user::{SETTINGS_CONFIG_FILENAME, legacy_default_storage_root}; use fabro_config::{ResolveError, Storage, envfile, legacy_env}; +use fabro_install::{ + InstallListenConfig, generate_jwt_keypair, merge_server_settings as merge_server_settings_impl, + write_github_app_settings, write_token_settings, +}; use fabro_model::Provider; use fabro_server::bind::Bind; use fabro_server::serve; @@ -32,8 +36,6 @@ use fabro_util::{dev_token, path, session_secret}; use fabro_vault::{SecretType as VaultSecretType, Vault}; use futures::future::BoxFuture; use rand::Rng; -use ring::rand::SystemRandom; -use ring::signature::{Ed25519KeyPair, KeyPair as _}; use tokio::net::TcpListener; use tokio::process::Command as TokioCommand; use tokio::sync::oneshot; @@ -52,59 +54,11 @@ use crate::shared::provider_auth::{ }; use crate::{server_client, user_config}; -// --------------------------------------------------------------------------- -// JWT keypair generation -// --------------------------------------------------------------------------- - -const ED25519_SPKI_PREFIX: [u8; 12] = [ - 0x30, 0x2A, 0x30, 0x05, 0x06, 0x03, 0x2B, 0x65, 0x70, 0x03, 0x21, 0x00, -]; -const ED25519_PUBLIC_KEY_LEN: usize = 32; 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"; -fn pem_encode(label: &str, bytes: &[u8]) -> String { - let body = BASE64_STANDARD.encode(bytes); - let mut pem = String::new(); - pem.push_str("-----BEGIN "); - pem.push_str(label); - pem.push_str("-----\n"); - for chunk in body.as_bytes().chunks(64) { - pem.push_str(std::str::from_utf8(chunk).expect("base64 output should be valid UTF-8")); - pem.push('\n'); - } - pem.push_str("-----END "); - pem.push_str(label); - pem.push_str("-----\n"); - pem -} - -fn ed25519_public_key_spki(public_key: &[u8]) -> Result> { - if public_key.len() != ED25519_PUBLIC_KEY_LEN { - bail!("generated Ed25519 public key had unexpected length"); - } - - let mut spki = Vec::with_capacity(ED25519_SPKI_PREFIX.len() + public_key.len()); - spki.extend_from_slice(&ED25519_SPKI_PREFIX); - spki.extend_from_slice(public_key); - Ok(spki) -} - -fn generate_jwt_keypair() -> Result<(String, String)> { - let pkcs8 = Ed25519KeyPair::generate_pkcs8(&SystemRandom::new()) - .map_err(|_| anyhow!("failed to generate Ed25519 keypair"))?; - let keypair = Ed25519KeyPair::from_pkcs8(pkcs8.as_ref()) - .map_err(|_| anyhow!("failed to parse generated Ed25519 keypair"))?; - let public_der = ed25519_public_key_spki(keypair.public_key().as_ref())?; - - Ok(( - pem_encode("PRIVATE KEY", pkcs8.as_ref()), - pem_encode("PUBLIC KEY", &public_der), - )) -} - // --------------------------------------------------------------------------- // Auth status display // --------------------------------------------------------------------------- @@ -143,19 +97,6 @@ fn print_auth_status( // Config TOML generation // --------------------------------------------------------------------------- -fn root_table_mut(doc: &mut toml::Value) -> Result<&mut toml::Table> { - doc.as_table_mut() - .context("settings.toml root is not a table") -} - -fn ensure_table<'a>(table: &'a mut toml::Table, key: &str) -> Result<&'a mut toml::Table> { - table - .entry(key.to_string()) - .or_insert_with(|| toml::Value::Table(toml::Table::default())) - .as_table_mut() - .with_context(|| format!("settings.toml [{key}] is not a table")) -} - /// Default web URL used by `fabro install` when `--web-url` is omitted. pub(crate) fn default_web_url() -> String { format!("http://127.0.0.1:{}", serve::DEFAULT_TCP_PORT) @@ -170,132 +111,11 @@ fn merge_server_settings(doc: &mut toml::Value, web_url: &str) -> Result<()> { .split('/') .next() .unwrap_or(web_url); - - 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")?; - listen.insert("type".to_string(), toml::Value::String("tcp".to_string())); - listen.insert( - "address".to_string(), - toml::Value::String(authority.to_string()), - ); - - let web = ensure_table(server, "web")?; - web.insert("enabled".to_string(), toml::Value::Boolean(true)); - web.insert("url".to_string(), toml::Value::String(web_url.to_string())); - - let auth = ensure_table(server, "auth")?; - auth.insert( - "methods".to_string(), - 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())); - - Ok(()) -} - -fn github_integration_table(doc: &mut toml::Value) -> Result<&mut toml::Table> { - let root = doc - .as_table_mut() - .context("settings.toml root is not a table")?; - let server = root - .entry("server") - .or_insert(toml::Value::Table(toml::Table::default())); - let server_table = server - .as_table_mut() - .context("settings.toml [server] is not a table")?; - let integrations = server_table - .entry("integrations") - .or_insert(toml::Value::Table(toml::Table::default())); - let integrations_table = integrations - .as_table_mut() - .context("settings.toml [server.integrations] is not a table")?; - let github = integrations_table - .entry("github") - .or_insert(toml::Value::Table(toml::Table::default())); - github - .as_table_mut() - .context("settings.toml [server.integrations.github] is not a table") -} - -fn write_token_settings(doc: &mut toml::Value) -> Result<()> { - if let Some(server) = doc.get_mut("server").and_then(toml::Value::as_table_mut) { - if let Some(auth) = server.get_mut("auth").and_then(toml::Value::as_table_mut) { - if let Some(methods) = auth.get_mut("methods").and_then(toml::Value::as_array_mut) { - methods.retain(|value| value.as_str() != Some("github")); - if methods.is_empty() { - methods.push(toml::Value::String("dev-token".to_string())); - } - } - auth.remove("github"); - } - } - - let github = github_integration_table(doc)?; - github.insert("strategy".into(), toml::Value::String("token".to_string())); - github.remove("app_id"); - github.remove("slug"); - github.remove("client_id"); - Ok(()) -} - -fn write_github_app_settings( - doc: &mut toml::Value, - app_id: &str, - slug: &str, - client_id: &str, - allowed_usernames: &[String], -) -> Result<()> { - anyhow::ensure!( - !allowed_usernames.is_empty(), - "GitHub App install requires at least one allowed GitHub username" - ); - - let root = root_table_mut(doc)?; - let server = ensure_table(root, "server")?; - let auth = ensure_table(server, "auth")?; - let methods = auth - .entry("methods".to_string()) - .or_insert_with(|| toml::Value::Array(Vec::new())) - .as_array_mut() - .context("settings.toml [server.auth].methods is not an array")?; - if !methods.iter().any(|value| value.as_str() == Some("github")) { - methods.push(toml::Value::String("github".to_string())); - } - let github_auth = ensure_table(auth, "github")?; - github_auth.insert( - "allowed_usernames".to_string(), - toml::Value::Array( - allowed_usernames - .iter() - .cloned() - .map(toml::Value::String) - .collect(), - ), - ); - - let github = github_integration_table(doc)?; - github.insert("strategy".into(), toml::Value::String("app".to_string())); - github.insert("app_id".into(), toml::Value::String(app_id.to_string())); - github.insert("slug".into(), toml::Value::String(slug.to_string())); - github.insert( - "client_id".into(), - toml::Value::String(client_id.to_string()), - ); - Ok(()) + merge_server_settings_impl( + doc, + web_url, + &InstallListenConfig::Tcp(authority.to_string()), + ) } #[cfg(test)] diff --git a/lib/crates/fabro-install/src/lib.rs b/lib/crates/fabro-install/src/lib.rs index 0f4334416..5286f53d1 100644 --- a/lib/crates/fabro-install/src/lib.rs +++ b/lib/crates/fabro-install/src/lib.rs @@ -290,11 +290,11 @@ pub fn persist_install_outputs_direct( storage_dir: &Path, server_env_secrets: &[(String, String)], vault_secrets: &[VaultSecretWrite], - settings_write: Option>, + settings_write: Option<&PendingSettingsWrite<'_>>, ) -> Result<()> { persist_server_env_secrets(storage_dir, server_env_secrets)?; - if let Some(ref write) = settings_write { + 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()))?; @@ -308,7 +308,7 @@ pub fn persist_install_outputs_direct( if let Err(err) = persist_vault_secrets_direct(storage_dir, vault_secrets) { let mut rollback_failures = Vec::new(); - if let Some(ref write) = settings_write { + 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()); } @@ -332,7 +332,7 @@ pub fn persist_install_outputs_direct( #[cfg(test)] mod tests { - use fabro_config::Storage; + use fabro_config::{Storage, envfile}; use fabro_vault::{SecretType as VaultSecretType, Vault}; use super::{ @@ -353,7 +353,7 @@ mod tests { #[test] fn config_toml_has_auth_strategies() { - use fabro_types::settings::SettingsLayer; + use fabro_types::settings::{ServerAuthMethod, SettingsLayer}; let toml_str = format_config_toml(); let cfg: SettingsLayer = fabro_config::parse_settings_layer(&toml_str).unwrap(); @@ -362,10 +362,7 @@ mod tests { .as_ref() .and_then(|s| s.auth.as_ref()) .expect("server.auth should be set"); - assert_eq!( - auth.methods, - Some(vec![fabro_types::settings::ServerAuthMethod::DevToken]) - ); + assert_eq!(auth.methods, Some(vec![ServerAuthMethod::DevToken])); } #[test] @@ -464,7 +461,7 @@ name = "custom" secret_type: VaultSecretType::Environment, description: None, }], - Some(PendingSettingsWrite { + Some(&PendingSettingsWrite { path: &settings_path, contents: "_version = 1\n[server]\nfoo = \"bar\"\n", previous_contents: Some("_version = 1\n[server]\n"), @@ -481,8 +478,7 @@ name = "custom" assert_eq!(restored.get("EXISTING_SECRET"), Some("keep")); assert_eq!(restored.get("bad-secret-name"), None); - let server_env = - fabro_config::envfile::read_env_file(&storage.server_state().env_path()).unwrap(); + let server_env = envfile::read_env_file(&storage.server_state().env_path()).unwrap(); assert_eq!( server_env.get("SESSION_SECRET").map(String::as_str), Some("session") @@ -491,6 +487,8 @@ name = "custom" #[test] fn merge_server_settings_keeps_tcp_bind_separate_from_public_web_url() { + use fabro_types::settings::server::ServerListenSettings; + let mut doc = toml::Value::Table(toml::Table::default()); merge_server_settings( &mut doc, @@ -506,10 +504,10 @@ name = "custom" let resolved = fabro_config::resolve_server_from_file(&settings).expect("settings should resolve"); match resolved.listen { - fabro_types::settings::server::ServerListenSettings::Tcp { address, .. } => { + ServerListenSettings::Tcp { address, .. } => { assert_eq!(address.to_string(), "0.0.0.0:32276"); } - fabro_types::settings::server::ServerListenSettings::Unix { .. } => { + ServerListenSettings::Unix { .. } => { panic!("expected tcp listen settings"); } } diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 34e89e21b..80500bb77 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -903,7 +903,7 @@ async fn post_install_finish( state.storage_dir.as_ref(), &server_env_secrets, &vault_secrets, - Some(PendingSettingsWrite { + Some(&PendingSettingsWrite { path: state.config_path.as_ref(), contents: &settings_toml, previous_contents: previous_settings.as_deref(),