fix(cli): surface OAuth2 client-credentials env vars in auth status - #17653
fix(cli): surface OAuth2 client-credentials env vars in auth status#17653devin-ai-integration[bot] wants to merge 4 commits into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![nitpickybot[bot]](https://avatars.githubusercontent.com/in/2901896?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Summary
Solid, well-scoped fix: slots model for multi-value schemes plus a credential_slots() introspection hook. Main concerns are the interaction between provider slots and the synthesized keyring slot (an OAuth login-flow scheme with client-id/secret env vars now requires all slots, likely flipping logged_in to false for users who logged in via keyring), and test env-var mutation without serialization.
- 🟡 2 warning(s)
- 🔵 2 suggestion(s)
To request another review, comment /ai-review on this pull request.
| SchemeBinding::Custom(provider) => { | ||
| let mut slots: Vec<Vec<AuthCredentialSource>> = provider | ||
| .credential_slots() | ||
| .into_iter() | ||
| .map(|slot| slot.into_iter().flat_map(flatten_chain).collect()) | ||
| .filter(|slot: &Vec<AuthCredentialSource>| !slot.is_empty()) | ||
| .collect(); | ||
| if login_flows.iter().any(|f| f.scheme_name() == scheme_name) { | ||
| vec![AuthCredentialSource::keyring(cli_name, scheme_name)] | ||
| } else { | ||
| Vec::new() | ||
| slots.push(vec![AuthCredentialSource::keyring(cli_name, scheme_name)]); | ||
| } | ||
| slots |
There was a problem hiding this comment.
🟡 warning
The keyring slot is pushed as an additional required slot alongside the provider's env slots. For an OAuth scheme that has both a registered login flow and client_id_env/client_secret_env (authorization-code + client-secret is common), a user who logged in via the browser flow now gets logged_in: false because the env slots don't resolve — a regression versus the old "any source resolves" behaviour.
Semantically the keyring token and the client-credentials env vars are alternatives (either grants a token), not conjunctive requirements. Consider folding the keyring source into an alternative slot group, or skipping the env slots when a keyring token is present.
| fn status_reports_oauth2_client_credentials_env_vars() { | ||
| use crate::auth::root_builder::{AuthSchemeBuilder, OAuth2Auth}; | ||
| std::env::set_var("STATUS_TEST_OAUTH_CLIENT_ID", "id"); | ||
| std::env::set_var("STATUS_TEST_OAUTH_CLIENT_SECRET", "secret"); | ||
| let (name, binding) = OAuth2Auth::new("oAuth2ClientCredentials") | ||
| .token_url("https://example.com/oauth/token") | ||
| .client_id_env("STATUS_TEST_OAUTH_CLIENT_ID") | ||
| .client_secret_env("STATUS_TEST_OAUTH_CLIENT_SECRET") | ||
| .into_binding(); | ||
|
|
||
| let entry = status_entry_for("my-cli", &name, &binding, &[]); | ||
| assert_eq!(entry["logged_in"], true); | ||
| let states: Vec<&str> = entry["sources"] | ||
| .as_array() | ||
| .unwrap() | ||
| .iter() | ||
| .map(|s| s["state"].as_str().unwrap()) | ||
| .collect(); | ||
| assert_eq!(states, ["active", "active"]); | ||
|
|
||
| std::env::remove_var("STATUS_TEST_OAUTH_CLIENT_SECRET"); | ||
| let entry = status_entry_for("my-cli", &name, &binding, &[]); | ||
| assert_eq!(entry["logged_in"], false); | ||
| std::env::remove_var("STATUS_TEST_OAUTH_CLIENT_ID"); | ||
| } |
There was a problem hiding this comment.
🟡 warning
std::env::set_var/remove_var are process-global and (as of Rust 2024) unsafe; with cargo test running tests in parallel threads this is a data race against any other test reading env. Also, if an assertion panics before remove_var, the vars leak into other tests. Wrap in the crate's env-mutation test guard/mutex if one exists (the existing keyring/env tests likely have one), and reset via a scope guard rather than trailing statements.
| let mut seen = std::collections::HashSet::new(); | ||
| env_vars.retain(|var| seen.insert(*var)); |
There was a problem hiding this comment.
🔵 suggestion
Minor: empty env var names (e.g. an unconfigured client_secret_env) would be emitted as an Env("") slot that can never resolve, making logged_in permanently false. Worth filtering blanks alongside the dedupe:
| let mut seen = std::collections::HashSet::new(); | |
| env_vars.retain(|var| seen.insert(*var)); | |
| let mut seen = std::collections::HashSet::new(); | |
| env_vars.retain(|var| !var.is_empty() && seen.insert(*var)); |
| .iter() | ||
| .map(|s| { | ||
| let slots = expand_slots(scheme_name, binding, login_flows, cli_name); | ||
| let mut all_slots_active = !slots.is_empty(); |
There was a problem hiding this comment.
🔵 suggestion
all_slots_active starts as !slots.is_empty() here, but the human-readable path (line 470) initialises all_slots_active = true and returns early when slots.is_empty(). Same result, but the divergent initialisation is easy to break later — consider mirroring the early-return for empty slots in the JSON path too.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 potential issue.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
E2E test:
|
| Before (main) | After (this PR), only client id set |
|---|---|
- main:
(no credential sources bound)/logged_in: falseeven with all env vars exported (bug reproduced) - PR, env unset: client-id, client-secret and required token-endpoint env vars listed as
missing;logged_in: false - PR, all set: all slots
✓ active,logged_in: true - PR, only
ACME_CLIENT_ID:✓ active(not shadowed), restmissing,logged_in: false - PR, env unset + valid
~/.config/oauth-test/credentials.json: single✓ active cached OAuth token (<path>),logged_in: true; expired token falls back to env slots - Regression: basic (2 slots) and bearer schemes unchanged
All env vars set / cached token / regression
Note: required token-endpoint env(...) request properties also count as slots (4 in this fixture), so logged_in requires them too — matches what request-time auth needs.
…the scheme
Two follow-ups to the credential-slot work in this branch.
A valid cached access token used to *replace* the acquisition env vars in
`credential_slots()`, so once a token was cached — and `inject_oauth2_caches`
wires a disk cache into every generated CLI before dispatch — `auth status`
showed only `cached OAuth token (<path>)` and dropped OAUTH_CLIENT_ID /
OAUTH_CLIENT_SECRET entirely. That is the one question the customer was
asking. The hook now returns `CredentialSlots { required, alternatives }`:
`required` slots are ANDed, `alternatives` are ORed against the whole set.
OAuth2TokenProvider always emits the env slots and adds a valid cached token
as an alternative, so the env vars stay visible and a cached token still
satisfies the scheme on its own.
The remedy line also always suggested `auth login --with-token`, which writes
the keyring. OAuth client credentials and basic auth never read the keyring,
so a token pasted there was silently ignored at request time. `remedy_line`
now picks by how the scheme actually resolves credentials: login flow ->
`auth login`; keyring source present -> `--with-token`; otherwise name the
env vars to set.
Both status paths now share `evaluate_slots`, so the human and JSON renderings
can no longer drift.
Verified end to end on a generated CLI (OAuth client credentials over a
namespaced token endpoint, alongside three basic schemes): env vars listed
with per-slot state, remedy naming exactly the missing vars, cached token
reported alongside the env vars, and bearer bindings still pointing at
`--with-token`.
Co-Authored-By: Claude <noreply@anthropic.com>
…d shadowing `status_reports_oauth2_client_credentials_env_vars` mutated the process environment without `#[serial]`, against a convention the module documents inline (login.rs: "`#[serial]` keeps the process-global env mutation from racing the other env-touching tests"). It matters more than it looks: the seed fixtures vendor this crate and the cli generator's testScript runs `cargo test --locked --all-features`, so the full 1987-test suite executes inside every fixture — and cli-oauth, cli-basic-auth, cli-header-auth and cli-oauth-login-flow are all absent from allowedFailures, so they gate PRs. ADR-0008 and ARCHITECTURE.md 8.21 still described the pre-slot model, and the ADR's failure-mode table pinned the literal `Run <bin> auth login` string that remedy_line now varies three ways. The code cites ADR-0008 from credential.rs and app.rs, so leaving it stale misleads the next reader. Amended rather than superseded: this refines an accepted decision, it does not reverse it. The new text also records the one known divergence between `logged_in` and `has_credentials` rather than claiming they agree by construction. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Description
Linear ticket: Refs (none — from Slack report)
A customer with an
oauth/client-credentialsscheme (client-id-env: OAUTH_CLIENT_ID,client-secret-env: OAUTH_CLIENT_SECRET) reported the generated CLI "is not picking them up":auth statusprinted(no credential sources bound)for the OAuth scheme.Root cause: the generator does emit
.client_id_env("OAUTH_CLIENT_ID").client_secret_env("OAUTH_CLIENT_SECRET"), andOAuth2Auth::into_binding()turns that into anOAuth2TokenProviderstored asSchemeBinding::Custom. The runtime'sauth statustreated everyCustombinding as opaque (only synthesising a keyring source when a login flow was registered), so the env vars were read fine at request time but never shown — andlogged_inwasfalsein--output json.Reproducing the customer's config surfaced a second bug in the same output: their three
basicschemes reported a setTWILIO_AUTH_TOKENasshadowed. Status flattened username+password into one precedence list, so the password looked like it was losing to the username. That reads as "your auth token is being ignored" and invites the user to go chase a problem that doesn't exist.Their exact config, before and after (same binary, only the vendored runtime swapped):
No change to request-time auth behaviour.
Changes Made
CredentialSlots— a status-introspection hook forCustomproviders (auth/credential.rs)AuthProvider::credential_slots() -> CredentialSlots(default empty). Providers behindSchemeBinding::Customare otherwise opaque to the status surface; overriding this lets it enumerate their env vars like a builtin bearer/basic binding.OAuth2TokenProviderimplements it:client_id_env,client_secret_env, required custom token-endpoint property env vars (contract mode), andrefresh_token_envfor the refresh-token grant. A valid cached access token is added as an alternative, so it satisfies the scheme on its own without hiding the env vars that mint it — the question the customer was actually asking. (inject_oauth2_cacheswires a disk cache into every generated CLI before dispatch, so a cached token is the common case after the first successful call.)login.rs:expand_sources→expand_slotsToken= 1 required slot,Basic= 2 (username, password),Custom=provider.credential_slots()+ a keyring slot when a login flow exists.evaluate_slotsresolves every source once and classifies it; both the human and JSON paths render from it, so they can't drift.logged_inis true when every required slot resolves, or an alternative does.activeinstead ofshadowed.remedy_linepicks the fix that matches how the scheme actually resolves credentials.auth login --with-tokenwrites the keyring, and OAuth client credentials / basic auth never read it — suggesting it sent the user down a path that silently does nothing. Now: login flow →auth login; keyring source present →--with-token; otherwise name the env vars to set.Docs —
ADR-0008§ "Shadowing-aware UX" andARCHITECTURE.md§ 8.21 described the pre-slot model, and the ADR's failure-mode table pinned the literalRun \auth login`string thatremedy_line` now varies. Both amended (the ADR gains a Remedy selection subsection), plus the decisions index row. Amended rather than superseded — this refines an accepted decision, it doesn't reverse it.Changelog entry under
generators/cli/changes/unreleased/.Testing
Unit tests added/updated —
cargo test --libingenerators/cli/sdk: 1987 passed (three consecutiveauth::runs clean after serializing the env-mutating test).alternative_satisfies_scheme_without_hiding_required_env_vars— cached-token alternative reportsactivewhile the env slots still render asmissing, andlogged_instays true.remedy_names_missing_env_vars_when_the_scheme_never_reads_the_keyring,remedy_suggests_with_token_when_the_scheme_reads_the_keyring,remedy_points_at_the_login_flow_when_one_is_declared.expand_slots_lists_oauth2_client_credentials_env_vars,status_reports_oauth2_client_credentials_env_vars(both env vars set →logged_in: true, statesactive, active; secret unset →logged_in: false),expand_slots_keeps_basic_halves_in_separate_slots.credential_slots_report_cached_token_when_env_unset— asserts the alternative and the required env slots coexist.Manual testing completed — generated a CLI from the customer's shape (OAuth client credentials with a namespaced
oauth::POST /v2/tokenreference, alongside threebasicschemes over shared env vars) viapnpm seed run --generator cli --local, built it, and exercisedauth statusin every state:missing, remedy names all missing env varsactive/missing, remedy names only what's leftactive, no remedy line,logged_in: true✓ active cached OAuth token (<path>)followed by themissingenv vars,logged_in: trueRun '<cli> auth login --with-token'Authorization: Bearer <token>Note:
cargo clippy -D warningshas pre-existing errors onmainin this crate; none introduced here.No regressions outside
auth statuscredential_slots()has exactly one non-test caller —expand_slots(login.rs:546), reached only fromhandle_statusandstatus_entry_for. Nothing on the request path can observe it.oauth2.rsis 2 import lines plus a 52-line pure insertion (zero lines removed or modified), soapply,has_credentials,has_credentials_for_url,credential_hints, the token exchange/refresh path and all ofTokenCacheare byte-identical;provider.rsandcredential.rsare pure insertions;builder.rs,schemes.rs,compose.rs,oauth_login.rs,keyring_store.rsanderror.rsaren't in the diff.Verified behaviourally by building one generated CLI (three
basicschemes + OAuth client credentials) and swapping only the vendored runtime betweenmainand this branch. Byte-identical on both:--help,<group> --help,auth --help,--schema,--dry-run, theAuthorizationheader the API actually received on all four endpoints, response bodies, exit codes, the no-credentials error, and the 401 path for both a basic and an OAuth endpoint (including credential-source disclosure). Differing: only the threeauth statuscaptures.One machine-readable change:
logged_inflipstrue→falsefor abasicscheme with a single half set. That corrects a status surface that contradicted the request path —BasicAuthProviderinFullmode requires both halves inhas_credentials()(schemes.rs:170) andapply()returns the request unmodified when either is missing (schemes.rs:205), confirmed on the wire asAuthorization=<none>:Known gaps (not addressed here)
BearerAuthProvider/BasicAuthProvider/HeaderAuthProviderstill use the empty default, so schemes registered via.auth_provider(...)— e.g. generated password-omitted basic — continue to print(no credential sources bound). Each is a few lines since they already own theirAuthCredentialSource.compose.rswrappers forwardcredential_hintsbut notcredential_slots. Latent only: the generator doesn't emit them today.AnyAuthProvideris OR-of-AND, which flat slots can't express.Custombindings is still pushed as required rather than as an alternative. Not reachable from generated CLIs (client credentials never register a login flow; PKCE/device-code replace the binding withOAuth2KeyringProvider), but it's a one-line move now thatalternativesexists.credential_slotsdoesn't model the "expired access token + persisted refresh token + contract refresh endpoint" case thathas_credentials_for_urlaccepts, so that narrow state reportslogged_in: falsewhile requests succeed.Link to Devin session: https://app.devin.ai/sessions/d9ff8ec39c404dc7b310aed342579599
Open in Devin Desktop: https://app.devin.ai/desktop/session/d9ff8ec39c404dc7b310aed342579599?variant=devin