mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
scope json breakout to windows only
This commit is contained in:
@@ -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<Option<AuthDotJson>> {
|
||||
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<Option<AuthDotJson>> {
|
||||
fn load_auth_from_keyring(&self, base_key: &str) -> std::io::Result<Option<AuthDotJson>> {
|
||||
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<Option<AuthDotJson>> {
|
||||
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<bool> {
|
||||
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<bool> {
|
||||
self.delete_keyring_entry(base_key)
|
||||
}
|
||||
}
|
||||
|
||||
impl AuthStorageBackend for KeyringAuthStorage {
|
||||
fn load(&self) -> std::io::Result<Option<AuthDotJson>> {
|
||||
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<bool> {
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -595,15 +555,22 @@ mod tests {
|
||||
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(),
|
||||
@@ -677,12 +644,12 @@ mod tests {
|
||||
}
|
||||
|
||||
#[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)?;
|
||||
|
||||
@@ -691,6 +658,30 @@ mod tests {
|
||||
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");
|
||||
@@ -751,8 +742,8 @@ mod tests {
|
||||
|
||||
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(),
|
||||
@@ -866,8 +857,8 @@ mod tests {
|
||||
.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(())
|
||||
}
|
||||
@@ -891,8 +882,8 @@ mod tests {
|
||||
|
||||
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(),
|
||||
|
||||
@@ -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)]
|
||||
|
||||
@@ -22,6 +22,8 @@ pub struct SplitJsonKeyringError {
|
||||
message: String,
|
||||
}
|
||||
|
||||
pub type JsonKeyringError = SplitJsonKeyringError;
|
||||
|
||||
impl SplitJsonKeyringError {
|
||||
fn new(message: impl Into<String>) -> Self {
|
||||
Self {
|
||||
@@ -87,6 +89,80 @@ struct SplitJsonManifest {
|
||||
|
||||
type SplitJsonLeafValues = Vec<(String, Vec<u8>)>;
|
||||
|
||||
#[cfg(windows)]
|
||||
pub fn load_json_from_keyring<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
base_key: &str,
|
||||
) -> Result<Option<Value>, 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<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
base_key: &str,
|
||||
) -> Result<Option<Value>, 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<K: KeyringStore + ?Sized>(
|
||||
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<K: KeyringStore + ?Sized>(
|
||||
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<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
base_key: &str,
|
||||
) -> Result<bool, JsonKeyringError> {
|
||||
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<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
base_key: &str,
|
||||
) -> Result<bool, JsonKeyringError> {
|
||||
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<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
@@ -178,6 +254,52 @@ pub fn delete_split_json_from_keyring<K: KeyringStore + ?Sized>(
|
||||
Ok(removed)
|
||||
}
|
||||
|
||||
fn load_full_json_from_keyring<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
base_key: &str,
|
||||
) -> Result<Option<Value>, 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<K: KeyringStore + ?Sized>(
|
||||
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<K: KeyringStore + ?Sized>(
|
||||
keyring_store: &K,
|
||||
service: &str,
|
||||
base_key: &str,
|
||||
) -> Result<bool, SplitJsonKeyringError> {
|
||||
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();
|
||||
|
||||
@@ -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<K: KeyringStore>(
|
||||
url: &str,
|
||||
) -> Result<Option<StoredOAuthTokens>> {
|
||||
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<K: KeyringStore>(
|
||||
) -> 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<K: KeyringStore>(
|
||||
url: &str,
|
||||
) -> Result<bool> {
|
||||
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<K: KeyringStore>(
|
||||
}
|
||||
}
|
||||
};
|
||||
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()
|
||||
|
||||
Reference in New Issue
Block a user