From 5b772ac63cf25e682d329119cd46a6874b94e334 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 12:35:36 +1000 Subject: [PATCH 01/13] feat: introduce a keyring abstraction to help with dep injection --- crates/goose/src/keyring/mock.rs | 72 ++++++++++++++++++++++++++++++ crates/goose/src/keyring/mod.rs | 25 +++++++++++ crates/goose/src/keyring/system.rs | 46 +++++++++++++++++++ 3 files changed, 143 insertions(+) create mode 100644 crates/goose/src/keyring/mock.rs create mode 100644 crates/goose/src/keyring/mod.rs create mode 100644 crates/goose/src/keyring/system.rs diff --git a/crates/goose/src/keyring/mock.rs b/crates/goose/src/keyring/mock.rs new file mode 100644 index 000000000000..e3aecda35178 --- /dev/null +++ b/crates/goose/src/keyring/mock.rs @@ -0,0 +1,72 @@ +use super::{KeyringBackend, KeyringError}; +use anyhow::Result; +use async_trait::async_trait; +use std::collections::HashMap; +use std::sync::{Arc, RwLock}; + +#[derive(Clone, Default)] +pub struct MockKeyringBackend { + storage: Arc>>, +} + +impl MockKeyringBackend { + pub fn new() -> Self { + Self::default() + } + + pub fn clear(&self) { + self.storage + .write() + .expect("Mock keyring lock poisoned") + .clear(); + } + + pub fn contains(&self, service: &str, username: &str) -> bool { + let key = format!("{}:{}", service, username); + self.storage + .read() + .expect("Mock keyring lock poisoned") + .contains_key(&key) + } + + fn make_key(service: &str, username: &str) -> String { + format!("{}:{}", service, username) + } +} + +#[async_trait] +impl KeyringBackend for MockKeyringBackend { + async fn get_password(&self, service: &str, username: &str) -> Result { + let key = Self::make_key(service, username); + let storage = self.storage + .read() + .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))?; + + storage + .get(&key) + .cloned() + .ok_or_else(|| KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + }) + .map_err(anyhow::Error::from) + } + + async fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + let key = Self::make_key(service, username); + self.storage + .write() + .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))? + .insert(key, password.to_string()); + Ok(()) + } + + async fn delete_password(&self, service: &str, username: &str) -> Result<()> { + let key = Self::make_key(service, username); + self.storage + .write() + .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))? + .remove(&key); + Ok(()) + } +} diff --git a/crates/goose/src/keyring/mod.rs b/crates/goose/src/keyring/mod.rs new file mode 100644 index 000000000000..83d25ccd417c --- /dev/null +++ b/crates/goose/src/keyring/mod.rs @@ -0,0 +1,25 @@ +use anyhow::Result; +use async_trait::async_trait; + +#[async_trait] +pub trait KeyringBackend: Send + Sync { + async fn get_password(&self, service: &str, username: &str) -> Result; + async fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()>; + async fn delete_password(&self, service: &str, username: &str) -> Result<()>; +} + +#[derive(Debug, thiserror::Error)] +pub enum KeyringError { + #[error("No entry found for service '{service}' user '{username}'")] + NotFound { service: String, username: String }, + #[error("Access denied to keyring")] + AccessDenied, + #[error("Keyring backend error: {0}")] + Backend(String), +} + +pub mod mock; +pub mod system; + +pub use mock::MockKeyringBackend; +pub use system::SystemKeyringBackend; diff --git a/crates/goose/src/keyring/system.rs b/crates/goose/src/keyring/system.rs new file mode 100644 index 000000000000..641562c7fe09 --- /dev/null +++ b/crates/goose/src/keyring/system.rs @@ -0,0 +1,46 @@ +use super::{KeyringBackend, KeyringError}; +use anyhow::Result; +use async_trait::async_trait; +use keyring::Entry; + +pub struct SystemKeyringBackend; + +#[async_trait] +impl KeyringBackend for SystemKeyringBackend { + async fn get_password(&self, service: &str, username: &str) -> Result { + let entry = + Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; + + entry + .get_password() + .map_err(|e| match e { + keyring::Error::NoEntry => KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + }, + _ => KeyringError::Backend(e.to_string()), + }) + .map_err(anyhow::Error::from) + } + + async fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + let entry = + Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; + + entry + .set_password(password) + .map_err(|e| KeyringError::Backend(e.to_string())) + .map_err(anyhow::Error::from) + } + + async fn delete_password(&self, service: &str, username: &str) -> Result<()> { + let entry = + Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; + + match entry.delete_credential() { + Ok(()) => Ok(()), + Err(keyring::Error::NoEntry) => Ok(()), // Already deleted is fine + Err(e) => Err(anyhow::Error::from(KeyringError::Backend(e.to_string()))), + } + } +} From 6cfe4948d8887a539789c082fac09a916c3c6825 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 12:57:50 +1000 Subject: [PATCH 02/13] feat: use dep injection for keyring ops --- crates/goose/src/config/base.rs | 252 ++++++++++++++++++++++++-- crates/goose/src/lib.rs | 1 + crates/goose/src/providers/factory.rs | 138 ++++++++------ 3 files changed, 322 insertions(+), 69 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index eabdaf4de6c8..198717377e82 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -1,6 +1,6 @@ +use crate::keyring::{KeyringBackend, SystemKeyringBackend}; use etcetera::{choose_app_strategy, AppStrategy, AppStrategyArgs}; use fs2::FileExt; -use keyring::Entry; use once_cell::sync::{Lazy, OnceCell}; use serde::Deserialize; use serde_json::Value; @@ -9,7 +9,9 @@ use std::env; use std::fs::OpenOptions; use std::io::Write; use std::path::{Path, PathBuf}; +use std::sync::Arc; use thiserror::Error; +use tokio::runtime::Runtime; pub static APP_STRATEGY: Lazy = Lazy::new(|| AppStrategyArgs { top_level_domain: "Block".to_string(), @@ -23,6 +25,11 @@ const KEYRING_USERNAME: &str = "secrets"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; +// Shared runtime for sync wrapper operations +static ASYNC_RUNTIME: Lazy = Lazy::new(|| { + Runtime::new().expect("Failed to create tokio runtime for config operations") +}); + #[derive(Error, Debug)] pub enum ConfigError { #[error("Configuration value not found: {0}")] @@ -106,6 +113,7 @@ impl From for ConfigError { pub struct Config { config_path: PathBuf, secrets: SecretStorage, + keyring: Arc, } enum SecretStorage { @@ -118,6 +126,13 @@ static GLOBAL_CONFIG: OnceCell = OnceCell::new(); impl Default for Config { fn default() -> Self { + Self::with_keyring(Arc::new(SystemKeyringBackend)) + } +} + +impl Config { + /// Create a new configuration instance with a custom keyring backend + pub fn with_keyring(keyring: Arc) -> Self { // choose_app_strategy().config_dir() // - macOS/Linux: ~/.config/goose/ // - Windows: ~\AppData\Roaming\Block\goose\config\ @@ -137,14 +152,14 @@ impl Default for Config { service: KEYRING_SERVICE.to_string(), }, }; + Config { config_path, secrets, + keyring, } } -} -impl Config { /// Get the global configuration instance. /// /// This will initialize the configuration with the default path (~/.config/goose/config.yaml) @@ -163,6 +178,7 @@ impl Config { secrets: SecretStorage::Keyring { service: service.to_string(), }, + keyring: Arc::new(SystemKeyringBackend), }) } @@ -179,6 +195,7 @@ impl Config { secrets: SecretStorage::File { path: secrets_path.as_ref().to_path_buf(), }, + keyring: Arc::new(SystemKeyringBackend), }) } @@ -476,19 +493,46 @@ impl Config { Ok(()) } + // Helper function to execute async keyring operations in sync context + fn execute_async(&self, future: F) -> Result + where + F: std::future::Future>, + { + // Try to use current runtime first (for better performance in async contexts) + if let Ok(handle) = tokio::runtime::Handle::try_current() { + handle.block_on(future) + } else { + // Use shared runtime for sync contexts + ASYNC_RUNTIME.block_on(future) + } + } + // Load current secrets from the keyring pub fn load_secrets(&self) -> Result, ConfigError> { + self.execute_async(self.load_secrets_async()) + } + + // Load current secrets from the keyring (async version) + pub async fn load_secrets_async(&self) -> Result, ConfigError> { match &self.secrets { SecretStorage::Keyring { service } => { - let entry = Entry::new(service, KEYRING_USERNAME)?; - - match entry.get_password() { + match self.keyring.get_password(service, KEYRING_USERNAME).await { Ok(content) => { let values: HashMap = serde_json::from_str(&content)?; Ok(values) } - Err(keyring::Error::NoEntry) => Ok(HashMap::new()), - Err(e) => Err(ConfigError::KeyringError(e.to_string())), + Err(e) => { + // Check if it's a "not found" error + if let Some(keyring_err) = e.downcast_ref::() { + match keyring_err { + crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()), + _ => Err(ConfigError::KeyringError(e.to_string())), + } + } else { + // For other error types, propagate them + Err(ConfigError::KeyringError(e.to_string())) + } + } } } SecretStorage::File { path } => { @@ -651,14 +695,21 @@ impl Config { /// - There is an error accessing the keyring /// - There is an error serializing the value pub fn set_secret(&self, key: &str, value: Value) -> Result<(), ConfigError> { - let mut values = self.load_secrets()?; + self.execute_async(self.set_secret_async(key, value)) + } + + /// Set a secret value in the system keyring (async version). + pub async fn set_secret_async(&self, key: &str, value: Value) -> Result<(), ConfigError> { + let mut values = self.load_secrets_async().await?; values.insert(key.to_string(), value); match &self.secrets { SecretStorage::Keyring { service } => { let json_value = serde_json::to_string(&values)?; - let entry = Entry::new(service, KEYRING_USERNAME)?; - entry.set_password(&json_value)?; + self.keyring + .set_password(service, KEYRING_USERNAME, &json_value) + .await + .map_err(|e| ConfigError::KeyringError(e.to_string()))?; } SecretStorage::File { path } => { let yaml_value = serde_yaml::to_string(&values)?; @@ -679,14 +730,21 @@ impl Config { /// - There is an error accessing the keyring /// - There is an error serializing the remaining values pub fn delete_secret(&self, key: &str) -> Result<(), ConfigError> { - let mut values = self.load_secrets()?; + self.execute_async(self.delete_secret_async(key)) + } + + /// Delete a secret from the system keyring (async version). + pub async fn delete_secret_async(&self, key: &str) -> Result<(), ConfigError> { + let mut values = self.load_secrets_async().await?; values.remove(key); match &self.secrets { SecretStorage::Keyring { service } => { let json_value = serde_json::to_string(&values)?; - let entry = Entry::new(service, KEYRING_USERNAME)?; - entry.set_password(&json_value)?; + self.keyring + .set_password(service, KEYRING_USERNAME, &json_value) + .await + .map_err(|e| ConfigError::KeyringError(e.to_string()))?; } SecretStorage::File { path } => { let yaml_value = serde_yaml::to_string(&values)?; @@ -758,7 +816,7 @@ mod tests { use tempfile::NamedTempFile; fn cleanup_keyring() -> Result<(), ConfigError> { - let entry = Entry::new(TEST_KEYRING_SERVICE, KEYRING_USERNAME)?; + let entry = keyring::Entry::new(TEST_KEYRING_SERVICE, KEYRING_USERNAME)?; match entry.delete_credential() { Ok(_) => Ok(()), Err(keyring::Error::NoEntry) => Ok(()), @@ -902,6 +960,45 @@ mod tests { Ok(()) } + #[test] + fn test_secret_management_with_mock() -> Result<(), ConfigError> { + use crate::keyring::MockKeyringBackend; + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the mock keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring.clone()); + + // Test setting and getting a simple secret + config.set_secret("api_key", Value::String("secret123".to_string()))?; + let value: String = config.get_secret("api_key")?; + assert_eq!(value, "secret123"); + + // Verify it's in the mock keyring + assert!(mock_keyring.contains("goose", "secrets")); + + // Test environment variable override + std::env::set_var("API_KEY", "env_secret"); + let value: String = config.get_secret("api_key")?; + assert_eq!(value, "env_secret"); + std::env::remove_var("API_KEY"); + + // Test deleting a secret + config.delete_secret("api_key")?; + let result: Result = config.get_secret("api_key"); + assert!(matches!(result, Err(ConfigError::NotFound(_)))); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + #[test] #[serial] fn test_multiple_secrets() -> Result<(), ConfigError> { @@ -932,6 +1029,131 @@ mod tests { Ok(()) } + #[test] + fn test_multiple_secrets_with_mock() -> Result<(), ConfigError> { + use crate::keyring::MockKeyringBackend; + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the mock keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring.clone()); + + // Set multiple secrets + config.set_secret("key1", Value::String("secret1".to_string()))?; + config.set_secret("key2", Value::String("secret2".to_string()))?; + + // Verify both exist + let value1: String = config.get_secret("key1")?; + let value2: String = config.get_secret("key2")?; + assert_eq!(value1, "secret1"); + assert_eq!(value2, "secret2"); + + // Delete one secret + config.delete_secret("key1")?; + + // Verify key1 is gone but key2 remains + let result1: Result = config.get_secret("key1"); + let value2: String = config.get_secret("key2")?; + assert!(matches!(result1, Err(ConfigError::NotFound(_)))); + assert_eq!(value2, "secret2"); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + + #[test] + fn test_keyring_error_propagation() -> Result<(), ConfigError> { + use crate::keyring::{KeyringBackend, KeyringError}; + use async_trait::async_trait; + + // Create a failing keyring that returns backend errors + struct FailingKeyring; + + #[async_trait] + impl KeyringBackend for FailingKeyring { + async fn get_password(&self, _: &str, _: &str) -> anyhow::Result { + Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) + } + async fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { + Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) + } + async fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { + Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) + } + } + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the failing keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let failing_keyring = Arc::new(FailingKeyring); + let config = Config::with_keyring(failing_keyring); + + // This should return an error, not an empty HashMap + let result = config.load_secrets(); + assert!(result.is_err()); + assert!(result.unwrap_err().to_string().contains("Keyring service unavailable")); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + + #[test] + fn test_keyring_not_found_returns_empty() -> Result<(), ConfigError> { + use crate::keyring::{KeyringBackend, KeyringError}; + use async_trait::async_trait; + + // Create a keyring that always returns NotFound + struct NotFoundKeyring; + + #[async_trait] + impl KeyringBackend for NotFoundKeyring { + async fn get_password(&self, service: &str, username: &str) -> anyhow::Result { + Err(KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + }.into()) + } + async fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { + Ok(()) + } + async fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { + Ok(()) + } + } + + // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the not-found keyring + let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok(); + env::remove_var("GOOSE_DISABLE_KEYRING"); + + let not_found_keyring = Arc::new(NotFoundKeyring); + let config = Config::with_keyring(not_found_keyring); + + // This should return an empty HashMap, not an error + let result = config.load_secrets()?; + assert_eq!(result.len(), 0); + + // Restore GOOSE_DISABLE_KEYRING + match saved_disable { + Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val), + None => env::remove_var("GOOSE_DISABLE_KEYRING"), + } + + Ok(()) + } + #[test] fn test_concurrent_writes() -> Result<(), ConfigError> { use std::sync::{Arc, Barrier, Mutex}; diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index 83c4934d76fa..597e5f21b4da 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -1,6 +1,7 @@ pub mod agents; pub mod config; pub mod context_mgmt; +pub mod keyring; pub mod message; pub mod model; pub mod permission; diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index 6c6f0f9b605c..c8f82985b725 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -231,9 +231,21 @@ mod tests { #[test] fn test_create_lead_worker_provider() { // Save current env vars - let saved_lead = env::var("GOOSE_LEAD_MODEL").ok(); - let saved_provider = env::var("GOOSE_LEAD_PROVIDER").ok(); - let saved_turns = env::var("GOOSE_LEAD_TURNS").ok(); + let saved_vars = [ + ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), + ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), + ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), + ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), + ("ANTHROPIC_API_KEY", env::var("ANTHROPIC_API_KEY").ok()), + ("GOOSE_DISABLE_KEYRING", env::var("GOOSE_DISABLE_KEYRING").ok()), + ]; + + // Disable keyring to avoid keychain popups during tests + env::set_var("GOOSE_DISABLE_KEYRING", "1"); + + // Set fake API keys to avoid keychain access + env::set_var("OPENAI_API_KEY", "fake-test-key"); + env::set_var("ANTHROPIC_API_KEY", "fake-test-key"); // Test with basic lead model configuration env::set_var("GOOSE_LEAD_MODEL", "gpt-4o"); @@ -241,16 +253,21 @@ mod tests { // This will try to create a lead/worker provider let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // The creation might succeed or fail depending on API keys, but we can verify the logic path + // Since we provided fake API keys, the creation should proceed to the provider instantiation + // It may still fail at the provider level, but we should not get keychain-related errors match result { Ok(_) => { - // If it succeeds, it means we created a lead/worker provider successfully - // This would happen if API keys are available in the test environment + // If it succeeds with fake keys, the logic worked } Err(error) => { - // If it fails, it should be due to missing API keys, confirming we tried to create providers - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); + // Should not fail due to missing secrets, but may fail due to invalid fake keys + let error_msg = error.to_string().to_lowercase(); + // Ensure it's not a keychain/secret-related error + assert!( + !error_msg.contains("not found") || // Configuration not found + error_msg.contains("invalid") || // Invalid API key format + error_msg.contains("unauthorized") // API validation failed + ); } } @@ -259,26 +276,20 @@ mod tests { env::set_var("GOOSE_LEAD_TURNS", "5"); let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // Similar validation as above - will fail due to missing API keys but confirms the logic + // Similar validation as above - confirms the logic path - // Restore env vars - match saved_lead { - Some(val) => env::set_var("GOOSE_LEAD_MODEL", val), - None => env::remove_var("GOOSE_LEAD_MODEL"), - } - match saved_provider { - Some(val) => env::set_var("GOOSE_LEAD_PROVIDER", val), - None => env::remove_var("GOOSE_LEAD_PROVIDER"), - } - match saved_turns { - Some(val) => env::set_var("GOOSE_LEAD_TURNS", val), - None => env::remove_var("GOOSE_LEAD_TURNS"), + // Restore all env vars + for (key, value) in saved_vars { + match value { + Some(val) => env::set_var(key, val), + None => env::remove_var(key), + } } } #[test] fn test_lead_model_env_vars_with_defaults() { - // Save current env vars + // Save current env vars including API keys let saved_vars = [ ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), @@ -291,28 +302,42 @@ mod tests { "GOOSE_LEAD_FALLBACK_TURNS", env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), ), + ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), + ("GOOSE_DISABLE_KEYRING", env::var("GOOSE_DISABLE_KEYRING").ok()), ]; + // Disable keyring to avoid keychain popups during tests + env::set_var("GOOSE_DISABLE_KEYRING", "1"); + // Clear all lead env vars for (key, _) in &saved_vars { env::remove_var(key); } + // Set fake API key to avoid keychain access + env::set_var("OPENAI_API_KEY", "fake-test-key"); + // Set only the required lead model env::set_var("GOOSE_LEAD_MODEL", "grok-3"); // This should use defaults for all other values let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // Should attempt to create lead/worker provider (will fail due to missing API keys but confirms logic) + // Should attempt to create lead/worker provider with fake keys match result { Ok(_) => { - // Success means we have API keys and created the provider + // Success means the logic path worked } Err(error) => { - // Should fail due to missing API keys, confirming we tried to create providers - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); + // Should not fail due to missing secrets since we provided fake keys + let error_msg = error.to_string().to_lowercase(); + // Allow various provider-level failures but not keychain issues + assert!( + error_msg.contains("invalid") || + error_msg.contains("unauthorized") || + error_msg.contains("model") || + !error_msg.contains("not found") + ); } } @@ -335,12 +360,19 @@ mod tests { #[test] fn test_create_regular_provider_without_lead_config() { - // Save current env vars - let saved_lead = env::var("GOOSE_LEAD_MODEL").ok(); - let saved_provider = env::var("GOOSE_LEAD_PROVIDER").ok(); - let saved_turns = env::var("GOOSE_LEAD_TURNS").ok(); - let saved_threshold = env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(); - let saved_fallback = env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(); + // Save current env vars including API key + let saved_vars = [ + ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), + ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), + ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), + ("GOOSE_LEAD_FAILURE_THRESHOLD", env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok()), + ("GOOSE_LEAD_FALLBACK_TURNS", env::var("GOOSE_LEAD_FALLBACK_TURNS").ok()), + ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), + ("GOOSE_DISABLE_KEYRING", env::var("GOOSE_DISABLE_KEYRING").ok()), + ]; + + // Disable keyring to avoid keychain popups during tests + env::set_var("GOOSE_DISABLE_KEYRING", "1"); // Ensure all GOOSE_LEAD_* variables are not set env::remove_var("GOOSE_LEAD_MODEL"); @@ -348,38 +380,36 @@ mod tests { env::remove_var("GOOSE_LEAD_TURNS"); env::remove_var("GOOSE_LEAD_FAILURE_THRESHOLD"); env::remove_var("GOOSE_LEAD_FALLBACK_TURNS"); + + // Set fake API key to avoid keychain access + env::set_var("OPENAI_API_KEY", "fake-test-key"); // This should try to create a regular provider let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // The creation might succeed or fail depending on API keys + // The creation should proceed with fake API key match result { Ok(_) => { // If it succeeds, it means we created a regular provider successfully - // This would happen if API keys are available in the test environment } Err(error) => { - // If it fails, it should be due to missing API keys - let error_msg = error.to_string(); - assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret")); + // Should not fail due to missing secrets since we provided fake key + let error_msg = error.to_string().to_lowercase(); + // Allow provider-level failures but not keychain issues + assert!( + error_msg.contains("invalid") || + error_msg.contains("unauthorized") || + !error_msg.contains("not found") + ); } } - // Restore env vars - if let Some(val) = saved_lead { - env::set_var("GOOSE_LEAD_MODEL", val); - } - if let Some(val) = saved_provider { - env::set_var("GOOSE_LEAD_PROVIDER", val); - } - if let Some(val) = saved_turns { - env::set_var("GOOSE_LEAD_TURNS", val); - } - if let Some(val) = saved_threshold { - env::set_var("GOOSE_LEAD_FAILURE_THRESHOLD", val); - } - if let Some(val) = saved_fallback { - env::set_var("GOOSE_LEAD_FALLBACK_TURNS", val); + // Restore all env vars + for (key, value) in saved_vars { + match value { + Some(val) => env::set_var(key, val), + None => env::remove_var(key), + } } } From 7172ff5d60fe19c33610fa9b7ffaa32c6c5f5917 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 14:04:10 +1000 Subject: [PATCH 03/13] fix: lints and fmts --- crates/goose/src/config/base.rs | 20 ++++++++----- crates/goose/src/keyring/mock.rs | 3 +- crates/goose/src/providers/factory.rs | 43 ++++++++++++++++++--------- 3 files changed, 43 insertions(+), 23 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 198717377e82..987e9a48fdbc 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -26,9 +26,8 @@ const KEYRING_USERNAME: &str = "secrets"; const TEST_KEYRING_SERVICE: &str = "goose-test"; // Shared runtime for sync wrapper operations -static ASYNC_RUNTIME: Lazy = Lazy::new(|| { - Runtime::new().expect("Failed to create tokio runtime for config operations") -}); +static ASYNC_RUNTIME: Lazy = + Lazy::new(|| Runtime::new().expect("Failed to create tokio runtime for config operations")); #[derive(Error, Debug)] pub enum ConfigError { @@ -523,7 +522,8 @@ impl Config { } Err(e) => { // Check if it's a "not found" error - if let Some(keyring_err) = e.downcast_ref::() { + if let Some(keyring_err) = e.downcast_ref::() + { match keyring_err { crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()), _ => Err(ConfigError::KeyringError(e.to_string())), @@ -1075,7 +1075,7 @@ mod tests { // Create a failing keyring that returns backend errors struct FailingKeyring; - + #[async_trait] impl KeyringBackend for FailingKeyring { async fn get_password(&self, _: &str, _: &str) -> anyhow::Result { @@ -1099,7 +1099,10 @@ mod tests { // This should return an error, not an empty HashMap let result = config.load_secrets(); assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("Keyring service unavailable")); + assert!(result + .unwrap_err() + .to_string() + .contains("Keyring service unavailable")); // Restore GOOSE_DISABLE_KEYRING match saved_disable { @@ -1117,14 +1120,15 @@ mod tests { // Create a keyring that always returns NotFound struct NotFoundKeyring; - + #[async_trait] impl KeyringBackend for NotFoundKeyring { async fn get_password(&self, service: &str, username: &str) -> anyhow::Result { Err(KeyringError::NotFound { service: service.to_string(), username: username.to_string(), - }.into()) + } + .into()) } async fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { Ok(()) diff --git a/crates/goose/src/keyring/mock.rs b/crates/goose/src/keyring/mock.rs index e3aecda35178..1c7ceea776f5 100644 --- a/crates/goose/src/keyring/mock.rs +++ b/crates/goose/src/keyring/mock.rs @@ -38,7 +38,8 @@ impl MockKeyringBackend { impl KeyringBackend for MockKeyringBackend { async fn get_password(&self, service: &str, username: &str) -> Result { let key = Self::make_key(service, username); - let storage = self.storage + let storage = self + .storage .read() .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))?; diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index c8f82985b725..cbf3288ad58d 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -237,7 +237,10 @@ mod tests { ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), ("ANTHROPIC_API_KEY", env::var("ANTHROPIC_API_KEY").ok()), - ("GOOSE_DISABLE_KEYRING", env::var("GOOSE_DISABLE_KEYRING").ok()), + ( + "GOOSE_DISABLE_KEYRING", + env::var("GOOSE_DISABLE_KEYRING").ok(), + ), ]; // Disable keyring to avoid keychain popups during tests @@ -266,7 +269,7 @@ mod tests { assert!( !error_msg.contains("not found") || // Configuration not found error_msg.contains("invalid") || // Invalid API key format - error_msg.contains("unauthorized") // API validation failed + error_msg.contains("unauthorized") // API validation failed ); } } @@ -303,7 +306,10 @@ mod tests { env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), ), ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ("GOOSE_DISABLE_KEYRING", env::var("GOOSE_DISABLE_KEYRING").ok()), + ( + "GOOSE_DISABLE_KEYRING", + env::var("GOOSE_DISABLE_KEYRING").ok(), + ), ]; // Disable keyring to avoid keychain popups during tests @@ -333,10 +339,10 @@ mod tests { let error_msg = error.to_string().to_lowercase(); // Allow various provider-level failures but not keychain issues assert!( - error_msg.contains("invalid") || - error_msg.contains("unauthorized") || - error_msg.contains("model") || - !error_msg.contains("not found") + error_msg.contains("invalid") + || error_msg.contains("unauthorized") + || error_msg.contains("model") + || !error_msg.contains("not found") ); } } @@ -365,10 +371,19 @@ mod tests { ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), - ("GOOSE_LEAD_FAILURE_THRESHOLD", env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok()), - ("GOOSE_LEAD_FALLBACK_TURNS", env::var("GOOSE_LEAD_FALLBACK_TURNS").ok()), + ( + "GOOSE_LEAD_FAILURE_THRESHOLD", + env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(), + ), + ( + "GOOSE_LEAD_FALLBACK_TURNS", + env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), + ), ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ("GOOSE_DISABLE_KEYRING", env::var("GOOSE_DISABLE_KEYRING").ok()), + ( + "GOOSE_DISABLE_KEYRING", + env::var("GOOSE_DISABLE_KEYRING").ok(), + ), ]; // Disable keyring to avoid keychain popups during tests @@ -380,7 +395,7 @@ mod tests { env::remove_var("GOOSE_LEAD_TURNS"); env::remove_var("GOOSE_LEAD_FAILURE_THRESHOLD"); env::remove_var("GOOSE_LEAD_FALLBACK_TURNS"); - + // Set fake API key to avoid keychain access env::set_var("OPENAI_API_KEY", "fake-test-key"); @@ -397,9 +412,9 @@ mod tests { let error_msg = error.to_string().to_lowercase(); // Allow provider-level failures but not keychain issues assert!( - error_msg.contains("invalid") || - error_msg.contains("unauthorized") || - !error_msg.contains("not found") + error_msg.contains("invalid") + || error_msg.contains("unauthorized") + || !error_msg.contains("not found") ); } } From b3e8815ee82739165fef1ee42d0c7d8d77b32a75 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 14:04:23 +1000 Subject: [PATCH 04/13] feat: more just recipes for qol --- Justfile | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/Justfile b/Justfile index 9641cf285d54..d02d9e20dc22 100644 --- a/Justfile +++ b/Justfile @@ -177,6 +177,28 @@ run-server: @echo "Running server..." cargo run -p goose-server +# Run tests across all crates with keyring disabled +test: + @echo "Running tests with keyring disabled..." + GOOSE_DISABLE_KEYRING=1 cargo test --workspace + +# Run linting checks across all crates +lint: + @echo "Running linting checks..." + cargo clippy --workspace --all-targets --all-features -- -D warnings + +# Format code across all crates +format: + @echo "Formatting code..." + cargo fmt --all + +# Quality assurance: format, lint, and test everything +qa: + @echo "Running quality assurance: format, lint, and test..." + @just format + @just lint + @just test + # make GUI with latest binary lint-ui: cd ui/desktop && npm run lint:check From 164c067c982148e7db8b1fecea60a5e388db8df6 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 14:35:29 +1000 Subject: [PATCH 05/13] refactor: reuse keyring abstraction from goose crate --- Cargo.lock | 1 + crates/goose-mcp/Cargo.toml | 1 + crates/goose-mcp/src/google_drive/mod.rs | 9 ++ .../goose-mcp/src/google_drive/oauth_pkce.rs | 26 +++- crates/goose-mcp/src/google_drive/storage.rs | 126 +++++++++++++----- 5 files changed, 120 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9541e0b7dbf9..cadd579d67ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3629,6 +3629,7 @@ dependencies = [ "google-docs1", "google-drive3", "google-sheets4", + "goose", "http-body-util", "hyper 1.6.0", "ignore", diff --git a/crates/goose-mcp/Cargo.toml b/crates/goose-mcp/Cargo.toml index b6b77d0182ac..6b76d875cb23 100644 --- a/crates/goose-mcp/Cargo.toml +++ b/crates/goose-mcp/Cargo.toml @@ -13,6 +13,7 @@ workspace = true [dependencies] mcp-core = { path = "../mcp-core" } mcp-server = { path = "../mcp-server" } +goose = { path = "../goose" } anyhow = "1.0.94" tokio = { version = "1", features = ["full"] } tracing = "0.1" diff --git a/crates/goose-mcp/src/google_drive/mod.rs b/crates/goose-mcp/src/google_drive/mod.rs index 1f1aeae70ca2..2d63faec586e 100644 --- a/crates/goose-mcp/src/google_drive/mod.rs +++ b/crates/goose-mcp/src/google_drive/mod.rs @@ -161,12 +161,21 @@ impl GoogleDriveRouter { Err(_) => false, }; + // Create the appropriate keyring backend based on environment + let keyring: Arc = + if std::env::var("GOOSE_DISABLE_KEYRING").is_ok() { + Arc::new(goose::keyring::MockKeyringBackend::new()) + } else { + Arc::new(goose::keyring::SystemKeyringBackend) + }; + // Create a credentials manager for storing tokens securely let credentials_manager = Arc::new(CredentialsManager::new( credentials_path.clone(), fallback_to_disk, KEYCHAIN_SERVICE.to_string(), KEYCHAIN_USERNAME.to_string(), + keyring, )); // Read the OAuth credentials from the keyfile diff --git a/crates/goose-mcp/src/google_drive/oauth_pkce.rs b/crates/goose-mcp/src/google_drive/oauth_pkce.rs index 47708b953259..3fd1b8776a07 100644 --- a/crates/goose-mcp/src/google_drive/oauth_pkce.rs +++ b/crates/goose-mcp/src/google_drive/oauth_pkce.rs @@ -193,10 +193,14 @@ impl PkceOAuth2Client { }; // Store updated token data - self.credentials_manager + match self + .credentials_manager .write_credentials(&token_data) - .map(|_| debug!("Successfully stored token data")) - .unwrap_or_else(|e| error!("Failed to store token data: {}", e)); + .await + { + Ok(_) => debug!("Successfully stored token data"), + Err(e) => error!("Failed to store token data: {}", e), + } } else { debug!("No refresh token provided in OAuth flow response"); } @@ -248,10 +252,14 @@ impl PkceOAuth2Client { }; // Store updated token data - self.credentials_manager + match self + .credentials_manager .write_credentials(&token_data) - .map(|_| debug!("Successfully stored token data")) - .unwrap_or_else(|e| error!("Failed to store token data: {}", e)); + .await + { + Ok(_) => debug!("Successfully stored token data"), + Err(e) => error!("Failed to store token data: {}", e), + } Ok(access_token) } @@ -318,7 +326,11 @@ impl GetToken for PkceOAuth2Client { > { Box::pin(async move { // Try to read token data from storage to check if we have a valid token - if let Ok(token_data) = self.credentials_manager.read_credentials::() { + if let Ok(token_data) = self + .credentials_manager + .read_credentials::() + .await + { // Verify the project_id matches if token_data.project_id == self.project_id { // Convert stored scopes to &str slices for comparison diff --git a/crates/goose-mcp/src/google_drive/storage.rs b/crates/goose-mcp/src/google_drive/storage.rs index 8e8f3c08dec3..0a51f4e5681f 100644 --- a/crates/goose-mcp/src/google_drive/storage.rs +++ b/crates/goose-mcp/src/google_drive/storage.rs @@ -1,16 +1,17 @@ use anyhow::Result; -use keyring::Entry; +use goose::keyring::{KeyringBackend, KeyringError}; use serde::{de::DeserializeOwned, Serialize}; use std::fs; use std::path::Path; +use std::sync::Arc; use thiserror::Error; use tracing::{debug, error, warn}; #[allow(dead_code)] #[derive(Error, Debug)] pub enum StorageError { - #[error("Failed to access keychain: {0}")] - KeyringError(#[from] keyring::Error), + #[error("Failed to access keyring: {0}")] + KeyringError(String), #[error("Failed to access file system: {0}")] FileSystemError(#[from] std::io::Error), #[error("No credentials found")] @@ -21,6 +22,15 @@ pub enum StorageError { SerializationError(#[from] serde_json::Error), } +impl From for StorageError { + fn from(err: KeyringError) -> Self { + match err { + KeyringError::NotFound { .. } => StorageError::NotFound, + _ => StorageError::KeyringError(err.to_string()), + } + } +} + /// CredentialsManager handles secure storage of OAuth credentials. /// It attempts to store credentials in the system keychain first, /// with fallback to file system storage if keychain access fails and fallback is enabled. @@ -29,6 +39,7 @@ pub struct CredentialsManager { fallback_to_disk: bool, keychain_service: String, keychain_username: String, + keyring: Arc, } impl CredentialsManager { @@ -37,12 +48,14 @@ impl CredentialsManager { fallback_to_disk: bool, keychain_service: String, keychain_username: String, + keyring: Arc, ) -> Self { Self { credentials_path, fallback_to_disk, keychain_service, keychain_username, + keyring, } } @@ -53,17 +66,19 @@ impl CredentialsManager { /// /// # Type Parameters /// - /// * `T` - The type to deserialize the credentials into. Must implement `serde::de::DeserializeOwned`. + /// * `T` - The type to deserialize to. Must implement `serde::DeserializeOwned`. /// /// # Returns /// - /// * `Ok(T)` - The deserialized credentials + /// * `Ok(T)` - The deserialized data /// * `Err(StorageError)` - If reading or deserialization fails /// /// # Examples /// /// ```no_run /// # use goose_mcp::google_drive::storage::CredentialsManager; + /// # use goose::keyring::SystemKeyringBackend; + /// # use std::sync::Arc; /// use serde::{Serialize, Deserialize}; /// /// #[derive(Serialize, Deserialize)] @@ -73,34 +88,46 @@ impl CredentialsManager { /// expiry: u64, /// } /// + /// # async fn example() -> Result<(), Box> { + /// let keyring = Arc::new(SystemKeyringBackend::new()); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), /// true, // fallback to disk if keychain fails /// String::from("test_service"), - /// String::from("test_user") + /// String::from("test_user"), + /// keyring /// ); - /// match manager.read_credentials::() { - /// Ok(token) => println!("Token expires at: {}", token.expiry), + /// match manager.read_credentials::().await { + /// Ok(token) => println!("Access token: {}", token.access_token), /// Err(e) => eprintln!("Failed to read token: {}", e), /// } + /// # Ok(()) + /// # } /// ``` - pub fn read_credentials(&self) -> Result + pub async fn read_credentials(&self) -> Result where T: DeserializeOwned, { - let json_str = Entry::new(&self.keychain_service, &self.keychain_username) - .and_then(|entry| entry.get_password()) + let json_str = self + .keyring + .get_password(&self.keychain_service, &self.keychain_username) + .await .inspect(|_| { debug!("Successfully read credentials from keychain"); }) .or_else(|e| { if self.fallback_to_disk { - debug!("Falling back to file system due to keyring error: {}", e); + warn!("Falling back to file system due to keyring error: {}", e); self.read_from_file() } else { - match e { - keyring::Error::NoEntry => Err(StorageError::NotFound), - _ => Err(StorageError::KeyringError(e)), + // Convert anyhow::Error back to our error type + if let Some(keyring_err) = e.downcast_ref::() { + match keyring_err { + KeyringError::NotFound { .. } => Err(StorageError::NotFound), + _ => Err(StorageError::KeyringError(e.to_string())), + } + } else { + Err(StorageError::KeyringError(e.to_string())) } } })?; @@ -149,6 +176,8 @@ impl CredentialsManager { /// /// ```no_run /// # use goose_mcp::google_drive::storage::CredentialsManager; + /// # use goose::keyring::SystemKeyringBackend; + /// # use std::sync::Arc; /// use serde::{Serialize, Deserialize}; /// /// #[derive(Serialize, Deserialize)] @@ -158,30 +187,36 @@ impl CredentialsManager { /// expiry: u64, /// } /// + /// # async fn example() -> Result<(), Box> { /// let token = OAuthToken { /// access_token: String::from("access_token_value"), /// refresh_token: String::from("refresh_token_value"), /// expiry: 1672531200, // Unix timestamp /// }; /// + /// let keyring = Arc::new(SystemKeyringBackend::new()); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), /// true, // fallback to disk if keychain fails /// String::from("test_service"), - /// String::from("test_user") + /// String::from("test_user"), + /// keyring /// ); - /// if let Err(e) = manager.write_credentials(&token) { + /// if let Err(e) = manager.write_credentials(&token).await { /// eprintln!("Failed to write token: {}", e); /// } + /// # Ok(()) + /// # } /// ``` - pub fn write_credentials(&self, content: &T) -> Result<(), StorageError> + pub async fn write_credentials(&self, content: &T) -> Result<(), StorageError> where T: Serialize, { let json_str = serde_json::to_string(content).map_err(StorageError::SerializationError)?; - Entry::new(&self.keychain_service, &self.keychain_username) - .and_then(|entry| entry.set_password(&json_str)) + self.keyring + .set_password(&self.keychain_service, &self.keychain_username, &json_str) + .await .inspect(|_| { debug!("Successfully wrote credentials to keychain"); }) @@ -190,13 +225,14 @@ impl CredentialsManager { warn!("Falling back to file system due to keyring error: {}", e); self.write_to_file(&json_str) } else { - Err(StorageError::KeyringError(e)) + Err(StorageError::KeyringError(e.to_string())) } }) } fn write_to_file(&self, content: &str) -> Result<(), StorageError> { let path = Path::new(&self.credentials_path); + if let Some(parent) = path.parent() { if !parent.exists() { match fs::create_dir_all(parent) { @@ -229,6 +265,7 @@ impl Clone for CredentialsManager { fallback_to_disk: self.fallback_to_disk, keychain_service: self.keychain_service.clone(), keychain_username: self.keychain_username.clone(), + keyring: self.keyring.clone(), } } } @@ -236,6 +273,7 @@ impl Clone for CredentialsManager { #[cfg(test)] mod tests { use super::*; + use goose::keyring::MockKeyringBackend; use serde::{Deserialize, Serialize}; use tempfile::tempdir; @@ -256,32 +294,38 @@ mod tests { } } - #[test] - fn test_read_write_from_keychain() { + #[tokio::test] + async fn test_read_write_from_keychain() { // Create a temporary directory for test files let temp_dir = tempdir().expect("Failed to create temp dir"); let cred_path = temp_dir.path().join("test_credentials.json"); let cred_path_str = cred_path.to_str().unwrap().to_string(); + // Create a mock keyring backend + let keyring = Arc::new(MockKeyringBackend::new()); + // Create a credentials manager with fallback enabled - // Using a unique service name to ensure keychain operation fails let manager = CredentialsManager::new( cred_path_str, true, // fallback to disk "test_service".to_string(), "test_user".to_string(), + keyring, ); // Test credentials to store let creds = TestCredentials::new(); - // Write should write to keychain - let write_result = manager.write_credentials(&creds); - assert!(write_result.is_ok(), "Write should succeed with fallback"); + // Write should succeed with mock keyring + let write_result = manager.write_credentials(&creds).await; + assert!( + write_result.is_ok(), + "Write should succeed with mock keyring" + ); - // Read should read from keychain - let read_result = manager.read_credentials::(); - assert!(read_result.is_ok(), "Read should succeed with fallback"); + // Read should succeed with mock keyring + let read_result = manager.read_credentials::().await; + assert!(read_result.is_ok(), "Read should succeed with mock keyring"); // Verify the read credentials match what we wrote assert_eq!( @@ -291,28 +335,36 @@ mod tests { ); } - #[test] - fn test_no_fallback_not_found() { + #[tokio::test] + async fn test_no_fallback_not_found() { // Create a temporary directory for test files let temp_dir = tempdir().expect("Failed to create temp dir"); let cred_path = temp_dir.path().join("nonexistent_credentials.json"); let cred_path_str = cred_path.to_str().unwrap().to_string(); + // Create a mock keyring backend (empty by default) + let keyring = Arc::new(MockKeyringBackend::new()); + // Create a credentials manager with fallback disabled let manager = CredentialsManager::new( cred_path_str, false, // no fallback to disk "test_service_that_should_not_exist".to_string(), "test_user_no_fallback".to_string(), + keyring, ); - // Read should fail with NotFound or KeyringError depending on the system - let read_result = manager.read_credentials::(); + // Read should fail with NotFound since mock keyring is empty and no fallback + let read_result = manager.read_credentials::().await; println!("{:?}", read_result); assert!( read_result.is_err(), "Read should fail when credentials don't exist" ); + assert!( + matches!(read_result.unwrap_err(), StorageError::NotFound), + "Should return NotFound error" + ); } #[test] @@ -323,15 +375,17 @@ mod tests { assert!(matches!(storage_error, StorageError::SerializationError(_))); } - #[test] - fn test_file_system_error_handling() { + #[tokio::test] + async fn test_file_system_error_handling() { // Test handling of file system errors by using an invalid path let invalid_path = String::from("/nonexistent_directory/credentials.json"); + let keyring = Arc::new(MockKeyringBackend::new()); let manager = CredentialsManager::new( invalid_path, true, "test_service".to_string(), "test_user".to_string(), + keyring, ); // Create test credentials From 81a4523d86b860cff6630e4f05d0a1a9f39db061 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 14:39:10 +1000 Subject: [PATCH 06/13] chore: add a lint-all just directive --- Justfile | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Justfile b/Justfile index d02d9e20dc22..91091d437608 100644 --- a/Justfile +++ b/Justfile @@ -185,6 +185,11 @@ test: # Run linting checks across all crates lint: @echo "Running linting checks..." + cargo clippy --workspace --all-features -- -D warnings + +# Run comprehensive linting checks (includes tests, examples, benchmarks) +lint-all: + @echo "Running comprehensive linting checks..." cargo clippy --workspace --all-targets --all-features -- -D warnings # Format code across all crates @@ -377,16 +382,16 @@ set windows-shell := ["powershell.exe", "-NoLogo", "-Command"] ### Build the core code ### profile = --release or "" for debug ### allparam = OR/AND/ANY/NONE --workspace --all-features --all-targets -win-bld profile allparam: +win-bld profile allparam: cargo run {{profile}} -p goose-server --bin generate_schema cargo build {{profile}} {{allparam}} ### Build just debug -win-bld-dbg: +win-bld-dbg: just win-bld " " " " ### Build debug and test, examples,... -win-bld-dbg-all: +win-bld-dbg-all: just win-bld " " "--workspace --all-targets --all-features" ### Build just release @@ -449,8 +454,8 @@ win-total-rls *allparam: just win-bld-rls{{allparam}} just win-run-rls -### Build and run the Kotlin example with -### auto-generated bindings for goose-llm +### Build and run the Kotlin example with +### auto-generated bindings for goose-llm kotlin-example: # Build Rust dylib and generate Kotlin bindings cargo build -p goose-llm From 8fc3a31d59769b534a1919f8d94aeed84203542d Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 14:40:05 +1000 Subject: [PATCH 07/13] fix: fix nested runtime issues by using shared future rt --- crates/goose-mcp/src/google_drive/storage.rs | 4 ++-- crates/goose/src/config/base.rs | 9 ++------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/crates/goose-mcp/src/google_drive/storage.rs b/crates/goose-mcp/src/google_drive/storage.rs index 0a51f4e5681f..c30a1e69b485 100644 --- a/crates/goose-mcp/src/google_drive/storage.rs +++ b/crates/goose-mcp/src/google_drive/storage.rs @@ -89,7 +89,7 @@ impl CredentialsManager { /// } /// /// # async fn example() -> Result<(), Box> { - /// let keyring = Arc::new(SystemKeyringBackend::new()); + /// let keyring = Arc::new(SystemKeyringBackend); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), /// true, // fallback to disk if keychain fails @@ -194,7 +194,7 @@ impl CredentialsManager { /// expiry: 1672531200, // Unix timestamp /// }; /// - /// let keyring = Arc::new(SystemKeyringBackend::new()); + /// let keyring = Arc::new(SystemKeyringBackend); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), /// true, // fallback to disk if keychain fails diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 987e9a48fdbc..aed0b16020b4 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -497,13 +497,8 @@ impl Config { where F: std::future::Future>, { - // Try to use current runtime first (for better performance in async contexts) - if let Ok(handle) = tokio::runtime::Handle::try_current() { - handle.block_on(future) - } else { - // Use shared runtime for sync contexts - ASYNC_RUNTIME.block_on(future) - } + // Always use shared runtime to avoid nested runtime issues + ASYNC_RUNTIME.block_on(future) } // Load current secrets from the keyring From 07c8e737fc2d8ed123c791fcb268bbaa33217dbc Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 16:31:13 +1000 Subject: [PATCH 08/13] refactor: move file based keyring logic to own module --- .../goose-mcp/src/google_drive/oauth_pkce.rs | 18 +- crates/goose-mcp/src/google_drive/storage.rs | 39 ++- crates/goose/src/config/base.rs | 172 ++++-------- crates/goose/src/keyring/file.rs | 252 ++++++++++++++++++ crates/goose/src/keyring/mock.rs | 8 +- crates/goose/src/keyring/mod.rs | 11 +- crates/goose/src/keyring/system.rs | 8 +- crates/goose/src/providers/factory.rs | 20 -- 8 files changed, 332 insertions(+), 196 deletions(-) create mode 100644 crates/goose/src/keyring/file.rs diff --git a/crates/goose-mcp/src/google_drive/oauth_pkce.rs b/crates/goose-mcp/src/google_drive/oauth_pkce.rs index 3fd1b8776a07..e65a25200ae5 100644 --- a/crates/goose-mcp/src/google_drive/oauth_pkce.rs +++ b/crates/goose-mcp/src/google_drive/oauth_pkce.rs @@ -193,11 +193,7 @@ impl PkceOAuth2Client { }; // Store updated token data - match self - .credentials_manager - .write_credentials(&token_data) - .await - { + match self.credentials_manager.write_credentials(&token_data) { Ok(_) => debug!("Successfully stored token data"), Err(e) => error!("Failed to store token data: {}", e), } @@ -252,11 +248,7 @@ impl PkceOAuth2Client { }; // Store updated token data - match self - .credentials_manager - .write_credentials(&token_data) - .await - { + match self.credentials_manager.write_credentials(&token_data) { Ok(_) => debug!("Successfully stored token data"), Err(e) => error!("Failed to store token data: {}", e), } @@ -326,11 +318,7 @@ impl GetToken for PkceOAuth2Client { > { Box::pin(async move { // Try to read token data from storage to check if we have a valid token - if let Ok(token_data) = self - .credentials_manager - .read_credentials::() - .await - { + if let Ok(token_data) = self.credentials_manager.read_credentials::() { // Verify the project_id matches if token_data.project_id == self.project_id { // Convert stored scopes to &str slices for comparison diff --git a/crates/goose-mcp/src/google_drive/storage.rs b/crates/goose-mcp/src/google_drive/storage.rs index c30a1e69b485..7a9df5565e68 100644 --- a/crates/goose-mcp/src/google_drive/storage.rs +++ b/crates/goose-mcp/src/google_drive/storage.rs @@ -88,7 +88,7 @@ impl CredentialsManager { /// expiry: u64, /// } /// - /// # async fn example() -> Result<(), Box> { + /// # fn example() -> Result<(), Box> { /// let keyring = Arc::new(SystemKeyringBackend); /// let manager = CredentialsManager::new( /// String::from("/path/to/credentials.json"), @@ -97,21 +97,18 @@ impl CredentialsManager { /// String::from("test_user"), /// keyring /// ); - /// match manager.read_credentials::().await { + /// match manager.read_credentials::() { /// Ok(token) => println!("Access token: {}", token.access_token), /// Err(e) => eprintln!("Failed to read token: {}", e), /// } /// # Ok(()) /// # } /// ``` - pub async fn read_credentials(&self) -> Result + pub fn read_credentials(&self) -> Result where T: DeserializeOwned, { - let json_str = self - .keyring - .get_password(&self.keychain_service, &self.keychain_username) - .await + let json_str = self.keyring.get_password(&self.keychain_service, &self.keychain_username) .inspect(|_| { debug!("Successfully read credentials from keychain"); }) @@ -187,7 +184,7 @@ impl CredentialsManager { /// expiry: u64, /// } /// - /// # async fn example() -> Result<(), Box> { + /// # fn example() -> Result<(), Box> { /// let token = OAuthToken { /// access_token: String::from("access_token_value"), /// refresh_token: String::from("refresh_token_value"), @@ -202,21 +199,19 @@ impl CredentialsManager { /// String::from("test_user"), /// keyring /// ); - /// if let Err(e) = manager.write_credentials(&token).await { + /// if let Err(e) = manager.write_credentials(&token) { /// eprintln!("Failed to write token: {}", e); /// } /// # Ok(()) /// # } /// ``` - pub async fn write_credentials(&self, content: &T) -> Result<(), StorageError> + pub fn write_credentials(&self, content: &T) -> Result<(), StorageError> where T: Serialize, { let json_str = serde_json::to_string(content).map_err(StorageError::SerializationError)?; - self.keyring - .set_password(&self.keychain_service, &self.keychain_username, &json_str) - .await + self.keyring.set_password(&self.keychain_service, &self.keychain_username, &json_str) .inspect(|_| { debug!("Successfully wrote credentials to keychain"); }) @@ -294,8 +289,8 @@ mod tests { } } - #[tokio::test] - async fn test_read_write_from_keychain() { + #[test] + fn test_read_write_from_keychain() { // Create a temporary directory for test files let temp_dir = tempdir().expect("Failed to create temp dir"); let cred_path = temp_dir.path().join("test_credentials.json"); @@ -317,14 +312,14 @@ mod tests { let creds = TestCredentials::new(); // Write should succeed with mock keyring - let write_result = manager.write_credentials(&creds).await; + let write_result = manager.write_credentials(&creds); assert!( write_result.is_ok(), "Write should succeed with mock keyring" ); // Read should succeed with mock keyring - let read_result = manager.read_credentials::().await; + let read_result = manager.read_credentials::(); assert!(read_result.is_ok(), "Read should succeed with mock keyring"); // Verify the read credentials match what we wrote @@ -335,8 +330,8 @@ mod tests { ); } - #[tokio::test] - async fn test_no_fallback_not_found() { + #[test] + fn test_no_fallback_not_found() { // Create a temporary directory for test files let temp_dir = tempdir().expect("Failed to create temp dir"); let cred_path = temp_dir.path().join("nonexistent_credentials.json"); @@ -355,7 +350,7 @@ mod tests { ); // Read should fail with NotFound since mock keyring is empty and no fallback - let read_result = manager.read_credentials::().await; + let read_result = manager.read_credentials::(); println!("{:?}", read_result); assert!( read_result.is_err(), @@ -375,8 +370,8 @@ mod tests { assert!(matches!(storage_error, StorageError::SerializationError(_))); } - #[tokio::test] - async fn test_file_system_error_handling() { + #[test] + fn test_file_system_error_handling() { // Test handling of file system errors by using an invalid path let invalid_path = String::from("/nonexistent_directory/credentials.json"); let keyring = Arc::new(MockKeyringBackend::new()); diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index aed0b16020b4..8249fef4d62c 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -1,4 +1,4 @@ -use crate::keyring::{KeyringBackend, SystemKeyringBackend}; +use crate::keyring::{FileKeyringBackend, KeyringBackend, SystemKeyringBackend}; use etcetera::{choose_app_strategy, AppStrategy, AppStrategyArgs}; use fs2::FileExt; use once_cell::sync::{Lazy, OnceCell}; @@ -11,7 +11,6 @@ use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::Arc; use thiserror::Error; -use tokio::runtime::Runtime; pub static APP_STRATEGY: Lazy = Lazy::new(|| AppStrategyArgs { top_level_domain: "Block".to_string(), @@ -25,9 +24,6 @@ const KEYRING_USERNAME: &str = "secrets"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; -// Shared runtime for sync wrapper operations -static ASYNC_RUNTIME: Lazy = - Lazy::new(|| Runtime::new().expect("Failed to create tokio runtime for config operations")); #[derive(Error, Debug)] pub enum ConfigError { @@ -111,13 +107,8 @@ impl From for ConfigError { /// For Goose-specific configuration, consider prefixing with "goose_" to avoid conflicts. pub struct Config { config_path: PathBuf, - secrets: SecretStorage, keyring: Arc, -} - -enum SecretStorage { - Keyring { service: String }, - File { path: PathBuf }, + keyring_service: String, } // Global instance @@ -125,7 +116,16 @@ static GLOBAL_CONFIG: OnceCell = OnceCell::new(); impl Default for Config { fn default() -> Self { - Self::with_keyring(Arc::new(SystemKeyringBackend)) + let config_dir = choose_app_strategy(APP_STRATEGY.clone()) + .expect("goose requires a home dir") + .config_dir(); + + let keyring: Arc = if env::var("GOOSE_DISABLE_KEYRING").is_ok() { + Arc::new(FileKeyringBackend::new(config_dir.join("secrets.yaml"))) + } else { + Arc::new(SystemKeyringBackend) + }; + Self::with_keyring(keyring) } } @@ -143,19 +143,10 @@ impl Config { let config_path = config_dir.join("config.yaml"); - let secrets = match env::var("GOOSE_DISABLE_KEYRING") { - Ok(_) => SecretStorage::File { - path: config_dir.join("secrets.yaml"), - }, - Err(_) => SecretStorage::Keyring { - service: KEYRING_SERVICE.to_string(), - }, - }; - Config { config_path, - secrets, keyring, + keyring_service: KEYRING_SERVICE.to_string(), } } @@ -174,10 +165,8 @@ impl Config { pub fn new>(config_path: P, service: &str) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - secrets: SecretStorage::Keyring { - service: service.to_string(), - }, keyring: Arc::new(SystemKeyringBackend), + keyring_service: service.to_string(), }) } @@ -191,10 +180,8 @@ impl Config { ) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - secrets: SecretStorage::File { - path: secrets_path.as_ref().to_path_buf(), - }, - keyring: Arc::new(SystemKeyringBackend), + keyring: Arc::new(FileKeyringBackend::new(secrets_path.as_ref().to_path_buf())), + keyring_service: KEYRING_SERVICE.to_string(), }) } @@ -492,59 +479,26 @@ impl Config { Ok(()) } - // Helper function to execute async keyring operations in sync context - fn execute_async(&self, future: F) -> Result - where - F: std::future::Future>, - { - // Always use shared runtime to avoid nested runtime issues - ASYNC_RUNTIME.block_on(future) - } - - // Load current secrets from the keyring pub fn load_secrets(&self) -> Result, ConfigError> { - self.execute_async(self.load_secrets_async()) - } - - // Load current secrets from the keyring (async version) - pub async fn load_secrets_async(&self) -> Result, ConfigError> { - match &self.secrets { - SecretStorage::Keyring { service } => { - match self.keyring.get_password(service, KEYRING_USERNAME).await { - Ok(content) => { - let values: HashMap = serde_json::from_str(&content)?; - Ok(values) - } - Err(e) => { - // Check if it's a "not found" error - if let Some(keyring_err) = e.downcast_ref::() - { - match keyring_err { - crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()), - _ => Err(ConfigError::KeyringError(e.to_string())), - } - } else { - // For other error types, propagate them - Err(ConfigError::KeyringError(e.to_string())) - } - } - } + match self.keyring.get_password(&self.keyring_service, KEYRING_USERNAME) { + Ok(content) => { + let values: HashMap = serde_json::from_str(&content)?; + Ok(values) } - SecretStorage::File { path } => { - if path.exists() { - let file_content = std::fs::read_to_string(path)?; - let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?; - let json_value: Value = serde_json::to_value(yaml_value)?; - match json_value { - Value::Object(map) => Ok(map.into_iter().collect()), - _ => Ok(HashMap::new()), + Err(e) => { + if let Some(keyring_err) = e.downcast_ref::() { + match keyring_err { + crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()), + _ => Err(ConfigError::KeyringError(e.to_string())), } } else { - Ok(HashMap::new()) + Err(ConfigError::KeyringError(e.to_string())) } } } } + + // check all possible places for a parameter pub fn get(&self, key: &str, is_secret: bool) -> Result { @@ -690,29 +644,16 @@ impl Config { /// - There is an error accessing the keyring /// - There is an error serializing the value pub fn set_secret(&self, key: &str, value: Value) -> Result<(), ConfigError> { - self.execute_async(self.set_secret_async(key, value)) - } - - /// Set a secret value in the system keyring (async version). - pub async fn set_secret_async(&self, key: &str, value: Value) -> Result<(), ConfigError> { - let mut values = self.load_secrets_async().await?; + let mut values = self.load_secrets()?; values.insert(key.to_string(), value); - - match &self.secrets { - SecretStorage::Keyring { service } => { - let json_value = serde_json::to_string(&values)?; - self.keyring - .set_password(service, KEYRING_USERNAME, &json_value) - .await - .map_err(|e| ConfigError::KeyringError(e.to_string()))?; - } - SecretStorage::File { path } => { - let yaml_value = serde_yaml::to_string(&values)?; - std::fs::write(path, yaml_value)?; - } - }; + let json_value = serde_json::to_string(&values)?; + self.keyring + .set_password(&self.keyring_service, KEYRING_USERNAME, &json_value) + .map_err(|e| ConfigError::KeyringError(e.to_string()))?; Ok(()) } + + /// Delete a secret from the system keyring. /// @@ -725,29 +666,16 @@ impl Config { /// - There is an error accessing the keyring /// - There is an error serializing the remaining values pub fn delete_secret(&self, key: &str) -> Result<(), ConfigError> { - self.execute_async(self.delete_secret_async(key)) - } - - /// Delete a secret from the system keyring (async version). - pub async fn delete_secret_async(&self, key: &str) -> Result<(), ConfigError> { - let mut values = self.load_secrets_async().await?; + let mut values = self.load_secrets()?; values.remove(key); - - match &self.secrets { - SecretStorage::Keyring { service } => { - let json_value = serde_json::to_string(&values)?; - self.keyring - .set_password(service, KEYRING_USERNAME, &json_value) - .await - .map_err(|e| ConfigError::KeyringError(e.to_string()))?; - } - SecretStorage::File { path } => { - let yaml_value = serde_yaml::to_string(&values)?; - std::fs::write(path, yaml_value)?; - } - }; + let json_value = serde_json::to_string(&values)?; + self.keyring + .set_password(&self.keyring_service, KEYRING_USERNAME, &json_value) + .map_err(|e| ConfigError::KeyringError(e.to_string()))?; Ok(()) } + + } /// Load init-config.yaml from workspace root if it exists. @@ -1066,20 +994,18 @@ mod tests { #[test] fn test_keyring_error_propagation() -> Result<(), ConfigError> { use crate::keyring::{KeyringBackend, KeyringError}; - use async_trait::async_trait; // Create a failing keyring that returns backend errors struct FailingKeyring; - #[async_trait] impl KeyringBackend for FailingKeyring { - async fn get_password(&self, _: &str, _: &str) -> anyhow::Result { + fn get_password(&self, _: &str, _: &str) -> anyhow::Result { Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) } - async fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { + fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) } - async fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { + fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) } } @@ -1111,24 +1037,22 @@ mod tests { #[test] fn test_keyring_not_found_returns_empty() -> Result<(), ConfigError> { use crate::keyring::{KeyringBackend, KeyringError}; - use async_trait::async_trait; // Create a keyring that always returns NotFound struct NotFoundKeyring; - #[async_trait] impl KeyringBackend for NotFoundKeyring { - async fn get_password(&self, service: &str, username: &str) -> anyhow::Result { + fn get_password(&self, service: &str, username: &str) -> anyhow::Result { Err(KeyringError::NotFound { service: service.to_string(), username: username.to_string(), } .into()) } - async fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { + fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { Ok(()) } - async fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { + fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> { Ok(()) } } diff --git a/crates/goose/src/keyring/file.rs b/crates/goose/src/keyring/file.rs new file mode 100644 index 000000000000..fdf690c2d637 --- /dev/null +++ b/crates/goose/src/keyring/file.rs @@ -0,0 +1,252 @@ +use super::{KeyringBackend, KeyringError}; +use anyhow::Result; +use serde_json::Value; +use std::collections::HashMap; +use std::path::PathBuf; + +pub struct FileKeyringBackend { + secrets_path: PathBuf, +} + +impl FileKeyringBackend { + pub fn new(secrets_path: PathBuf) -> Self { + Self { secrets_path } + } + + fn load_all_secrets(&self) -> Result> { + if self.secrets_path.exists() { + let file_content = std::fs::read_to_string(&self.secrets_path)?; + let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?; + let json_value: Value = serde_json::to_value(yaml_value)?; + match json_value { + Value::Object(map) => { + let mut result = HashMap::new(); + for (key, value) in map { + if let Some(string_value) = value.as_str() { + result.insert(key, string_value.to_string()); + } else { + // Convert non-string values to JSON strings + result.insert(key, serde_json::to_string(&value)?); + } + } + Ok(result) + } + _ => Ok(HashMap::new()), + } + } else { + Ok(HashMap::new()) + } + } + + fn save_all_secrets(&self, secrets: &HashMap) -> Result<()> { + // Convert strings back to appropriate JSON values + let mut json_map = serde_json::Map::new(); + for (key, value) in secrets { + // Try to parse as JSON first, fall back to string + let json_value = serde_json::from_str(value).unwrap_or_else(|_| Value::String(value.clone())); + json_map.insert(key.clone(), json_value); + } + + let yaml_value = serde_yaml::to_string(&json_map)?; + + // Ensure parent directory exists + if let Some(parent) = self.secrets_path.parent() { + std::fs::create_dir_all(parent)?; + } + + std::fs::write(&self.secrets_path, yaml_value)?; + Ok(()) + } + + fn make_key(service: &str, username: &str) -> String { + format!("{}:{}", service, username) + } +} + +impl KeyringBackend for FileKeyringBackend { + fn get_password(&self, service: &str, username: &str) -> Result { + let key = Self::make_key(service, username); + let secrets = self.load_all_secrets()?; + + secrets.get(&key).cloned().ok_or_else(|| { + KeyringError::NotFound { + service: service.to_string(), + username: username.to_string(), + } + .into() + }) + } + + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + let key = Self::make_key(service, username); + let mut secrets = self.load_all_secrets()?; + secrets.insert(key, password.to_string()); + self.save_all_secrets(&secrets)?; + Ok(()) + } + + fn delete_password(&self, service: &str, username: &str) -> Result<()> { + let key = Self::make_key(service, username); + let mut secrets = self.load_all_secrets()?; + secrets.remove(&key); + self.save_all_secrets(&secrets)?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::NamedTempFile; + + #[test] + fn test_basic_operations() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test setting a password + backend.set_password("test_service", "test_user", "test_password")?; + + // Test getting the password + let password = backend.get_password("test_service", "test_user")?; + assert_eq!(password, "test_password"); + + // Test deleting the password + backend.delete_password("test_service", "test_user")?; + + // Test that getting deleted password returns NotFound error + let result = backend.get_password("test_service", "test_user"); + assert!(result.is_err()); + if let Err(e) = result { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } else { + panic!("Expected KeyringError::NotFound, got: {:?}", e); + } + } + + Ok(()) + } + + #[test] + fn test_multiple_services() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Set passwords for different services + backend.set_password("service1", "user1", "password1")?; + backend.set_password("service2", "user2", "password2")?; + backend.set_password("service1", "user2", "password3")?; + + // Verify all passwords can be retrieved correctly + assert_eq!(backend.get_password("service1", "user1")?, "password1"); + assert_eq!(backend.get_password("service2", "user2")?, "password2"); + assert_eq!(backend.get_password("service1", "user2")?, "password3"); + + // Delete one password and verify others remain + backend.delete_password("service1", "user1")?; + assert!(backend.get_password("service1", "user1").is_err()); + assert_eq!(backend.get_password("service2", "user2")?, "password2"); + assert_eq!(backend.get_password("service1", "user2")?, "password3"); + + Ok(()) + } + + #[test] + fn test_password_update() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Set initial password + backend.set_password("service", "user", "old_password")?; + assert_eq!(backend.get_password("service", "user")?, "old_password"); + + // Update password + backend.set_password("service", "user", "new_password")?; + assert_eq!(backend.get_password("service", "user")?, "new_password"); + + Ok(()) + } + + #[test] + fn test_nonexistent_file() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let file_path = temp_file.path().to_path_buf(); + drop(temp_file); // Delete the file + + let backend = FileKeyringBackend::new(file_path); + + // Getting from non-existent file should return NotFound + let result = backend.get_password("service", "user"); + assert!(result.is_err()); + if let Err(e) = result { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } + } + + // Setting should create the file + backend.set_password("service", "user", "password")?; + assert_eq!(backend.get_password("service", "user")?, "password"); + + Ok(()) + } + + #[test] + fn test_empty_password() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test setting and getting empty password + backend.set_password("service", "user", "")?; + assert_eq!(backend.get_password("service", "user")?, ""); + + Ok(()) + } + + #[test] + fn test_special_characters_in_credentials() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test with special characters in service, user, and password + let service = "service-with-dashes_and_underscores.and.dots"; + let user = "user@domain.com"; + let password = "password with spaces & special chars: !@#$%^&*()"; + + backend.set_password(service, user, password)?; + assert_eq!(backend.get_password(service, user)?, password); + + Ok(()) + } + + #[test] + fn test_json_content_in_password() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Test storing JSON content as password + let json_password = r#"{"access_token":"abc123","refresh_token":"def456","expires_in":3600}"#; + backend.set_password("oauth_service", "user", json_password)?; + + let retrieved = backend.get_password("oauth_service", "user")?; + + // Parse both as JSON to compare content regardless of key order + let original: serde_json::Value = serde_json::from_str(json_password).unwrap(); + let retrieved_parsed: serde_json::Value = serde_json::from_str(&retrieved).unwrap(); + assert_eq!(original, retrieved_parsed); + + Ok(()) + } + + #[test] + fn test_delete_nonexistent_password() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Deleting non-existent password should not error (idempotent) + backend.delete_password("nonexistent_service", "nonexistent_user")?; + + Ok(()) + } +} \ No newline at end of file diff --git a/crates/goose/src/keyring/mock.rs b/crates/goose/src/keyring/mock.rs index 1c7ceea776f5..3792ea57f51e 100644 --- a/crates/goose/src/keyring/mock.rs +++ b/crates/goose/src/keyring/mock.rs @@ -1,6 +1,5 @@ use super::{KeyringBackend, KeyringError}; use anyhow::Result; -use async_trait::async_trait; use std::collections::HashMap; use std::sync::{Arc, RwLock}; @@ -34,9 +33,8 @@ impl MockKeyringBackend { } } -#[async_trait] impl KeyringBackend for MockKeyringBackend { - async fn get_password(&self, service: &str, username: &str) -> Result { + fn get_password(&self, service: &str, username: &str) -> Result { let key = Self::make_key(service, username); let storage = self .storage @@ -53,7 +51,7 @@ impl KeyringBackend for MockKeyringBackend { .map_err(anyhow::Error::from) } - async fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { let key = Self::make_key(service, username); self.storage .write() @@ -62,7 +60,7 @@ impl KeyringBackend for MockKeyringBackend { Ok(()) } - async fn delete_password(&self, service: &str, username: &str) -> Result<()> { + fn delete_password(&self, service: &str, username: &str) -> Result<()> { let key = Self::make_key(service, username); self.storage .write() diff --git a/crates/goose/src/keyring/mod.rs b/crates/goose/src/keyring/mod.rs index 83d25ccd417c..eb30b8d8607e 100644 --- a/crates/goose/src/keyring/mod.rs +++ b/crates/goose/src/keyring/mod.rs @@ -1,13 +1,12 @@ use anyhow::Result; -use async_trait::async_trait; -#[async_trait] pub trait KeyringBackend: Send + Sync { - async fn get_password(&self, service: &str, username: &str) -> Result; - async fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()>; - async fn delete_password(&self, service: &str, username: &str) -> Result<()>; + fn get_password(&self, service: &str, username: &str) -> Result; + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()>; + fn delete_password(&self, service: &str, username: &str) -> Result<()>; } + #[derive(Debug, thiserror::Error)] pub enum KeyringError { #[error("No entry found for service '{service}' user '{username}'")] @@ -18,8 +17,10 @@ pub enum KeyringError { Backend(String), } +pub mod file; pub mod mock; pub mod system; +pub use file::FileKeyringBackend; pub use mock::MockKeyringBackend; pub use system::SystemKeyringBackend; diff --git a/crates/goose/src/keyring/system.rs b/crates/goose/src/keyring/system.rs index 641562c7fe09..4274bc58962a 100644 --- a/crates/goose/src/keyring/system.rs +++ b/crates/goose/src/keyring/system.rs @@ -1,13 +1,11 @@ use super::{KeyringBackend, KeyringError}; use anyhow::Result; -use async_trait::async_trait; use keyring::Entry; pub struct SystemKeyringBackend; -#[async_trait] impl KeyringBackend for SystemKeyringBackend { - async fn get_password(&self, service: &str, username: &str) -> Result { + fn get_password(&self, service: &str, username: &str) -> Result { let entry = Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; @@ -23,7 +21,7 @@ impl KeyringBackend for SystemKeyringBackend { .map_err(anyhow::Error::from) } - async fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { + fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> { let entry = Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; @@ -33,7 +31,7 @@ impl KeyringBackend for SystemKeyringBackend { .map_err(anyhow::Error::from) } - async fn delete_password(&self, service: &str, username: &str) -> Result<()> { + fn delete_password(&self, service: &str, username: &str) -> Result<()> { let entry = Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?; diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index cbf3288ad58d..733bce1a39eb 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -237,14 +237,8 @@ mod tests { ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), ("ANTHROPIC_API_KEY", env::var("ANTHROPIC_API_KEY").ok()), - ( - "GOOSE_DISABLE_KEYRING", - env::var("GOOSE_DISABLE_KEYRING").ok(), - ), ]; - // Disable keyring to avoid keychain popups during tests - env::set_var("GOOSE_DISABLE_KEYRING", "1"); // Set fake API keys to avoid keychain access env::set_var("OPENAI_API_KEY", "fake-test-key"); @@ -306,15 +300,8 @@ mod tests { env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), ), ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ( - "GOOSE_DISABLE_KEYRING", - env::var("GOOSE_DISABLE_KEYRING").ok(), - ), ]; - // Disable keyring to avoid keychain popups during tests - env::set_var("GOOSE_DISABLE_KEYRING", "1"); - // Clear all lead env vars for (key, _) in &saved_vars { env::remove_var(key); @@ -380,15 +367,8 @@ mod tests { env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), ), ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ( - "GOOSE_DISABLE_KEYRING", - env::var("GOOSE_DISABLE_KEYRING").ok(), - ), ]; - // Disable keyring to avoid keychain popups during tests - env::set_var("GOOSE_DISABLE_KEYRING", "1"); - // Ensure all GOOSE_LEAD_* variables are not set env::remove_var("GOOSE_LEAD_MODEL"); env::remove_var("GOOSE_LEAD_PROVIDER"); From 61d4225bc11cb0592b9ca2063003c8e05c2320f0 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Sun, 6 Jul 2025 19:00:15 +1000 Subject: [PATCH 09/13] refactor(tests): use a guard pattern to restore env vars --- crates/goose-mcp/src/google_drive/storage.rs | 7 +- crates/goose/src/config/base.rs | 14 +- crates/goose/src/keyring/file.rs | 20 +-- crates/goose/src/keyring/mod.rs | 1 - crates/goose/src/providers/factory.rs | 129 +++++++++---------- 5 files changed, 82 insertions(+), 89 deletions(-) diff --git a/crates/goose-mcp/src/google_drive/storage.rs b/crates/goose-mcp/src/google_drive/storage.rs index 7a9df5565e68..73156b62165f 100644 --- a/crates/goose-mcp/src/google_drive/storage.rs +++ b/crates/goose-mcp/src/google_drive/storage.rs @@ -108,7 +108,9 @@ impl CredentialsManager { where T: DeserializeOwned, { - let json_str = self.keyring.get_password(&self.keychain_service, &self.keychain_username) + let json_str = self + .keyring + .get_password(&self.keychain_service, &self.keychain_username) .inspect(|_| { debug!("Successfully read credentials from keychain"); }) @@ -211,7 +213,8 @@ impl CredentialsManager { { let json_str = serde_json::to_string(content).map_err(StorageError::SerializationError)?; - self.keyring.set_password(&self.keychain_service, &self.keychain_username, &json_str) + self.keyring + .set_password(&self.keychain_service, &self.keychain_username, &json_str) .inspect(|_| { debug!("Successfully wrote credentials to keychain"); }) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 8249fef4d62c..9f239b4846d0 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -24,7 +24,6 @@ const KEYRING_USERNAME: &str = "secrets"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; - #[derive(Error, Debug)] pub enum ConfigError { #[error("Configuration value not found: {0}")] @@ -119,7 +118,7 @@ impl Default for Config { let config_dir = choose_app_strategy(APP_STRATEGY.clone()) .expect("goose requires a home dir") .config_dir(); - + let keyring: Arc = if env::var("GOOSE_DISABLE_KEYRING").is_ok() { Arc::new(FileKeyringBackend::new(config_dir.join("secrets.yaml"))) } else { @@ -480,7 +479,10 @@ impl Config { } pub fn load_secrets(&self) -> Result, ConfigError> { - match self.keyring.get_password(&self.keyring_service, KEYRING_USERNAME) { + match self + .keyring + .get_password(&self.keyring_service, KEYRING_USERNAME) + { Ok(content) => { let values: HashMap = serde_json::from_str(&content)?; Ok(values) @@ -497,8 +499,6 @@ impl Config { } } } - - // check all possible places for a parameter pub fn get(&self, key: &str, is_secret: bool) -> Result { @@ -652,8 +652,6 @@ impl Config { .map_err(|e| ConfigError::KeyringError(e.to_string()))?; Ok(()) } - - /// Delete a secret from the system keyring. /// @@ -674,8 +672,6 @@ impl Config { .map_err(|e| ConfigError::KeyringError(e.to_string()))?; Ok(()) } - - } /// Load init-config.yaml from workspace root if it exists. diff --git a/crates/goose/src/keyring/file.rs b/crates/goose/src/keyring/file.rs index fdf690c2d637..6fffe7284cdd 100644 --- a/crates/goose/src/keyring/file.rs +++ b/crates/goose/src/keyring/file.rs @@ -43,17 +43,18 @@ impl FileKeyringBackend { let mut json_map = serde_json::Map::new(); for (key, value) in secrets { // Try to parse as JSON first, fall back to string - let json_value = serde_json::from_str(value).unwrap_or_else(|_| Value::String(value.clone())); + let json_value = + serde_json::from_str(value).unwrap_or_else(|_| Value::String(value.clone())); json_map.insert(key.clone(), json_value); } - + let yaml_value = serde_yaml::to_string(&json_map)?; - + // Ensure parent directory exists if let Some(parent) = self.secrets_path.parent() { std::fs::create_dir_all(parent)?; } - + std::fs::write(&self.secrets_path, yaml_value)?; Ok(()) } @@ -67,7 +68,7 @@ impl KeyringBackend for FileKeyringBackend { fn get_password(&self, service: &str, username: &str) -> Result { let key = Self::make_key(service, username); let secrets = self.load_all_secrets()?; - + secrets.get(&key).cloned().ok_or_else(|| { KeyringError::NotFound { service: service.to_string(), @@ -226,11 +227,12 @@ mod tests { let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); // Test storing JSON content as password - let json_password = r#"{"access_token":"abc123","refresh_token":"def456","expires_in":3600}"#; + let json_password = + r#"{"access_token":"abc123","refresh_token":"def456","expires_in":3600}"#; backend.set_password("oauth_service", "user", json_password)?; - + let retrieved = backend.get_password("oauth_service", "user")?; - + // Parse both as JSON to compare content regardless of key order let original: serde_json::Value = serde_json::from_str(json_password).unwrap(); let retrieved_parsed: serde_json::Value = serde_json::from_str(&retrieved).unwrap(); @@ -249,4 +251,4 @@ mod tests { Ok(()) } -} \ No newline at end of file +} diff --git a/crates/goose/src/keyring/mod.rs b/crates/goose/src/keyring/mod.rs index eb30b8d8607e..e21d1274cdc8 100644 --- a/crates/goose/src/keyring/mod.rs +++ b/crates/goose/src/keyring/mod.rs @@ -6,7 +6,6 @@ pub trait KeyringBackend: Send + Sync { fn delete_password(&self, service: &str, username: &str) -> Result<()>; } - #[derive(Debug, thiserror::Error)] pub enum KeyringError { #[error("No entry found for service '{service}' user '{username}'")] diff --git a/crates/goose/src/providers/factory.rs b/crates/goose/src/providers/factory.rs index 733bce1a39eb..e0a48ad44ed7 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -180,6 +180,39 @@ mod tests { use mcp_core::{content::TextContent, Role}; use std::env; + /// Helper to save and restore environment variables for testing + struct EnvVarGuard { + saved_vars: Vec<(&'static str, Option)>, + } + + impl EnvVarGuard { + fn new(var_names: &[&'static str]) -> Self { + let saved_vars = var_names + .iter() + .map(|&name| (name, env::var(name).ok())) + .collect(); + Self { saved_vars } + } + + fn clear_all(&self) { + for (key, _) in &self.saved_vars { + env::remove_var(key); + } + } + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + // Restore all env vars + for (key, value) in &self.saved_vars { + match value { + Some(val) => env::set_var(key, val), + None => env::remove_var(key), + } + } + } + } + #[allow(dead_code)] #[derive(Clone)] struct MockTestProvider { @@ -230,15 +263,13 @@ mod tests { #[test] fn test_create_lead_worker_provider() { - // Save current env vars - let saved_vars = [ - ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), - ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), - ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), - ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ("ANTHROPIC_API_KEY", env::var("ANTHROPIC_API_KEY").ok()), - ]; - + let _guard = EnvVarGuard::new(&[ + "GOOSE_LEAD_MODEL", + "GOOSE_LEAD_PROVIDER", + "GOOSE_LEAD_TURNS", + "OPENAI_API_KEY", + "ANTHROPIC_API_KEY", + ]); // Set fake API keys to avoid keychain access env::set_var("OPENAI_API_KEY", "fake-test-key"); @@ -275,37 +306,22 @@ mod tests { let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); // Similar validation as above - confirms the logic path - // Restore all env vars - for (key, value) in saved_vars { - match value { - Some(val) => env::set_var(key, val), - None => env::remove_var(key), - } - } + // EnvVarGuard will automatically restore all env vars when dropped } #[test] fn test_lead_model_env_vars_with_defaults() { - // Save current env vars including API keys - let saved_vars = [ - ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), - ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), - ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), - ( - "GOOSE_LEAD_FAILURE_THRESHOLD", - env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(), - ), - ( - "GOOSE_LEAD_FALLBACK_TURNS", - env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), - ), - ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ]; + let guard = EnvVarGuard::new(&[ + "GOOSE_LEAD_MODEL", + "GOOSE_LEAD_PROVIDER", + "GOOSE_LEAD_TURNS", + "GOOSE_LEAD_FAILURE_THRESHOLD", + "GOOSE_LEAD_FALLBACK_TURNS", + "OPENAI_API_KEY", + ]); // Clear all lead env vars - for (key, _) in &saved_vars { - env::remove_var(key); - } + guard.clear_all(); // Set fake API key to avoid keychain access env::set_var("OPENAI_API_KEY", "fake-test-key"); @@ -342,39 +358,22 @@ mod tests { let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); // Should still attempt to create lead/worker provider with custom settings - // Restore all env vars - for (key, value) in saved_vars { - match value { - Some(val) => env::set_var(key, val), - None => env::remove_var(key), - } - } + // EnvVarGuard will automatically restore all env vars when dropped } #[test] fn test_create_regular_provider_without_lead_config() { - // Save current env vars including API key - let saved_vars = [ - ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()), - ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()), - ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()), - ( - "GOOSE_LEAD_FAILURE_THRESHOLD", - env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(), - ), - ( - "GOOSE_LEAD_FALLBACK_TURNS", - env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), - ), - ("OPENAI_API_KEY", env::var("OPENAI_API_KEY").ok()), - ]; + let guard = EnvVarGuard::new(&[ + "GOOSE_LEAD_MODEL", + "GOOSE_LEAD_PROVIDER", + "GOOSE_LEAD_TURNS", + "GOOSE_LEAD_FAILURE_THRESHOLD", + "GOOSE_LEAD_FALLBACK_TURNS", + "OPENAI_API_KEY", + ]); // Ensure all GOOSE_LEAD_* variables are not set - env::remove_var("GOOSE_LEAD_MODEL"); - env::remove_var("GOOSE_LEAD_PROVIDER"); - env::remove_var("GOOSE_LEAD_TURNS"); - env::remove_var("GOOSE_LEAD_FAILURE_THRESHOLD"); - env::remove_var("GOOSE_LEAD_FALLBACK_TURNS"); + guard.clear_all(); // Set fake API key to avoid keychain access env::set_var("OPENAI_API_KEY", "fake-test-key"); @@ -399,13 +398,7 @@ mod tests { } } - // Restore all env vars - for (key, value) in saved_vars { - match value { - Some(val) => env::set_var(key, val), - None => env::remove_var(key), - } - } + // EnvVarGuard will automatically restore all env vars when dropped } #[test] From 076536f4f8943214f4ddb51a0ad339af127970ff Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Mon, 7 Jul 2025 12:33:13 +1000 Subject: [PATCH 10/13] fix: ensure old secrets.yaml format still works --- crates/goose/src/keyring/file.rs | 58 +++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/crates/goose/src/keyring/file.rs b/crates/goose/src/keyring/file.rs index 6fffe7284cdd..24f26ca0cbc2 100644 --- a/crates/goose/src/keyring/file.rs +++ b/crates/goose/src/keyring/file.rs @@ -21,14 +21,34 @@ impl FileKeyringBackend { match json_value { Value::Object(map) => { let mut result = HashMap::new(); - for (key, value) in map { + + // Check if this is the new format (has "goose:secrets" key) + let service_key = Self::make_key("goose", "secrets"); + if let Some(service_value) = map.get(&service_key) { + // New format: decode JSON from the service key + if let Some(json_str) = service_value.as_str() { + if let Ok(secrets_map) = serde_json::from_str::>(json_str) { + for (key, value) in secrets_map { + if let Some(string_value) = value.as_str() { + result.insert(key, string_value.to_string()); + } else { + result.insert(key, serde_json::to_string(&value)?); + } + } + return Ok(result); + } + } + } + + // Legacy format: direct key-value mapping (read-only) + for (key, value) in &map { if let Some(string_value) = value.as_str() { - result.insert(key, string_value.to_string()); + result.insert(key.clone(), string_value.to_string()); } else { - // Convert non-string values to JSON strings - result.insert(key, serde_json::to_string(&value)?); + result.insert(key.clone(), serde_json::to_string(&value)?); } } + Ok(result) } _ => Ok(HashMap::new()), @@ -251,4 +271,34 @@ mod tests { Ok(()) } + + #[test] + fn test_legacy_format_compatibility() -> Result<()> { + let temp_file = NamedTempFile::new().unwrap(); + + // Write a legacy format secrets.yaml file + let legacy_content = r#"openai_api_key: sk-abc123 +anthropic_api_key: ant-def456 +complex_config: + nested: value + number: 42 +"#; + std::fs::write(temp_file.path(), legacy_content)?; + + // Load with FileKeyringBackend - should read legacy format + let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); + + // Load secrets should work with legacy format (read-only) + let secrets = backend.load_all_secrets()?; + assert_eq!(secrets.get("openai_api_key").unwrap(), "sk-abc123"); + assert_eq!(secrets.get("anthropic_api_key").unwrap(), "ant-def456"); + assert!(secrets.get("complex_config").unwrap().contains("nested")); + + // Verify the original file format is preserved (no auto-migration) + let file_content = std::fs::read_to_string(temp_file.path())?; + assert!(file_content.contains("openai_api_key: sk-abc123")); + assert!(!file_content.contains("goose:secrets")); + + Ok(()) + } } From 8a00bf404e79169915e85f0bcff159d04a783ca8 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Mon, 7 Jul 2025 12:39:48 +1000 Subject: [PATCH 11/13] fix: make the env assertion more robust for GOOSE_DISABLE_KEYRING --- crates/goose/src/config/base.rs | 37 ++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 9f239b4846d0..2a89aa4d792a 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -24,6 +24,19 @@ const KEYRING_USERNAME: &str = "secrets"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; +/// Check if an environment variable is set to a truthy value +/// Truthy values: "1", "true", "yes", "on" (case insensitive) +/// Falsy values: "0", "false", "no", "off", "" or unset +fn is_env_var_truthy(var_name: &str) -> bool { + match env::var(var_name) { + Ok(value) => { + let normalized = value.trim().to_lowercase(); + matches!(normalized.as_str(), "1" | "true" | "yes" | "on") + } + Err(_) => false, + } +} + #[derive(Error, Debug)] pub enum ConfigError { #[error("Configuration value not found: {0}")] @@ -119,7 +132,7 @@ impl Default for Config { .expect("goose requires a home dir") .config_dir(); - let keyring: Arc = if env::var("GOOSE_DISABLE_KEYRING").is_ok() { + let keyring: Arc = if is_env_var_truthy("GOOSE_DISABLE_KEYRING") { Arc::new(FileKeyringBackend::new(config_dir.join("secrets.yaml"))) } else { Arc::new(SystemKeyringBackend) @@ -1073,6 +1086,28 @@ mod tests { Ok(()) } + #[test] + fn test_env_var_truthy_values() { + // Test truthy values + for value in ["1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True"] { + env::set_var("TEST_TRUTHY_VAR", value); + assert!(is_env_var_truthy("TEST_TRUTHY_VAR"), "Value '{}' should be truthy", value); + } + + // Test falsy values + for value in ["0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random"] { + env::set_var("TEST_TRUTHY_VAR", value); + assert!(!is_env_var_truthy("TEST_TRUTHY_VAR"), "Value '{}' should be falsy", value); + } + + // Test unset variable + env::remove_var("TEST_TRUTHY_VAR"); + assert!(!is_env_var_truthy("TEST_TRUTHY_VAR"), "Unset variable should be falsy"); + + // Clean up + env::remove_var("TEST_TRUTHY_VAR"); + } + #[test] fn test_concurrent_writes() -> Result<(), ConfigError> { use std::sync::{Arc, Barrier, Mutex}; From 84e2440a4b0de3786a474fea0f046fcdef889466 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Mon, 7 Jul 2025 12:45:59 +1000 Subject: [PATCH 12/13] refactor: make secrets file a constant --- crates/goose/src/config/base.rs | 28 ++++++++++++++++++++++------ crates/goose/src/keyring/file.rs | 20 +++++++++++--------- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 2a89aa4d792a..9b5c7b5f563a 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -20,6 +20,7 @@ pub static APP_STRATEGY: Lazy = Lazy::new(|| AppStrategyArgs { const KEYRING_SERVICE: &str = "goose"; const KEYRING_USERNAME: &str = "secrets"; +const SECRETS_FILE_NAME: &str = "secrets.yaml"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; @@ -133,7 +134,7 @@ impl Default for Config { .config_dir(); let keyring: Arc = if is_env_var_truthy("GOOSE_DISABLE_KEYRING") { - Arc::new(FileKeyringBackend::new(config_dir.join("secrets.yaml"))) + Arc::new(FileKeyringBackend::new(config_dir.join(SECRETS_FILE_NAME))) } else { Arc::new(SystemKeyringBackend) }; @@ -1089,20 +1090,35 @@ mod tests { #[test] fn test_env_var_truthy_values() { // Test truthy values - for value in ["1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True"] { + for value in [ + "1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True", + ] { env::set_var("TEST_TRUTHY_VAR", value); - assert!(is_env_var_truthy("TEST_TRUTHY_VAR"), "Value '{}' should be truthy", value); + assert!( + is_env_var_truthy("TEST_TRUTHY_VAR"), + "Value '{}' should be truthy", + value + ); } // Test falsy values - for value in ["0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random"] { + for value in [ + "0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random", + ] { env::set_var("TEST_TRUTHY_VAR", value); - assert!(!is_env_var_truthy("TEST_TRUTHY_VAR"), "Value '{}' should be falsy", value); + assert!( + !is_env_var_truthy("TEST_TRUTHY_VAR"), + "Value '{}' should be falsy", + value + ); } // Test unset variable env::remove_var("TEST_TRUTHY_VAR"); - assert!(!is_env_var_truthy("TEST_TRUTHY_VAR"), "Unset variable should be falsy"); + assert!( + !is_env_var_truthy("TEST_TRUTHY_VAR"), + "Unset variable should be falsy" + ); // Clean up env::remove_var("TEST_TRUTHY_VAR"); diff --git a/crates/goose/src/keyring/file.rs b/crates/goose/src/keyring/file.rs index 24f26ca0cbc2..e6a80069cd27 100644 --- a/crates/goose/src/keyring/file.rs +++ b/crates/goose/src/keyring/file.rs @@ -21,13 +21,15 @@ impl FileKeyringBackend { match json_value { Value::Object(map) => { let mut result = HashMap::new(); - + // Check if this is the new format (has "goose:secrets" key) let service_key = Self::make_key("goose", "secrets"); if let Some(service_value) = map.get(&service_key) { // New format: decode JSON from the service key if let Some(json_str) = service_value.as_str() { - if let Ok(secrets_map) = serde_json::from_str::>(json_str) { + if let Ok(secrets_map) = + serde_json::from_str::>(json_str) + { for (key, value) in secrets_map { if let Some(string_value) = value.as_str() { result.insert(key, string_value.to_string()); @@ -39,7 +41,7 @@ impl FileKeyringBackend { } } } - + // Legacy format: direct key-value mapping (read-only) for (key, value) in &map { if let Some(string_value) = value.as_str() { @@ -48,7 +50,7 @@ impl FileKeyringBackend { result.insert(key.clone(), serde_json::to_string(&value)?); } } - + Ok(result) } _ => Ok(HashMap::new()), @@ -275,7 +277,7 @@ mod tests { #[test] fn test_legacy_format_compatibility() -> Result<()> { let temp_file = NamedTempFile::new().unwrap(); - + // Write a legacy format secrets.yaml file let legacy_content = r#"openai_api_key: sk-abc123 anthropic_api_key: ant-def456 @@ -284,21 +286,21 @@ complex_config: number: 42 "#; std::fs::write(temp_file.path(), legacy_content)?; - + // Load with FileKeyringBackend - should read legacy format let backend = FileKeyringBackend::new(temp_file.path().to_path_buf()); - + // Load secrets should work with legacy format (read-only) let secrets = backend.load_all_secrets()?; assert_eq!(secrets.get("openai_api_key").unwrap(), "sk-abc123"); assert_eq!(secrets.get("anthropic_api_key").unwrap(), "ant-def456"); assert!(secrets.get("complex_config").unwrap().contains("nested")); - + // Verify the original file format is preserved (no auto-migration) let file_content = std::fs::read_to_string(temp_file.path())?; assert!(file_content.contains("openai_api_key: sk-abc123")); assert!(!file_content.contains("goose:secrets")); - + Ok(()) } } From 838bf7660b7daed14e7bb5b4f8ad62103cbc6b97 Mon Sep 17 00:00:00 2001 From: Prem Pillai Date: Wed, 9 Jul 2025 19:40:12 +1000 Subject: [PATCH 13/13] wip --- crates/goose-mcp/src/google_drive/mod.rs | 9 +- crates/goose/src/config/base.rs | 93 ++---- crates/goose/src/keyring/factory.rs | 345 +++++++++++++++++++++++ crates/goose/src/keyring/mod.rs | 41 +++ 4 files changed, 404 insertions(+), 84 deletions(-) create mode 100644 crates/goose/src/keyring/factory.rs diff --git a/crates/goose-mcp/src/google_drive/mod.rs b/crates/goose-mcp/src/google_drive/mod.rs index 2d63faec586e..cf0cf0f433c6 100644 --- a/crates/goose-mcp/src/google_drive/mod.rs +++ b/crates/goose-mcp/src/google_drive/mod.rs @@ -161,13 +161,8 @@ impl GoogleDriveRouter { Err(_) => false, }; - // Create the appropriate keyring backend based on environment - let keyring: Arc = - if std::env::var("GOOSE_DISABLE_KEYRING").is_ok() { - Arc::new(goose::keyring::MockKeyringBackend::new()) - } else { - Arc::new(goose::keyring::SystemKeyringBackend) - }; + // Use factory to create keyring backend consistently + let keyring = goose::keyring::create_default_keyring(); // Create a credentials manager for storing tokens securely let credentials_manager = Arc::new(CredentialsManager::new( diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 9b5c7b5f563a..143d28453421 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -1,4 +1,6 @@ -use crate::keyring::{FileKeyringBackend, KeyringBackend, SystemKeyringBackend}; +use crate::keyring::{ + create_default_keyring, create_keyring_with_file_path, FileKeyringBackend, KeyringBackend, +}; use etcetera::{choose_app_strategy, AppStrategy, AppStrategyArgs}; use fs2::FileExt; use once_cell::sync::{Lazy, OnceCell}; @@ -25,19 +27,6 @@ const SECRETS_FILE_NAME: &str = "secrets.yaml"; #[cfg(test)] const TEST_KEYRING_SERVICE: &str = "goose-test"; -/// Check if an environment variable is set to a truthy value -/// Truthy values: "1", "true", "yes", "on" (case insensitive) -/// Falsy values: "0", "false", "no", "off", "" or unset -fn is_env_var_truthy(var_name: &str) -> bool { - match env::var(var_name) { - Ok(value) => { - let normalized = value.trim().to_lowercase(); - matches!(normalized.as_str(), "1" | "true" | "yes" | "on") - } - Err(_) => false, - } -} - #[derive(Error, Debug)] pub enum ConfigError { #[error("Configuration value not found: {0}")] @@ -133,11 +122,8 @@ impl Default for Config { .expect("goose requires a home dir") .config_dir(); - let keyring: Arc = if is_env_var_truthy("GOOSE_DISABLE_KEYRING") { - Arc::new(FileKeyringBackend::new(config_dir.join(SECRETS_FILE_NAME))) - } else { - Arc::new(SystemKeyringBackend) - }; + // Use factory with custom file path to maintain same behavior + let keyring = create_keyring_with_file_path(config_dir.join(SECRETS_FILE_NAME)); Self::with_keyring(keyring) } } @@ -178,7 +164,7 @@ impl Config { pub fn new>(config_path: P, service: &str) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - keyring: Arc::new(SystemKeyringBackend), + keyring: create_default_keyring(), keyring_service: service.to_string(), }) } @@ -745,18 +731,8 @@ pub fn load_init_config_from_workspace() -> Result, Confi #[cfg(test)] mod tests { use super::*; - use serial_test::serial; use tempfile::NamedTempFile; - fn cleanup_keyring() -> Result<(), ConfigError> { - let entry = keyring::Entry::new(TEST_KEYRING_SERVICE, KEYRING_USERNAME)?; - match entry.delete_credential() { - Ok(_) => Ok(()), - Err(keyring::Error::NoEntry) => Ok(()), - Err(e) => Err(ConfigError::KeyringError(e.to_string())), - } - } - #[test] fn test_basic_config() -> Result<(), ConfigError> { let temp_file = NamedTempFile::new().unwrap(); @@ -867,11 +843,12 @@ mod tests { } #[test] - #[serial] fn test_secret_management() -> Result<(), ConfigError> { - cleanup_keyring()?; - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + use crate::keyring::MockKeyringBackend; + + let _temp_file = NamedTempFile::new().unwrap(); + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring); // Test setting and getting a simple secret config.set_secret("api_key", Value::String("secret123".to_string()))?; @@ -889,7 +866,6 @@ mod tests { let result: Result = config.get_secret("api_key"); assert!(matches!(result, Err(ConfigError::NotFound(_)))); - cleanup_keyring()?; Ok(()) } @@ -933,11 +909,12 @@ mod tests { } #[test] - #[serial] fn test_multiple_secrets() -> Result<(), ConfigError> { - cleanup_keyring()?; - let temp_file = NamedTempFile::new().unwrap(); - let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; + use crate::keyring::MockKeyringBackend; + + let _temp_file = NamedTempFile::new().unwrap(); + let mock_keyring = Arc::new(MockKeyringBackend::new()); + let config = Config::with_keyring(mock_keyring); // Set multiple secrets config.set_secret("key1", Value::String("secret1".to_string()))?; @@ -958,7 +935,6 @@ mod tests { assert!(matches!(result1, Err(ConfigError::NotFound(_)))); assert_eq!(value2, "secret2"); - cleanup_keyring()?; Ok(()) } @@ -1087,43 +1063,6 @@ mod tests { Ok(()) } - #[test] - fn test_env_var_truthy_values() { - // Test truthy values - for value in [ - "1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True", - ] { - env::set_var("TEST_TRUTHY_VAR", value); - assert!( - is_env_var_truthy("TEST_TRUTHY_VAR"), - "Value '{}' should be truthy", - value - ); - } - - // Test falsy values - for value in [ - "0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random", - ] { - env::set_var("TEST_TRUTHY_VAR", value); - assert!( - !is_env_var_truthy("TEST_TRUTHY_VAR"), - "Value '{}' should be falsy", - value - ); - } - - // Test unset variable - env::remove_var("TEST_TRUTHY_VAR"); - assert!( - !is_env_var_truthy("TEST_TRUTHY_VAR"), - "Unset variable should be falsy" - ); - - // Clean up - env::remove_var("TEST_TRUTHY_VAR"); - } - #[test] fn test_concurrent_writes() -> Result<(), ConfigError> { use std::sync::{Arc, Barrier, Mutex}; diff --git a/crates/goose/src/keyring/factory.rs b/crates/goose/src/keyring/factory.rs new file mode 100644 index 000000000000..545d566c5aca --- /dev/null +++ b/crates/goose/src/keyring/factory.rs @@ -0,0 +1,345 @@ +#[cfg(test)] +use super::KeyringError; +use super::{FileKeyringBackend, KeyringBackend, MockKeyringBackend, SystemKeyringBackend}; +use etcetera::AppStrategy; +use std::path::PathBuf; +use std::sync::Arc; +use std::{env, path::Path}; + +/// Factory for creating keyring backends with environment-based defaults. +/// +/// This factory provides a consistent way to create keyring backends across +/// the entire codebase, handling environment variable detection and providing +/// sensible defaults based on the execution context. +/// +/// # Environment Variable Priority +/// +/// The factory checks environment variables in this order: +/// 1. `GOOSE_USE_MOCK_KEYRING` (highest priority) - Forces mock backend for testing +/// 2. `GOOSE_DISABLE_KEYRING` - Forces file-based backend for production without OS keyring +/// 3. Default behavior - Uses system keyring for production +pub struct KeyringFactory; + +impl KeyringFactory { + /// Create a keyring backend using environment-based defaults. + /// + /// This is the most common use case - let the factory decide which backend + /// to use based on environment variables and execution context. + /// + /// # Examples + /// + /// ```rust,no_run + /// use goose::keyring::KeyringFactory; + /// + /// let keyring = KeyringFactory::create_default(); + /// keyring.set_password("service", "user", "password").unwrap(); + /// ``` + pub fn create_default() -> Arc { + Self::create_with_config(DefaultKeyringConfig::new()) + } + + /// Create a keyring backend with custom configuration. + /// + /// This allows overriding default behavior such as specifying a custom + /// file path for the file-based backend. + /// + /// # Examples + /// + /// ```rust,no_run + /// use goose::keyring::{KeyringFactory, DefaultKeyringConfig}; + /// use std::path::PathBuf; + /// + /// let config = DefaultKeyringConfig::new() + /// .with_file_path(PathBuf::from("/custom/path/secrets.yaml")); + /// let keyring = KeyringFactory::create_with_config(config); + /// ``` + pub fn create_with_config(config: DefaultKeyringConfig) -> Arc { + // Priority 1: Mock keyring for testing + if Self::is_env_var_truthy("GOOSE_USE_MOCK_KEYRING") { + return Arc::new(MockKeyringBackend::new()); + } + + // Priority 2: File-based keyring when system keyring disabled + if Self::is_env_var_truthy("GOOSE_DISABLE_KEYRING") { + let file_path = config + .file_path + .unwrap_or_else(|| Self::default_config_dir().join("secrets.yaml")); + return Arc::new(FileKeyringBackend::new(file_path)); + } + + // Priority 3: System keyring (default) + Arc::new(SystemKeyringBackend) + } + + /// Check if an environment variable is set to a truthy value. + /// + /// This matches the same logic used throughout the goose codebase for + /// consistency in environment variable interpretation. + /// + /// Truthy values: "1", "true", "yes", "on" (case insensitive) + /// Falsy values: "0", "false", "no", "off", "" or unset + fn is_env_var_truthy(var_name: &str) -> bool { + match env::var(var_name) { + Ok(value) => { + let normalized = value.trim().to_lowercase(); + matches!(normalized.as_str(), "1" | "true" | "yes" | "on") + } + Err(_) => false, + } + } + + /// Get the default configuration directory using the same logic as Config. + /// + /// This ensures consistency with the rest of the goose configuration system. + fn default_config_dir() -> PathBuf { + use etcetera::{choose_app_strategy, AppStrategyArgs}; + + let strategy = AppStrategyArgs { + top_level_domain: "Block".to_string(), + author: "Block".to_string(), + app_name: "goose".to_string(), + }; + + choose_app_strategy(strategy) + .expect("goose requires a home dir") + .config_dir() + } +} + +/// Configuration options for keyring factory creation. +/// +/// This struct allows customizing the behavior of the keyring factory +/// without breaking the simple default use case. +#[derive(Default)] +pub struct DefaultKeyringConfig { + /// Optional custom file path for FileKeyringBackend. + /// + /// If not provided, defaults to `{config_dir}/secrets.yaml` + pub file_path: Option, +} + +impl DefaultKeyringConfig { + /// Create a new configuration with default values. + pub fn new() -> Self { + Default::default() + } + + /// Set a custom file path for the file-based keyring backend. + /// + /// This path will be used when `GOOSE_DISABLE_KEYRING` is set but + /// `GOOSE_USE_MOCK_KEYRING` is not set. + pub fn with_file_path>(mut self, path: P) -> Self { + self.file_path = Some(path.as_ref().to_path_buf()); + self + } +} + +#[cfg(test)] +mod tests { + use super::*; + use serial_test::serial; + use std::env; + use tempfile::tempdir; + + /// Test utility for managing environment variables in tests. + /// + /// This ensures that environment variables are properly restored + /// after each test, preventing test interference. + struct EnvGuard { + key: String, + original_value: Option, + } + + impl EnvGuard { + fn set(key: &str, value: &str) -> Self { + let original_value = env::var(key).ok(); + env::set_var(key, value); + EnvGuard { + key: key.to_string(), + original_value, + } + } + + fn remove(key: &str) -> Self { + let original_value = env::var(key).ok(); + env::remove_var(key); + EnvGuard { + key: key.to_string(), + original_value, + } + } + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.original_value { + Some(value) => env::set_var(&self.key, value), + None => env::remove_var(&self.key), + } + } + } + + #[test] + #[serial] + fn test_mock_keyring_priority() { + let _guard1 = EnvGuard::set("GOOSE_USE_MOCK_KEYRING", "true"); + let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true"); + + let keyring = KeyringFactory::create_default(); + + // Should use MockKeyringBackend despite GOOSE_DISABLE_KEYRING being set + // We can verify this by checking that a non-existent key returns NotFound + match keyring.get_password("test_service", "test_user") { + Err(e) => { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } + } + Ok(_) => panic!("Mock keyring should return NotFound for non-existent keys"), + } + } + + #[test] + #[serial] + fn test_file_keyring_when_disabled() { + let _guard1 = EnvGuard::remove("GOOSE_USE_MOCK_KEYRING"); + let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true"); + + let temp_dir = tempdir().unwrap(); + let keyring = KeyringFactory::create_with_config( + DefaultKeyringConfig::new().with_file_path(temp_dir.path().join("test_secrets.yaml")), + ); + + // Use unique service/user names to avoid conflicts + let service = format!("service_{}", std::process::id()); + let user = format!("user_{}", std::process::id()); + + // Verify it creates a FileKeyringBackend by testing file operations + keyring.set_password(&service, &user, "password").unwrap(); + assert_eq!(keyring.get_password(&service, &user).unwrap(), "password"); + + // Verify the file was actually created + assert!(temp_dir.path().join("test_secrets.yaml").exists()); + } + + #[test] + #[serial] + fn test_system_keyring_default() { + // For this test, we'll use mock keyring to avoid OS keyring popups + // but verify the logic by ensuring that when neither override is set, + // the factory doesn't choose the file backend + let _guard1 = EnvGuard::set("GOOSE_USE_MOCK_KEYRING", "true"); + let _guard2 = EnvGuard::remove("GOOSE_DISABLE_KEYRING"); + + let keyring = KeyringFactory::create_default(); + + // Verify it creates a MockKeyringBackend (since we forced it for testing) + // The main thing we're testing is that the logic flow is correct + match keyring.get_password("non_existent_service", "non_existent_user") { + Err(e) => { + if let Some(keyring_err) = e.downcast_ref::() { + assert!(matches!(keyring_err, KeyringError::NotFound { .. })); + } + } + Ok(_) => panic!("Mock keyring should return NotFound for non-existent keys"), + } + } + + #[test] + #[serial] + fn test_default_config_with_custom_path() { + let _guard1 = EnvGuard::remove("GOOSE_USE_MOCK_KEYRING"); + let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true"); + + let temp_dir = tempdir().unwrap(); + let custom_path = temp_dir.path().join("custom_secrets.yaml"); + + let config = DefaultKeyringConfig::new().with_file_path(&custom_path); + let keyring = KeyringFactory::create_with_config(config); + + // Use unique service/user names to avoid conflicts + let service = format!("test_service_{}", std::process::id()); + let user = format!("test_user_{}", std::process::id()); + + // Test that the custom path is used + keyring + .set_password(&service, &user, "test_password") + .unwrap(); + + // Verify the custom file was created + assert!(custom_path.exists()); + + // Verify we can retrieve the password + assert_eq!( + keyring.get_password(&service, &user).unwrap(), + "test_password" + ); + } + + #[test] + #[serial] + fn test_env_var_truthy_values() { + // Test truthy values + for value in [ + "1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True", + ] { + let _guard = EnvGuard::set("TEST_TRUTHY_VAR", value); + assert!( + KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"), + "Value '{}' should be truthy", + value + ); + } + + // Test falsy values + for value in [ + "0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random", + ] { + let _guard = EnvGuard::set("TEST_TRUTHY_VAR", value); + assert!( + !KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"), + "Value '{}' should be falsy", + value + ); + } + + // Test unset variable + let _guard = EnvGuard::remove("TEST_TRUTHY_VAR"); + assert!( + !KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"), + "Unset variable should be falsy" + ); + } + + #[test] + #[serial] + fn test_factory_consistency_with_existing_is_env_var_truthy() { + // This test ensures our factory's is_env_var_truthy matches the existing implementation + // We'll test a few key values to ensure consistency + + let test_cases = [ + ("1", true), + ("true", true), + ("TRUE", true), + ("yes", true), + ("on", true), + ("0", false), + ("false", false), + ("no", false), + ("off", false), + ("", false), + ("random", false), + ]; + + for (value, expected) in test_cases { + let _guard = EnvGuard::set("TEST_CONSISTENCY_VAR", value); + assert_eq!( + KeyringFactory::is_env_var_truthy("TEST_CONSISTENCY_VAR"), + expected, + "Value '{}' should be {}", + value, + if expected { "truthy" } else { "falsy" } + ); + } + } +} diff --git a/crates/goose/src/keyring/mod.rs b/crates/goose/src/keyring/mod.rs index e21d1274cdc8..5a6117691301 100644 --- a/crates/goose/src/keyring/mod.rs +++ b/crates/goose/src/keyring/mod.rs @@ -16,10 +16,51 @@ pub enum KeyringError { Backend(String), } +pub mod factory; pub mod file; pub mod mock; pub mod system; +pub use factory::{DefaultKeyringConfig, KeyringFactory}; pub use file::FileKeyringBackend; pub use mock::MockKeyringBackend; pub use system::SystemKeyringBackend; + +/// Convenience function for creating a keyring backend with environment-based defaults. +/// +/// This is the most common use case - it automatically selects the appropriate +/// keyring backend based on environment variables: +/// - `GOOSE_USE_MOCK_KEYRING=true` → MockKeyringBackend (for testing) +/// - `GOOSE_DISABLE_KEYRING=true` → FileKeyringBackend (for systems without keyring) +/// - Default → SystemKeyringBackend (for normal operation) +/// +/// # Examples +/// +/// ```rust,no_run +/// use goose::keyring::create_default_keyring; +/// +/// let keyring = create_default_keyring(); +/// keyring.set_password("service", "user", "password").unwrap(); +/// ``` +pub fn create_default_keyring() -> std::sync::Arc { + KeyringFactory::create_default() +} + +/// Convenience function for creating a keyring backend with a custom file path. +/// +/// This is useful when you need to store secrets in a specific location +/// while still respecting the environment variable hierarchy. +/// +/// # Examples +/// +/// ```rust,no_run +/// use goose::keyring::create_keyring_with_file_path; +/// use std::path::PathBuf; +/// +/// let keyring = create_keyring_with_file_path(PathBuf::from("/custom/secrets.yaml")); +/// ``` +pub fn create_keyring_with_file_path( + file_path: std::path::PathBuf, +) -> std::sync::Arc { + KeyringFactory::create_with_config(DefaultKeyringConfig::new().with_file_path(file_path)) +}