From 4369410df70ffff1e638c461ffa4395ac665f1d2 Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Thu, 7 May 2026 02:55:55 -0500 Subject: [PATCH] fix(auth): show credential source table ## Summary - show config, keyring, and env credential sources in deepseek auth status - point env-only auth failures at auth status and auth set recovery commands - document auth status and provider key precedence ## Test plan - cargo test -p deepseek-tui-cli auth_ --locked - cargo test -p deepseek-tui-cli --locked - cargo test -p deepseek-tui env_only_auth_error_gets_recovery_hint --locked - cargo test -p deepseek-config api_key --locked - cargo test -p deepseek-config config_file_resolves_above_env_and_keyring --locked - cargo test -p deepseek-config keyring_resolves_when_config_file_empty_even_if_env_is_set --locked - cargo test -p deepseek-secrets --locked - cargo fmt --all -- --check - git diff --check - cargo clippy -p deepseek-tui-cli -p deepseek-tui -p deepseek-secrets --all-targets --all-features --locked -- -D warnings Closes #907 --- README.md | 8 +- crates/cli/src/lib.rs | 211 ++++++++++++++++++++++++---- crates/secrets/src/lib.rs | 11 +- crates/tui/src/core/engine.rs | 3 +- crates/tui/src/core/engine/tests.rs | 1 + docs/CONFIGURATION.md | 9 +- 6 files changed, 202 insertions(+), 41 deletions(-) diff --git a/README.md b/README.md index d69af630..44ad5026 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,7 @@ You can also set it ahead of time: ```bash deepseek auth set --provider deepseek # saves to ~/.deepseek/config.toml +deepseek auth status # shows the active credential source export DEEPSEEK_API_KEY="YOUR_KEY" # env var alternative; use ~/.zshenv for non-interactive shells deepseek @@ -103,9 +104,10 @@ deepseek doctor # verify setup ``` If `deepseek doctor` says the rejected key came from `DEEPSEEK_API_KEY`, remove -the stale export from your shell startup file, open a fresh shell, then run -`deepseek auth set --provider deepseek`. Saved config keys take precedence over -the environment and are easier to rotate. +the stale export from your shell startup file, open a fresh shell, or run +`deepseek auth set --provider deepseek`. Use `deepseek auth status` to see the +config, keyring, and env-var source state without printing the key. Saved config +keys take precedence over the keyring and environment and are easier to rotate. > To rotate or remove a saved key: `deepseek auth clear --provider deepseek`. diff --git a/crates/cli/src/lib.rs b/crates/cli/src/lib.rs index d9a0aac4..c5ec1209 100644 --- a/crates/cli/src/lib.rs +++ b/crates/cli/src/lib.rs @@ -721,28 +721,59 @@ fn clear_provider_api_key_from_config(store: &mut ConfigStore, provider: Provide } fn provider_env_set(provider: ProviderKind) -> bool { - deepseek_secrets::env_for(provider_slot(provider)).is_some() + provider_env_value(provider).is_some() } -fn provider_config_set(store: &ConfigStore, provider: ProviderKind) -> bool { +fn provider_env_vars(provider: ProviderKind) -> &'static [&'static str] { + match provider { + ProviderKind::Deepseek => &["DEEPSEEK_API_KEY"], + ProviderKind::Openrouter => &["OPENROUTER_API_KEY"], + ProviderKind::Novita => &["NOVITA_API_KEY"], + ProviderKind::NvidiaNim => &["NVIDIA_API_KEY", "NVIDIA_NIM_API_KEY", "DEEPSEEK_API_KEY"], + ProviderKind::Fireworks => &["FIREWORKS_API_KEY"], + ProviderKind::Sglang => &["SGLANG_API_KEY"], + ProviderKind::Vllm => &["VLLM_API_KEY"], + ProviderKind::Ollama => &["OLLAMA_API_KEY"], + ProviderKind::Openai => &["OPENAI_API_KEY"], + } +} + +fn provider_env_value(provider: ProviderKind) -> Option<(&'static str, String)> { + provider_env_vars(provider).iter().find_map(|var| { + std::env::var(var) + .ok() + .filter(|value| !value.trim().is_empty()) + .map(|value| (*var, value)) + }) +} + +fn provider_config_api_key(store: &ConfigStore, provider: ProviderKind) -> Option<&str> { let slot = store .config .providers .for_provider(provider) .api_key - .as_ref(); + .as_deref(); let root = (provider == ProviderKind::Deepseek) - .then_some(store.config.api_key.as_ref()) + .then_some(store.config.api_key.as_deref()) .flatten(); - slot.or(root).is_some_and(|v| !v.trim().is_empty()) + slot.or(root).filter(|v| !v.trim().is_empty()) } -fn provider_keyring_set(secrets: &Secrets, provider: ProviderKind) -> bool { +fn provider_config_set(store: &ConfigStore, provider: ProviderKind) -> bool { + provider_config_api_key(store, provider).is_some() +} + +fn provider_keyring_api_key(secrets: &Secrets, provider: ProviderKind) -> Option { secrets .get(provider_slot(provider)) .ok() .flatten() - .is_some_and(|v| !v.trim().is_empty()) + .filter(|v| !v.trim().is_empty()) +} + +fn provider_keyring_set(secrets: &Secrets, provider: ProviderKind) -> bool { + provider_keyring_api_key(secrets, provider).is_some() } fn write_provider_api_key_to_keyring( @@ -757,6 +788,72 @@ fn clear_provider_api_key_from_keyring(secrets: &Secrets, provider: ProviderKind let _ = secrets.delete(provider_slot(provider)); } +fn auth_status_lines(store: &ConfigStore, secrets: &Secrets) -> Vec { + let provider = store.config.provider; + let config_key = provider_config_api_key(store, provider); + let keyring_key = provider_keyring_api_key(secrets, provider); + let env_key = provider_env_value(provider); + + let active_source = if config_key.is_some() { + "config" + } else if keyring_key.is_some() { + "keyring" + } else if env_key.is_some() { + "env" + } else { + "missing" + }; + let active_last4 = config_key + .map(last4_label) + .or_else(|| keyring_key.as_deref().map(last4_label)) + .or_else(|| env_key.as_ref().map(|(_, value)| last4_label(value))); + let active_label = active_last4 + .map(|last4| format!("{active_source} (last4: {last4})")) + .unwrap_or_else(|| active_source.to_string()); + + let env_var_label = env_key + .as_ref() + .map(|(name, _)| (*name).to_string()) + .unwrap_or_else(|| provider_env_vars(provider).join("/")); + let env_status = env_key + .as_ref() + .map(|(_, value)| format!("set, last4: {}", last4_label(value))) + .unwrap_or_else(|| "unset".to_string()); + + vec![ + format!("provider: {}", provider.as_str()), + format!("active source: {active_label}"), + "lookup order: config -> keyring -> env".to_string(), + format!( + "config file: {} ({})", + store.path().display(), + source_status(config_key, "missing") + ), + format!( + "keyring: {} ({})", + secrets.backend_name(), + source_status(keyring_key.as_deref(), "missing") + ), + format!("env var: {env_var_label} ({env_status})"), + ] +} + +fn source_status(value: Option<&str>, missing_label: &str) -> String { + value + .map(|v| format!("set, last4: {}", last4_label(v))) + .unwrap_or_else(|| missing_label.to_string()) +} + +fn last4_label(value: &str) -> String { + let trimmed = value.trim(); + let chars: Vec = trimmed.chars().collect(); + if chars.len() <= 4 { + return "".to_string(); + } + let last4: String = chars[chars.len() - 4..].iter().collect(); + format!("...{last4}") +} + fn run_auth_command(store: &mut ConfigStore, command: AuthCommand) -> Result<()> { run_auth_command_with_secrets(store, command, &Secrets::auto_detect()) } @@ -768,28 +865,9 @@ fn run_auth_command_with_secrets( ) -> Result<()> { match command { AuthCommand::Status => { - let provider = store.config.provider; - println!("provider: {}", provider.as_str()); - println!("credential precedence: config -> keyring -> env"); - let slot = provider_slot(provider); - let file_set = provider_config_set(store, provider); - let keyring_set = (!file_set).then(|| provider_keyring_set(secrets, provider)); - let env_set = provider_env_set(provider); - let active = if file_set { - "config" - } else if keyring_set == Some(true) { - "keyring" - } else if env_set { - "env" - } else { - "missing" - }; - println!( - "{slot} auth: config={}, keyring={}, env={}, active={active}", - file_set, - keyring_status_short(keyring_set), - env_set - ); + for line in auth_status_lines(store, secrets) { + println!("{line}"); + } Ok(()) } AuthCommand::Set { @@ -1491,6 +1569,8 @@ fn read_api_key_from_stdin() -> Result { mod tests { use super::*; use clap::error::ErrorKind; + use std::ffi::OsString; + use std::sync::{Mutex, OnceLock}; fn parse_ok(argv: &[&str]) -> Cli { Cli::try_parse_from(argv).unwrap_or_else(|err| panic!("parse failed for {argv:?}: {err}")) @@ -1502,6 +1582,41 @@ mod tests { err.to_string() } + fn env_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + .lock() + .unwrap_or_else(|p| p.into_inner()) + } + + struct ScopedEnvVar { + name: &'static str, + previous: Option, + } + + impl ScopedEnvVar { + fn set(name: &'static str, value: &str) -> Self { + let previous = std::env::var_os(name); + // Safety: tests using this helper serialize with env_lock() and + // restore the original value in Drop. + unsafe { std::env::set_var(name, value) }; + Self { name, previous } + } + } + + impl Drop for ScopedEnvVar { + fn drop(&mut self) { + // Safety: tests using this helper serialize with env_lock(). + unsafe { + if let Some(previous) = self.previous.take() { + std::env::set_var(self.name, previous); + } else { + std::env::remove_var(self.name); + } + } + } + } + #[test] fn clap_command_definition_is_consistent() { Cli::command().debug_assert(); @@ -2105,6 +2220,44 @@ mod tests { let _ = std::fs::remove_file(path); } + #[test] + fn auth_status_reports_all_active_provider_sources_with_last4() { + use deepseek_secrets::{InMemoryKeyringStore, KeyringStore}; + use std::sync::Arc; + + let _lock = env_lock(); + let _env = ScopedEnvVar::set("DEEPSEEK_API_KEY", "sk-env-1111"); + + let nanos = chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default(); + let path = std::env::temp_dir().join(format!( + "deepseek-cli-auth-status-table-test-{}-{nanos}.toml", + std::process::id() + )); + let mut store = ConfigStore::load(Some(path.clone())).expect("store should load"); + store.config.provider = ProviderKind::Deepseek; + store.config.api_key = Some("sk-config-3333".to_string()); + store.config.providers.deepseek.api_key = Some("sk-config-3333".to_string()); + + let inner = Arc::new(InMemoryKeyringStore::new()); + inner.set("deepseek", "sk-keyring-2222").unwrap(); + let secrets = Secrets::new(inner); + + let output = auth_status_lines(&store, &secrets).join("\n"); + + assert!(output.contains("provider: deepseek")); + assert!(output.contains("active source: config (last4: ...3333)")); + assert!(output.contains("lookup order: config -> keyring -> env")); + assert!(output.contains("config file: ")); + assert!(output.contains("set, last4: ...3333")); + assert!(output.contains("keyring: in-memory (test) (set, last4: ...2222)")); + assert!(output.contains("env var: DEEPSEEK_API_KEY (set, last4: ...1111)")); + assert!(!output.contains("sk-config-3333")); + assert!(!output.contains("sk-keyring-2222")); + assert!(!output.contains("sk-env-1111")); + + let _ = std::fs::remove_file(path); + } + #[test] fn dispatch_keyring_recovery_self_heals_into_config_file() { use deepseek_secrets::{InMemoryKeyringStore, KeyringStore}; diff --git a/crates/secrets/src/lib.rs b/crates/secrets/src/lib.rs index 2207ad6c..0f58489f 100644 --- a/crates/secrets/src/lib.rs +++ b/crates/secrets/src/lib.rs @@ -5,13 +5,10 @@ //! a file-based fallback for headless Linux (`FileKeyringStore`), and //! an in-memory store for tests (`InMemoryKeyringStore`). //! -//! Higher-level lookup goes through [`Secrets::resolve`], which checks -//! the keyring first and falls back to environment variables. The -//! caller (typically the config crate) then falls back to plaintext -//! TOML if both are empty — that final layer lives outside this crate -//! so the precedence is explicit at the call site. -//! -//! Hard rule: **keyring → env → config-file**. Never swap. +//! Higher-level lookup through [`Secrets::resolve`] checks the keyring first +//! and falls back to environment variables. Config-file precedence lives in the +//! config crate so user-facing commands can keep `config -> keyring -> env` +//! explicit at the call site. #![deny(missing_docs)] use std::collections::HashMap; diff --git a/crates/tui/src/core/engine.rs b/crates/tui/src/core/engine.rs index de6f0e59..6c08cfc9 100644 --- a/crates/tui/src/core/engine.rs +++ b/crates/tui/src/core/engine.rs @@ -380,7 +380,8 @@ impl Engine { Some(format!( "The rejected key came from {env_var}; no saved config key is present.\n\ - Run `deepseek auth set --provider {provider}` to save a valid key in ~/.deepseek/config.toml, \ + Run `deepseek auth status` to inspect credential sources, then \ + `deepseek auth set --provider {provider}` to save a valid key in ~/.deepseek/config.toml, \ or remove the stale export and open a fresh shell.", provider = provider.as_str() )) diff --git a/crates/tui/src/core/engine/tests.rs b/crates/tui/src/core/engine/tests.rs index c4e9c598..c2fbe2c6 100644 --- a/crates/tui/src/core/engine/tests.rs +++ b/crates/tui/src/core/engine/tests.rs @@ -93,6 +93,7 @@ fn env_only_auth_error_gets_recovery_hint() { assert!(message.contains("DEEPSEEK_API_KEY")); assert!(message.contains("no saved config key is present")); + assert!(message.contains("deepseek auth status")); assert!(message.contains("deepseek auth set --provider deepseek")); } diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index e64529ca..c6780eeb 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -55,6 +55,12 @@ the legacy `deepseek login --api-key ...` alias) saves the key to `~/.deepseek/config.toml`, and `deepseek --model deepseek-v4-flash` is forwarded to the TUI as `DEEPSEEK_MODEL`. +Credential lookup uses `config -> keyring -> env` after any explicit CLI +`--api-key`. Run `deepseek auth status` to inspect the active provider's config +file, OS keyring backend, environment variable, winning source, and last-four +label without printing the key itself. The command only probes the active +provider's keyring entry. + For hosted or self-hosted providers, set `provider = "nvidia-nim"`, `"fireworks"`, `"sglang"`, `"vllm"`, or `"ollama"` or pass `deepseek --provider `. The facade saves provider credentials to the shared user config and forwards the resolved @@ -131,7 +137,8 @@ If a profile is selected but missing, DeepSeek TUI exits with an error listing a ## Environment Variables -These override config values: +Most runtime environment variables override config values. API-key variables are +fallbacks after saved config and keyring credentials: - `DEEPSEEK_API_KEY` - `DEEPSEEK_BASE_URL`