diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index b1e04b8685..4dd799e4b4 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -23,6 +23,9 @@ use crate::token_data::TokenData; use codex_app_server_protocol::AuthMode; use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; +use codex_keyring_store::delete_json_from_keyring; +use codex_keyring_store::load_json_from_keyring; +use codex_keyring_store::save_json_to_keyring; use once_cell::sync::Lazy; /// Determine where Codex should store CLI auth credentials. @@ -162,62 +165,58 @@ impl KeyringAuthStorage { } } - fn load_from_keyring(&self, key: &str) -> std::io::Result> { - match self.keyring_store.load(KEYRING_SERVICE, key) { - Ok(Some(serialized)) => serde_json::from_str(&serialized).map(Some).map_err(|err| { - std::io::Error::other(format!( - "failed to deserialize CLI auth from keyring: {err}" - )) - }), - Ok(None) => Ok(None), - Err(error) => Err(std::io::Error::other(format!( - "failed to load CLI auth from keyring: {}", - error.message() - ))), - } - } + fn load_auth_from_keyring(&self, base_key: &str) -> std::io::Result> { + let Some(value) = + load_json_from_keyring(self.keyring_store.as_ref(), KEYRING_SERVICE, base_key) + .map_err(|err| { + std::io::Error::other(format!("failed to load CLI auth from keyring: {err}")) + })? + else { + return Ok(None); + }; - fn save_to_keyring(&self, key: &str, value: &str) -> std::io::Result<()> { - match self.keyring_store.save(KEYRING_SERVICE, key, value) { - Ok(()) => Ok(()), - Err(error) => { - let message = format!( - "failed to write OAuth tokens to keyring: {}", - error.message() - ); - warn!("{message}"); - Err(std::io::Error::other(message)) - } - } + serde_json::from_value(value).map(Some).map_err(|err| { + std::io::Error::other(format!( + "failed to deserialize CLI auth from keyring: {err}" + )) + }) } } impl AuthStorageBackend for KeyringAuthStorage { fn load(&self) -> std::io::Result> { let key = compute_store_key(&self.codex_home)?; - self.load_from_keyring(&key) + self.load_auth_from_keyring(&key) } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { - let key = compute_store_key(&self.codex_home)?; - // Simpler error mapping per style: prefer method reference over closure - let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; - self.save_to_keyring(&key, &serialized)?; + let base_key = compute_store_key(&self.codex_home)?; + let value = serde_json::to_value(auth).map_err(std::io::Error::other)?; + + save_json_to_keyring( + self.keyring_store.as_ref(), + KEYRING_SERVICE, + &base_key, + &value, + ) + .map_err(|err| std::io::Error::other(format!("failed to write auth to keyring: {err}")))?; + if let Err(err) = delete_file_if_exists(&self.codex_home) { warn!("failed to remove CLI auth fallback file: {err}"); } + Ok(()) } fn delete(&self) -> std::io::Result { - let key = compute_store_key(&self.codex_home)?; - let keyring_removed = self - .keyring_store - .delete(KEYRING_SERVICE, &key) - .map_err(|err| { - std::io::Error::other(format!("failed to delete auth from keyring: {err}")) - })?; + let base_key = compute_store_key(&self.codex_home)?; + let keyring_removed = + delete_json_from_keyring(self.keyring_store.as_ref(), KEYRING_SERVICE, &base_key) + .map_err(|err| { + std::io::Error::other(format!("failed to delete auth from keyring: {err}")) + })?; let file_removed = delete_file_if_exists(&self.codex_home)?; + Ok(keyring_removed || file_removed) } } diff --git a/codex-rs/login/src/auth/storage_tests.rs b/codex-rs/login/src/auth/storage_tests.rs index 4bf72c11b9..de8199644f 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -6,9 +6,88 @@ use pretty_assertions::assert_eq; use serde_json::json; use tempfile::tempdir; +use codex_keyring_store::CredentialStoreError; use codex_keyring_store::tests::MockKeyringStore; use keyring::Error as KeyringError; +#[derive(Clone, Debug)] +struct SaveSecretErrorKeyringStore { + inner: MockKeyringStore, +} + +impl KeyringStore for SaveSecretErrorKeyringStore { + fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { + self.inner.load(service, account) + } + + fn load_secret( + &self, + service: &str, + account: &str, + ) -> Result>, CredentialStoreError> { + self.inner.load_secret(service, account) + } + + fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { + self.inner.save(service, account, value) + } + + fn save_secret( + &self, + _service: &str, + _account: &str, + _value: &[u8], + ) -> Result<(), CredentialStoreError> { + Err(CredentialStoreError::new(KeyringError::Invalid( + "error".into(), + "save".into(), + ))) + } + + fn delete(&self, service: &str, account: &str) -> Result { + self.inner.delete(service, account) + } +} + +#[derive(Clone, Debug)] +struct LoadSecretErrorKeyringStore { + inner: MockKeyringStore, +} + +impl KeyringStore for LoadSecretErrorKeyringStore { + fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { + self.inner.load(service, account) + } + + fn load_secret( + &self, + _service: &str, + _account: &str, + ) -> Result>, CredentialStoreError> { + Err(CredentialStoreError::new(KeyringError::Invalid( + "error".into(), + "load".into(), + ))) + } + + fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { + self.inner.save(service, account, value) + } + + fn save_secret( + &self, + service: &str, + account: &str, + value: &[u8], + ) -> Result<(), CredentialStoreError> { + self.inner.save_secret(service, account, value) + } + + fn delete(&self, service: &str, account: &str) -> Result { + self.inner.delete(service, account) + } +} + #[tokio::test] async fn file_storage_load_returns_auth_dot_json() -> anyhow::Result<()> { let codex_home = tempdir()?; @@ -97,19 +176,16 @@ fn ephemeral_storage_save_load_delete_is_in_memory_only() -> anyhow::Result<()> Ok(()) } -fn seed_keyring_and_fallback_auth_file_for_delete( - mock_keyring: &MockKeyringStore, +fn seed_keyring_and_fallback_auth_file_for_delete( + storage: &KeyringAuthStorage, codex_home: &Path, - compute_key: F, -) -> anyhow::Result<(String, PathBuf)> -where - F: FnOnce() -> std::io::Result, -{ - let key = compute_key()?; - mock_keyring.save(KEYRING_SERVICE, &key, "{}")?; + auth: &AuthDotJson, +) -> anyhow::Result<(String, PathBuf)> { + storage.save(auth)?; + let base_key = compute_store_key(codex_home)?; let auth_file = get_auth_file(codex_home); std::fs::write(&auth_file, "stale")?; - Ok((key, auth_file)) + Ok((base_key, auth_file)) } fn seed_keyring_with_auth( @@ -128,15 +204,26 @@ where fn assert_keyring_saved_auth_and_removed_fallback( mock_keyring: &MockKeyringStore, - key: &str, + base_key: &str, codex_home: &Path, expected: &AuthDotJson, ) { - let saved_value = mock_keyring - .saved_value(key) - .expect("keyring entry should exist"); - let expected_serialized = serde_json::to_string(expected).expect("serialize expected auth"); - assert_eq!(saved_value, expected_serialized); + let expected_json = serde_json::to_value(expected).expect("auth should serialize"); + let loaded = load_json_from_keyring(mock_keyring, KEYRING_SERVICE, base_key) + .expect("auth should load from keyring") + .expect("auth should exist"); + assert_eq!(loaded, expected_json); + #[cfg(windows)] + assert!( + mock_keyring.saved_secret(base_key).is_none(), + "windows should store auth using split keyring entries" + ); + #[cfg(not(windows))] + assert_eq!( + mock_keyring.saved_secret(base_key), + Some(serde_json::to_vec(&expected_json).expect("auth should serialize")), + "non-windows should store auth as one JSON secret" + ); let auth_file = get_auth_file(codex_home); assert!( !auth_file.exists(), @@ -185,7 +272,7 @@ fn auth_with_prefix(prefix: &str) -> AuthDotJson { } #[test] -fn keyring_auth_storage_load_returns_deserialized_auth() -> anyhow::Result<()> { +fn keyring_auth_storage_load_supports_legacy_single_entry() -> anyhow::Result<()> { let codex_home = tempdir()?; let mock_keyring = MockKeyringStore::default(); let storage = KeyringAuthStorage::new( @@ -204,6 +291,29 @@ fn keyring_auth_storage_load_returns_deserialized_auth() -> anyhow::Result<()> { &expected, )?; + let loaded = storage.load()?; + + #[cfg(not(windows))] + { + assert_eq!(Some(expected), loaded); + } + + #[cfg(windows)] + { + assert_eq!(None, loaded); + } + Ok(()) +} + +#[test] +fn keyring_auth_storage_load_returns_deserialized_keyring_auth() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = KeyringAuthStorage::new(codex_home.path().to_path_buf(), Arc::new(mock_keyring)); + let expected = auth_with_prefix("keyring"); + + storage.save(&expected)?; + let loaded = storage.load()?; assert_eq!(Some(expected), loaded); Ok(()) @@ -256,17 +366,16 @@ fn keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let (key, auth_file) = - seed_keyring_and_fallback_auth_file_for_delete(&mock_keyring, codex_home.path(), || { - compute_store_key(codex_home.path()) - })?; + let auth = auth_with_prefix("delete"); + let (base_key, auth_file) = + seed_keyring_and_fallback_auth_file_for_delete(&storage, codex_home.path(), &auth)?; let removed = storage.delete()?; assert!(removed, "delete should report removal"); assert!( - !mock_keyring.contains(&key), - "keyring entry should be removed" + load_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &base_key)?.is_none(), + "keyring auth should be removed" ); assert!( !auth_file.exists(), @@ -279,16 +388,9 @@ fn keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> fn auto_auth_storage_load_prefers_keyring_value() -> anyhow::Result<()> { let codex_home = tempdir()?; let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - ); + let storage = AutoAuthStorage::new(codex_home.path().to_path_buf(), Arc::new(mock_keyring)); let keyring_auth = auth_with_prefix("keyring"); - seed_keyring_with_auth( - &mock_keyring, - || compute_store_key(codex_home.path()), - &keyring_auth, - )?; + storage.keyring_storage.save(&keyring_auth)?; let file_auth = auth_with_prefix("file"); storage.file_storage.save(&file_auth)?; @@ -316,12 +418,10 @@ fn auto_auth_storage_load_uses_file_when_keyring_empty() -> anyhow::Result<()> { fn auto_auth_storage_load_falls_back_when_keyring_errors() -> anyhow::Result<()> { let codex_home = tempdir()?; let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - ); - let key = compute_store_key(codex_home.path())?; - mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "load".into())); + let failing_keyring = LoadSecretErrorKeyringStore { + inner: mock_keyring, + }; + let storage = AutoAuthStorage::new(codex_home.path().to_path_buf(), Arc::new(failing_keyring)); let expected = auth_with_prefix("fallback"); storage.file_storage.save(&expected)?; @@ -360,12 +460,11 @@ fn auto_auth_storage_save_prefers_keyring() -> anyhow::Result<()> { fn auto_auth_storage_save_falls_back_when_keyring_errors() -> anyhow::Result<()> { let codex_home = tempdir()?; let mock_keyring = MockKeyringStore::default(); - let storage = AutoAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - ); + let failing_keyring = SaveSecretErrorKeyringStore { + inner: mock_keyring.clone(), + }; + let storage = AutoAuthStorage::new(codex_home.path().to_path_buf(), Arc::new(failing_keyring)); let key = compute_store_key(codex_home.path())?; - mock_keyring.set_error(&key, KeyringError::Invalid("error".into(), "save".into())); let auth = auth_with_prefix("fallback"); storage.save(&auth)?; @@ -381,8 +480,8 @@ fn auto_auth_storage_save_falls_back_when_keyring_errors() -> anyhow::Result<()> .context("fallback auth should exist")?; assert_eq!(saved, auth); assert!( - mock_keyring.saved_value(&key).is_none(), - "keyring should not contain value when save fails" + load_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &key)?.is_none(), + "keyring should not point to saved auth when save fails" ); Ok(()) } @@ -395,17 +494,19 @@ fn auto_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { codex_home.path().to_path_buf(), Arc::new(mock_keyring.clone()), ); - let (key, auth_file) = - seed_keyring_and_fallback_auth_file_for_delete(&mock_keyring, codex_home.path(), || { - compute_store_key(codex_home.path()) - })?; + let auth = auth_with_prefix("auto-delete"); + let (base_key, auth_file) = seed_keyring_and_fallback_auth_file_for_delete( + storage.keyring_storage.as_ref(), + codex_home.path(), + &auth, + )?; let removed = storage.delete()?; assert!(removed, "delete should report removal"); assert!( - !mock_keyring.contains(&key), - "keyring entry should be removed" + load_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &base_key)?.is_none(), + "keyring auth should be removed" ); assert!( !auth_file.exists(),