tkmind/tkmind_go
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

revert: refactor: abstract keyring logic to better enable DI (#3358)

Browse Source
This commit is contained in:
Prem Pillai
2025-07-11 16:58:38 +10:00
committed by GitHub
parent 67664a2a51
commit 626f00e290
14 changed files with 211 additions and 1278 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/goose/src/config/base.rs
+90 -221
View File
@@ -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<AppStrategyArgs> = Lazy::new(|| AppStrategyArgs {
@@ -22,7 +19,6 @@ pub static APP_STRATEGY: Lazy<AppStrategyArgs> = 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<keyring::Error> for ConfigError {
/// For Goose-specific configuration, consider prefixing with "goose_" to avoid conflicts.
pub struct Config {
config_path: PathBuf,
keyring: Arc<dyn KeyringBackend>,
keyring_service: String,
secrets: SecretStorage,
}
enum SecretStorage {
Keyring { service: String },
File { path: PathBuf },
}
// Global instance
@@ -118,19 +118,6 @@ static GLOBAL_CONFIG: OnceCell<Config> = 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<dyn KeyringBackend>) -> 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<P: AsRef<Path>>(config_path: P, service: &str) -> Result<Self, ConfigError> {
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<Self, ConfigError> {
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<HashMap<String, Value>, ConfigError> {
match self
.keyring
.get_password(&self.keyring_service, KEYRING_USERNAME)
{
Ok(content) => {
let values: HashMap<String, Value> = 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<String, Value> = 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::<crate::keyring::KeyringError>() {
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<HashMap<String, Value>, 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,12 +876,11 @@ 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);
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()))?;
@@ -866,55 +898,16 @@ mod tests {
let result: Result<String, ConfigError> = config.get_secret("api_key");
assert!(matches!(result, Err(ConfigError::NotFound(_))));
cleanup_keyring()?;
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<String, ConfigError> = 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> {
use crate::keyring::MockKeyringBackend;
let _temp_file = NamedTempFile::new().unwrap();
let mock_keyring = Arc::new(MockKeyringBackend::new());
let config = Config::with_keyring(mock_keyring);
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()))?;
@@ -935,131 +928,7 @@ mod tests {
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());
// 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<String, ConfigError> = 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};
// Create a failing keyring that returns backend errors
struct FailingKeyring;
impl KeyringBackend for FailingKeyring {
fn get_password(&self, _: &str, _: &str) -> anyhow::Result<String> {
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<String> {
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(())
}