From 43d73cb4a1dc5bad37237fad5e19982dc4b1b377 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 13 Apr 2026 07:02:03 -0400 Subject: [PATCH] Clean up provider auth helper duplication Share provider display names and OAuth expiry helpers across auth, CLI, and server code, and simplify the small match arms and helper plumbing that full-workspace clippy surfaced during final verification. --- lib/crates/fabro-auth/src/credential.rs | 5 +++ lib/crates/fabro-auth/src/refresh.rs | 9 +---- lib/crates/fabro-auth/src/resolve.rs | 36 ++++++++----------- .../fabro-auth/src/strategies/codex_device.rs | 11 +++--- lib/crates/fabro-cli/src/commands/install.rs | 6 ++-- .../fabro-cli/src/shared/provider_auth.rs | 25 +++---------- lib/crates/fabro-llm/src/client.rs | 3 +- lib/crates/fabro-model/src/provider.rs | 15 ++++++++ lib/crates/fabro-server/src/server_secrets.rs | 22 +++--------- 9 files changed, 52 insertions(+), 80 deletions(-) diff --git a/lib/crates/fabro-auth/src/credential.rs b/lib/crates/fabro-auth/src/credential.rs index 67d95d297..ac5479a77 100644 --- a/lib/crates/fabro-auth/src/credential.rs +++ b/lib/crates/fabro-auth/src/credential.rs @@ -42,6 +42,11 @@ pub struct OAuthTokens { pub expires_at: DateTime, } +pub(crate) fn expires_at_from_now(expires_in: Option) -> DateTime { + let seconds = i64::try_from(expires_in.unwrap_or(3600)).unwrap_or(i64::MAX); + Utc::now() + Duration::seconds(seconds) +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct OAuthConfig { pub auth_url: String, diff --git a/lib/crates/fabro-auth/src/refresh.rs b/lib/crates/fabro-auth/src/refresh.rs index a1d240d55..c1eb04be4 100644 --- a/lib/crates/fabro-auth/src/refresh.rs +++ b/lib/crates/fabro-auth/src/refresh.rs @@ -1,11 +1,4 @@ -use chrono::{Duration, Utc}; - -use crate::credential::{AuthCredential, AuthDetails, OAuthTokens}; - -fn expires_at_from_now(expires_in: Option) -> chrono::DateTime { - let seconds = i64::try_from(expires_in.unwrap_or(3600)).unwrap_or(i64::MAX); - Utc::now() + Duration::seconds(seconds) -} +use crate::credential::{AuthCredential, AuthDetails, OAuthTokens, expires_at_from_now}; pub async fn refresh_oauth_credential( credential: &AuthCredential, diff --git a/lib/crates/fabro-auth/src/resolve.rs b/lib/crates/fabro-auth/src/resolve.rs index a89178788..be9462fd3 100644 --- a/lib/crates/fabro-auth/src/resolve.rs +++ b/lib/crates/fabro-auth/src/resolve.rs @@ -5,6 +5,7 @@ use fabro_model::Provider; use fabro_vault::Vault; use shlex::try_quote; use tokio::sync::RwLock as AsyncRwLock; +use tokio::task::spawn_blocking; use crate::credential::{ApiKeyHeader, AuthCredential, AuthDetails, credential_id_for}; use crate::refresh::refresh_oauth_credential; @@ -107,7 +108,7 @@ impl CredentialResolver { })?; let refreshed_for_store = refreshed.clone(); let vault = Arc::clone(&self.vault); - tokio::task::spawn_blocking(move || { + spawn_blocking(move || { let mut vault = vault.blocking_write(); vault_set_credential(&mut vault, &credential_id, &refreshed_for_store) .map(|_| ()) @@ -130,7 +131,7 @@ impl CredentialResolver { self.to_api_credential(&vault, &credential), )), CredentialUsage::CliAgent(kind) => Ok(ResolvedCredential::Cli( - self.to_cli_credential(&credential, kind), + Self::to_cli_credential(&credential, kind), )), } } @@ -147,7 +148,7 @@ impl CredentialResolver { } } - for env_var in env_vars_for(provider) { + for env_var in provider.api_key_env_vars() { if let Some(value) = self.lookup_env_or_vault(vault, env_var) { return Ok(AuthCredential { provider, @@ -169,10 +170,13 @@ impl CredentialResolver { Provider::Anthropic => self.lookup_env_or_vault(vault, "ANTHROPIC_BASE_URL"), Provider::OpenAi => self.lookup_env_or_vault(vault, "OPENAI_BASE_URL"), Provider::Gemini => self.lookup_env_or_vault(vault, "GEMINI_BASE_URL"), - Provider::Kimi | Provider::Zai | Provider::Minimax | Provider::Inception => None, - Provider::OpenAiCompatible => None, + Provider::Kimi + | Provider::Zai + | Provider::Minimax + | Provider::Inception + | Provider::OpenAiCompatible => None, }; - let mut api_credential = match &credential.details { + match &credential.details { AuthDetails::ApiKey { key } => ApiCredential { provider: credential.provider, auth_header: match credential.provider { @@ -213,14 +217,10 @@ impl CredentialResolver { project_id: self.lookup_env_or_vault(vault, "OPENAI_PROJECT_ID"), } } - }; - if api_credential.provider == Provider::OpenAi && api_credential.codex_mode { - api_credential.base_url = Some("https://chatgpt.com/backend-api/codex".to_string()); } - api_credential } - fn to_cli_credential(&self, credential: &AuthCredential, kind: CliAgentKind) -> CliCredential { + fn to_cli_credential(credential: &AuthCredential, kind: CliAgentKind) -> CliCredential { let mut env_vars = HashMap::new(); let login_command = match (&credential.provider, &credential.details, kind) { (Provider::OpenAi, AuthDetails::ApiKey { key }, CliAgentKind::Codex) => { @@ -241,7 +241,7 @@ impl CredentialResolver { Some(codex_login_command(&tokens.access_token)) } (_, AuthDetails::ApiKey { key }, _) => { - if let Some(name) = env_vars_for(credential.provider).first() { + if let Some(name) = credential.provider.api_key_env_vars().first() { env_vars.insert((*name).to_string(), key.clone()); } None @@ -260,22 +260,17 @@ impl CredentialResolver { } fn codex_login_command(api_key: &str) -> String { - let quoted = try_quote(api_key) - .map(std::borrow::Cow::into_owned) - .unwrap_or_else(|_| api_key.to_string()); + let quoted = + try_quote(api_key).map_or_else(|_| api_key.to_string(), std::borrow::Cow::into_owned); format!("PATH=\"$HOME/.local/bin:$PATH\" echo {quoted} | codex login --with-api-key") } -fn env_vars_for(provider: Provider) -> &'static [&'static str] { - provider.api_key_env_vars() -} - fn credential_ids_for(provider: Provider, usage: CredentialUsage) -> &'static [&'static str] { match (provider, usage) { - (Provider::OpenAi, CredentialUsage::ApiRequest) => &["openai"], (Provider::OpenAi, CredentialUsage::CliAgent(CliAgentKind::Codex)) => { &["openai_codex", "openai"] } + (Provider::OpenAi, _) => &["openai"], (Provider::Anthropic, _) => &["anthropic"], (Provider::Gemini, _) => &["gemini"], (Provider::Kimi, _) => &["kimi"], @@ -283,7 +278,6 @@ fn credential_ids_for(provider: Provider, usage: CredentialUsage) -> &'static [& (Provider::Minimax, _) => &["minimax"], (Provider::Inception, _) => &["inception"], (Provider::OpenAiCompatible, _) => &[], - (Provider::OpenAi, _) => &["openai"], } } diff --git a/lib/crates/fabro-auth/src/strategies/codex_device.rs b/lib/crates/fabro-auth/src/strategies/codex_device.rs index 60bc22493..d5d5442ec 100644 --- a/lib/crates/fabro-auth/src/strategies/codex_device.rs +++ b/lib/crates/fabro-auth/src/strategies/codex_device.rs @@ -9,7 +9,9 @@ use serde_json::json; use tokio::time::sleep; use crate::context::{AuthContextRequest, AuthContextResponse}; -use crate::credential::{AuthCredential, AuthDetails, OAuthConfig, OAuthTokens}; +use crate::credential::{ + AuthCredential, AuthDetails, OAuthConfig, OAuthTokens, expires_at_from_now, +}; use crate::strategy::AuthStrategy; const DEVICE_AUTH_TIMEOUT: Duration = Duration::from_secs(15 * 60); @@ -30,11 +32,6 @@ fn join_url(base: &str, path: &str) -> String { format!("{}{}", base.trim_end_matches('/'), path) } -fn expires_at_from_now(expires_in: Option) -> chrono::DateTime { - let seconds = i64::try_from(expires_in.unwrap_or(3600)).unwrap_or(i64::MAX); - chrono::Utc::now() + chrono::Duration::seconds(seconds) -} - #[derive(Debug, Deserialize)] struct JwtPayload { #[serde(default)] @@ -155,7 +152,7 @@ impl CodexDeviceStrategy { } match payload.status.as_deref() { - Some("pending") | Some("running") | None => { + Some("pending" | "running") | None => { sleep(DEVICE_AUTH_POLL_INTERVAL).await; } Some(other) => { diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 58d965e24..6f6907617 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -711,7 +711,7 @@ async fn persist_vault_secrets( .body(CreateSecretRequest { name: secret.name.clone(), value: secret.value.clone(), - type_: secret.type_.clone(), + type_: secret.type_, description: secret.description.clone(), }) .send() @@ -726,14 +726,14 @@ async fn persist_vault_secrets( store.set( &secret.name, &secret.value, - local_secret_type(&secret.type_), + local_secret_type(secret.type_), secret.description.as_deref(), )?; } Ok(()) } -fn local_secret_type(secret_type: &ApiSecretType) -> SecretType { +fn local_secret_type(secret_type: ApiSecretType) -> SecretType { match secret_type { ApiSecretType::Environment => SecretType::Environment, ApiSecretType::File => SecretType::File, diff --git a/lib/crates/fabro-cli/src/shared/provider_auth.rs b/lib/crates/fabro-cli/src/shared/provider_auth.rs index d8fc83411..e9d832743 100644 --- a/lib/crates/fabro-cli/src/shared/provider_auth.rs +++ b/lib/crates/fabro-cli/src/shared/provider_auth.rs @@ -36,16 +36,7 @@ pub(crate) fn provider_key_url(provider: Provider) -> &'static str { } pub(crate) fn provider_display_name(provider: Provider) -> &'static str { - match provider { - Provider::Anthropic => "Anthropic", - Provider::OpenAi => "OpenAI", - Provider::Gemini => "Gemini", - Provider::Kimi => "Kimi", - Provider::Zai => "Zai", - Provider::Minimax => "Minimax", - Provider::Inception => "Inception", - Provider::OpenAiCompatible => "OpenAI Compatible", - } + provider.display_name() } // --------------------------------------------------------------------------- @@ -176,25 +167,18 @@ pub(crate) async fn authenticate_provider_with_method( ) -> Result { let mut strategy = strategy_for(provider, method); let request = strategy.init().await?; - present_to_user(&request, s, printer)?; + present_to_user(&request, s, printer); let response = await_user_response(&request, s, printer).await?; strategy.complete(response).await } -pub(crate) fn present_to_user( - request: &AuthContextRequest, - s: &Styles, - printer: Printer, -) -> Result<()> { +pub(crate) fn present_to_user(request: &AuthContextRequest, s: &Styles, printer: Printer) { match request { AuthContextRequest::ApiKey { provider, env_var_names, } => { - let env_var = env_var_names - .first() - .map(String::as_str) - .unwrap_or("API_KEY"); + let env_var = env_var_names.first().map_or("API_KEY", String::as_str); let url = provider_key_url(*provider); fabro_util::printerr!( printer, @@ -224,7 +208,6 @@ pub(crate) fn present_to_user( ); } } - Ok(()) } pub(crate) async fn await_user_response( diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index 3b2db727b..f589c47be 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -412,8 +412,7 @@ impl Client { fn auth_value(auth_header: &ApiKeyHeader) -> String { match auth_header { - ApiKeyHeader::Bearer(value) => value.clone(), - ApiKeyHeader::Custom { value, .. } => value.clone(), + ApiKeyHeader::Bearer(value) | ApiKeyHeader::Custom { value, .. } => value.clone(), } } diff --git a/lib/crates/fabro-model/src/provider.rs b/lib/crates/fabro-model/src/provider.rs index e5666850a..be192efc8 100644 --- a/lib/crates/fabro-model/src/provider.rs +++ b/lib/crates/fabro-model/src/provider.rs @@ -81,6 +81,21 @@ impl Provider { .unwrap_or(Self::Anthropic) } + /// Human-readable display name for the provider. + #[must_use] + pub fn display_name(self) -> &'static str { + match self { + Self::Anthropic => "Anthropic", + Self::OpenAi => "OpenAI", + Self::Gemini => "Gemini", + Self::Kimi => "Kimi", + Self::Zai => "Zai", + Self::Minimax => "Minimax", + Self::Inception => "Inception", + Self::OpenAiCompatible => "OpenAI Compatible", + } + } + /// Stable lowercase string representation used in `Request.provider`, /// adapter names, and other serialization boundaries. #[must_use] diff --git a/lib/crates/fabro-server/src/server_secrets.rs b/lib/crates/fabro-server/src/server_secrets.rs index aa9b9ff5b..232f96d92 100644 --- a/lib/crates/fabro-server/src/server_secrets.rs +++ b/lib/crates/fabro-server/src/server_secrets.rs @@ -109,8 +109,7 @@ impl ProviderCredentials { .await { Ok(ResolvedCredential::Api(credential)) => api_credentials.push(credential), - Ok(ResolvedCredential::Cli(_)) => {} - Err(ResolveError::NotConfigured(_)) => {} + Ok(ResolvedCredential::Cli(_)) | Err(ResolveError::NotConfigured(_)) => {} Err(err) => auth_issues.push((*provider, err)), } } @@ -131,32 +130,19 @@ pub(crate) struct LlmClientResult { pub auth_issues: Vec<(Provider, ResolveError)>, } -pub(crate) fn provider_display_name(provider: Provider) -> &'static str { - match provider { - Provider::Anthropic => "Anthropic", - Provider::OpenAi => "OpenAI", - Provider::Gemini => "Gemini", - Provider::Kimi => "Kimi", - Provider::Zai => "Zai", - Provider::Minimax => "Minimax", - Provider::Inception => "Inception", - Provider::OpenAiCompatible => "OpenAI Compatible", - } -} - pub(crate) fn auth_issue_message(provider: Provider, err: &ResolveError) -> String { match err { ResolveError::NotConfigured(_) => { - format!("{} is not configured", provider_display_name(provider)) + format!("{} is not configured", provider.display_name()) } ResolveError::RefreshFailed { source, .. } => format!( "{} requires re-authentication: {}", - provider_display_name(provider), + provider.display_name(), source ), ResolveError::RefreshTokenMissing(_) => format!( "{} requires re-authentication: refresh token missing", - provider_display_name(provider) + provider.display_name() ), } }