mirror of
https://github.com/openai/codex.git
synced 2026-05-04 21:32:21 +03:00
Refactor auth providers to mutate request headers (#17866)
## Summary - Move auth header construction into the `AuthProvider::add_auth_headers` contract. - Inline `CoreAuthProvider` header mutation in its provider impl and remove the shared header-map helper. - Update HTTP, websocket, file upload, sideband websocket, and test auth callsites to use the provider method. - Add direct coverage for `CoreAuthProvider` auth header mutation. ## Testing - `just fmt` - `cargo test -p codex-api` - `cargo test -p codex-core client::tests::auth_request_telemetry_context_tracks_attached_auth_and_retry_phase` - `cargo test -p codex-core` failed on unrelated/reproducible `tools::handlers::multi_agents::tests::multi_agent_v2_followup_task_interrupts_busy_child_without_losing_message` --------- Co-authored-by: Celia Chen <celia@openai.com>
This commit is contained in:
@@ -12,6 +12,7 @@ use codex_protocol::error::RetryLimitReachedError;
|
||||
use codex_protocol::error::UnexpectedResponseError;
|
||||
use codex_protocol::error::UsageLimitReachedError;
|
||||
use http::HeaderMap;
|
||||
use http::HeaderValue;
|
||||
use serde::Deserialize;
|
||||
use serde_json::Value;
|
||||
|
||||
@@ -200,11 +201,16 @@ impl CoreAuthProvider {
|
||||
}
|
||||
|
||||
impl ApiAuthProvider for CoreAuthProvider {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
self.token.clone()
|
||||
}
|
||||
|
||||
fn account_id(&self) -> Option<String> {
|
||||
self.account_id.clone()
|
||||
fn add_auth_headers(&self, headers: &mut HeaderMap) {
|
||||
if let Some(token) = self.token.as_ref()
|
||||
&& let Ok(header) = HeaderValue::from_str(&format!("Bearer {token}"))
|
||||
{
|
||||
let _ = headers.insert(http::header::AUTHORIZATION, header);
|
||||
}
|
||||
if let Some(account_id) = self.account_id.as_ref()
|
||||
&& let Ok(header) = HeaderValue::from_str(account_id)
|
||||
{
|
||||
let _ = headers.insert("ChatGPT-Account-ID", header);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -141,3 +141,24 @@ fn core_auth_provider_reports_when_auth_header_will_attach() {
|
||||
assert!(auth.auth_header_attached());
|
||||
assert_eq!(auth.auth_header_name(), Some("authorization"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn core_auth_provider_adds_auth_headers() {
|
||||
let auth = CoreAuthProvider::for_test(Some("access-token"), Some("workspace-123"));
|
||||
let mut headers = HeaderMap::new();
|
||||
|
||||
crate::AuthProvider::add_auth_headers(&auth, &mut headers);
|
||||
|
||||
assert_eq!(
|
||||
headers
|
||||
.get(http::header::AUTHORIZATION)
|
||||
.and_then(|value| value.to_str().ok()),
|
||||
Some("Bearer access-token")
|
||||
);
|
||||
assert_eq!(
|
||||
headers
|
||||
.get("ChatGPT-Account-ID")
|
||||
.and_then(|value| value.to_str().ok()),
|
||||
Some("workspace-123")
|
||||
);
|
||||
}
|
||||
|
||||
@@ -1,33 +1,10 @@
|
||||
use codex_client::Request;
|
||||
use http::HeaderMap;
|
||||
use http::HeaderValue;
|
||||
|
||||
/// Provides bearer and account identity information for API requests.
|
||||
/// Adds authentication headers to API requests.
|
||||
///
|
||||
/// Implementations should be cheap and non-blocking; any asynchronous
|
||||
/// refresh or I/O should be handled by higher layers before requests
|
||||
/// reach this interface.
|
||||
pub trait AuthProvider: Send + Sync {
|
||||
fn bearer_token(&self) -> Option<String>;
|
||||
fn account_id(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn add_auth_headers_to_header_map<A: AuthProvider>(auth: &A, headers: &mut HeaderMap) {
|
||||
if let Some(token) = auth.bearer_token()
|
||||
&& let Ok(header) = HeaderValue::from_str(&format!("Bearer {token}"))
|
||||
{
|
||||
let _ = headers.insert(http::header::AUTHORIZATION, header);
|
||||
}
|
||||
if let Some(account_id) = auth.account_id()
|
||||
&& let Ok(header) = HeaderValue::from_str(&account_id)
|
||||
{
|
||||
let _ = headers.insert("ChatGPT-Account-ID", header);
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn add_auth_headers<A: AuthProvider>(auth: &A, mut req: Request) -> Request {
|
||||
add_auth_headers_to_header_map(auth, &mut req.headers);
|
||||
req
|
||||
fn add_auth_headers(&self, headers: &mut HeaderMap);
|
||||
}
|
||||
|
||||
@@ -90,9 +90,7 @@ mod tests {
|
||||
struct DummyAuth;
|
||||
|
||||
impl AuthProvider for DummyAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
fn add_auth_headers(&self, _headers: &mut HeaderMap) {}
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -103,9 +103,7 @@ mod tests {
|
||||
struct DummyAuth;
|
||||
|
||||
impl AuthProvider for DummyAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
fn add_auth_headers(&self, _headers: &mut HeaderMap) {}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
|
||||
@@ -132,9 +132,7 @@ mod tests {
|
||||
struct DummyAuth;
|
||||
|
||||
impl AuthProvider for DummyAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
fn add_auth_headers(&self, _headers: &mut HeaderMap) {}
|
||||
}
|
||||
|
||||
fn provider(base_url: &str) -> Provider {
|
||||
|
||||
@@ -284,8 +284,11 @@ mod tests {
|
||||
struct DummyAuth;
|
||||
|
||||
impl AuthProvider for DummyAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
Some("test-token".to_string())
|
||||
fn add_auth_headers(&self, headers: &mut HeaderMap) {
|
||||
headers.insert(
|
||||
http::header::AUTHORIZATION,
|
||||
HeaderValue::from_static("Bearer test-token"),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
use crate::auth::AuthProvider;
|
||||
use crate::auth::add_auth_headers_to_header_map;
|
||||
use crate::common::ResponseEvent;
|
||||
use crate::common::ResponseStream;
|
||||
use crate::common::ResponsesWsRequest;
|
||||
@@ -310,7 +309,7 @@ impl<A: AuthProvider> ResponsesWebsocketClient<A> {
|
||||
|
||||
let mut headers =
|
||||
merge_request_headers(&self.provider.headers, extra_headers, default_headers);
|
||||
add_auth_headers_to_header_map(&self.auth, &mut headers);
|
||||
self.auth.add_auth_headers(&mut headers);
|
||||
|
||||
let (stream, server_reasoning_included, models_etag, server_model) =
|
||||
connect_websocket(ws_url, headers, turn_state.clone()).await?;
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
use crate::auth::AuthProvider;
|
||||
use crate::auth::add_auth_headers;
|
||||
use crate::error::ApiError;
|
||||
use crate::provider::Provider;
|
||||
use crate::telemetry::run_with_request_telemetry;
|
||||
@@ -56,7 +55,8 @@ impl<T: HttpTransport, A: AuthProvider> EndpointSession<T, A> {
|
||||
if let Some(body) = body {
|
||||
req.body = Some(RequestBody::Json(body.clone()));
|
||||
}
|
||||
add_auth_headers(&self.auth, req)
|
||||
self.auth.add_auth_headers(&mut req.headers);
|
||||
req
|
||||
}
|
||||
|
||||
pub(crate) async fn execute(
|
||||
|
||||
@@ -256,17 +256,14 @@ fn authorized_request(
|
||||
method: reqwest::Method,
|
||||
url: &str,
|
||||
) -> reqwest::RequestBuilder {
|
||||
let mut headers = http::HeaderMap::new();
|
||||
auth.add_auth_headers(&mut headers);
|
||||
|
||||
let client = build_reqwest_client();
|
||||
let mut request = client
|
||||
client
|
||||
.request(method, url)
|
||||
.timeout(OPENAI_FILE_REQUEST_TIMEOUT);
|
||||
if let Some(token) = auth.bearer_token() {
|
||||
request = request.bearer_auth(token);
|
||||
}
|
||||
if let Some(account_id) = auth.account_id() {
|
||||
request = request.header("chatgpt-account-id", account_id);
|
||||
}
|
||||
request
|
||||
.timeout(OPENAI_FILE_REQUEST_TIMEOUT)
|
||||
.headers(headers)
|
||||
}
|
||||
|
||||
fn build_reqwest_client() -> reqwest::Client {
|
||||
|
||||
@@ -91,9 +91,7 @@ impl HttpTransport for RecordingTransport {
|
||||
struct NoAuth;
|
||||
|
||||
impl AuthProvider for NoAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
fn add_auth_headers(&self, _headers: &mut HeaderMap) {}
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
@@ -112,12 +110,14 @@ impl StaticAuth {
|
||||
}
|
||||
|
||||
impl AuthProvider for StaticAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
Some(self.token.clone())
|
||||
}
|
||||
|
||||
fn account_id(&self) -> Option<String> {
|
||||
Some(self.account_id.clone())
|
||||
fn add_auth_headers(&self, headers: &mut HeaderMap) {
|
||||
let token = &self.token;
|
||||
if let Ok(header) = HeaderValue::from_str(&format!("Bearer {token}")) {
|
||||
headers.insert(http::header::AUTHORIZATION, header);
|
||||
}
|
||||
if let Ok(header) = HeaderValue::from_str(&self.account_id) {
|
||||
headers.insert("ChatGPT-Account-ID", header);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -24,9 +24,7 @@ use wiremock::matchers::path;
|
||||
struct DummyAuth;
|
||||
|
||||
impl AuthProvider for DummyAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
fn add_auth_headers(&self, _headers: &mut HeaderMap) {}
|
||||
}
|
||||
|
||||
fn provider(base_url: &str) -> Provider {
|
||||
|
||||
@@ -53,9 +53,7 @@ impl HttpTransport for FixtureSseTransport {
|
||||
struct NoAuth;
|
||||
|
||||
impl AuthProvider for NoAuth {
|
||||
fn bearer_token(&self) -> Option<String> {
|
||||
None
|
||||
}
|
||||
fn add_auth_headers(&self, _headers: &mut HeaderMap) {}
|
||||
}
|
||||
|
||||
fn provider(name: &str) -> Provider {
|
||||
|
||||
Reference in New Issue
Block a user