From 4232b4c07032c5535d08683ce35316513003152f Mon Sep 17 00:00:00 2001 From: Greg Harper Date: Tue, 17 Mar 2026 11:52:39 -0700 Subject: [PATCH] Simplify macOS keyring store helpers --- codex-rs/keyring-store/src/lib.rs | 163 +++++++++++++++------------- codex-rs/keyring-store/src/macos.rs | 18 --- 2 files changed, 86 insertions(+), 95 deletions(-) diff --git a/codex-rs/keyring-store/src/lib.rs b/codex-rs/keyring-store/src/lib.rs index a27b5a26da..402347791c 100644 --- a/codex-rs/keyring-store/src/lib.rs +++ b/codex-rs/keyring-store/src/lib.rs @@ -47,6 +47,89 @@ impl fmt::Display for CredentialStoreError { impl Error for CredentialStoreError {} +#[cfg(target_os = "macos")] +fn load_password(service: &str, account: &str) -> Result, CredentialStoreError> { + macos::load_password(service, account).map_err(|error| { + trace!( + "keyring.load macos native error, service={service}, account={account}, error={error}" + ); + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + }) +} + +#[cfg(not(target_os = "macos"))] +fn load_password(service: &str, account: &str) -> Result, CredentialStoreError> { + let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; + match entry.get_password() { + Ok(password) => { + trace!("keyring.load success, service={service}, account={account}"); + Ok(Some(password)) + } + Err(keyring::Error::NoEntry) => { + trace!("keyring.load no entry, service={service}, account={account}"); + Ok(None) + } + Err(error) => { + trace!("keyring.load error, service={service}, account={account}, error={error}"); + Err(CredentialStoreError::new(error)) + } + } +} + +#[cfg(target_os = "macos")] +fn save_password(service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { + macos::save_password(service, account, value).map_err(|error| { + trace!( + "keyring.save macos native error, service={service}, account={account}, error={error}" + ); + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + }) +} + +#[cfg(not(target_os = "macos"))] +fn save_password(service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { + let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; + match entry.set_password(value) { + Ok(()) => { + trace!("keyring.save success, service={service}, account={account}"); + Ok(()) + } + Err(error) => { + trace!("keyring.save error, service={service}, account={account}, error={error}"); + Err(CredentialStoreError::new(error)) + } + } +} + +#[cfg(target_os = "macos")] +fn delete_password(service: &str, account: &str) -> Result { + macos::delete_password(service, account).map_err(|error| { + trace!( + "keyring.delete macos native error, service={service}, account={account}, error={error}" + ); + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + }) +} + +#[cfg(not(target_os = "macos"))] +fn delete_password(service: &str, account: &str) -> Result { + let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; + match entry.delete_credential() { + Ok(()) => { + trace!("keyring.delete success, service={service}, account={account}"); + Ok(true) + } + Err(keyring::Error::NoEntry) => { + trace!("keyring.delete no entry, service={service}, account={account}"); + Ok(false) + } + Err(error) => { + trace!("keyring.delete error, service={service}, account={account}, error={error}"); + Err(CredentialStoreError::new(error)) + } + } +} + /// Shared credential store abstraction for keyring-backed implementations. pub trait KeyringStore: Debug + Send + Sync { fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError>; @@ -60,33 +143,7 @@ pub struct DefaultKeyringStore; impl KeyringStore for DefaultKeyringStore { fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { trace!("keyring.load start, service={service}, account={account}"); - #[cfg(target_os = "macos")] - { - macos::load_password(service, account).map_err(|error| { - trace!( - "keyring.load macos native error, service={service}, account={account}, error={error}" - ); - CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) - }) - } - - #[cfg(not(target_os = "macos"))] - let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; - #[cfg(not(target_os = "macos"))] - match entry.get_password() { - Ok(password) => { - trace!("keyring.load success, service={service}, account={account}"); - Ok(Some(password)) - } - Err(keyring::Error::NoEntry) => { - trace!("keyring.load no entry, service={service}, account={account}"); - Ok(None) - } - Err(error) => { - trace!("keyring.load error, service={service}, account={account}, error={error}"); - Err(CredentialStoreError::new(error)) - } - } + load_password(service, account) } fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { @@ -94,60 +151,12 @@ impl KeyringStore for DefaultKeyringStore { "keyring.save start, service={service}, account={account}, value_len={}", value.len() ); - #[cfg(target_os = "macos")] - { - macos::save_password(service, account, value).map_err(|error| { - trace!( - "keyring.save macos native error, service={service}, account={account}, error={error}" - ); - CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) - }) - } - - #[cfg(not(target_os = "macos"))] - let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; - #[cfg(not(target_os = "macos"))] - match entry.set_password(value) { - Ok(()) => { - trace!("keyring.save success, service={service}, account={account}"); - Ok(()) - } - Err(error) => { - trace!("keyring.save error, service={service}, account={account}, error={error}"); - Err(CredentialStoreError::new(error)) - } - } + save_password(service, account, value) } fn delete(&self, service: &str, account: &str) -> Result { trace!("keyring.delete start, service={service}, account={account}"); - #[cfg(target_os = "macos")] - { - macos::delete_password(service, account).map_err(|error| { - trace!( - "keyring.delete macos native error, service={service}, account={account}, error={error}" - ); - CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) - }) - } - - #[cfg(not(target_os = "macos"))] - let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; - #[cfg(not(target_os = "macos"))] - match entry.delete_credential() { - Ok(()) => { - trace!("keyring.delete success, service={service}, account={account}"); - Ok(true) - } - Err(keyring::Error::NoEntry) => { - trace!("keyring.delete no entry, service={service}, account={account}"); - Ok(false) - } - Err(error) => { - trace!("keyring.delete error, service={service}, account={account}, error={error}"); - Err(CredentialStoreError::new(error)) - } - } + delete_password(service, account) } } diff --git a/codex-rs/keyring-store/src/macos.rs b/codex-rs/keyring-store/src/macos.rs index 06096cfb89..671ef817e6 100644 --- a/codex-rs/keyring-store/src/macos.rs +++ b/codex-rs/keyring-store/src/macos.rs @@ -396,22 +396,4 @@ mod tests { ); assert_eq!(store.shared_writes.get(), 1); } - - #[test] - fn delete_removes_both_shared_and_legacy_passwords() { - let store = MockPasswordStore::default() - .with_shared_password("shared-value") - .with_legacy_password("legacy-value"); - - let deleted = delete_password_with_store(&store, "Codex Auth", "test-account").unwrap(); - - assert!(deleted); - assert_eq!(store.shared.borrow().clone(), None); - assert_eq!(store.legacy.borrow().clone(), None); - } - - #[test] - fn shared_access_group_matches_configured_team_prefix() { - assert_eq!(SHARED_ACCESS_GROUP, "2DC432GLL2.com.openai.codex.shared"); - } }