diff --git a/Cargo.lock b/Cargo.lock index cadd579d67ff..9541e0b7dbf9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3629,7 +3629,6 @@ dependencies = [ "google-docs1", "google-drive3", "google-sheets4", - "goose", "http-body-util", "hyper 1.6.0", "ignore", diff --git a/Justfile b/Justfile index 91091d437608..9641cf285d54 100644 --- a/Justfile +++ b/Justfile @@ -177,33 +177,6 @@ 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-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 -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 @@ -382,16 +355,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 @@ -454,8 +427,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 diff --git a/crates/goose-mcp/Cargo.toml b/crates/goose-mcp/Cargo.toml index 6b76d875cb23..b6b77d0182ac 100644 --- a/crates/goose-mcp/Cargo.toml +++ b/crates/goose-mcp/Cargo.toml @@ -13,7 +13,6 @@ 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 cf0cf0f433c6..1f1aeae70ca2 100644 --- a/crates/goose-mcp/src/google_drive/mod.rs +++ b/crates/goose-mcp/src/google_drive/mod.rs @@ -161,16 +161,12 @@ impl GoogleDriveRouter { Err(_) => false, }; - // 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( 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 e65a25200ae5..47708b953259 100644 --- a/crates/goose-mcp/src/google_drive/oauth_pkce.rs +++ b/crates/goose-mcp/src/google_drive/oauth_pkce.rs @@ -193,10 +193,10 @@ impl PkceOAuth2Client { }; // Store updated token data - match self.credentials_manager.write_credentials(&token_data) { - Ok(_) => debug!("Successfully stored token data"), - Err(e) => error!("Failed to store token data: {}", e), - } + self.credentials_manager + .write_credentials(&token_data) + .map(|_| debug!("Successfully stored token data")) + .unwrap_or_else(|e| error!("Failed to store token data: {}", e)); } else { debug!("No refresh token provided in OAuth flow response"); } @@ -248,10 +248,10 @@ impl PkceOAuth2Client { }; // Store updated token data - match self.credentials_manager.write_credentials(&token_data) { - Ok(_) => debug!("Successfully stored token data"), - Err(e) => error!("Failed to store token data: {}", e), - } + self.credentials_manager + .write_credentials(&token_data) + .map(|_| debug!("Successfully stored token data")) + .unwrap_or_else(|e| error!("Failed to store token data: {}", e)); Ok(access_token) } diff --git a/crates/goose-mcp/src/google_drive/storage.rs b/crates/goose-mcp/src/google_drive/storage.rs index 73156b62165f..8e8f3c08dec3 100644 --- a/crates/goose-mcp/src/google_drive/storage.rs +++ b/crates/goose-mcp/src/google_drive/storage.rs @@ -1,17 +1,16 @@ use anyhow::Result; -use goose::keyring::{KeyringBackend, KeyringError}; +use keyring::Entry; 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 keyring: {0}")] - KeyringError(String), + #[error("Failed to access keychain: {0}")] + KeyringError(#[from] keyring::Error), #[error("Failed to access file system: {0}")] FileSystemError(#[from] std::io::Error), #[error("No credentials found")] @@ -22,15 +21,6 @@ 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. @@ -39,7 +29,6 @@ pub struct CredentialsManager { fallback_to_disk: bool, keychain_service: String, keychain_username: String, - keyring: Arc, } impl CredentialsManager { @@ -48,14 +37,12 @@ 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, } } @@ -66,19 +53,17 @@ impl CredentialsManager { /// /// # Type Parameters /// - /// * `T` - The type to deserialize to. Must implement `serde::DeserializeOwned`. + /// * `T` - The type to deserialize the credentials into. Must implement `serde::de::DeserializeOwned`. /// /// # Returns /// - /// * `Ok(T)` - The deserialized data + /// * `Ok(T)` - The deserialized credentials /// * `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)] @@ -88,45 +73,34 @@ impl CredentialsManager { /// expiry: u64, /// } /// - /// # fn example() -> Result<(), Box> { - /// let keyring = Arc::new(SystemKeyringBackend); /// 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"), - /// keyring + /// String::from("test_user") /// ); /// match manager.read_credentials::() { - /// Ok(token) => println!("Access token: {}", token.access_token), + /// Ok(token) => println!("Token expires at: {}", token.expiry), /// Err(e) => eprintln!("Failed to read token: {}", e), /// } - /// # Ok(()) - /// # } /// ``` pub fn read_credentials(&self) -> Result where T: DeserializeOwned, { - let json_str = self - .keyring - .get_password(&self.keychain_service, &self.keychain_username) + let json_str = Entry::new(&self.keychain_service, &self.keychain_username) + .and_then(|entry| entry.get_password()) .inspect(|_| { debug!("Successfully read credentials from keychain"); }) .or_else(|e| { if self.fallback_to_disk { - warn!("Falling back to file system due to keyring error: {}", e); + debug!("Falling back to file system due to keyring error: {}", e); self.read_from_file() } else { - // 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())) + match e { + keyring::Error::NoEntry => Err(StorageError::NotFound), + _ => Err(StorageError::KeyringError(e)), } } })?; @@ -175,8 +149,6 @@ 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)] @@ -186,26 +158,21 @@ impl CredentialsManager { /// expiry: u64, /// } /// - /// # 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); /// 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"), - /// keyring + /// String::from("test_user") /// ); /// if let Err(e) = manager.write_credentials(&token) { /// eprintln!("Failed to write token: {}", e); /// } - /// # Ok(()) - /// # } /// ``` pub fn write_credentials(&self, content: &T) -> Result<(), StorageError> where @@ -213,8 +180,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) + Entry::new(&self.keychain_service, &self.keychain_username) + .and_then(|entry| entry.set_password(&json_str)) .inspect(|_| { debug!("Successfully wrote credentials to keychain"); }) @@ -223,14 +190,13 @@ impl CredentialsManager { warn!("Falling back to file system due to keyring error: {}", e); self.write_to_file(&json_str) } else { - Err(StorageError::KeyringError(e.to_string())) + Err(StorageError::KeyringError(e)) } }) } 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) { @@ -263,7 +229,6 @@ 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(), } } } @@ -271,7 +236,6 @@ impl Clone for CredentialsManager { #[cfg(test)] mod tests { use super::*; - use goose::keyring::MockKeyringBackend; use serde::{Deserialize, Serialize}; use tempfile::tempdir; @@ -299,31 +263,25 @@ mod tests { 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 succeed with mock keyring + // Write should write to keychain let write_result = manager.write_credentials(&creds); - assert!( - write_result.is_ok(), - "Write should succeed with mock keyring" - ); + assert!(write_result.is_ok(), "Write should succeed with fallback"); - // Read 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 mock keyring"); + assert!(read_result.is_ok(), "Read should succeed with fallback"); // Verify the read credentials match what we wrote assert_eq!( @@ -340,29 +298,21 @@ mod tests { 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 since mock keyring is empty and no fallback + // Read should fail with NotFound or KeyringError depending on the system let read_result = manager.read_credentials::(); 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] @@ -377,13 +327,11 @@ mod tests { 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 diff --git a/crates/goose/src/config/base.rs b/crates/goose/src/config/base.rs index 143d28453421..eabdaf4de6c8 100644 --- a/crates/goose/src/config/base.rs +++ b/crates/goose/src/config/base.rs @@ -1,8 +1,6 @@ -use crate::keyring::{ - create_default_keyring, create_keyring_with_file_path, FileKeyringBackend, KeyringBackend, -}; 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; @@ -11,7 +9,6 @@ use std::env; use std::fs::OpenOptions; use std::io::Write; use std::path::{Path, PathBuf}; -use std::sync::Arc; use thiserror::Error; pub static APP_STRATEGY: Lazy = Lazy::new(|| AppStrategyArgs { @@ -22,7 +19,6 @@ 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"; @@ -109,8 +105,12 @@ impl From for ConfigError { /// For Goose-specific configuration, consider prefixing with "goose_" to avoid conflicts. pub struct Config { config_path: PathBuf, - keyring: Arc, - keyring_service: String, + secrets: SecretStorage, +} + +enum SecretStorage { + Keyring { service: String }, + File { path: PathBuf }, } // Global instance @@ -118,19 +118,6 @@ static GLOBAL_CONFIG: OnceCell = OnceCell::new(); impl Default for Config { fn default() -> Self { - let config_dir = choose_app_strategy(APP_STRATEGY.clone()) - .expect("goose requires a home dir") - .config_dir(); - - // 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) - } -} - -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\ @@ -142,13 +129,22 @@ 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, - keyring, - keyring_service: KEYRING_SERVICE.to_string(), + secrets, } } +} +impl Config { /// Get the global configuration instance. /// /// This will initialize the configuration with the default path (~/.config/goose/config.yaml) @@ -164,8 +160,9 @@ impl Config { pub fn new>(config_path: P, service: &str) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - keyring: create_default_keyring(), - keyring_service: service.to_string(), + secrets: SecretStorage::Keyring { + service: service.to_string(), + }, }) } @@ -179,8 +176,9 @@ impl Config { ) -> Result { Ok(Config { config_path: config_path.as_ref().to_path_buf(), - keyring: Arc::new(FileKeyringBackend::new(secrets_path.as_ref().to_path_buf())), - keyring_service: KEYRING_SERVICE.to_string(), + secrets: SecretStorage::File { + path: secrets_path.as_ref().to_path_buf(), + }, }) } @@ -478,23 +476,32 @@ impl Config { Ok(()) } + // Load current secrets from the keyring pub fn load_secrets(&self) -> Result, ConfigError> { - match self - .keyring - .get_password(&self.keyring_service, KEYRING_USERNAME) - { - Ok(content) => { - let values: HashMap = serde_json::from_str(&content)?; - Ok(values) + match &self.secrets { + SecretStorage::Keyring { service } => { + let entry = Entry::new(service, KEYRING_USERNAME)?; + + match entry.get_password() { + 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) => { - if let Some(keyring_err) = e.downcast_ref::() { - match keyring_err { - crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()), - _ => Err(ConfigError::KeyringError(e.to_string())), + 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()), } } else { - Err(ConfigError::KeyringError(e.to_string())) + Ok(HashMap::new()) } } } @@ -646,10 +653,18 @@ impl Config { pub fn set_secret(&self, key: &str, value: Value) -> Result<(), ConfigError> { let mut values = self.load_secrets()?; values.insert(key.to_string(), 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()))?; + + 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)?; + } + SecretStorage::File { path } => { + let yaml_value = serde_yaml::to_string(&values)?; + std::fs::write(path, yaml_value)?; + } + }; Ok(()) } @@ -666,10 +681,18 @@ impl Config { pub fn delete_secret(&self, key: &str) -> Result<(), ConfigError> { let mut values = self.load_secrets()?; values.remove(key); - 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()))?; + + 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)?; + } + SecretStorage::File { path } => { + let yaml_value = serde_yaml::to_string(&values)?; + std::fs::write(path, yaml_value)?; + } + }; Ok(()) } } @@ -731,8 +754,18 @@ 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 = 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(); @@ -843,51 +876,17 @@ mod tests { } #[test] + #[serial] fn test_secret_management() -> Result<(), ConfigError> { - 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()))?; - let value: String = config.get_secret("api_key")?; - assert_eq!(value, "secret123"); - - // 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(_)))); - - 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()); + cleanup_keyring()?; + let temp_file = NamedTempFile::new().unwrap(); + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; // 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")?; @@ -899,55 +898,16 @@ mod tests { 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"), - } - + cleanup_keyring()?; Ok(()) } #[test] + #[serial] fn test_multiple_secrets() -> Result<(), ConfigError> { - 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()))?; - 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"); - - 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()); + cleanup_keyring()?; + let temp_file = NamedTempFile::new().unwrap(); + let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?; // Set multiple secrets config.set_secret("key1", Value::String("secret1".to_string()))?; @@ -968,98 +928,7 @@ mod tests { 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}; - - // Create a failing keyring that returns backend errors - struct FailingKeyring; - - impl KeyringBackend for FailingKeyring { - fn get_password(&self, _: &str, _: &str) -> anyhow::Result { - Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) - } - fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { - Err(KeyringError::Backend("Keyring service unavailable".to_string()).into()) - } - 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}; - - // Create a keyring that always returns NotFound - struct NotFoundKeyring; - - impl KeyringBackend for NotFoundKeyring { - fn get_password(&self, service: &str, username: &str) -> anyhow::Result { - Err(KeyringError::NotFound { - service: service.to_string(), - username: username.to_string(), - } - .into()) - } - fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> { - Ok(()) - } - 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"), - } - + cleanup_keyring()?; Ok(()) } diff --git a/crates/goose/src/keyring/factory.rs b/crates/goose/src/keyring/factory.rs deleted file mode 100644 index 545d566c5aca..000000000000 --- a/crates/goose/src/keyring/factory.rs +++ /dev/null @@ -1,345 +0,0 @@ -#[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/file.rs b/crates/goose/src/keyring/file.rs deleted file mode 100644 index e6a80069cd27..000000000000 --- a/crates/goose/src/keyring/file.rs +++ /dev/null @@ -1,306 +0,0 @@ -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(); - - // 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.clone(), string_value.to_string()); - } else { - result.insert(key.clone(), 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(()) - } - - #[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(()) - } -} diff --git a/crates/goose/src/keyring/mock.rs b/crates/goose/src/keyring/mock.rs deleted file mode 100644 index 3792ea57f51e..000000000000 --- a/crates/goose/src/keyring/mock.rs +++ /dev/null @@ -1,71 +0,0 @@ -use super::{KeyringBackend, KeyringError}; -use anyhow::Result; -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) - } -} - -impl KeyringBackend for MockKeyringBackend { - 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) - } - - 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(()) - } - - 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 deleted file mode 100644 index 5a6117691301..000000000000 --- a/crates/goose/src/keyring/mod.rs +++ /dev/null @@ -1,66 +0,0 @@ -use anyhow::Result; - -pub trait KeyringBackend: Send + Sync { - 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}'")] - NotFound { service: String, username: String }, - #[error("Access denied to keyring")] - AccessDenied, - #[error("Keyring backend error: {0}")] - 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)) -} diff --git a/crates/goose/src/keyring/system.rs b/crates/goose/src/keyring/system.rs deleted file mode 100644 index 4274bc58962a..000000000000 --- a/crates/goose/src/keyring/system.rs +++ /dev/null @@ -1,44 +0,0 @@ -use super::{KeyringBackend, KeyringError}; -use anyhow::Result; -use keyring::Entry; - -pub struct SystemKeyringBackend; - -impl KeyringBackend for SystemKeyringBackend { - 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) - } - - 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) - } - - 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()))), - } - } -} diff --git a/crates/goose/src/lib.rs b/crates/goose/src/lib.rs index 597e5f21b4da..83c4934d76fa 100644 --- a/crates/goose/src/lib.rs +++ b/crates/goose/src/lib.rs @@ -1,7 +1,6 @@ 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 e0a48ad44ed7..6c6f0f9b605c 100644 --- a/crates/goose/src/providers/factory.rs +++ b/crates/goose/src/providers/factory.rs @@ -180,39 +180,6 @@ 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 { @@ -263,17 +230,10 @@ mod tests { #[test] fn test_create_lead_worker_provider() { - 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"); - env::set_var("ANTHROPIC_API_KEY", "fake-test-key"); + // 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(); // Test with basic lead model configuration env::set_var("GOOSE_LEAD_MODEL", "gpt-4o"); @@ -281,21 +241,16 @@ mod tests { // This will try to create a lead/worker provider let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // 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 + // The creation might succeed or fail depending on API keys, but we can verify the logic path match result { Ok(_) => { - // If it succeeds with fake keys, the logic worked + // If it succeeds, it means we created a lead/worker provider successfully + // This would happen if API keys are available in the test environment } Err(error) => { - // 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 - ); + // 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")); } } @@ -304,27 +259,44 @@ mod tests { env::set_var("GOOSE_LEAD_TURNS", "5"); let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // Similar validation as above - confirms the logic path + // Similar validation as above - will fail due to missing API keys but confirms the logic - // EnvVarGuard will automatically restore all env vars when dropped + // 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"), + } } #[test] fn test_lead_model_env_vars_with_defaults() { - let guard = EnvVarGuard::new(&[ - "GOOSE_LEAD_MODEL", - "GOOSE_LEAD_PROVIDER", - "GOOSE_LEAD_TURNS", - "GOOSE_LEAD_FAILURE_THRESHOLD", - "GOOSE_LEAD_FALLBACK_TURNS", - "OPENAI_API_KEY", - ]); + // 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()), + ( + "GOOSE_LEAD_FAILURE_THRESHOLD", + env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(), + ), + ( + "GOOSE_LEAD_FALLBACK_TURNS", + env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(), + ), + ]; // Clear all lead env vars - guard.clear_all(); - - // Set fake API key to avoid keychain access - env::set_var("OPENAI_API_KEY", "fake-test-key"); + for (key, _) in &saved_vars { + env::remove_var(key); + } // Set only the required lead model env::set_var("GOOSE_LEAD_MODEL", "grok-3"); @@ -332,21 +304,15 @@ mod tests { // 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 with fake keys + // Should attempt to create lead/worker provider (will fail due to missing API keys but confirms logic) match result { Ok(_) => { - // Success means the logic path worked + // Success means we have API keys and created the provider } Err(error) => { - // 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") - ); + // 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")); } } @@ -358,47 +324,63 @@ mod tests { let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); // Should still attempt to create lead/worker provider with custom settings - // EnvVarGuard will automatically restore all env vars when dropped + // 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_create_regular_provider_without_lead_config() { - let guard = EnvVarGuard::new(&[ - "GOOSE_LEAD_MODEL", - "GOOSE_LEAD_PROVIDER", - "GOOSE_LEAD_TURNS", - "GOOSE_LEAD_FAILURE_THRESHOLD", - "GOOSE_LEAD_FALLBACK_TURNS", - "OPENAI_API_KEY", - ]); + // 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(); // Ensure all GOOSE_LEAD_* variables are not set - guard.clear_all(); - - // Set fake API key to avoid keychain access - env::set_var("OPENAI_API_KEY", "fake-test-key"); + 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"); // This should try to create a regular provider let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string())); - // The creation should proceed with fake API key + // The creation might succeed or fail depending on API keys 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) => { - // 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") - ); + // 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")); } } - // EnvVarGuard will automatically restore all env vars when dropped + // 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); + } } #[test]