feat: OAuth scope step-up via command metadata by jpage-godaddy · Pull Request #18 · godaddy/cli-engine

jpage-godaddy · 2026-06-10T15:25:13Z

Summary

Adds a non-breaking way for an AuthProvider to receive a command's full metadata and act on it, and uses it to restore OAuth scope step-up from the original TypeScript CLI code base — a capability the 0.2.0 line did not cover (a command can declare the scopes it needs, and the provider re-authenticates when the cached token lacks them).

The generic seam is metadata-based rather than scope-specific, so non-OAuth providers (exec, API-key, etc.) are entirely unaffected.

Changes

AuthProvider::get_credential_for(&CredentialRequest) — new defaulted trait method that delegates to get_credential. CredentialRequest bundles env/command/tier/&CommandMeta. The framework now calls this during credential resolution, so a provider can inspect CommandMeta (e.g. scopes). Existing providers keep working untouched (get_credential is unchanged).
PkceAuthProvider overrides it: decodes the access-token JWT scope claim; if it already covers the required scopes, returns the cached token; otherwise re-runs the PKCE flow requesting the union (defaults ∪ granted ∪ required), then replaces the stored token. Fails fast with an actionable error in non-interactive contexts instead of hanging on the browser callback.
CommandSpec::with_scopes(...) declares required scopes (sugar over auth_metadata["scopes"]).
Resolver owns the CommandMeta and gains resolve_with_scopes; CommandContext::credential_with_scopes lets handlers add runtime-derived scopes. peek()'s signature is preserved (the OnceCell is kept as a write-once mirror).
auth login --scope (repeatable) via new public login_and_build_with_scopes / Dispatcher::login_with_scopes; login_and_build preserved (delegates with &[]).

Compatibility

Non-breaking: no existing public signature changed (get_credential, login_and_build, peek intact); new API is additive with defaults. Suitable for a 0.2.1 release.

Why interactive step-up

The authorization server supports only the authorization_code grant interactively and has no token-exchange / incremental-authorization grant, and refresh tokens don't widen scopes — so widening scopes requires a fresh PKCE flow (browser re-consent).

Tests

Added: scopes_from_jwt parsing; get_credential_for returns cached token when scopes are covered; CommandSpec::with_scopes round-trip through metadata(); a middleware test asserting command scopes reach the provider, that step-up requests the union, and that already-covered scopes don't re-call. Full suite green: cargo fmt --check, clippy --all-targets -D warnings, rustdoc -D warnings, zero missing-docs, 248 lib + integration tests.

🤖 Generated with Claude Code

Add a non-breaking way for an auth provider to receive a command's full metadata and act on it, and use it to restore OAuth scope step-up that the 0.1.x->0.2.0 line did not cover. - AuthProvider gains a defaulted `get_credential_for(&CredentialRequest)` that delegates to `get_credential`; the framework calls it during resolution so a provider can read `CommandMeta` (e.g. `scopes`). Non-OAuth providers are unaffected. - PkceAuthProvider overrides it: when the cached token's JWT `scope` claim does not cover the required scopes, it re-runs the PKCE flow requesting the union (defaults + granted + required), failing fast in non-interactive contexts. - CommandSpec::with_scopes declares required scopes; the resolver threads them to the provider and adds resolve_with_scopes / CommandContext::credential_with_scopes for runtime-derived scopes. peek() is preserved. - `auth login --scope` (repeatable) via login_and_build_with_scopes / Dispatcher::login_with_scopes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR adds an additive metadata “seam” to auth credential resolution so an AuthProvider can inspect full command metadata (notably OAuth scopes) and perform OAuth scope step-up when a cached token lacks required scopes.

Changes:

