-
Notifications
You must be signed in to change notification settings - Fork 14.3k
refactor: unify external auth resolution #31421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
74e1df4
1b6c68b
77f76f1
544653f
8393b8e
9f8d177
ce1f9e4
b31594d
7077b30
3ae9aab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| use std::sync::Arc; | ||
| use std::sync::RwLock; | ||
|
|
||
| use codex_app_server_protocol::ChatgptAuthTokensRefreshParams; | ||
| use codex_app_server_protocol::ChatgptAuthTokensRefreshReason; | ||
| use codex_app_server_protocol::ChatgptAuthTokensRefreshResponse; | ||
| use codex_app_server_protocol::ServerRequestPayload; | ||
| use codex_login::CodexAuth; | ||
| use codex_login::ExternalAuthFuture; | ||
| use codex_login::auth::ExternalAuth; | ||
| use codex_login::auth::ExternalAuthRefreshContext; | ||
| use codex_login::auth::ExternalAuthRefreshReason; | ||
| use tokio::time::Duration; | ||
| use tokio::time::timeout; | ||
|
|
||
| use crate::outgoing_message::OutgoingMessageSender; | ||
|
|
||
| const EXTERNAL_AUTH_REFRESH_TIMEOUT: Duration = Duration::from_secs(10); | ||
|
|
||
| pub(crate) struct ExternalAuthBridge { | ||
| outgoing: Arc<OutgoingMessageSender>, | ||
| auth: RwLock<CodexAuth>, | ||
| } | ||
|
|
||
| impl ExternalAuthBridge { | ||
| pub(crate) fn new(outgoing: Arc<OutgoingMessageSender>, auth: CodexAuth) -> Self { | ||
| Self { | ||
| outgoing, | ||
| auth: RwLock::new(auth), | ||
| } | ||
| } | ||
|
|
||
| async fn refresh(&self, context: ExternalAuthRefreshContext) -> std::io::Result<CodexAuth> { | ||
| let reason = match context.reason { | ||
| ExternalAuthRefreshReason::Unauthorized => ChatgptAuthTokensRefreshReason::Unauthorized, | ||
| }; | ||
| let params = ChatgptAuthTokensRefreshParams { | ||
| reason, | ||
| previous_account_id: context.previous_account_id, | ||
| }; | ||
|
|
||
| let (request_id, rx) = self | ||
| .outgoing | ||
| .send_request(ServerRequestPayload::ChatgptAuthTokensRefresh(params)) | ||
| .await; | ||
| let result = match timeout(EXTERNAL_AUTH_REFRESH_TIMEOUT, rx).await { | ||
| Ok(result) => { | ||
| let result = result.map_err(|err| { | ||
| std::io::Error::other(format!("auth refresh request canceled: {err}")) | ||
| })?; | ||
| result.map_err(|err| { | ||
| std::io::Error::other(format!( | ||
| "auth refresh request failed: code={} message={}", | ||
| err.code, err.message | ||
| )) | ||
| })? | ||
| } | ||
| Err(_) => { | ||
| let _canceled = self.outgoing.cancel_request(&request_id).await; | ||
| return Err(std::io::Error::other(format!( | ||
| "auth refresh request timed out after {}s", | ||
| EXTERNAL_AUTH_REFRESH_TIMEOUT.as_secs() | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| let response: ChatgptAuthTokensRefreshResponse = | ||
| serde_json::from_value(result).map_err(std::io::Error::other)?; | ||
| let auth = CodexAuth::from_external_chatgpt_tokens( | ||
| response.access_token.as_str(), | ||
| response.chatgpt_account_id.as_str(), | ||
| response.chatgpt_plan_type.as_deref(), | ||
| )?; | ||
| *self | ||
| .auth | ||
| .write() | ||
| .map_err(|_| std::io::Error::other("external auth lock is poisoned"))? = auth.clone(); | ||
|
pakrym-oai marked this conversation as resolved.
|
||
| Ok(auth) | ||
| } | ||
| } | ||
|
|
||
| impl ExternalAuth for ExternalAuthBridge { | ||
| fn resolve(&self) -> ExternalAuthFuture<'_, CodexAuth> { | ||
| Box::pin(async { | ||
| self.auth | ||
| .read() | ||
| .map(|auth| auth.clone()) | ||
| .map_err(|_| std::io::Error::other("external auth lock is poisoned")) | ||
| }) | ||
| } | ||
|
|
||
| fn refresh(&self, context: ExternalAuthRefreshContext) -> ExternalAuthFuture<'_, CodexAuth> { | ||
| Box::pin(ExternalAuthBridge::refresh(self, context)) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,9 +50,6 @@ use crate::transport::AppServerTransport; | |
| use crate::transport::RemoteControlHandle; | ||
| use codex_analytics::AnalyticsEventsClient; | ||
| use codex_analytics::AppServerRpcTransport; | ||
| use codex_app_server_protocol::ChatgptAuthTokensRefreshParams; | ||
| use codex_app_server_protocol::ChatgptAuthTokensRefreshReason; | ||
| use codex_app_server_protocol::ChatgptAuthTokensRefreshResponse; | ||
| use codex_app_server_protocol::ClientNotification; | ||
| use codex_app_server_protocol::ClientRequest; | ||
| use codex_app_server_protocol::ClientResponsePayload; | ||
|
|
@@ -63,7 +60,6 @@ use codex_app_server_protocol::JSONRPCErrorError; | |
| use codex_app_server_protocol::JSONRPCNotification; | ||
| use codex_app_server_protocol::JSONRPCRequest; | ||
| use codex_app_server_protocol::JSONRPCResponse; | ||
| use codex_app_server_protocol::ServerRequestPayload; | ||
| use codex_app_server_protocol::experimental_required_message; | ||
| use codex_arg0::Arg0DispatchPaths; | ||
| use codex_chatgpt::workspace_settings; | ||
|
|
@@ -74,12 +70,7 @@ use codex_feedback::CodexFeedback; | |
| use codex_goal_extension::GoalService; | ||
| use codex_home::CodexHomeUserInstructionsProvider; | ||
| use codex_login::AuthManager; | ||
| use codex_login::CodexAuth; | ||
| use codex_login::auth::ExternalAuth; | ||
| use codex_login::auth::ExternalAuthRefreshContext; | ||
| use codex_login::auth::ExternalAuthRefreshReason; | ||
| use codex_protocol::ThreadId; | ||
| use codex_protocol::auth::AuthMode as LoginAuthMode; | ||
| use codex_protocol::protocol::SessionSource; | ||
| use codex_protocol::protocol::W3cTraceContext; | ||
| use codex_rollout::StateDbHandle; | ||
|
|
@@ -95,7 +86,6 @@ use tracing::Instrument; | |
|
|
||
| use crate::models_refresh_worker::ModelsRefreshWorker; | ||
|
|
||
| const EXTERNAL_AUTH_REFRESH_TIMEOUT: Duration = Duration::from_secs(10); | ||
| const CONNECTION_RPC_DRAIN_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); | ||
|
|
||
| fn deserialize_client_request( | ||
|
|
@@ -109,77 +99,6 @@ fn deserialize_client_request( | |
| }) | ||
| } | ||
|
|
||
| #[derive(Clone)] | ||
| struct ExternalAuthRefreshBridge { | ||
| outgoing: Arc<OutgoingMessageSender>, | ||
| } | ||
|
|
||
| impl ExternalAuthRefreshBridge { | ||
| fn map_reason(reason: ExternalAuthRefreshReason) -> ChatgptAuthTokensRefreshReason { | ||
| match reason { | ||
| ExternalAuthRefreshReason::Unauthorized => ChatgptAuthTokensRefreshReason::Unauthorized, | ||
| } | ||
| } | ||
|
|
||
| async fn refresh(&self, context: ExternalAuthRefreshContext) -> std::io::Result<CodexAuth> { | ||
| let params = ChatgptAuthTokensRefreshParams { | ||
| reason: Self::map_reason(context.reason), | ||
| previous_account_id: context.previous_account_id, | ||
| }; | ||
|
|
||
| let (request_id, rx) = self | ||
| .outgoing | ||
| .send_request(ServerRequestPayload::ChatgptAuthTokensRefresh(params)) | ||
| .await; | ||
|
|
||
| let result = match timeout(EXTERNAL_AUTH_REFRESH_TIMEOUT, rx).await { | ||
| Ok(result) => { | ||
| // Two failure scenarios: | ||
| // 1) `oneshot::Receiver` failed (sender dropped) => request canceled/channel closed. | ||
| // 2) client answered with JSON-RPC error payload => propagate code/message. | ||
| let result = result.map_err(|err| { | ||
| std::io::Error::other(format!("auth refresh request canceled: {err}")) | ||
| })?; | ||
| result.map_err(|err| { | ||
| std::io::Error::other(format!( | ||
| "auth refresh request failed: code={} message={}", | ||
| err.code, err.message | ||
| )) | ||
| })? | ||
| } | ||
| Err(_) => { | ||
| let _canceled = self.outgoing.cancel_request(&request_id).await; | ||
| return Err(std::io::Error::other(format!( | ||
| "auth refresh request timed out after {}s", | ||
| EXTERNAL_AUTH_REFRESH_TIMEOUT.as_secs() | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| let response: ChatgptAuthTokensRefreshResponse = | ||
| serde_json::from_value(result).map_err(std::io::Error::other)?; | ||
|
|
||
| CodexAuth::from_external_chatgpt_tokens( | ||
| response.access_token.as_str(), | ||
| response.chatgpt_account_id.as_str(), | ||
| response.chatgpt_plan_type.as_deref(), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| impl ExternalAuth for ExternalAuthRefreshBridge { | ||
| fn auth_mode(&self) -> LoginAuthMode { | ||
| LoginAuthMode::Chatgpt | ||
| } | ||
|
|
||
| fn refresh( | ||
| &self, | ||
| context: ExternalAuthRefreshContext, | ||
| ) -> codex_login::ExternalAuthFuture<'_, CodexAuth> { | ||
| Box::pin(ExternalAuthRefreshBridge::refresh(self, context)) | ||
| } | ||
| } | ||
|
|
||
| pub(crate) struct MessageProcessor { | ||
| outgoing: Arc<OutgoingMessageSender>, | ||
| models_refresh_worker: ModelsRefreshWorker, | ||
|
|
@@ -324,9 +243,6 @@ impl MessageProcessor { | |
| remote_control_handle, | ||
| plugin_startup_tasks, | ||
| } = args; | ||
| auth_manager.set_external_auth(Arc::new(ExternalAuthRefreshBridge { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point in the But in the PR as written, now the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess my higher level question is: can we make
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I guess the problem is that we start the app server even if the user is logged out and then
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, you can create AuthManager without
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see, and then there are cases like |
||
| outgoing: outgoing.clone(), | ||
| })); | ||
| let thread_state_manager = ThreadStateManager::new(); | ||
| // The thread store is intentionally process-scoped. Config reloads can | ||
| // affect per-thread behavior, but they must not move newly started, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| use super::*; | ||
| use crate::auth_mode::auth_mode_to_api; | ||
| use crate::external_auth::ExternalAuthBridge; | ||
| use chrono::DateTime; | ||
|
|
||
| mod rate_limit_resets; | ||
|
|
@@ -617,14 +618,19 @@ impl AccountRequestProcessor { | |
| ))); | ||
| } | ||
|
|
||
| login_with_chatgpt_auth_tokens( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was saving tokens to ephemeral storage directly and had normal reload pick them up instead of having external auth bridge be the source of truth. |
||
| &self.config.codex_home, | ||
| let auth = CodexAuth::from_external_chatgpt_tokens( | ||
| &access_token, | ||
| &chatgpt_account_id, | ||
| chatgpt_plan_type.as_deref(), | ||
| ) | ||
| .map_err(|err| internal_error(format!("failed to set external auth: {err}")))?; | ||
| self.auth_manager.reload().await; | ||
| self.auth_manager | ||
|
pakrym-oai marked this conversation as resolved.
|
||
| .set_external_auth(Arc::new(ExternalAuthBridge::new( | ||
| Arc::clone(&self.outgoing), | ||
| auth, | ||
| ))) | ||
|
pakrym-oai marked this conversation as resolved.
|
||
| .await | ||
| .map_err(|err| internal_error(format!("failed to set external auth: {err}")))?; | ||
| self.config_manager.replace_cloud_config_bundle_loader( | ||
| self.auth_manager.clone(), | ||
| self.config.chatgpt_base_url.clone(), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.