mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-26 15:40:18 +00:00
feat(cli): show credential auth status in tool info (#1572)
* feat(cli): show credential auth status in `tool info` `ironclaw tool info` now checks the secrets store and shows whether each required credential is configured or missing, consolidated into a single Auth section that deduplicates across http.credentials, auth, and setup.required_secrets. Secrets already shown in Auth are filtered from the Secrets section to avoid redundancy. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(cli): address review feedback on tool info auth status - Fix clippy collapsible-if by using `if let` + `&&` - Use HashMap<String, usize> for O(1) dedup instead of HashSet + linear scan - Add --user flag to `tool info` for checking non-default user credentials - Show "? unknown" on secrets store errors instead of silently reporting missing - Surface secrets store init failure via eprintln instead of silent .ok() - Sort auth entries by secret name for deterministic output Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(cli): only filter secrets when auth section renders, add regression test When the secrets store fails to initialize, the Auth section is not rendered. Previously, secret names were still filtered from the Secrets section, causing credential names to disappear entirely. Now secrets are only filtered when the Auth section will actually be displayed. Adds test verifying auth secret deduplication across auth, setup, and http.credentials sections, plus secrets store existence checks. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * refactor(cli): extract collect_auth_secrets helper, always render Auth section Address review feedback: - Extract dedup logic into `collect_auth_secrets()` so the test exercises the same code path as production (not a re-implementation) - Always render the Auth section when auth secrets exist, showing "? unknown" status when the secrets store is unavailable instead of hiding credential names entirely - Lazily init secrets store only when capabilities contain auth secrets, avoiding spurious warnings for tools with no auth - Add test for empty capabilities edge case Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * style(cli): move HashMap/HashSet imports to top of file Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(cli): use correct tagged JSON format for credential location in test The CredentialLocationSchema uses serde tagged enum format ({"type": "bearer"}), not a bare string ("AuthorizationBearer"). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Illia Polosukhin
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
82822d7b25
commit
656151783c
+270
-16
@@ -2,6 +2,7 @@
|
||||
//!
|
||||
//! Commands for installing, listing, removing, and authenticating WASM tools.
|
||||
|
||||
use std::collections::{HashMap, HashSet};
|
||||
use std::io::Write;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::Arc;
|
||||
@@ -79,6 +80,10 @@ pub enum ToolCommand {
|
||||
/// Directory to look for tool (default: ~/.ironclaw/tools/)
|
||||
#[arg(short, long)]
|
||||
dir: Option<PathBuf>,
|
||||
|
||||
/// User ID for checking credential status (default: "default")
|
||||
#[arg(short, long, default_value = "default")]
|
||||
user: String,
|
||||
},
|
||||
|
||||
/// Configure authentication for a tool
|
||||
@@ -124,7 +129,11 @@ pub async fn run_tool_command(cmd: ToolCommand) -> anyhow::Result<()> {
|
||||
} => install_tool(path, name, capabilities, target, release, skip_build, force).await,
|
||||
ToolCommand::List { dir, verbose } => list_tools(dir, verbose).await,
|
||||
ToolCommand::Remove { name, dir } => remove_tool(name, dir).await,
|
||||
ToolCommand::Info { name_or_path, dir } => show_tool_info(name_or_path, dir).await,
|
||||
ToolCommand::Info {
|
||||
name_or_path,
|
||||
dir,
|
||||
user,
|
||||
} => show_tool_info(name_or_path, dir, user).await,
|
||||
ToolCommand::Auth { name, dir, user } => auth_tool(name, dir, user).await,
|
||||
ToolCommand::Setup { name, dir, user } => setup_tool(name, dir, user).await,
|
||||
}
|
||||
@@ -388,7 +397,11 @@ async fn remove_tool(name: String, dir: Option<PathBuf>) -> anyhow::Result<()> {
|
||||
}
|
||||
|
||||
/// Show information about a tool.
|
||||
async fn show_tool_info(name_or_path: String, dir: Option<PathBuf>) -> anyhow::Result<()> {
|
||||
async fn show_tool_info(
|
||||
name_or_path: String,
|
||||
dir: Option<PathBuf>,
|
||||
user_id: String,
|
||||
) -> anyhow::Result<()> {
|
||||
let wasm_path = if name_or_path.ends_with(".wasm") {
|
||||
PathBuf::from(&name_or_path)
|
||||
} else {
|
||||
@@ -423,7 +436,37 @@ async fn show_tool_info(name_or_path: String, dir: Option<PathBuf>) -> anyhow::R
|
||||
println!("\nCapabilities ({}):", caps_path.display());
|
||||
let content = fs::read_to_string(&caps_path).await?;
|
||||
match CapabilitiesFile::from_json(&content) {
|
||||
Ok(caps) => print_capabilities_detail(&caps),
|
||||
Ok(caps) => {
|
||||
// Lazily init secrets store only when auth secrets need checking.
|
||||
let has_auth = caps.auth.is_some()
|
||||
|| caps
|
||||
.setup
|
||||
.as_ref()
|
||||
.is_some_and(|s| !s.required_secrets.is_empty())
|
||||
|| caps
|
||||
.http
|
||||
.as_ref()
|
||||
.is_some_and(|h| !h.credentials.is_empty());
|
||||
let secrets_store = if has_auth {
|
||||
match init_secrets_store().await {
|
||||
Ok(store) => Some(store),
|
||||
Err(e) => {
|
||||
eprintln!(" Warning: could not init secrets store: {}", e);
|
||||
None
|
||||
}
|
||||
}
|
||||
} else {
|
||||
None
|
||||
};
|
||||
print_capabilities_detail(
|
||||
&caps,
|
||||
secrets_store
|
||||
.as_ref()
|
||||
.map(|s| s.as_ref() as &(dyn SecretsStore + Send + Sync)),
|
||||
&user_id,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
Err(e) => println!(" Error parsing: {}", e),
|
||||
}
|
||||
} else {
|
||||
@@ -476,8 +519,89 @@ fn print_capabilities_summary(caps: &CapabilitiesFile) {
|
||||
}
|
||||
}
|
||||
|
||||
/// Per-secret info collected from all auth-related capability sections.
|
||||
struct AuthSecretInfo {
|
||||
secret_name: String,
|
||||
/// Human-readable label (from auth.display_name or setup prompt).
|
||||
description: Option<String>,
|
||||
/// Injection location (from http.credentials).
|
||||
location: Option<String>,
|
||||
}
|
||||
|
||||
/// Collected auth secrets and the set of secret names they cover.
|
||||
struct CollectedAuthSecrets {
|
||||
secrets: Vec<AuthSecretInfo>,
|
||||
/// Secret names present in `secrets`, for filtering the Secrets capability section.
|
||||
seen_names: HashSet<String>,
|
||||
}
|
||||
|
||||
/// Collect and deduplicate auth secrets from all auth-related capability sections.
|
||||
///
|
||||
/// Priority for the description label: auth.display_name > setup.required_secrets.prompt.
|
||||
/// Injection location is merged from http.credentials.
|
||||
fn collect_auth_secrets(caps: &CapabilitiesFile) -> CollectedAuthSecrets {
|
||||
let mut secrets: Vec<AuthSecretInfo> = Vec::new();
|
||||
let mut seen: HashMap<String, usize> = HashMap::new();
|
||||
|
||||
// auth.display_name is the best label — seed first.
|
||||
if let Some(ref auth) = caps.auth {
|
||||
let index = secrets.len();
|
||||
seen.insert(auth.secret_name.clone(), index);
|
||||
secrets.push(AuthSecretInfo {
|
||||
secret_name: auth.secret_name.clone(),
|
||||
description: auth.display_name.clone(),
|
||||
location: None,
|
||||
});
|
||||
}
|
||||
|
||||
// setup.required_secrets.prompt is second-best label.
|
||||
if let Some(ref setup) = caps.setup {
|
||||
for secret in &setup.required_secrets {
|
||||
if !seen.contains_key(&secret.name) {
|
||||
let index = secrets.len();
|
||||
seen.insert(secret.name.clone(), index);
|
||||
secrets.push(AuthSecretInfo {
|
||||
secret_name: secret.name.clone(),
|
||||
description: Some(secret.prompt.clone()),
|
||||
location: None,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Merge injection location from http.credentials.
|
||||
if let Some(ref http) = caps.http {
|
||||
for cred in http.credentials.values() {
|
||||
let loc = format!("{:?}", cred.location);
|
||||
if let Some(&index) = seen.get(&cred.secret_name) {
|
||||
secrets[index].location = Some(loc);
|
||||
} else {
|
||||
let index = secrets.len();
|
||||
seen.insert(cred.secret_name.clone(), index);
|
||||
secrets.push(AuthSecretInfo {
|
||||
secret_name: cred.secret_name.clone(),
|
||||
description: None,
|
||||
location: Some(loc),
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
let seen_names = seen.into_keys().collect();
|
||||
CollectedAuthSecrets {
|
||||
secrets,
|
||||
seen_names,
|
||||
}
|
||||
}
|
||||
|
||||
/// Print detailed capabilities.
|
||||
fn print_capabilities_detail(caps: &CapabilitiesFile) {
|
||||
async fn print_capabilities_detail(
|
||||
caps: &CapabilitiesFile,
|
||||
secrets_store: Option<&(dyn SecretsStore + Send + Sync)>,
|
||||
user_id: &str,
|
||||
) {
|
||||
let mut collected = collect_auth_secrets(caps);
|
||||
|
||||
if let Some(ref http) = caps.http {
|
||||
println!(" HTTP:");
|
||||
for endpoint in &http.allowlist {
|
||||
@@ -490,13 +614,6 @@ fn print_capabilities_detail(caps: &CapabilitiesFile) {
|
||||
println!(" {} {} {}", methods, endpoint.host, path);
|
||||
}
|
||||
|
||||
if !http.credentials.is_empty() {
|
||||
println!(" Credentials:");
|
||||
for (key, cred) in &http.credentials {
|
||||
println!(" {}: {} -> {:?}", key, cred.secret_name, cred.location);
|
||||
}
|
||||
}
|
||||
|
||||
if let Some(ref rate) = http.rate_limit {
|
||||
println!(
|
||||
" Rate limit: {}/min, {}/hour",
|
||||
@@ -505,12 +622,24 @@ fn print_capabilities_detail(caps: &CapabilitiesFile) {
|
||||
}
|
||||
}
|
||||
|
||||
// Filter secrets already covered by the auth section (always rendered when non-empty).
|
||||
if let Some(ref secrets) = caps.secrets
|
||||
&& !secrets.allowed_names.is_empty()
|
||||
{
|
||||
println!(" Secrets (existence check only):");
|
||||
for name in &secrets.allowed_names {
|
||||
println!(" {}", name);
|
||||
let extra: Vec<_> = if collected.secrets.is_empty() {
|
||||
secrets.allowed_names.iter().collect()
|
||||
} else {
|
||||
secrets
|
||||
.allowed_names
|
||||
.iter()
|
||||
.filter(|name| !collected.seen_names.contains(name.as_str()))
|
||||
.collect()
|
||||
};
|
||||
if !extra.is_empty() {
|
||||
println!(" Secrets (existence check only):");
|
||||
for name in extra {
|
||||
println!(" {}", name);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -531,6 +660,38 @@ fn print_capabilities_detail(caps: &CapabilitiesFile) {
|
||||
println!(" {}", prefix);
|
||||
}
|
||||
}
|
||||
|
||||
// Consolidated auth status — sorted by secret name for deterministic output.
|
||||
if !collected.secrets.is_empty() {
|
||||
collected
|
||||
.secrets
|
||||
.sort_by(|a, b| a.secret_name.cmp(&b.secret_name));
|
||||
println!(" Auth:");
|
||||
for info in &collected.secrets {
|
||||
let (icon, label) = match secrets_store {
|
||||
Some(store) => match store.exists(user_id, &info.secret_name).await {
|
||||
Ok(true) => ("\u{2713}", "configured"),
|
||||
Ok(false) => ("\u{2717}", "missing"),
|
||||
Err(e) => {
|
||||
eprintln!(
|
||||
" Warning: failed to check secret `{}`: {}",
|
||||
info.secret_name, e
|
||||
);
|
||||
("?", "unknown")
|
||||
}
|
||||
},
|
||||
None => ("?", "unknown"),
|
||||
};
|
||||
let mut parts = info.secret_name.clone();
|
||||
if let Some(ref desc) = info.description {
|
||||
parts = format!("{} ({})", parts, desc);
|
||||
}
|
||||
if let Some(ref loc) = info.location {
|
||||
parts = format!("{} -> {}", parts, loc);
|
||||
}
|
||||
println!(" {} {} {}", parts, icon, label);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Validate a tool name to prevent path traversal.
|
||||
@@ -677,8 +838,7 @@ async fn combine_provider_scopes(
|
||||
secret_name: &str,
|
||||
base_oauth: &crate::tools::wasm::OAuthConfigSchema,
|
||||
) -> crate::tools::wasm::OAuthConfigSchema {
|
||||
let mut all_scopes: std::collections::HashSet<String> =
|
||||
base_oauth.scopes.iter().cloned().collect();
|
||||
let mut all_scopes: HashSet<String> = base_oauth.scopes.iter().cloned().collect();
|
||||
|
||||
if let Ok(mut entries) = tokio::fs::read_dir(tools_dir).await {
|
||||
while let Ok(Some(entry)) = entries.next_entry().await {
|
||||
@@ -1127,6 +1287,8 @@ async fn setup_tool(name: String, dir: Option<PathBuf>, user_id: String) -> anyh
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::secrets::{CreateSecretParams, SecretsStore};
|
||||
use crate::testing::credentials::test_secrets_store;
|
||||
|
||||
#[test]
|
||||
fn test_format_size() {
|
||||
@@ -1143,4 +1305,96 @@ mod tests {
|
||||
assert!(dir.to_string_lossy().contains(".ironclaw"));
|
||||
assert!(dir.to_string_lossy().contains("tools"));
|
||||
}
|
||||
|
||||
/// Verify that auth secrets are deduplicated across auth, setup, and http.credentials,
|
||||
/// and that credential status is checked against the secrets store.
|
||||
#[tokio::test]
|
||||
async fn test_auth_secret_dedup_and_status() {
|
||||
let caps = CapabilitiesFile::from_json(
|
||||
r#"{
|
||||
"auth": {
|
||||
"secret_name": "gh_token",
|
||||
"display_name": "GitHub"
|
||||
},
|
||||
"setup": {
|
||||
"required_secrets": [
|
||||
{ "name": "gh_token", "prompt": "GitHub PAT" },
|
||||
{ "name": "extra_key", "prompt": "Extra API Key" }
|
||||
]
|
||||
},
|
||||
"http": {
|
||||
"allowlist": [{ "host": "api.github.com" }],
|
||||
"credentials": {
|
||||
"github": {
|
||||
"secret_name": "gh_token",
|
||||
"location": { "type": "bearer" },
|
||||
"host_patterns": ["api.github.com"]
|
||||
}
|
||||
}
|
||||
},
|
||||
"secrets": {
|
||||
"allowed_names": ["gh_token", "gh_*"]
|
||||
}
|
||||
}"#,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let collected = collect_auth_secrets(&caps);
|
||||
|
||||
// gh_token should appear once (from auth), with location merged from credentials.
|
||||
// extra_key should appear once (from setup).
|
||||
assert_eq!(collected.secrets.len(), 2);
|
||||
let gh = collected
|
||||
.secrets
|
||||
.iter()
|
||||
.find(|s| s.secret_name == "gh_token")
|
||||
.unwrap();
|
||||
assert_eq!(gh.description.as_deref(), Some("GitHub"));
|
||||
assert!(
|
||||
gh.location.is_some(),
|
||||
"location should be merged from http.credentials"
|
||||
);
|
||||
|
||||
let extra = collected
|
||||
.secrets
|
||||
.iter()
|
||||
.find(|s| s.secret_name == "extra_key")
|
||||
.unwrap();
|
||||
assert_eq!(extra.description.as_deref(), Some("Extra API Key"));
|
||||
assert!(extra.location.is_none());
|
||||
|
||||
// Secrets section should filter gh_token (in seen_names) but keep gh_* (wildcard).
|
||||
let secrets = caps.secrets.as_ref().unwrap();
|
||||
let extra_secrets: Vec<_> = secrets
|
||||
.allowed_names
|
||||
.iter()
|
||||
.filter(|name| !collected.seen_names.contains(name.as_str()))
|
||||
.collect();
|
||||
assert_eq!(extra_secrets, vec!["gh_*"]);
|
||||
|
||||
// Verify store check: missing secret -> exists returns false.
|
||||
let store = test_secrets_store();
|
||||
assert!(!store.exists("default", "gh_token").await.unwrap());
|
||||
|
||||
// Store gh_token and verify it's found.
|
||||
store
|
||||
.create(
|
||||
"default",
|
||||
CreateSecretParams::new("gh_token", "ghp_test123"),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(store.exists("default", "gh_token").await.unwrap());
|
||||
// extra_key still missing.
|
||||
assert!(!store.exists("default", "extra_key").await.unwrap());
|
||||
}
|
||||
|
||||
/// No auth sections → collect_auth_secrets returns empty.
|
||||
#[test]
|
||||
fn test_collect_auth_secrets_empty_caps() {
|
||||
let caps = CapabilitiesFile::default();
|
||||
let collected = collect_auth_secrets(&caps);
|
||||
assert!(collected.secrets.is_empty());
|
||||
assert!(collected.seen_names.is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user