From 482e2d028785a26f68989ec6f917224607d6dfbb Mon Sep 17 00:00:00 2001 From: mikhail-oai Date: Tue, 10 Mar 2026 15:50:14 -0400 Subject: [PATCH] scope json breakout to windows only --- codex-rs/keyring-store/src/lib.rs | 4 + codex-rs/keyring-store/src/split_json.rs | 214 +++++++++++++++++++++++ codex-rs/login/src/auth/storage.rs | 62 ++----- codex-rs/login/src/auth/storage_tests.rs | 57 ++++-- codex-rs/rmcp-client/src/oauth.rs | 103 ++++++----- 5 files changed, 326 insertions(+), 114 deletions(-) diff --git a/codex-rs/keyring-store/src/lib.rs b/codex-rs/keyring-store/src/lib.rs index 3cd55a50ca..a635ab230e 100644 --- a/codex-rs/keyring-store/src/lib.rs +++ b/codex-rs/keyring-store/src/lib.rs @@ -7,9 +7,13 @@ use tracing::trace; mod split_json; +pub use split_json::JsonKeyringError; pub use split_json::SplitJsonKeyringError; +pub use split_json::delete_json_from_keyring; pub use split_json::delete_split_json_from_keyring; +pub use split_json::load_json_from_keyring; pub use split_json::load_split_json_from_keyring; +pub use split_json::save_json_to_keyring; pub use split_json::save_split_json_to_keyring; #[derive(Debug)] diff --git a/codex-rs/keyring-store/src/split_json.rs b/codex-rs/keyring-store/src/split_json.rs index 282aa57297..75ac219b79 100644 --- a/codex-rs/keyring-store/src/split_json.rs +++ b/codex-rs/keyring-store/src/split_json.rs @@ -22,6 +22,8 @@ pub struct SplitJsonKeyringError { message: String, } +pub type JsonKeyringError = SplitJsonKeyringError; + impl SplitJsonKeyringError { fn new(message: impl Into) -> Self { Self { @@ -87,6 +89,80 @@ struct SplitJsonManifest { type SplitJsonLeafValues = Vec<(String, Vec)>; +#[cfg(windows)] +pub fn load_json_from_keyring( + keyring_store: &K, + service: &str, + base_key: &str, +) -> Result, JsonKeyringError> { + if let Some(value) = load_split_json_from_keyring(keyring_store, service, base_key)? { + return Ok(Some(value)); + } + load_full_json_from_keyring(keyring_store, service, base_key) +} + +#[cfg(not(windows))] +pub fn load_json_from_keyring( + keyring_store: &K, + service: &str, + base_key: &str, +) -> Result, JsonKeyringError> { + if let Some(value) = load_full_json_from_keyring(keyring_store, service, base_key)? { + return Ok(Some(value)); + } + load_split_json_from_keyring(keyring_store, service, base_key) +} + +#[cfg(windows)] +pub fn save_json_to_keyring( + keyring_store: &K, + service: &str, + base_key: &str, + value: &Value, +) -> Result<(), JsonKeyringError> { + save_split_json_to_keyring(keyring_store, service, base_key, value)?; + if let Err(err) = delete_full_json_from_keyring(keyring_store, service, base_key) { + warn!("failed to remove stale full JSON record from keyring: {err}"); + } + Ok(()) +} + +#[cfg(not(windows))] +pub fn save_json_to_keyring( + keyring_store: &K, + service: &str, + base_key: &str, + value: &Value, +) -> Result<(), JsonKeyringError> { + save_full_json_to_keyring(keyring_store, service, base_key, value)?; + if let Err(err) = delete_split_json_from_keyring(keyring_store, service, base_key) { + warn!("failed to remove stale split JSON record from keyring: {err}"); + } + Ok(()) +} + +#[cfg(windows)] +pub fn delete_json_from_keyring( + keyring_store: &K, + service: &str, + base_key: &str, +) -> Result { + let split_removed = delete_split_json_from_keyring(keyring_store, service, base_key)?; + let full_removed = delete_full_json_from_keyring(keyring_store, service, base_key)?; + Ok(split_removed || full_removed) +} + +#[cfg(not(windows))] +pub fn delete_json_from_keyring( + keyring_store: &K, + service: &str, + base_key: &str, +) -> Result { + let full_removed = delete_full_json_from_keyring(keyring_store, service, base_key)?; + let split_removed = delete_split_json_from_keyring(keyring_store, service, base_key)?; + Ok(full_removed || split_removed) +} + pub fn load_split_json_from_keyring( keyring_store: &K, service: &str, @@ -178,6 +254,52 @@ pub fn delete_split_json_from_keyring( Ok(removed) } +fn load_full_json_from_keyring( + keyring_store: &K, + service: &str, + base_key: &str, +) -> Result, SplitJsonKeyringError> { + if let Some(bytes) = load_secret_from_keyring(keyring_store, service, base_key, "JSON record")? + { + let value = serde_json::from_slice(&bytes).map_err(|err| { + SplitJsonKeyringError::new(format!( + "failed to deserialize JSON record from keyring secret: {err}" + )) + })?; + return Ok(Some(value)); + } + + match keyring_store.load(service, base_key) { + Ok(Some(serialized)) => serde_json::from_str(&serialized).map(Some).map_err(|err| { + SplitJsonKeyringError::new(format!( + "failed to deserialize JSON record from keyring password: {err}" + )) + }), + Ok(None) => Ok(None), + Err(error) => Err(credential_store_error("load", "JSON record", error)), + } +} + +fn save_full_json_to_keyring( + keyring_store: &K, + service: &str, + base_key: &str, + value: &Value, +) -> Result<(), SplitJsonKeyringError> { + let bytes = serde_json::to_vec(value).map_err(|err| { + SplitJsonKeyringError::new(format!("failed to serialize JSON record: {err}")) + })?; + save_secret_to_keyring(keyring_store, service, base_key, &bytes, "JSON record") +} + +fn delete_full_json_from_keyring( + keyring_store: &K, + service: &str, + base_key: &str, +) -> Result { + delete_keyring_entry(keyring_store, service, base_key, "JSON record") +} + fn flatten_split_json( value: &Value, ) -> Result<(SplitJsonManifest, SplitJsonLeafValues), SplitJsonKeyringError> { @@ -587,12 +709,16 @@ mod tests { use super::MANIFEST_ENTRY; use super::ROOT_PATH_SENTINEL; use super::VALUE_ENTRY_PREFIX; + use super::delete_json_from_keyring; use super::delete_split_json_from_keyring; use super::layout_key; + use super::load_json_from_keyring; use super::load_split_json_from_keyring; use super::revision_key; + use super::save_json_to_keyring; use super::save_split_json_to_keyring; use super::value_key; + use crate::KeyringStore; use crate::tests::MockKeyringStore; use pretty_assertions::assert_eq; use serde_json::json; @@ -600,6 +726,94 @@ mod tests { const SERVICE: &str = "Test Service"; const BASE_KEY: &str = "base"; + #[test] + fn json_storage_round_trips_using_platform_backend() { + let store = MockKeyringStore::default(); + let expected = json!({ + "token": "secret", + "nested": {"id": 7} + }); + + save_json_to_keyring(&store, SERVICE, BASE_KEY, &expected).expect("JSON should save"); + + let loaded = load_json_from_keyring(&store, SERVICE, BASE_KEY) + .expect("JSON should load") + .expect("JSON should exist"); + assert_eq!(loaded, expected); + + #[cfg(windows)] + { + assert!( + store.saved_secret(BASE_KEY).is_none(), + "windows should not store the full JSON record under the base key" + ); + assert!( + store.contains(&layout_key(BASE_KEY, ACTIVE_REVISION_ENTRY)), + "windows should track an active split revision" + ); + } + + #[cfg(not(windows))] + { + assert_eq!( + store.saved_secret(BASE_KEY), + Some(serde_json::to_vec(&expected).expect("JSON should serialize")), + ); + assert!( + !store.contains(&layout_key(BASE_KEY, ACTIVE_REVISION_ENTRY)), + "non-windows should not create split revision metadata" + ); + } + } + + #[cfg(not(windows))] + #[test] + fn json_storage_loads_split_json_compatibility_on_non_windows() { + let store = MockKeyringStore::default(); + let expected = json!({ + "token": "secret", + "nested": {"id": 9} + }); + + save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &expected) + .expect("split JSON should save"); + + let loaded = load_json_from_keyring(&store, SERVICE, BASE_KEY) + .expect("JSON should load") + .expect("JSON should exist"); + assert_eq!(loaded, expected); + } + + #[test] + fn json_storage_delete_removes_platform_and_compat_entries() { + let store = MockKeyringStore::default(); + let current = json!({"current": true}); + let split = json!({"split": true}); + + save_json_to_keyring(&store, SERVICE, BASE_KEY, ¤t).expect("JSON should save"); + save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &split) + .expect("split JSON should save"); + store + .save( + SERVICE, + BASE_KEY, + &serde_json::to_string(¤t).expect("JSON should serialize"), + ) + .expect("legacy JSON should save"); + + let removed = delete_json_from_keyring(&store, SERVICE, BASE_KEY) + .expect("JSON delete should succeed"); + + assert!(removed); + assert!( + load_json_from_keyring(&store, SERVICE, BASE_KEY) + .expect("JSON load should succeed") + .is_none() + ); + assert!(!store.contains(BASE_KEY)); + assert!(!store.contains(&layout_key(BASE_KEY, ACTIVE_REVISION_ENTRY))); + } + #[test] fn split_json_round_trips_nested_values() { let store = MockKeyringStore::default(); diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index 65fd199517..9b23815a9f 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -23,9 +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_split_json_from_keyring; -use codex_keyring_store::load_split_json_from_keyring; -use codex_keyring_store::save_split_json_to_keyring; +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. @@ -165,28 +165,11 @@ impl KeyringAuthStorage { } } - fn load_legacy_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_split_auth_from_keyring(&self, base_key: &str) -> std::io::Result> { + fn load_auth_from_keyring(&self, base_key: &str) -> std::io::Result> { let Some(value) = - load_split_json_from_keyring(self.keyring_store.as_ref(), KEYRING_SERVICE, base_key) + load_json_from_keyring(self.keyring_store.as_ref(), KEYRING_SERVICE, base_key) .map_err(|err| { - std::io::Error::other(format!( - "failed to load split CLI auth from keyring: {err}" - )) + std::io::Error::other(format!("failed to load CLI auth from keyring: {err}")) })? else { return Ok(None); @@ -197,46 +180,24 @@ impl KeyringAuthStorage { )) }) } - - fn load_from_keyring(&self, base_key: &str) -> std::io::Result> { - if let Some(auth) = self.load_split_auth_from_keyring(base_key)? { - return Ok(Some(auth)); - } - self.load_legacy_from_keyring(base_key) - } - - fn delete_keyring_entry(&self, key: &str) -> std::io::Result { - self.keyring_store - .delete(KEYRING_SERVICE, key) - .map_err(|err| { - std::io::Error::other(format!("failed to delete auth from keyring: {err}")) - }) - } - - fn delete_legacy_from_keyring_only(&self, base_key: &str) -> std::io::Result { - self.delete_keyring_entry(base_key) - } } 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 base_key = compute_store_key(&self.codex_home)?; let value = serde_json::to_value(auth).map_err(std::io::Error::other)?; - save_split_json_to_keyring( + 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) = self.delete_legacy_from_keyring_only(&base_key) { - warn!("failed to remove legacy auth entries from keyring: {err}"); - } if let Err(err) = delete_file_if_exists(&self.codex_home) { warn!("failed to remove CLI auth fallback file: {err}"); } @@ -245,14 +206,13 @@ impl AuthStorageBackend for KeyringAuthStorage { fn delete(&self) -> std::io::Result { let base_key = compute_store_key(&self.codex_home)?; - let split_removed = - delete_split_json_from_keyring(self.keyring_store.as_ref(), KEYRING_SERVICE, &base_key) + 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 legacy_removed = self.delete_legacy_from_keyring_only(&base_key)?; let file_removed = delete_file_if_exists(&self.codex_home)?; - Ok(split_removed || legacy_removed || file_removed) + 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 6e24eef146..01ff479413 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -2,6 +2,7 @@ use super::*; use crate::token_data::IdTokenInfo; use anyhow::Context; use base64::Engine; +use pretty_assertions::assert_eq; use serde_json::json; use tempfile::tempdir; @@ -207,15 +208,22 @@ fn assert_keyring_saved_auth_and_removed_fallback( codex_home: &Path, expected: &AuthDotJson, ) { - assert!( - mock_keyring.saved_value(base_key).is_none(), - "legacy keyring entry should not be used for split auth storage" - ); - let loaded = load_split_json_from_keyring(mock_keyring, KEYRING_SERVICE, base_key) - .expect("split auth should load from keyring") - .expect("split auth should exist"); 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(), @@ -289,11 +297,11 @@ fn keyring_auth_storage_load_supports_legacy_single_entry() -> anyhow::Result<() } #[test] -fn keyring_auth_storage_load_returns_deserialized_split_auth() -> anyhow::Result<()> { +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("split"); + let expected = auth_with_prefix("keyring"); storage.save(&expected)?; @@ -302,6 +310,25 @@ fn keyring_auth_storage_load_returns_deserialized_split_auth() -> anyhow::Result Ok(()) } +#[test] +fn keyring_auth_storage_load_supports_split_json_compatibility() -> 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.clone()), + ); + let expected = auth_with_prefix("split-compat"); + let key = compute_store_key(codex_home.path())?; + let value = serde_json::to_value(&expected)?; + + codex_keyring_store::save_split_json_to_keyring(&mock_keyring, KEYRING_SERVICE, &key, &value)?; + + let loaded = storage.load()?; + assert_eq!(Some(expected), loaded); + Ok(()) +} + #[test] fn keyring_auth_storage_compute_store_key_for_home_directory() -> anyhow::Result<()> { let codex_home = PathBuf::from("~/.codex"); @@ -357,8 +384,8 @@ fn keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> assert!(removed, "delete should report removal"); assert!( - load_split_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &base_key)?.is_none(), - "split auth should be removed" + load_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &base_key)?.is_none(), + "keyring auth should be removed" ); assert!( !auth_file.exists(), @@ -470,8 +497,8 @@ fn auto_auth_storage_save_falls_back_when_keyring_errors() -> anyhow::Result<()> .context("fallback auth should exist")?; assert_eq!(saved, auth); assert!( - load_split_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &key)?.is_none(), - "keyring should not point to saved split auth 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(()) } @@ -495,8 +522,8 @@ fn auto_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { assert!(removed, "delete should report removal"); assert!( - load_split_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &base_key)?.is_none(), - "split auth should be removed" + load_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &base_key)?.is_none(), + "keyring auth should be removed" ); assert!( !auth_file.exists(), diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 938fc0d716..8787b7c05e 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -45,9 +45,9 @@ use tracing::warn; use codex_keyring_store::DefaultKeyringStore; use codex_keyring_store::KeyringStore; -use codex_keyring_store::delete_split_json_from_keyring; -use codex_keyring_store::load_split_json_from_keyring; -use codex_keyring_store::save_split_json_to_keyring; +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 rmcp::transport::auth::AuthorizationManager; use tokio::sync::Mutex; @@ -158,24 +158,15 @@ fn load_oauth_tokens_from_keyring( url: &str, ) -> Result> { let key = compute_store_key(server_name, url)?; - if let Some(value) = load_split_json_from_keyring(keyring_store, KEYRING_SERVICE, &key) + let Some(value) = load_json_from_keyring(keyring_store, KEYRING_SERVICE, &key) .map_err(|err| Error::msg(err.to_string()))? - { - let mut tokens: StoredOAuthTokens = serde_json::from_value(value) - .context("failed to deserialize OAuth tokens from keyring")?; - refresh_expires_in_from_timestamp(&mut tokens); - return Ok(Some(tokens)); - } - match keyring_store.load(KEYRING_SERVICE, &key) { - Ok(Some(serialized)) => { - let mut tokens: StoredOAuthTokens = serde_json::from_str(&serialized) - .context("failed to deserialize OAuth tokens from keyring")?; - refresh_expires_in_from_timestamp(&mut tokens); - Ok(Some(tokens)) - } - Ok(None) => Ok(None), - Err(error) => Err(Error::new(error.into_error())), - } + else { + return Ok(None); + }; + let mut tokens: StoredOAuthTokens = + serde_json::from_value(value).context("failed to deserialize OAuth tokens from keyring")?; + refresh_expires_in_from_timestamp(&mut tokens); + Ok(Some(tokens)) } pub fn save_oauth_tokens( @@ -204,14 +195,8 @@ fn save_oauth_tokens_with_keyring( ) -> Result<()> { let value = serde_json::to_value(tokens).context("failed to serialize OAuth tokens")?; let key = compute_store_key(server_name, &tokens.url)?; - match save_split_json_to_keyring(keyring_store, KEYRING_SERVICE, &key, &value) { + match save_json_to_keyring(keyring_store, KEYRING_SERVICE, &key, &value) { Ok(()) => { - if let Err(error) = keyring_store.delete(KEYRING_SERVICE, &key) { - warn!( - "failed to remove legacy OAuth tokens from keyring: {}", - error.message() - ); - } if let Err(error) = delete_oauth_tokens_from_file(&key) { warn!("failed to remove OAuth tokens from fallback storage: {error:?}"); } @@ -257,7 +242,7 @@ fn delete_oauth_tokens_from_keyring_and_file( url: &str, ) -> Result { let key = compute_store_key(server_name, url)?; - let split_removed = match delete_split_json_from_keyring(keyring_store, KEYRING_SERVICE, &key) { + let keyring_removed = match delete_json_from_keyring(keyring_store, KEYRING_SERVICE, &key) { Ok(removed) => removed, Err(error) => { let message = error.to_string(); @@ -271,23 +256,8 @@ fn delete_oauth_tokens_from_keyring_and_file( } } }; - let legacy_removed = match keyring_store.delete(KEYRING_SERVICE, &key) { - Ok(removed) => removed, - Err(error) => { - let message = error.message(); - warn!("failed to delete OAuth tokens from keyring: {message}"); - match store_mode { - OAuthCredentialsStoreMode::Auto | OAuthCredentialsStoreMode::Keyring => { - return Err(error.into_error()) - .context("failed to delete OAuth tokens from keyring"); - } - OAuthCredentialsStoreMode::File => false, - } - } - }; - let file_removed = delete_oauth_tokens_from_file(&key)?; - Ok(split_removed || legacy_removed || file_removed) + Ok(keyring_removed || file_removed) } #[derive(Clone)] @@ -631,6 +601,8 @@ mod tests { use super::*; use anyhow::Result; use codex_keyring_store::CredentialStoreError; + use codex_keyring_store::load_json_from_keyring; + use codex_keyring_store::save_json_to_keyring; use codex_keyring_store::save_split_json_to_keyring; use keyring::Error as KeyringError; use pretty_assertions::assert_eq; @@ -776,7 +748,7 @@ mod tests { let expected = tokens.clone(); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; let value = serde_json::to_value(&tokens)?; - save_split_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; + save_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; let loaded = super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? @@ -785,6 +757,22 @@ mod tests { Ok(()) } + #[test] + fn load_oauth_tokens_supports_split_json_compatibility() -> Result<()> { + let _env = TempCodexHome::new(); + let store = MockKeyringStore::default(); + let tokens = sample_tokens(); + let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; + let value = serde_json::to_value(&tokens)?; + save_split_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; + + let loaded = + super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? + .expect("tokens should load from split-json compatibility format"); + assert_tokens_match_without_expiry(&loaded, &tokens); + Ok(()) + } + #[test] fn load_oauth_tokens_supports_legacy_single_entry() -> Result<()> { let _env = TempCodexHome::new(); @@ -856,7 +844,16 @@ mod tests { let fallback_path = super::fallback_file_path()?; assert!(!fallback_path.exists(), "fallback file should be removed"); - assert!(store.saved_value(&key).is_none()); + #[cfg(windows)] + assert!( + store.saved_secret(&key).is_none(), + "windows should not store the full JSON record under the base key" + ); + #[cfg(not(windows))] + assert!( + store.saved_secret(&key).is_some(), + "non-windows should store the full JSON record as one secret" + ); let stored = super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? .expect("value saved to keyring"); @@ -890,6 +887,10 @@ mod tests { tokens.token_response.0.access_token().secret().as_str() ); assert!(mock_keyring.saved_value(&key).is_none()); + assert!( + load_json_from_keyring(&mock_keyring, KEYRING_SERVICE, &key)?.is_none(), + "keyring should not point at saved OAuth tokens when save fails" + ); Ok(()) } @@ -912,7 +913,10 @@ mod tests { &tokens.url, )?; assert!(removed); - assert!(!store.contains(&key)); + assert!( + load_json_from_keyring(&store, KEYRING_SERVICE, &key)?.is_none(), + "keyring entry should be removed" + ); assert!(!super::fallback_file_path()?.exists()); Ok(()) } @@ -937,7 +941,10 @@ mod tests { &tokens.url, )?; assert!(removed); - assert!(!store.contains(&key)); + assert!( + load_json_from_keyring(&store, KEYRING_SERVICE, &key)?.is_none(), + "keyring entry should be removed" + ); assert!( super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? .is_none()