Introduces CredentialRequest and AuthProvider::get_credential_for (defaulting to get_credential) and updates middleware resolution to pass command metadata/scopes into providers.
Adds OAuth scope step-up behavior to PkceAuthProvider by reading JWT scope claims and re-running PKCE with a widened scope union when needed.
Adds scope declaration/propagation APIs (CommandSpec::with_scopes, CredentialResolver::resolve_with_scopes, auth login --scope, and helper functions) plus tests covering scope propagation and step-up behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/foundation.rs	Adds tests for `CommandSpec::with_scopes` round-tripping and middleware/provider scope propagation + step-up behavior.
src/middleware.rs	Reworks credential resolution to pass `CommandMeta` via `CredentialRequest` and support scope step-up with a serialized resolver state.
src/lib.rs	Re-exports `CredentialRequest` and `login_and_build_with_scopes` as part of the public API surface.
src/command.rs	Adds `CommandSpec::with_scopes` and `CommandContext::credential_with_scopes` for declaring/adding required scopes.
src/auth/pkce.rs	Implements scope-aware `get_credential_for`, JWT scope parsing, and step-up PKCE re-auth logic.
src/auth/mod.rs	Defines `CredentialRequest` and adds defaulted `AuthProvider::get_credential_for`.
src/auth/dispatcher.rs	Routes `get_credential_for` and adds `login_with_scopes` for requesting additional scopes at login time.
src/auth/commands.rs	Adds `auth login --scope` support and `login_and_build_with_scopes`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- get_credential_for now fetches an existing token without launching a flow (new existing_token/reauthenticate helpers), so `auth login --scope X` runs a single PKCE flow for defaults ∪ required instead of two browser round-trips. - Non-interactive step-up error renders a readable scope list separately from an executable hint (`auth login --scope a --scope b`). - `auth login --scope` uses ArgAction::Append (one value per flag, repeatable); a bare `--scope` is now rejected instead of silently doing nothing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Resolves the Rust CI failure (unused_qualifications under -D warnings). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Per Copilot review: the type doc said resolution runs at most once, which is no longer true once resolve_with_scopes performs OAuth scope step-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Per Copilot review: - string_vec_arg now drops empty strings in the array branch too, so `--scope ""` no longer propagates an invalid empty scope. - Clarify that peek reflects the first resolved credential and that stable identity across step-up is an assumption, not a guarantee. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

…sistent Per Copilot review: - Non-interactive step-up guard now treats the session as interactive if any stdio stream is a TTY, so redirecting stderr alone doesn't block a user who can complete the browser flow. - Add CommandMeta::set_scopes to keep scopes and auth_metadata["scopes"] in sync; use it for runtime step-up (middleware) and login_with_scopes (dispatcher) so metadata-aware providers see the widened set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Per Copilot review: - CommandSpec::with_scopes(&[]) now removes auth_metadata["scopes"] instead of writing an empty string, matching CommandMeta::set_scopes. - middleware scope test builds metadata via set_scopes for a consistent state. - decode_jwt_claims doc notes scopes_from_jwt reads claims to decide step-up (an optimization, not a trust/authz decision; the server enforces scopes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Per Copilot review: `auth login` requires --env, so the suggested command now reads `auth login --env <env> --scope ...` and is copy-pasteable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Per Copilot review: - reauthenticate now persists the freshly obtained token (keychain write overwrites) before updating the cache, and no longer deletes the old token first — a failed save no longer logs the user out of a still-valid session. - pkce test builds CommandMeta via set_scopes for a consistent shape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

CredentialRequest is part of the AuthProvider surface (providers receive &CredentialRequest) and is likely to gain fields. Marking it #[non_exhaustive] and adding CredentialRequest::new keeps future field additions from being breaking changes for downstream crates, and gives external provider tests a stable way to build one. In-crate construction sites are switched to the constructor.

Scope step-up decided coverage solely from a JWT `scope` string claim, so access tokens that are opaque, use the `scp` claim, or encode scopes as a JSON array were treated as having no scopes — forcing a browser re-consent on every scoped command for those identity providers. Record the scopes a token was obtained with on StoredToken (from the token response `scope`, else the requested set) and treat them as granted, and parse `scp`/array claim forms in addition to a space-delimited `scope` string. Coverage is now jwt-claim ∪ recorded-scopes, so opaque tokens are covered. The step-up decision is extracted into a pure `plan_step_up` (Covered / Reauthenticate / MissingNonInteractive) so the non-interactive fail-fast path is unit-testable without real TTY detection or a flow. StoredToken.scopes uses #[serde(default)] so tokens stored before this change still load (empty set, falling back to the claim as before). Tests: scp/array claim parsing, recorded-scope coverage for opaque tokens, plan_step_up's three branches, and get_credential_for returning an opaque cached token whose recorded scopes cover the requirement.

After a step-up re-authentication, the freshly issued token was returned without confirming the authorization server actually granted the requested scopes. If the server declines (by policy), the handler received an under-scoped token that the API rejects with a 403, and an interactive retry would re-prompt against a grant the server keeps refusing. Verify coverage after (re)authentication and return a clear "authorization server did not grant required scope(s)" error when the difference is detectable (JWT scope claim or an echoed narrower token-response scope). Opaque tokens whose grant the server does not echo record the requested set, so an undetected decline still surfaces downstream as a 403. Test: ensure_granted rejects a JWT missing a required scope and accepts tokens (JWT or recorded-scope) that cover it.

