diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index 3709e484f4..edf4384843 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -216,34 +216,47 @@ impl DirectKeyringAuthStorage { } } - fn account(&self) -> std::io::Result { - compute_store_key(&self.codex_home) + 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 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)) + } + } } } impl AuthStorageBackend for DirectKeyringAuthStorage { fn load(&self) -> std::io::Result> { - let account = self.account()?; - match self - .keyring_store - .load(KEYRING_SERVICE, &account) - .map_err(|err| std::io::Error::other(err.message()))? - { - 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}" - )) - }), - None => Ok(None), - } + let key = compute_store_key(&self.codex_home)?; + self.load_from_keyring(&key) } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { - let account = self.account()?; + 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.keyring_store - .save(KEYRING_SERVICE, &account, &serialized) - .map_err(|err| std::io::Error::other(err.message()))?; + self.save_to_keyring(&key, &serialized)?; if let Err(err) = delete_file_if_exists(&self.codex_home) { warn!("failed to remove CLI auth fallback file: {err}"); } @@ -251,11 +264,13 @@ impl AuthStorageBackend for DirectKeyringAuthStorage { } fn delete(&self) -> std::io::Result { - let account = self.account()?; + let key = compute_store_key(&self.codex_home)?; let keyring_removed = self .keyring_store - .delete(KEYRING_SERVICE, &account) - .map_err(|err| std::io::Error::other(err.message()))?; + .delete(KEYRING_SERVICE, &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) } @@ -282,8 +297,11 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { match self .secrets_backend .get(&SecretScope::Global, &CLI_AUTH_SECRET_NAME) - .map_err(std::io::Error::other)? - { + .map_err(|err| { + std::io::Error::other(format!( + "failed to load CLI auth from encrypted auth storage: {err}" + )) + })? { Some(serialized) => serde_json::from_str(&serialized).map(Some).map_err(|err| { std::io::Error::other(format!( "failed to deserialize CLI auth from encrypted auth storage: {err}" @@ -297,7 +315,12 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; self.secrets_backend .set(&SecretScope::Global, &CLI_AUTH_SECRET_NAME, &serialized) - .map_err(std::io::Error::other)?; + .map_err(|err| { + let message = + format!("failed to write OAuth tokens to encrypted auth storage: {err}"); + warn!("{message}"); + std::io::Error::other(message) + })?; if let Err(err) = delete_file_if_exists(&self.codex_home) { warn!("failed to remove CLI auth fallback file: {err}"); } @@ -308,7 +331,11 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { let keyring_removed = self .secrets_backend .delete(&SecretScope::Global, &CLI_AUTH_SECRET_NAME) - .map_err(std::io::Error::other)?; + .map_err(|err| { + std::io::Error::other(format!( + "failed to delete auth from encrypted auth storage: {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 7f269917d7..4df49cd41d 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -374,6 +374,37 @@ fn direct_keyring_auth_storage_saves_legacy_keyring_entry() -> anyhow::Result<() Ok(()) } +#[test] +fn direct_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let storage = DirectKeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + let auth = auth_with_prefix("direct-delete"); + storage.save(&auth)?; + let auth_file = get_auth_file(codex_home.path()); + std::fs::write(&auth_file, "stale")?; + + let removed = storage.delete()?; + + assert!(removed, "delete should report removal"); + assert_eq!(storage.load()?, None, "keyring auth should be removed"); + assert!( + mock_keyring + .saved_value(&compute_store_key(codex_home.path())?) + .is_none(), + "legacy keyring auth entry should be removed" + ); + assert!( + !auth_file.exists(), + "fallback auth.json should be removed after keyring delete" + ); + assert!(!encrypted_auth_file(codex_home.path()).exists()); + Ok(()) +} + #[test] fn factory_uses_secrets_backend_only_when_requested() -> anyhow::Result<()> { let direct_home = tempdir()?; diff --git a/codex-rs/secrets/src/lib.rs b/codex-rs/secrets/src/lib.rs index b8a85ef6ac..280c723d36 100644 --- a/codex-rs/secrets/src/lib.rs +++ b/codex-rs/secrets/src/lib.rs @@ -19,7 +19,7 @@ mod sanitizer; pub use local::LocalSecretsBackend; pub use sanitizer::redact_secrets; -const KEYRING_SERVICE: &str = "Codex Auth"; +const KEYRING_SERVICE: &str = "codex"; #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct SecretName(String);