diff --git a/codex-rs/login/src/account.rs b/codex-rs/login/src/account.rs index 1712685ea2c6..0ddf775d44f9 100644 --- a/codex-rs/login/src/account.rs +++ b/codex-rs/login/src/account.rs @@ -14,6 +14,9 @@ use std::path::PathBuf; use crate::AuthDotJson; use crate::AuthKeyringBackendKind; use crate::account_lease::AccountLease; +use crate::account_lease::AuthRefreshGuard; +use crate::auth::load_auth_dot_json_with_guard; +use crate::auth::save_auth_with_guard; use crate::load_auth_dot_json; use crate::save_auth; @@ -104,17 +107,19 @@ impl AccountStore { root_store_mode: AuthCredentialsStoreMode, root_keyring_backend_kind: AuthKeyringBackendKind, ) -> std::io::Result { - let _root_refresh_guard = AccountLease::acquire_auth_refresh(&self.codex_home)?; - let root_auth = - load_auth_dot_json(&self.codex_home, root_store_mode, root_keyring_backend_kind)? - .ok_or_else(|| { - std::io::Error::new(std::io::ErrorKind::NotFound, "not logged in") - })?; + let root_refresh_guard = AuthRefreshGuard::acquire(&self.codex_home)?; + let root_auth = load_auth_dot_json_with_guard( + &self.codex_home, + root_store_mode, + root_keyring_backend_kind, + &root_refresh_guard, + )? + .ok_or_else(|| std::io::Error::new(std::io::ErrorKind::NotFound, "not logged in"))?; let mut auth = root_auth.clone(); let imported_from_root_marker = is_root_account_marker(&auth); let source_account_id = account_id_for_auth(&auth)?; let account_home = self.account_home(&source_account_id); - let _account_refresh_guard = AccountLease::acquire_auth_refresh(&account_home)?; + let _account_refresh_guard = AuthRefreshGuard::acquire(&account_home)?; if imported_from_root_marker { auth = load_auth_dot_json( &account_home, @@ -209,6 +214,7 @@ impl AccountStore { &auth, root_store_mode, root_keyring_backend_kind, + &root_refresh_guard, ) { let mut rollback_errors = Vec::new(); if let Err(rollback_err) = self.save_index(&previous_index) { @@ -219,11 +225,12 @@ impl AccountStore { { rollback_errors.push(format!("restore account auth: {rollback_err}")); } - if let Err(rollback_err) = save_auth( + if let Err(rollback_err) = save_auth_with_guard( &self.codex_home, &root_auth, root_store_mode, root_keyring_backend_kind, + &root_refresh_guard, ) { rollback_errors.push(format!("restore root auth: {rollback_err}")); } @@ -299,7 +306,7 @@ impl AccountStore { expected_auth: &AuthDotJson, ) -> std::io::Result { let account_home = self.account_home(account_id); - let _refresh_guard = AccountLease::acquire_auth_refresh(&account_home)?; + let _refresh_guard = AuthRefreshGuard::acquire(&account_home)?; let current_auth = load_auth_dot_json( &account_home, AuthCredentialsStoreMode::File, @@ -337,8 +344,8 @@ impl AccountStore { root_keyring_backend_kind: AuthKeyringBackendKind, ) -> std::io::Result { let account_home = self.account_home(account_id); - let _root_refresh_guard = AccountLease::acquire_auth_refresh(&self.codex_home)?; - let _account_refresh_guard = AccountLease::acquire_auth_refresh(&account_home)?; + let root_refresh_guard = AuthRefreshGuard::acquire(&self.codex_home)?; + let _account_refresh_guard = AuthRefreshGuard::acquire(&account_home)?; let _index_guard = self.acquire_index_lock()?; let (profile, account_home) = self .file_account_profiles()? @@ -367,6 +374,7 @@ impl AccountStore { &auth, root_store_mode, root_keyring_backend_kind, + &root_refresh_guard, )?; Ok(profile) } @@ -641,12 +649,13 @@ fn save_root_account_marker( auth: &AuthDotJson, store_mode: AuthCredentialsStoreMode, keyring_backend_kind: AuthKeyringBackendKind, + guard: &AuthRefreshGuard, ) -> std::io::Result<()> { let mut marker = auth.clone(); if let Some(tokens) = marker.tokens.as_mut() { tokens.refresh_token.clear(); } - save_auth(codex_home, &marker, store_mode, keyring_backend_kind) + save_auth_with_guard(codex_home, &marker, store_mode, keyring_backend_kind, guard) } fn restore_file_auth(auth_home: &Path, auth: Option<&AuthDotJson>) -> std::io::Result<()> { diff --git a/codex-rs/login/src/account_lease.rs b/codex-rs/login/src/account_lease.rs index 9619ad91da69..ceae114388c4 100644 --- a/codex-rs/login/src/account_lease.rs +++ b/codex-rs/login/src/account_lease.rs @@ -3,16 +3,14 @@ use std::fs::File; use std::fs::OpenOptions; use std::io; use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; pub(crate) struct AccountLease { file: File, } impl AccountLease { - pub(crate) fn acquire_auth_refresh(auth_home: &Path) -> io::Result { - Self::acquire(&auth_home.join(".auth-refresh.lock")) - } - pub(crate) fn acquire(path: &Path) -> io::Result { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; @@ -50,6 +48,41 @@ impl AccountLease { } } +#[derive(Clone)] +pub(crate) struct AuthRefreshGuard { + auth_home: PathBuf, + _lease: Arc, +} + +impl AuthRefreshGuard { + pub(crate) fn acquire(auth_home: &Path) -> io::Result { + let lease = AccountLease::acquire(&auth_home.join(".auth-refresh.lock"))?; + Ok(Self::new(auth_home, lease)) + } + + pub(crate) fn ensure_matches(&self, auth_home: &Path) -> io::Result<()> { + if self.auth_home == normalized(auth_home) { + Ok(()) + } else { + Err(io::Error::new( + io::ErrorKind::PermissionDenied, + "auth refresh guard does not match auth home", + )) + } + } + + fn new(auth_home: &Path, lease: AccountLease) -> Self { + Self { + auth_home: normalized(auth_home), + _lease: Arc::new(lease), + } + } +} + +fn normalized(path: &Path) -> PathBuf { + path.canonicalize().unwrap_or_else(|_| path.to_path_buf()) +} + fn is_contended(err: &io::Error) -> bool { err.kind() == io::ErrorKind::WouldBlock || cfg!(windows) && matches!(err.raw_os_error(), Some(32 | 33)) diff --git a/codex-rs/login/src/auth/auth_tests.rs b/codex-rs/login/src/auth/auth_tests.rs index 8b2faf30fe9d..cd02fe0b62ec 100644 --- a/codex-rs/login/src/auth/auth_tests.rs +++ b/codex-rs/login/src/auth/auth_tests.rs @@ -1,7 +1,9 @@ use super::*; use crate::auth::storage::FileAuthStorage; +use crate::auth::storage::create_auth_storage_with_store; use crate::auth::storage::get_auth_file; use crate::token_data::IdTokenInfo; +use anyhow::Context; use codex_protocol::account::PlanType as AccountPlanType; use codex_protocol::auth::AuthMode; use codex_protocol::auth::KnownPlan as InternalKnownPlan; @@ -9,6 +11,7 @@ use codex_protocol::auth::PlanType as InternalPlanType; use codex_protocol::protocol::SessionSource; use base64::Engine; +use codex_keyring_store::tests::MockKeyringStore; use codex_protocol::config_types::ForcedLoginMethod; use codex_protocol::config_types::ModelProviderAuthInfo; use pretty_assertions::assert_eq; @@ -32,7 +35,8 @@ const WORKSPACE_ID_SECOND_ALLOWED: &str = "123e4567-e89b-42d3-a456-426614174001" const WORKSPACE_ID_DISALLOWED: &str = "123e4567-e89b-42d3-a456-426614174002"; #[tokio::test] -async fn refresh_without_id_token() { +#[serial(codex_auth_env)] +async fn refresh_without_id_token() -> anyhow::Result<()> { let codex_home = tempdir().unwrap(); let fake_jwt = write_auth_file( AuthFileParams { @@ -43,24 +47,75 @@ async fn refresh_without_id_token() { codex_home.path(), ) .expect("failed to write auth file"); - - let storage = create_auth_storage( + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "access_token": "new-access-token", + "refresh_token": "new-refresh-token" + }))) + .expect(1) + .mount(&server) + .await; + let _refresh_url = EnvVarGuard::set( + REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR, + &format!("{}/oauth/token", server.uri()), + ); + let file_storage = FileAuthStorage::new(codex_home.path().to_path_buf()); + let mut initial_auth = file_storage.load()?.expect("auth file should exist"); + initial_auth + .tokens + .as_mut() + .context("auth tokens should exist")? + .account_id = Some("account-123".to_string()); + file_storage.save(&initial_auth)?; + const MARKER: &str = ".codex-plus-plus-auth-file-authority"; + let marker = codex_home.path().join(MARKER); + std::fs::write(&marker, "")?; + let mock_keyring = MockKeyringStore::default(); + let storage = create_auth_storage_with_store( codex_home.path().to_path_buf(), - AuthCredentialsStoreMode::File, - AuthKeyringBackendKind::default(), + AuthCredentialsStoreMode::Auto, + Arc::new(mock_keyring), + AuthKeyringBackendKind::Secrets, ); - let updated = super::persist_tokens( - &storage, - /*id_token*/ None, - Some("new-access-token".to_string()), - Some("new-refresh-token".to_string()), - ) - .expect("update_tokens should succeed"); + let state = ChatgptAuthState { + auth_dot_json: Arc::new(Mutex::new(Some(initial_auth))), + client: create_default_auth_client( + &refresh_token_endpoint(), + /*auth_route_config*/ None, + )?, + }; + let auth = CodexAuth::Chatgpt(ChatgptAuth { + state, + storage: Arc::clone(&storage), + }); + let manager = + AuthManager::from_auth_for_testing_with_home(auth, codex_home.path().to_path_buf()); + assert!(matches!( + manager + .reload_if_account_id_matches(Some("account-123"), /*guard*/ None) + .await, + ReloadOutcome::ReloadedNoChange + )); + let cached = manager.auth_cached().context("auth should remain cached")?; + let CodexAuth::Chatgpt(cached_chatgpt) = cached else { + anyhow::bail!("cached auth should remain ChatGPT auth"); + }; + assert!(Arc::ptr_eq(cached_chatgpt.storage(), &storage)); + + manager.refresh_token().await?; + let updated = file_storage.load()?.expect("authoritative auth file"); let tokens = updated.tokens.expect("tokens should exist"); assert_eq!(tokens.id_token.raw_jwt, fake_jwt); assert_eq!(tokens.access_token, "new-access-token"); assert_eq!(tokens.refresh_token, "new-refresh-token"); + assert!(marker.exists()); + let cached = manager.auth_cached().context("auth should remain cached")?; + assert_eq!(cached.get_token_data()?, tokens); + server.verify().await; + Ok(()) } #[test] @@ -1066,6 +1121,9 @@ async fn external_bearer_only_auth_manager_disables_auto_refresh_when_interval_i assert_eq!(first.as_deref(), Some("provider-token")); assert_eq!(second.as_deref(), Some("provider-token")); + manager.refresh_token().await.expect("external refresh"); + let refreshed = manager.auth().await.expect("refreshed auth"); + assert_eq!(refreshed.api_key(), Some("next-token")); } #[tokio::test] diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index f90e85d3b4a3..b9cfc505d65a 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -43,6 +43,7 @@ use crate::account::AccountStore; use crate::account::account_id_for_auth; use crate::account::is_root_account_marker; use crate::account_lease::AccountLease; +use crate::account_lease::AuthRefreshGuard; use crate::auth::AuthHeaders; pub use crate::auth::agent_identity::AgentIdentityAuth; pub use crate::auth::agent_identity::AgentIdentityAuthError; @@ -934,6 +935,20 @@ pub fn logout( storage.delete() } +fn logout_with_guard( + codex_home: &Path, + auth_credentials_store_mode: AuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + guard: &AuthRefreshGuard, +) -> std::io::Result { + create_auth_storage( + codex_home.to_path_buf(), + auth_credentials_store_mode, + keyring_backend_kind, + ) + .delete_with_guard(guard) +} + pub async fn logout_with_revoke( codex_home: &Path, auth_credentials_store_mode: AuthCredentialsStoreMode, @@ -1093,6 +1108,21 @@ pub fn save_auth( storage.save(auth) } +pub(crate) fn save_auth_with_guard( + codex_home: &Path, + auth: &AuthDotJson, + auth_credentials_store_mode: AuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + guard: &AuthRefreshGuard, +) -> std::io::Result<()> { + create_auth_storage( + codex_home.to_path_buf(), + auth_credentials_store_mode, + keyring_backend_kind, + ) + .save_with_guard(auth, guard) +} + /// Load the raw stored auth payload without applying environment overrides. /// /// Returns `None` when no credentials are stored. Prefer `AuthManager` for @@ -1111,6 +1141,20 @@ pub fn load_auth_dot_json( storage.load() } +pub(crate) fn load_auth_dot_json_with_guard( + codex_home: &Path, + auth_credentials_store_mode: AuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + guard: &AuthRefreshGuard, +) -> std::io::Result> { + create_auth_storage( + codex_home.to_path_buf(), + auth_credentials_store_mode, + keyring_backend_kind, + ) + .load_with_guard(guard) +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct AuthConfig { pub codex_home: PathBuf, @@ -1311,6 +1355,31 @@ fn logout_all_stores( Ok(removed_ephemeral || removed_managed) } +fn logout_all_stores_with_guard( + codex_home: &Path, + auth_credentials_store_mode: AuthCredentialsStoreMode, + keyring_backend_kind: AuthKeyringBackendKind, + guard: &AuthRefreshGuard, +) -> std::io::Result { + let removed_ephemeral = logout_with_guard( + codex_home, + AuthCredentialsStoreMode::Ephemeral, + AuthKeyringBackendKind::default(), + guard, + )?; + let removed_managed = if auth_credentials_store_mode == AuthCredentialsStoreMode::Ephemeral { + false + } else { + logout_with_guard( + codex_home, + auth_credentials_store_mode, + keyring_backend_kind, + guard, + )? + }; + Ok(removed_ephemeral || removed_managed) +} + #[allow(clippy::too_many_arguments)] async fn load_auth( codex_home: &Path, @@ -1321,6 +1390,32 @@ async fn load_auth( keyring_backend_kind: AuthKeyringBackendKind, agent_identity_authapi_base_url: Option<&str>, auth_route_config: Option<&AuthRouteConfig>, +) -> std::io::Result> { + load_auth_with_guard( + codex_home, + enable_codex_api_key_env, + auth_credentials_store_mode, + forced_chatgpt_workspace_id, + chatgpt_base_url, + keyring_backend_kind, + agent_identity_authapi_base_url, + auth_route_config, + /*guard*/ None, + ) + .await +} + +#[allow(clippy::too_many_arguments)] +async fn load_auth_with_guard( + codex_home: &Path, + enable_codex_api_key_env: bool, + auth_credentials_store_mode: AuthCredentialsStoreMode, + forced_chatgpt_workspace_id: Option<&[String]>, + chatgpt_base_url: Option<&str>, + keyring_backend_kind: AuthKeyringBackendKind, + agent_identity_authapi_base_url: Option<&str>, + auth_route_config: Option<&AuthRouteConfig>, + guard: Option<&AuthRefreshGuard>, ) -> std::io::Result> { // API key via env var takes precedence over any other auth method. if enable_codex_api_key_env && let Some(api_key) = read_codex_api_key_from_env() { @@ -1376,7 +1471,7 @@ async fn load_auth( return Ok(None); } - load_auth_from_storage( + load_auth_from_storage_with_guard( codex_home, auth_credentials_store_mode, forced_chatgpt_workspace_id, @@ -1384,6 +1479,7 @@ async fn load_auth( keyring_backend_kind, agent_identity_authapi_base_url, auth_route_config, + guard, ) .await } @@ -1397,13 +1493,44 @@ async fn load_auth_from_storage( keyring_backend_kind: AuthKeyringBackendKind, agent_identity_authapi_base_url: Option<&str>, auth_route_config: Option<&AuthRouteConfig>, +) -> std::io::Result> { + load_auth_from_storage_with_guard( + codex_home, + auth_credentials_store_mode, + forced_chatgpt_workspace_id, + chatgpt_base_url, + keyring_backend_kind, + agent_identity_authapi_base_url, + auth_route_config, + /*guard*/ None, + ) + .await +} + +#[allow(clippy::too_many_arguments)] +async fn load_auth_from_storage_with_guard( + codex_home: &Path, + auth_credentials_store_mode: AuthCredentialsStoreMode, + forced_chatgpt_workspace_id: Option<&[String]>, + chatgpt_base_url: Option<&str>, + keyring_backend_kind: AuthKeyringBackendKind, + agent_identity_authapi_base_url: Option<&str>, + auth_route_config: Option<&AuthRouteConfig>, + guard: Option<&AuthRefreshGuard>, ) -> std::io::Result> { let storage = create_auth_storage( codex_home.to_path_buf(), auth_credentials_store_mode, keyring_backend_kind, ); - let auth_dot_json = match run_blocking_io(move || storage.load()).await? { + let stored_auth = match guard { + Some(guard) => { + let guard = guard.clone(); + run_blocking_io(move || storage.load_with_guard(&guard)).await? + } + None => run_blocking_io(move || storage.load()).await?, + }; + let auth_dot_json = match stored_auth { Some(auth) => auth, None => return Ok(None), }; @@ -1432,17 +1559,35 @@ async fn run_blocking_io( .map_err(std::io::Error::other)? } -// Persist refreshed tokens into auth storage and update last_refresh. -fn persist_tokens( +fn persist_tokens_with_guard( storage: &Arc, id_token: Option, access_token: Option, refresh_token: Option, + refresh_file_guard: &AuthRefreshGuard, ) -> std::io::Result { let mut auth_dot_json = storage - .load()? + .load_with_guard(refresh_file_guard)? .ok_or(std::io::Error::other("Token data is not available."))?; + update_and_save_tokens( + storage, + &mut auth_dot_json, + id_token, + access_token, + refresh_token, + Some(refresh_file_guard), + ) +} + +fn update_and_save_tokens( + storage: &Arc, + auth_dot_json: &mut AuthDotJson, + id_token: Option, + access_token: Option, + refresh_token: Option, + guard: Option<&AuthRefreshGuard>, +) -> std::io::Result { let tokens = auth_dot_json.tokens.get_or_insert_with(TokenData::default); if let Some(id_token) = id_token { tokens.id_token = parse_chatgpt_jwt_claims(&id_token).map_err(std::io::Error::other)?; @@ -1454,8 +1599,11 @@ fn persist_tokens( tokens.refresh_token = refresh_token; } auth_dot_json.last_refresh = Some(Utc::now()); - storage.save(&auth_dot_json)?; - Ok(auth_dot_json) + match guard { + Some(guard) => storage.save_with_guard(auth_dot_json, guard), + None => storage.save(auth_dot_json), + }?; + Ok(auth_dot_json.clone()) } // Requests refreshed ChatGPT OAuth tokens from the auth service using a refresh token. @@ -1853,7 +2001,10 @@ impl UnauthorizedRecovery { UnauthorizedRecoveryStep::Reload => { match self .manager - .reload_if_account_id_matches(self.expected_account_id.as_deref()) + .reload_if_account_id_matches( + self.expected_account_id.as_deref(), + /*guard*/ None, + ) .await { ReloadOutcome::ReloadedChanged => { @@ -2406,13 +2557,13 @@ impl AuthManager { let Ok(_refresh_guard) = self.refresh_lock.acquire().await else { return false; }; - self.reload_unlocked().await + self.reload_unlocked(/*guard*/ None).await } - async fn reload_unlocked(&self) -> bool { + async fn reload_unlocked(&self, guard: Option<&AuthRefreshGuard>) -> bool { tracing::info!("Reloading auth"); let active_account_id_before_reload = self.active_account_id(); - let loaded_auth = self.load_auth().await; + let loaded_auth = self.load_auth(guard).await; if let (Some(account_id), Some(auth)) = ( active_account_id_before_reload.as_ref(), loaded_auth.auth.as_ref(), @@ -2434,6 +2585,7 @@ impl AuthManager { async fn reload_if_account_id_matches( &self, expected_account_id: Option<&str>, + guard: Option<&AuthRefreshGuard>, ) -> ReloadOutcome { let expected_account_id = match expected_account_id { Some(account_id) => account_id, @@ -2444,7 +2596,7 @@ impl AuthManager { }; let active_account_id_before_reload = self.active_account_id(); - let loaded_auth = self.load_auth().await; + let loaded_auth = self.load_auth(guard).await; let new_account_id = loaded_auth .auth .as_ref() @@ -2468,8 +2620,8 @@ impl AuthManager { cached_before_reload.as_ref(), loaded_auth.auth.as_ref(), ); - self.set_cached_auth(loaded_auth.auth); if auth_changed { + self.set_cached_auth(loaded_auth.auth); ReloadOutcome::ReloadedChanged } else { ReloadOutcome::ReloadedNoChange @@ -2527,7 +2679,7 @@ impl AuthManager { } } - async fn load_auth(&self) -> LoadedAuth { + async fn load_auth(&self, guard: Option<&AuthRefreshGuard>) -> LoadedAuth { if let Some(external_auth) = self.external_auth() { let auth = match self.resolve_external_auth(&external_auth).await { Ok(auth) => Some(auth), @@ -2540,10 +2692,12 @@ impl AuthManager { } if self.auth_storage_only { - return LoadedAuth::from_current_source(codex_plus_plus::file_auth::load(self).await); + return LoadedAuth::from_current_source( + codex_plus_plus::file_auth::load(self, guard).await, + ); } let forced_chatgpt_workspace_id = self.forced_chatgpt_workspace_id(); - let auth = load_auth( + let auth = load_auth_with_guard( &self.active_auth_home(), self.enable_codex_api_key_env, self.active_auth_credentials_store_mode(), @@ -2552,6 +2706,7 @@ impl AuthManager { self.active_keyring_backend_kind(), self.agent_identity_authapi_base_url.as_deref(), self.auth_route_config.as_ref(), + guard, ) .await .ok() @@ -2735,6 +2890,9 @@ impl AuthManager { REFRESH_TOKEN_UNKNOWN_MESSAGE.to_string(), )) })?; + if self.has_external_auth() { + return self.refresh_token_from_authority_impl(/*guard*/ None).await; + } if self .auth_cached() .as_ref() @@ -2742,20 +2900,20 @@ impl AuthManager { { return Ok(()); } - let _file_guard = self.acquire_refresh_file_lock().await?; - if matches!( - self.reconcile_imported_account_refresh_readiness().await?, - imported_account_refresh::ImportedAccountRefreshReadiness::Recovered - ) { + let file_guard = self.acquire_refresh_file_lock().await?; + let Some(file_guard) = self + .reconcile_imported_account_refresh_readiness(file_guard) + .await? + else { return Ok(()); - } + }; let auth_before_reload = self.auth_cached(); let expected_account_id = auth_before_reload .as_ref() .and_then(CodexAuth::get_account_id); match self - .reload_if_account_id_matches(expected_account_id.as_deref()) + .reload_if_account_id_matches(expected_account_id.as_deref(), Some(&file_guard)) .await { ReloadOutcome::ReloadedChanged => { @@ -2764,8 +2922,10 @@ impl AuthManager { } ReloadOutcome::ReloadedNoChange => { let attempted_account_id = self.active_account_id(); - let result = self.refresh_token_from_authority_impl().await; - self.recover_terminal_imported_refresh(result, attempted_account_id) + let result = self + .refresh_token_from_authority_impl(Some(&file_guard)) + .await; + self.recover_terminal_imported_refresh(result, attempted_account_id, file_guard) .await } ReloadOutcome::Skipped => { @@ -2790,10 +2950,13 @@ impl AuthManager { REFRESH_TOKEN_UNKNOWN_MESSAGE.to_string(), )) })?; - self.refresh_token_from_authority_impl().await + self.refresh_token_from_authority_impl(/*guard*/ None).await } - async fn refresh_token_from_authority_impl(&self) -> Result<(), RefreshTokenError> { + async fn refresh_token_from_authority_impl( + &self, + guard: Option<&AuthRefreshGuard>, + ) -> Result<(), RefreshTokenError> { tracing::info!("Refreshing token"); let auth = match self.auth_cached() { @@ -2816,8 +2979,16 @@ impl AuthManager { "Token data is not available.", )) })?; - self.refresh_and_persist_chatgpt_token(&chatgpt_auth, token_data.refresh_token) - .await + self.refresh_and_persist_chatgpt_token( + &chatgpt_auth, + token_data.refresh_token, + guard.ok_or_else(|| { + RefreshTokenError::Transient(std::io::Error::other( + "auth refresh guard is missing", + )) + })?, + ) + .await } CodexAuth::ApiKey(_) | CodexAuth::ChatgptAuthTokens(_) @@ -2848,7 +3019,8 @@ impl AuthManager { // Always reload to clear any cached auth (even if file absent). self.clear_external_auth(); self.clear_active_imported_account(); - self.reload_unlocked().await; + self.reload_unlocked(Some(auth_locks.guard_for(&self.codex_home)?)) + .await; Ok(removed) } @@ -2866,7 +3038,8 @@ impl AuthManager { // Always reload to clear any cached auth (even if file absent). self.clear_external_auth(); self.clear_active_imported_account(); - self.reload_unlocked().await; + self.reload_unlocked(Some(auth_locks.guard_for(&self.codex_home)?)) + .await; Ok(result) } @@ -2978,17 +3151,24 @@ impl AuthManager { &self, auth: &ChatgptAuth, refresh_token: String, + refresh_file_guard: &AuthRefreshGuard, ) -> Result<(), RefreshTokenError> { let refresh_response = request_chatgpt_token_refresh(refresh_token, auth.client()).await?; - persist_tokens( - auth.storage(), - refresh_response.id_token, - refresh_response.access_token, - refresh_response.refresh_token, - ) + let storage = Arc::clone(auth.storage()); + let blocking_guard = refresh_file_guard.clone(); + run_blocking_io(move || { + persist_tokens_with_guard( + &storage, + refresh_response.id_token, + refresh_response.access_token, + refresh_response.refresh_token, + &blocking_guard, + ) + }) + .await .map_err(RefreshTokenError::from)?; - self.reload_unlocked().await; + self.reload_unlocked(Some(refresh_file_guard)).await; Ok(()) } diff --git a/codex-rs/login/src/auth/manager/codex_plus_plus/file_auth.rs b/codex-rs/login/src/auth/manager/codex_plus_plus/file_auth.rs index 5bd0df8e24c8..2e1ef11398a7 100644 --- a/codex-rs/login/src/auth/manager/codex_plus_plus/file_auth.rs +++ b/codex-rs/login/src/auth/manager/codex_plus_plus/file_auth.rs @@ -13,6 +13,8 @@ use super::super::CodexAuth; use super::super::agent_identity_authapi_base_url; use super::super::chatgpt_auth_workspace_allowed; use super::super::load_auth_from_storage; +use super::super::load_auth_from_storage_with_guard; +use crate::account_lease::AuthRefreshGuard; use crate::outbound_proxy::AuthRouteConfig; pub(in crate::auth::manager) async fn new_manager( @@ -50,9 +52,12 @@ pub(in crate::auth::manager) async fn new_manager( Ok(Some(manager)) } -pub(in crate::auth::manager) async fn load(manager: &AuthManager) -> Option { +pub(in crate::auth::manager) async fn load( + manager: &AuthManager, + guard: Option<&AuthRefreshGuard>, +) -> Option { let forced_chatgpt_workspace_id = manager.forced_chatgpt_workspace_id(); - load_auth_from_storage( + load_auth_from_storage_with_guard( &manager.active_auth_home(), manager.active_auth_credentials_store_mode(), forced_chatgpt_workspace_id.as_deref(), @@ -60,6 +65,7 @@ pub(in crate::auth::manager) async fn load(manager: &AuthManager) -> Option) -> bool { root_auth .and_then(CodexAuth::get_current_auth_json) @@ -61,7 +57,7 @@ pub(in crate::auth::manager) struct ManagedAuthRefreshLocks { account_homes: Vec, index_readable: bool, index_guard: Option, - _refresh_guards: Vec, + refresh_guards: Vec, } impl ManagedAuthRefreshLocks { @@ -69,6 +65,16 @@ impl ManagedAuthRefreshLocks { &self.account_homes } + pub(in crate::auth::manager) fn guard_for( + &self, + auth_home: &Path, + ) -> std::io::Result<&AuthRefreshGuard> { + self.refresh_guards + .iter() + .find(|guard| guard.ensure_matches(auth_home).is_ok()) + .ok_or_else(|| std::io::Error::other("auth refresh guard is missing")) + } + pub(in crate::auth::manager) fn disable_all(&self) -> std::io::Result { if self.index_guard.is_none() { return Err(std::io::Error::other("account index lock is not held")); @@ -98,7 +104,7 @@ impl ManagedAuthRefreshLocks { impl AuthManager { pub(in crate::auth::manager) async fn acquire_refresh_file_lock( &self, - ) -> Result, RefreshTokenError> { + ) -> Result, RefreshTokenError> { if self.has_external_auth() { return Ok(None); } @@ -123,6 +129,10 @@ impl AuthManager { &self, locks: &ManagedAuthRefreshLocks, ) { + let Ok(root_guard) = locks.guard_for(&self.codex_home) else { + tracing::warn!("auth refresh guard is missing during revocation"); + return; + }; let mut auth_snapshots = Vec::new(); if let Some(auth) = self .auth_cached() @@ -135,6 +145,7 @@ impl AuthManager { &self.codex_home, AuthCredentialsStoreMode::Ephemeral, AuthKeyringBackendKind::default(), + root_guard, ) .filter(|auth| !is_root_account_marker(auth)) { @@ -145,16 +156,22 @@ impl AuthManager { &self.codex_home, self.auth_credentials_store_mode, self.keyring_backend_kind, + root_guard, ) .filter(|auth| !is_root_account_marker(auth)) { auth_snapshots.push(auth); } for account_home in locks.account_homes() { + let Ok(guard) = locks.guard_for(account_home) else { + tracing::warn!(auth_home = %account_home.display(), "auth refresh guard is missing during revocation"); + continue; + }; if let Some(auth) = load_auth_snapshot( account_home, AuthCredentialsStoreMode::File, AuthKeyringBackendKind::default(), + guard, ) { auth_snapshots.push(auth); } @@ -179,6 +196,7 @@ impl AuthManager { &self, result: Result<(), RefreshTokenError>, attempted_account_id: Option, + guard: AuthRefreshGuard, ) -> Result<(), RefreshTokenError> { let terminal = matches!( result @@ -200,7 +218,7 @@ impl AuthManager { .as_ref() .and_then(CodexAuth::get_account_id); if matches!( - self.reload_if_account_id_matches(expected_account_id.as_deref()) + self.reload_if_account_id_matches(expected_account_id.as_deref(), Some(&guard)) .await, ReloadOutcome::ReloadedChanged ) { @@ -211,6 +229,7 @@ impl AuthManager { AccountStore::new(self.codex_home.clone()) .record_login_required(&attempted_account_id) .map_err(RefreshTokenError::Transient)?; + drop(guard); if self.active_account_id().as_ref() == Some(&attempted_account_id) { self.move_off_imported_account_requiring_login(attempted_account_id) .await @@ -221,12 +240,13 @@ impl AuthManager { pub(in crate::auth::manager) async fn reconcile_imported_account_refresh_readiness( &self, - ) -> Result { + guard: Option, + ) -> Result, RefreshTokenError> { let Some(active_account_id) = self.active_account_id() else { - return Ok(ImportedAccountRefreshReadiness::Ready); + return Ok(guard); }; if self.active_auth_home() == self.codex_home { - return Ok(ImportedAccountRefreshReadiness::Ready); + return Ok(guard); } let login_required = AccountStore::new(self.codex_home.clone()) .list() @@ -235,28 +255,31 @@ impl AuthManager { .find(|account| account.id == active_account_id) .is_some_and(|account| account.login_required); if !login_required { - return Ok(ImportedAccountRefreshReadiness::Ready); + return Ok(guard); } + drop(guard); self.move_off_imported_account_requiring_login(active_account_id) .await?; - Ok(ImportedAccountRefreshReadiness::Recovered) + Ok(None) } pub(in crate::auth::manager) fn logout_all_managed_auth( &self, auth_locks: &ManagedAuthRefreshLocks, ) -> std::io::Result { - let mut removed = logout_all_stores( + let mut removed = logout_all_stores_with_guard( &self.codex_home, self.auth_credentials_store_mode, self.keyring_backend_kind, + auth_locks.guard_for(&self.codex_home)?, )?; for account_home in auth_locks.account_homes() { - removed |= logout_all_stores( + removed |= logout_all_stores_with_guard( account_home, AuthCredentialsStoreMode::File, AuthKeyringBackendKind::default(), + auth_locks.guard_for(account_home)?, )?; } removed |= auth_locks.disable_all()?; @@ -292,8 +315,8 @@ impl AuthManager { } } -fn acquire_refresh_file_lock(auth_home: &Path) -> std::io::Result { - AccountLease::acquire_auth_refresh(auth_home) +fn acquire_refresh_file_lock(auth_home: &Path) -> std::io::Result { + AuthRefreshGuard::acquire(auth_home) } fn acquire_managed_auth_refresh_locks( @@ -318,7 +341,7 @@ fn acquire_managed_auth_refresh_locks( account_homes, index_readable, index_guard: Some(index_guard), - _refresh_guards: refresh_guards, + refresh_guards, }); } } @@ -347,8 +370,9 @@ fn load_auth_snapshot( auth_home: &Path, store_mode: AuthCredentialsStoreMode, keyring_backend_kind: AuthKeyringBackendKind, + guard: &AuthRefreshGuard, ) -> Option { - match load_auth_dot_json(auth_home, store_mode, keyring_backend_kind) { + match load_auth_dot_json_with_guard(auth_home, store_mode, keyring_backend_kind, guard) { Ok(auth) => auth, Err(err) => { tracing::warn!( diff --git a/codex-rs/login/src/auth/storage.rs b/codex-rs/login/src/auth/storage.rs index ae195f3bff62..4a2757474e9e 100644 --- a/codex-rs/login/src/auth/storage.rs +++ b/codex-rs/login/src/auth/storage.rs @@ -35,6 +35,11 @@ use codex_secrets::SecretsBackendKind; use codex_secrets::SecretsManager; use once_cell::sync::Lazy; +mod codex_plus_plus; + +use crate::account_lease::AuthRefreshGuard; +use codex_plus_plus::FileAuthorityMarker; + /// Expected structure for $CODEX_HOME/auth.json. #[derive(Deserialize, Serialize, Clone, Debug, PartialEq)] pub struct AuthDotJson { @@ -162,8 +167,24 @@ pub(super) fn delete_file_if_exists(codex_home: &Path) -> std::io::Result pub(super) trait AuthStorageBackend: Debug + Send + Sync { fn load(&self) -> std::io::Result>; + fn load_with_guard(&self, _guard: &AuthRefreshGuard) -> std::io::Result> { + self.load() + } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()>; + fn save_with_guard( + &self, + auth: &AuthDotJson, + _guard: &AuthRefreshGuard, + ) -> std::io::Result<()> { + self.save(auth) + } + fn save_preserving_file(&self, auth: &AuthDotJson) -> std::io::Result<()> { + self.save(auth) + } fn delete(&self) -> std::io::Result; + fn delete_with_guard(&self, _guard: &AuthRefreshGuard) -> std::io::Result { + self.delete() + } } #[derive(Clone, Debug)] @@ -219,7 +240,9 @@ impl AuthStorageBackend for FileAuthStorage { } fn delete(&self) -> std::io::Result { - delete_file_if_exists(&self.codex_home) + let file_removed = delete_file_if_exists(&self.codex_home)?; + let marker_removed = FileAuthorityMarker::new(&self.codex_home).clear()?; + Ok(marker_removed || file_removed) } } @@ -286,6 +309,33 @@ impl DirectKeyringAuthStorage { } } } + + fn save_guarded(&self, auth: &AuthDotJson) -> std::io::Result<()> { + let marker = FileAuthorityMarker::new(&self.codex_home); + marker.prepare_keyring_save(auth)?; + self.save_preserving_file(auth)?; + delete_file_if_exists(&self.codex_home)?; + marker.clear()?; + Ok(()) + } + + fn delete_guarded(&self) -> std::io::Result { + let file_removed = delete_file_if_exists(&self.codex_home); + let keyring_removed = self.delete_keyring(); + let file_removed = file_removed?; + let keyring_removed = keyring_removed?; + let marker_removed = FileAuthorityMarker::new(&self.codex_home).clear()?; + Ok(keyring_removed || marker_removed || file_removed) + } + + fn delete_keyring(&self) -> std::io::Result { + let key = compute_store_key(&self.codex_home)?; + self.keyring_store + .delete(KEYRING_SERVICE, &key) + .map_err(|err| { + std::io::Error::other(format!("failed to delete auth from keyring: {err}")) + }) + } } impl AuthStorageBackend for DirectKeyringAuthStorage { @@ -295,26 +345,30 @@ impl AuthStorageBackend for DirectKeyringAuthStorage { } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.save_with_guard(auth, &guard) + } + + fn save_with_guard(&self, auth: &AuthDotJson, guard: &AuthRefreshGuard) -> std::io::Result<()> { + guard.ensure_matches(&self.codex_home)?; + self.save_guarded(auth) + } + + fn save_preserving_file(&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)?; - if let Err(err) = delete_file_if_exists(&self.codex_home) { - warn!("failed to remove CLI auth fallback file: {err}"); - } - Ok(()) + self.save_to_keyring(&key, &serialized) } 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 file_removed = delete_file_if_exists(&self.codex_home)?; - Ok(keyring_removed || file_removed) + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.delete_with_guard(&guard) + } + + fn delete_with_guard(&self, guard: &AuthRefreshGuard) -> std::io::Result { + guard.ensure_matches(&self.codex_home)?; + self.delete_guarded() } } @@ -349,6 +403,45 @@ impl SecretsKeyringAuthStorage { secrets_manager, } } + + fn save_to_keyring(&self, auth: &AuthDotJson) -> std::io::Result<()> { + let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; + self.secrets_manager + .set(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME, &serialized) + .map_err(|err| { + let message = + format!("failed to write OAuth tokens to encrypted auth storage: {err}"); + warn!("{message}"); + std::io::Error::other(message) + }) + } + + fn save_guarded(&self, auth: &AuthDotJson) -> std::io::Result<()> { + let marker = FileAuthorityMarker::new(&self.codex_home); + marker.prepare_keyring_save(auth)?; + self.save_to_keyring(auth)?; + delete_file_if_exists(&self.codex_home)?; + marker.clear()?; + Ok(()) + } + + fn delete_guarded(&self) -> std::io::Result { + let file_removed = delete_file_if_exists(&self.codex_home); + let keyring_removed = self + .secrets_manager + .delete(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME) + .map_err(|err| { + std::io::Error::other(format!( + "failed to delete auth from encrypted auth storage: {err}" + )) + }); + let direct_removed = self.direct_storage.delete_keyring(); + let file_removed = file_removed?; + let keyring_removed = keyring_removed?; + let direct_removed = direct_removed?; + let marker_removed = FileAuthorityMarker::new(&self.codex_home).clear()?; + Ok(keyring_removed || direct_removed || marker_removed || file_removed) + } } impl AuthStorageBackend for SecretsKeyringAuthStorage { @@ -371,40 +464,36 @@ impl AuthStorageBackend for SecretsKeyringAuthStorage { } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { - let serialized = serde_json::to_string(auth).map_err(std::io::Error::other)?; - self.secrets_manager - .set(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME, &serialized) - .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}"); - } - Ok(()) + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.save_with_guard(auth, &guard) + } + + fn save_with_guard(&self, auth: &AuthDotJson, guard: &AuthRefreshGuard) -> std::io::Result<()> { + guard.ensure_matches(&self.codex_home)?; + self.save_guarded(auth) + } + + fn save_preserving_file(&self, auth: &AuthDotJson) -> std::io::Result<()> { + self.save_to_keyring(auth) } fn delete(&self) -> std::io::Result { - let keyring_removed = self - .secrets_manager - .delete(&SecretScope::Global, &CODEX_AUTH_SECRET_NAME) - .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)?; - let direct_removed = self.direct_storage.delete()?; - Ok(keyring_removed || file_removed || direct_removed) + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.delete_with_guard(&guard) + } + + fn delete_with_guard(&self, guard: &AuthRefreshGuard) -> std::io::Result { + guard.ensure_matches(&self.codex_home)?; + self.delete_guarded() } } #[derive(Clone, Debug)] struct AutoAuthStorage { + codex_home: PathBuf, keyring_storage: Arc, file_storage: Arc, + file_authority: FileAuthorityMarker, } impl AutoAuthStorage { @@ -413,42 +502,61 @@ impl AutoAuthStorage { keyring_store: Arc, keyring_backend_kind: AuthKeyringBackendKind, ) -> Self { + let file_authority = FileAuthorityMarker::new(&codex_home); Self { keyring_storage: create_keyring_auth_storage( codex_home.clone(), keyring_store, keyring_backend_kind, ), - file_storage: Arc::new(FileAuthStorage::new(codex_home)), + file_storage: Arc::new(FileAuthStorage::new(codex_home.clone())), + file_authority, + codex_home, } } + + fn load_guarded(&self) -> std::io::Result> { + codex_plus_plus::load_auto_auth(self) + } + + fn save_guarded(&self, auth: &AuthDotJson, guard: &AuthRefreshGuard) -> std::io::Result<()> { + codex_plus_plus::save_auto_auth(self, auth, guard) + } + + fn delete_guarded(&self, guard: &AuthRefreshGuard) -> std::io::Result { + codex_plus_plus::delete_auto_auth(self, guard) + } } impl AuthStorageBackend for AutoAuthStorage { fn load(&self) -> std::io::Result> { - match self.keyring_storage.load() { - Ok(Some(auth)) => Ok(Some(auth)), - Ok(None) => self.file_storage.load(), - Err(err) => { - warn!("failed to load CLI auth from keyring, falling back to file storage: {err}"); - self.file_storage.load() - } - } + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.load_with_guard(&guard) + } + + fn load_with_guard(&self, guard: &AuthRefreshGuard) -> std::io::Result> { + guard.ensure_matches(&self.codex_home)?; + self.load_guarded() } fn save(&self, auth: &AuthDotJson) -> std::io::Result<()> { - match self.keyring_storage.save(auth) { - Ok(()) => Ok(()), - Err(err) => { - warn!("failed to save auth to keyring, falling back to file storage: {err}"); - self.file_storage.save(auth) - } - } + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.save_with_guard(auth, &guard) + } + + fn save_with_guard(&self, auth: &AuthDotJson, guard: &AuthRefreshGuard) -> std::io::Result<()> { + guard.ensure_matches(&self.codex_home)?; + self.save_guarded(auth, guard) } fn delete(&self) -> std::io::Result { - // Keyring storage will delete from disk as well - self.keyring_storage.delete() + let guard = AuthRefreshGuard::acquire(&self.codex_home)?; + self.delete_with_guard(&guard) + } + + fn delete_with_guard(&self, guard: &AuthRefreshGuard) -> std::io::Result { + guard.ensure_matches(&self.codex_home)?; + self.delete_guarded(guard) } } @@ -504,7 +612,7 @@ pub(super) fn create_auth_storage( create_auth_storage_with_store(codex_home, mode, keyring_store, keyring_backend_kind) } -fn create_auth_storage_with_store( +pub(super) fn create_auth_storage_with_store( codex_home: PathBuf, mode: AuthCredentialsStoreMode, keyring_store: Arc, diff --git a/codex-rs/login/src/auth/storage/codex_plus_plus/auto_auth.rs b/codex-rs/login/src/auth/storage/codex_plus_plus/auto_auth.rs new file mode 100644 index 000000000000..d638665ddd2b --- /dev/null +++ b/codex-rs/login/src/auth/storage/codex_plus_plus/auto_auth.rs @@ -0,0 +1,55 @@ +use super::super::AuthDotJson; +use super::super::AuthStorageBackend; +use super::super::AutoAuthStorage; +use crate::account_lease::AuthRefreshGuard; +use tracing::warn; + +pub(in crate::auth::storage) fn load( + storage: &AutoAuthStorage, +) -> std::io::Result> { + if let Some(auth) = storage + .file_authority + .load_authoritative(&storage.file_storage)? + { + return Ok(Some(auth)); + } + + match storage.keyring_storage.load() { + Ok(Some(auth)) => Ok(Some(auth)), + Ok(None) => storage.file_storage.load(), + Err(err) => { + warn!("failed to load CLI auth from keyring, falling back to file storage: {err}"); + storage.file_storage.load() + } + } +} + +pub(in crate::auth::storage) fn save( + storage: &AutoAuthStorage, + auth: &AuthDotJson, + guard: &AuthRefreshGuard, +) -> std::io::Result<()> { + if storage + .file_authority + .save_if_authoritative(&storage.file_storage, auth)? + { + return Ok(()); + } + + match storage.keyring_storage.save_with_guard(auth, guard) { + Ok(()) => Ok(()), + Err(err) => { + warn!("failed to save auth to keyring, falling back to file storage: {err}"); + storage + .file_authority + .save_fallback(&storage.file_storage, auth) + } + } +} + +pub(in crate::auth::storage) fn delete( + storage: &AutoAuthStorage, + guard: &AuthRefreshGuard, +) -> std::io::Result { + storage.keyring_storage.delete_with_guard(guard) +} diff --git a/codex-rs/login/src/auth/storage/codex_plus_plus/file_authority.rs b/codex-rs/login/src/auth/storage/codex_plus_plus/file_authority.rs new file mode 100644 index 000000000000..7f0659d5bf3d --- /dev/null +++ b/codex-rs/login/src/auth/storage/codex_plus_plus/file_authority.rs @@ -0,0 +1,105 @@ +use std::fs::OpenOptions; +use std::io; +#[cfg(unix)] +use std::os::unix::fs::OpenOptionsExt; +use std::path::Path; +use std::path::PathBuf; + +use super::super::AuthDotJson; +use super::super::AuthStorageBackend; +use super::super::FileAuthStorage; + +#[derive(Clone, Debug)] +pub(in super::super) struct FileAuthorityMarker { + path: PathBuf, +} + +impl FileAuthorityMarker { + pub(in super::super) fn new(codex_home: &Path) -> Self { + Self { + path: codex_home.join(".codex-plus-plus-auth-file-authority"), + } + } + + pub(in super::super) fn is_active(&self) -> io::Result { + match self.path.metadata() { + Ok(metadata) if metadata.is_file() => Ok(true), + Ok(_) => Err(io::Error::other( + "auth file-authority marker is not a regular file", + )), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false), + Err(err) => Err(err), + } + } + + pub(in super::super) fn load_authoritative( + &self, + file_storage: &FileAuthStorage, + ) -> io::Result> { + if !self.is_active()? { + return Ok(None); + } + let auth = file_storage.load()?.ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + "authoritative auth file is missing", + ) + })?; + Ok(Some(auth)) + } + + pub(in super::super) fn save_if_authoritative( + &self, + file_storage: &FileAuthStorage, + auth: &AuthDotJson, + ) -> io::Result { + if !self.is_active()? { + return Ok(false); + } + file_storage.save(auth)?; + Ok(true) + } + + pub(in super::super) fn save_fallback( + &self, + file_storage: &FileAuthStorage, + auth: &AuthDotJson, + ) -> io::Result<()> { + self.activate()?; + file_storage.save(auth) + } + + pub(in super::super) fn prepare_keyring_save(&self, auth: &AuthDotJson) -> io::Result<()> { + if self.is_active()? { + let codex_home = self.path.parent().ok_or(io::ErrorKind::InvalidInput)?; + FileAuthStorage::new(codex_home.to_path_buf()).save(auth)?; + } + Ok(()) + } + + pub(in super::super) fn activate(&self) -> io::Result<()> { + let parent = self.path.parent().ok_or(io::ErrorKind::InvalidInput)?; + std::fs::create_dir_all(parent)?; + let mut options = OpenOptions::new(); + options.create(true).write(true).truncate(false); + #[cfg(unix)] + options.mode(0o600); + options.open(&self.path)?.sync_all()?; + #[cfg(unix)] + std::fs::File::open(parent)?.sync_all()?; + Ok(()) + } + + pub(in super::super) fn clear(&self) -> io::Result { + match std::fs::remove_file(&self.path) { + Ok(()) => { + #[cfg(unix)] + std::fs::File::open(self.path.parent().ok_or(io::ErrorKind::InvalidInput)?)? + .sync_all()?; + Ok(true) + } + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(false), + Err(err) => Err(err), + } + } +} diff --git a/codex-rs/login/src/auth/storage/codex_plus_plus/mod.rs b/codex-rs/login/src/auth/storage/codex_plus_plus/mod.rs new file mode 100644 index 000000000000..425115be8467 --- /dev/null +++ b/codex-rs/login/src/auth/storage/codex_plus_plus/mod.rs @@ -0,0 +1,7 @@ +mod auto_auth; +mod file_authority; + +pub(super) use auto_auth::delete as delete_auto_auth; +pub(super) use auto_auth::load as load_auto_auth; +pub(super) use auto_auth::save as save_auto_auth; +pub(super) use file_authority::FileAuthorityMarker; diff --git a/codex-rs/login/src/auth/storage_tests.rs b/codex-rs/login/src/auth/storage_tests.rs index 647af79b656a..4ef4c1792102 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -9,6 +9,7 @@ use codex_secrets::SecretsManager; use codex_secrets::compute_keyring_account; use pretty_assertions::assert_eq; use serde_json::json; +use std::time::Duration; use tempfile::tempdir; use codex_keyring_store::tests::MockKeyringStore; @@ -274,10 +275,13 @@ fn file_storage_delete_removes_auth_file() -> anyhow::Result<()> { ); storage.save(&auth_dot_json)?; assert!(dir.path().join("auth.json").exists()); + let marker = FileAuthorityMarker::new(dir.path()); + marker.activate()?; let storage = FileAuthStorage::new(dir.path().to_path_buf()); let removed = storage.delete()?; assert!(removed); assert!(!dir.path().join("auth.json").exists()); + assert!(!marker.is_active()?); Ok(()) } @@ -485,10 +489,13 @@ fn direct_keyring_auth_storage_saves_legacy_keyring_entry() -> anyhow::Result<() ); let auth_file = get_auth_file(codex_home.path()); std::fs::write(&auth_file, "stale")?; + let marker = FileAuthorityMarker::new(codex_home.path()); + marker.activate()?; let auth = auth_with_prefix("direct"); storage.save(&auth)?; + assert!(!marker.is_active()?); let legacy_key = compute_store_key(codex_home.path())?; let saved_value = mock_keyring .saved_value(&legacy_key) @@ -503,6 +510,25 @@ fn direct_keyring_auth_storage_saves_legacy_keyring_entry() -> anyhow::Result<() Ok(()) } +#[test] +fn keyring_auth_storage_save_propagates_fallback_cleanup_failure() -> anyhow::Result<()> { + for backend in [ + AuthKeyringBackendKind::Direct, + AuthKeyringBackendKind::Secrets, + ] { + let codex_home = tempdir()?; + std::fs::create_dir(get_auth_file(codex_home.path()))?; + let storage = create_auth_storage_with_store( + codex_home.path().to_path_buf(), + AuthCredentialsStoreMode::Keyring, + Arc::new(MockKeyringStore::default()), + backend, + ); + assert!(storage.save(&auth_with_prefix("cleanup-failure")).is_err()); + } + Ok(()) +} + #[test] fn direct_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { let codex_home = tempdir()?; @@ -534,6 +560,26 @@ fn direct_keyring_auth_storage_delete_removes_keyring_and_file() -> anyhow::Resu Ok(()) } +#[test] +fn direct_keyring_auth_storage_delete_propagates_marker_clear_failure() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let storage = DirectKeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(MockKeyringStore::default()), + ); + let auth_file = get_auth_file(codex_home.path()); + std::fs::write(&auth_file, "fallback")?; + std::fs::create_dir( + codex_home + .path() + .join(".codex-plus-plus-auth-file-authority"), + )?; + + assert!(storage.delete().is_err()); + assert!(!auth_file.exists()); + Ok(()) +} + #[test] fn factory_uses_secrets_backend_only_when_requested() -> anyhow::Result<()> { let direct_home = tempdir()?; @@ -582,6 +628,8 @@ fn secrets_keyring_auth_storage_save_persists_and_removes_fallback_file() -> any ); let auth_file = get_auth_file(codex_home.path()); std::fs::write(&auth_file, "stale")?; + let marker = FileAuthorityMarker::new(codex_home.path()); + marker.activate()?; let auth = AuthDotJson { auth_mode: Some(AuthMode::Chatgpt), openai_api_key: None, @@ -599,6 +647,7 @@ fn secrets_keyring_auth_storage_save_persists_and_removes_fallback_file() -> any storage.save(&auth)?; + assert!(!marker.is_active()?); assert_keyring_saved_auth_and_removed_fallback(&mock_keyring, codex_home.path(), &auth)?; Ok(()) } @@ -665,6 +714,35 @@ fn secrets_keyring_auth_storage_delete_removes_legacy_direct_keyring_entry() -> Ok(()) } +#[test] +fn secrets_keyring_auth_storage_delete_attempts_keyring_cleanup_after_file_error() +-> anyhow::Result<()> { + let codex_home = tempdir()?; + let mock_keyring = MockKeyringStore::default(); + let auth = auth_with_prefix("delete-error"); + let direct_storage = DirectKeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + direct_storage.save(&auth)?; + let storage = SecretsKeyringAuthStorage::new( + codex_home.path().to_path_buf(), + Arc::new(mock_keyring.clone()), + ); + seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &auth)?; + let auth_file = get_auth_file(codex_home.path()); + std::fs::create_dir(&auth_file)?; + let marker = FileAuthorityMarker::new(codex_home.path()); + marker.activate()?; + + assert!(storage.delete().is_err()); + assert_eq!(storage.load()?, None); + assert_eq!(direct_storage.load()?, None); + assert!(auth_file.is_dir()); + assert!(marker.is_active()?); + Ok(()) +} + #[test] fn auto_auth_storage_load_prefers_keyring_value() -> anyhow::Result<()> { let codex_home = tempdir()?; @@ -803,3 +881,105 @@ fn auto_auth_storage_delete_removes_keyring_and_file() -> anyhow::Result<()> { ); Ok(()) } + +fn auto_auth_storage_with_mock(codex_home: &Path) -> (AutoAuthStorage, MockKeyringStore) { + let mock_keyring = MockKeyringStore::default(); + let storage = AutoAuthStorage::new( + codex_home.to_path_buf(), + Arc::new(mock_keyring.clone()), + AuthKeyringBackendKind::Secrets, + ); + (storage, mock_keyring) +} + +fn set_auto_keyring_error(mock_keyring: &MockKeyringStore, codex_home: &Path, operation: &str) { + mock_keyring.set_error( + &compute_keyring_account(codex_home), + KeyringError::Invalid("error".into(), operation.into()), + ); +} + +#[test] +fn auto_auth_storage_marks_file_authoritative_before_fallback_save() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let (storage, mock_keyring) = auto_auth_storage_with_mock(codex_home.path()); + set_auto_keyring_error(&mock_keyring, codex_home.path(), "save"); + std::fs::create_dir(get_auth_file(codex_home.path()))?; + assert!(storage.save(&auth_with_prefix("fallback")).is_err()); + assert!(storage.file_authority.is_active()?); + Ok(()) +} + +#[test] +fn auto_auth_storage_load_preserves_file_authority() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let (storage, _) = auto_auth_storage_with_mock(codex_home.path()); + let expected = auth_with_prefix("file"); + storage.file_authority.activate()?; + storage.file_storage.save(&expected)?; + assert_eq!(storage.load()?, Some(expected)); + assert!(storage.file_authority.is_active()?); + Ok(()) +} + +#[test] +fn auto_auth_storage_save_preserves_file_authority() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let (storage, _) = auto_auth_storage_with_mock(codex_home.path()); + let expected = auth_with_prefix("file"); + storage.file_authority.activate()?; + storage.save(&expected)?; + assert_eq!(storage.load()?, Some(expected)); + assert!(storage.file_authority.is_active()?); + Ok(()) +} + +#[test] +fn auto_auth_storage_marked_file_errors_never_return_keyring_auth() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let (storage, mock_keyring) = auto_auth_storage_with_mock(codex_home.path()); + seed_secrets_backend_with_auth( + &mock_keyring, + codex_home.path(), + &auth_with_prefix("stale-keyring"), + )?; + storage.file_authority.activate()?; + std::fs::write(get_auth_file(codex_home.path()), "not json")?; + assert!(storage.load().is_err()); + Ok(()) +} + +#[test] +fn auto_auth_storage_concurrent_load_waits_for_refresh_guard() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let (storage, mock_keyring) = auto_auth_storage_with_mock(codex_home.path()); + let expected = auth_with_prefix("keyring"); + seed_secrets_backend_with_auth(&mock_keyring, codex_home.path(), &expected)?; + let guard = AuthRefreshGuard::acquire(codex_home.path())?; + let (result_tx, result_rx) = std::sync::mpsc::channel(); + let loader = std::thread::spawn(move || result_tx.send(storage.load())); + assert!(result_rx.recv_timeout(Duration::from_millis(100)).is_err()); + drop(guard); + + let loaded = result_rx.recv_timeout(Duration::from_secs(2))??; + assert_eq!(loaded, Some(expected)); + assert!(loader.join().is_ok()); + Ok(()) +} + +#[test] +fn auto_auth_storage_delete_clears_marker_last() -> anyhow::Result<()> { + let codex_home = tempdir()?; + let (storage, mock_keyring) = auto_auth_storage_with_mock(codex_home.path()); + seed_secrets_backend_and_fallback_auth_file_for_delete( + &mock_keyring, + codex_home.path(), + &auth_with_prefix("delete"), + )?; + storage.file_authority.activate()?; + set_auto_keyring_error(&mock_keyring, codex_home.path(), "delete"); + + assert!(storage.delete().is_err()); + assert!(storage.file_authority.is_active()?); + Ok(()) +}