-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Rakesh/support device auth #3531
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
Conversation
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
let token_url = format!("{}/deviceauth/token", opts.issuer.trim_end_matches('/')); | ||
loop { | ||
let resp = client | ||
.post(&token_url) | ||
.header("Content-Type", "application/json") | ||
.body(format!( | ||
"{{\"client_id\":\"{}\",\"user_code\":\"{}\"}}", | ||
opts.client_id, uc.user_code | ||
)) | ||
.send() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Include device_code parameters when polling token endpoint
The polling loop sends POST {issuer}/deviceauth/token
with JSON body {"client_id":…, "user_code":…}
. In the OAuth 2.0 device authorization grant, the token request must be form‑encoded and include the device_code
returned from the first step together with grant_type=urn:ietf:params:oauth:grant-type:device_code
. Without those fields, a compliant issuer will respond with invalid_request
and this login flow can never succeed outside the mocked tests (which ignore the request body). Consider sending the device code and grant type per the spec instead of the user-facing code.
Useful? React with 👍 / 👎.
@codex fix comments |
Summary
Testing
|
8402309
to
ccb571e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
ccb571e
to
7df113e
Compare
@codex review the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
// Step 1: request a user code and polling interval | ||
// let usercode_url = format!("{}/devicecode/usercode", opts.issuer.trim_end_matches('/')); | ||
let usercode_url = format!("{}/deviceauth/usercode", "http://localhost:3007"); | ||
let mut payload: serde_json::Map<String, serde_json::Value> = serde_json::Map::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respect issuer when requesting device codes
The new device-code flow constructs the user-code endpoint with a hard-coded http://localhost:3007
instead of using the ServerOptions.issuer
that the caller provides. As a result the CLI will always attempt to contact a local server regardless of configuration, and will fail outright in real deployments (e.g., the default https://auth.openai.com
issuer or any overridden URL). The commented-out line shows the intended behavior; the current implementation makes the feature unusable.
Useful? React with 👍 / 👎.
7df113e
to
375e31a
Compare
@codex review this PR |
375e31a
to
bc835cc
Compare
💡 Codex Reviewcodex/codex-rs/login/src/device_code_auth.rs Lines 126 to 134 in 375e31a
[P1] Deserialize deviceauth success payload into tokens When the polling request succeeds, the implementation assumes the body contains a codex/codex-rs/login/src/device_code_auth.rs Lines 153 to 168 in 375e31a
[P1] Treat token_pending errors as retryable Polling only retries when the HTTP status is ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
bc835cc
to
6de3f1d
Compare
6de3f1d
to
ce7bc50
Compare
where | ||
D: Deserializer<'de>, | ||
{ | ||
let value = Option::<serde_json::Value>::deserialize(deserializer)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why id this value coming in different data types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is coming in as string, let me simplify this code.
|
||
use crate::server::ServerOptions; | ||
|
||
pub(crate) const DEVICE_AUTH_BASE_URL_ENV_VAR: &str = "CODEX_DEVICE_AUTH_BASE_URL"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this change the entire issuer address instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nvm, I missed the part that I introduced commandline option, this is redundent I will delete this.
// Step 1: request a user code and polling interval | ||
let usercode_url = format!("{auth_base_url}/devicecode/usercode"); | ||
let mut payload: serde_json::Map<String, serde_json::Value> = serde_json::Map::new(); | ||
payload.insert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use strongly typed struct for all request responses?
let start = std::time::Instant::now(); | ||
|
||
let token_url = format!("{auth_base_url}/deviceauth/token"); | ||
loop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this method can be split up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split the function into multiple smaller functions.
) | ||
.await?; | ||
|
||
// Try to exchange for an API key (optional best-effort) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we are doing for auth callback in regular chatgpt sign up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the old flow we don't use anymore, doesn't matter for device login.
96251bb
to
1745620
Compare
7d53bf7
to
5425e08
Compare
codex-rs/cli/src/main.rs
Outdated
/// EXPERIMENTAL: Use custom OAuth issuer base URL (advanced) | ||
/// Override the OAuth issuer base URL (advanced) | ||
#[arg(long = "experimental_issuer", value_name = "URL", hide = true)] | ||
issuer: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late nit:
issuer: Option<String>, | |
issuer_base_url: Option<String>, |
} | ||
|
||
#[derive(Serialize)] | ||
struct UserCodeReq<'a> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these lifetimes? Can we go with Strings as properties and simplify.
nit: consider Request/Response suffixes.
|
||
#[derive(Deserialize)] | ||
struct CodeSuccessResp { | ||
#[serde(alias = "device_code")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why alias, can the field be device_code
?
// ANSI escape code for bright yellow | ||
const YELLOW: &str = "\x1b[93m"; | ||
const RESET: &str = "\x1b[0m"; | ||
let warning = "WARN!!! device code authentication has potential risks and\n\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we might want to improve this copy
let uc = request_user_code(&client, &auth_base_url, &opts.client_id).await?; | ||
|
||
print_colored_warning_device_code(); | ||
println!("⏳ Generating a new 9-digit device code for authentication...\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this message be before the call?
.map_err(|err| std::io::Error::other(format!("device code exchange failed: {err}")))?; | ||
|
||
// Try to exchange for an API key (optional) | ||
let api_key = crate::server::obtain_api_key(&opts.issuer, &opts.client_id, &tokens.id_token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not need to do this.
let codex_home = tempdir().unwrap(); | ||
let mock_server = MockServer::start().await; | ||
|
||
// Mock::given(method("POST")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:rem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
@codex review the PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
if login_cli.use_device_code { | ||
run_login_with_device_code(login_cli.config_overrides, login_cli.issuer) | ||
.await; | ||
} else if let Some(api_key) = login_cli.api_key { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use existing issuer_base_url field when invoking device login
The new device-code branch calls run_login_with_device_code(login_cli.config_overrides, login_cli.issuer)
even though LoginCommand
only defines issuer_base_url
. Because no issuer
field exists on LoginCommand
, this code path does not compile. Pass the issuer_base_url
option (or add the field) so the CLI builds.
Useful? React with 👍 / 👎.
) -> std::io::Result<UserCodeResp> { | ||
let url = format!("{auth_base_url}/deviceauth/usercode"); | ||
let body = serde_json::to_string(&UserCodeReq { client_id }).map_err(std::io::Error::other)?; | ||
let resp = client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Serialize owned strings in device auth request payloads
Both request_user_code
and poll_for_token
construct UserCodeReq { client_id }
/ TokenPollReq { client_id, user_code }
where the structs declare String
fields but the variables are &str
. This mismatched ownership prevents the crate from compiling. Convert the borrowed values to String
(e.g. client_id.to_owned()
) before serializing. The same issue appears again when building TokenPollReq
a few lines below.
Useful? React with 👍 / 👎.
let tokens = crate::server::exchange_code_for_tokens( | ||
&opts.issuer, | ||
&opts.client_id, | ||
"", | ||
&empty_pkce, | ||
&code_resp.code, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access nonexistent field in device code exchange
CodeSuccessResp
defines authorization_code
, but the login flow passes &code_resp.code
to exchange_code_for_tokens
. Because code
is not a struct field, the file does not compile. Use the defined authorization_code
(or rename the struct field to match the API payload).
Useful? React with 👍 / 👎.
let tokens = crate::server::exchange_code_for_tokens( | ||
&opts.issuer, | ||
&opts.client_id, | ||
"", | ||
&empty_pkce, | ||
&code_resp.code, | ||
) | ||
.await | ||
.map_err(|err| std::io::Error::other(format!("device code exchange failed: {err}")))?; | ||
|
||
crate::server::persist_tokens_async( | ||
&opts.codex_home, | ||
None, | ||
tokens.id_token, | ||
tokens.access_token, | ||
tokens.refresh_token, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Device code login never exchanges tokens for an API key
After the device flow obtains tokens
it calls persist_tokens_async
with api_key
set to None
and returns. Unlike the browser-based login, obtain_api_key
is never invoked, so no API key is stored and mocked integration tests expecting a second /oauth/token
call will fail. Without persisting the API key the CLI cannot authenticate subsequent requests. Consider exchanging the ID token for an API key before saving credentials.
Useful? React with 👍 / 👎.
I have read the CLA Document and I hereby sign the CLA |
85f6ee8
to
7dbcd97
Compare
7dbcd97
to
bdabd77
Compare
External (non-OpenAI) Pull Request Requirements
Before opening this Pull Request, please read the dedicated "Contributing" markdown file or your PR may be closed:
https://github.com/openai/codex/blob/main/docs/contributing.md
If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes.
test
The error in the last line is since the poll endpoint is not yet implemented