`peek` (used for audit/activity identity) reflects the first resolved credential and is not replaced by a later step-up. If a step-up browser flow returned a different account, the elevated action would be attributed to the original identity. Rather than reflect-latest (peek lends a reference and must stay lock-free), enforce the invariant: resolve_scopes aborts when a re-resolution returns a credential whose subject/identity differs from the first, so the audited identity always matches the one that performed the action. Test: a provider that switches identity on the step-up resolution causes resolve_with_scopes to error.

The transport bearer injector resolves its token through the provider's scope-unaware path and caches the first token for the injector's lifetime, so it does not itself trigger scope step-up. Document the contract on resolve_with_scopes, CommandContext::credential_with_scopes, and the injector: a handler that needs wider scopes over HTTP must resolve them before the injector's first inject, which populates the provider cache the injector then reads. No behavior change.

jgowdy-godaddy · 2026-06-10T18:24:21Z

Added 5 commits addressing deep-review findings on this PR. Each finding is its own commit with tests. I skipped the save-before-evict fix because you already landed it in 364405d, and folded your --env hint improvement (8d42555) into the refactored error helper. CI re-ran green with and without --features pkce-auth.

114590e — refactor: make CredentialRequest non_exhaustive with a constructor

Fixes: CredentialRequest is part of the AuthProvider surface and likely to gain fields; as a plain pub-field struct, adding one is a breaking change for downstream crates.
Change: #[non_exhaustive] + CredentialRequest::new(...); in-crate construction sites use the constructor.
Value: future request fields won't break external providers/tests, which now have a stable way to build one.

548b988 — fix: make OAuth scope coverage robust for opaque and scp tokens

Fixes: coverage was decided solely from a JWT scope string claim, so opaque tokens, scp claims, and array-form scopes read as "no scopes" — forcing a browser re-consent on every scoped command for those IdPs.
Change: record the scopes a token was obtained with on StoredToken (#[serde(default)], so old keychain tokens still load) and treat them as granted; parse scp/array claim forms; coverage = jwt-claim ∪ recorded. Extracted a pure plan_step_up so the non-interactive decision is unit-testable.
Tests: scp/array parsing, recorded-scope coverage for opaque tokens, plan_step_up's three branches, and get_credential_for returning an opaque cached token whose recorded scopes cover the requirement.

ce569c0 — fix: error when step-up cannot obtain the required scopes

Fixes: after a step-up re-auth, the fresh token was returned without confirming the server actually granted the requested scopes; a policy decline produced a downstream 403 (and could re-prompt against a grant the server keeps refusing).
Change: ensure_granted verifies coverage post-reauth and returns a clear "authorization server did not grant required scope(s)" error when the gap is detectable (JWT claim or echoed token-response scope).
Tests: ensure_granted rejects a JWT missing a scope and accepts covering tokens (JWT or recorded).

01a7186 — fix: abort scope step-up that authenticates a different identity

Fixes: peek (audit/activity identity) reflects the first resolved credential; a step-up that returned a different account would misattribute the elevated action.
Change: resolve_scopes aborts when a re-resolution's subject/identity differs from the first, so the audited identity always matches the actor (keeps peek's lock-free reference-lending design intact).
Tests: a provider that switches identity on the step-up resolution causes resolve_with_scopes to error.

db142c3 — docs: document HTTP step-up ordering for the bearer injector

Fixes: the transport bearer injector resolves a scope-unaware token and caches the first one for its lifetime, so it doesn't itself trigger step-up.
Change: documents on resolve_with_scopes, CommandContext::credential_with_scopes, and the injector that a handler needing wider scopes over HTTP must resolve them before the injector's first inject. No behavior change (pre-existing code left intact).

🤖 I have created a release *beep* *boop* --- ## [0.2.1](cli-engine-v0.2.0...cli-engine-v0.2.1) (2026-06-10) ### Features * argv0 multi-call dispatch with link/shim installer ([#19](#19)) ([9e39f2f](9e39f2f)) * OAuth scope step-up via command metadata ([#18](#18)) ([f996e50](f996e50)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

jpage-godaddy requested a review from Copilot June 10, 2026 15:31

Copilot started reviewing on behalf of jpage-godaddy June 10, 2026 15:31 View session