From e0a94cf1e86c3d2198b687c0b5f98ac542409ed4 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 08:24:35 -0700 Subject: [PATCH 01/14] feat: OAuth scope step-up via command metadata 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 --- src/auth/commands.rs | 42 ++++++++++++- src/auth/dispatcher.rs | 44 ++++++++++++- src/auth/mod.rs | 37 ++++++++++- src/auth/pkce.rs | 137 ++++++++++++++++++++++++++++++++++++++++- src/command.rs | 36 +++++++++++ src/lib.rs | 6 +- src/middleware.rs | 114 +++++++++++++++++++++++++++------- tests/foundation.rs | 111 +++++++++++++++++++++++++++++++++ 8 files changed, 494 insertions(+), 33 deletions(-) diff --git a/src/auth/commands.rs b/src/auth/commands.rs index 9a780bb..e12ba99 100644 --- a/src/auth/commands.rs +++ b/src/auth/commands.rs @@ -48,12 +48,22 @@ pub fn auth_command_group(default_provider: &str, registered_names: &[String]) - .mutates(true) .no_auth(true) .with_arg(provider_arg(&effective_default, registered_names)) - .with_arg(Arg::new("env").long("env").value_name("ENV").required(true)), + .with_arg(Arg::new("env").long("env").value_name("ENV").required(true)) + .with_arg( + Arg::new("scope") + .long("scope") + .short('s') + .value_name("SCOPE") + .num_args(0..) + .help("Additional OAuth scope to request (repeatable)"), + ), async |context| { let provider = string_arg(&context.args, "provider"); let env = string_arg(&context.args, "env"); + let scopes = string_vec_arg(&context.args, "scope"); serde_json::to_value( - login_and_build(&context.middleware.auth, &provider, &env).await?, + login_and_build_with_scopes(&context.middleware.auth, &provider, &env, &scopes) + .await?, ) .map(CommandResult::new) .map_err(Into::into) @@ -106,6 +116,19 @@ fn string_arg(args: &serde_json::Map, name: &str) -> String { .to_owned() } +/// Reads a repeatable string argument as a `Vec`, accepting either a +/// JSON array (multiple values) or a single string. +fn string_vec_arg(args: &serde_json::Map, name: &str) -> Vec { + match args.get(name) { + Some(Value::Array(items)) => items + .iter() + .filter_map(|item| item.as_str().map(str::to_owned)) + .collect(), + Some(Value::String(value)) if !value.is_empty() => vec![value.clone()], + _ => Vec::new(), + } +} + fn provider_arg(default_provider: &str, registered_names: &[String]) -> Arg { let names = registered_names.join(", "); let help = format!("Auth provider name (one of: [{names}])"); @@ -125,7 +148,20 @@ pub async fn login_and_build( provider: &str, env: &str, ) -> Result { - let credential = dispatcher.login(provider, env).await?; + login_and_build_with_scopes(dispatcher, provider, env, &[]).await +} + +/// Like [`login_and_build`], but requests `additional_scopes` on top of the +/// provider's defaults (used by `auth login --scope`). +pub async fn login_and_build_with_scopes( + dispatcher: &Dispatcher, + provider: &str, + env: &str, + additional_scopes: &[String], +) -> Result { + let credential = dispatcher + .login_with_scopes(provider, env, additional_scopes) + .await?; Ok(AuthLoginResult { provider: provider.to_owned(), env: env.to_owned(), diff --git a/src/auth/dispatcher.rs b/src/auth/dispatcher.rs index 31c4511..67418e6 100644 --- a/src/auth/dispatcher.rs +++ b/src/auth/dispatcher.rs @@ -2,7 +2,8 @@ use std::sync::{Arc, RwLock}; use async_trait::async_trait; -use super::{AuthProvider, Credential}; +use super::{AuthProvider, Credential, CredentialRequest}; +use crate::middleware::CommandMeta; use crate::{CliCoreError, Result}; /// Routes auth operations to registered providers by name. @@ -77,13 +78,48 @@ impl Dispatcher { self.get(name)?.get_credential(env, command, tier).await } + /// Gets a credential from a named provider, passing the command's full + /// [`CredentialRequest`] so metadata-aware providers (e.g. OAuth scope + /// step-up) can act on it. + pub async fn get_credential_for( + &self, + name: &str, + req: &CredentialRequest<'_>, + ) -> Result { + self.get(name)?.get_credential_for(req).await + } + /// Clears any cached credential, ignoring logout failures, then authenticates. pub async fn login(&self, name: &str, env: &str) -> Result { + self.login_with_scopes(name, env, &[]).await + } + + /// Like [`login`](Dispatcher::login), but requests `additional_scopes` on top + /// of the provider's defaults. + /// + /// The scopes are carried as [`CommandMeta::scopes`] on a synthesized + /// request; providers without scope support ignore them. + pub async fn login_with_scopes( + &self, + name: &str, + env: &str, + additional_scopes: &[String], + ) -> Result { let provider = self.get(name)?; if let Err(err) = provider.logout(env).await { tracing::debug!(provider = name, error = %err, "ignoring logout error before login"); } - provider.get_credential(env, "", "").await + let meta = CommandMeta { + scopes: additional_scopes.to_vec(), + ..CommandMeta::default() + }; + let req = CredentialRequest { + env, + command: "", + tier: "", + meta: &meta, + }; + provider.get_credential_for(&req).await } /// Gets cached credential status from a named provider. @@ -176,6 +212,10 @@ impl AuthProvider for SingleProvider { .await } + async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { + self.dispatcher.get_credential_for(&self.name, req).await + } + async fn status(&self, env: &str) -> Result { self.dispatcher.status(&self.name, env).await } diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 15dc0d9..67937e4 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -23,8 +23,8 @@ pub mod pkce; use async_trait::async_trait; pub use commands::{ - AuthLoginResult, AuthStatusEntry, auth_command_group, login_and_build, logout_result, - status_result, to_status_entry, + AuthLoginResult, AuthStatusEntry, auth_command_group, login_and_build, + login_and_build_with_scopes, logout_result, status_result, to_status_entry, }; pub use credential::{CACHE_TTL, Credential}; pub use dispatcher::{Dispatcher, SingleProvider, StatusEntry}; @@ -34,6 +34,27 @@ pub use exec::{ }; use crate::Result; +use crate::middleware::CommandMeta; + +/// Everything an [`AuthProvider`] may inspect about the command requesting a +/// credential. +/// +/// This bundles the routing fields passed to [`AuthProvider::get_credential`] +/// (`env`, colon command path, and tier) together with the command's +/// [`CommandMeta`], so a provider can read richer metadata — for example an +/// OAuth provider reading [`CommandMeta::scopes`] to decide whether the cached +/// token is sufficient. Providers that do not need metadata can ignore it. +#[derive(Clone, Copy, Debug)] +pub struct CredentialRequest<'req> { + /// Target environment name. + pub env: &'req str, + /// Colon-separated command path, for example `project:list`. + pub command: &'req str, + /// Risk tier as a string, for example `read` or `mutate`. + pub tier: &'req str, + /// Metadata for the command requesting the credential. + pub meta: &'req CommandMeta, +} #[async_trait] /// Named auth provider used by middleware and transport injectors. @@ -47,6 +68,18 @@ pub trait AuthProvider: Send + Sync + std::fmt::Debug { /// Returns a credential for `env`, `command`, and `tier`. async fn get_credential(&self, env: &str, command: &str, tier: &str) -> Result; + /// Returns a credential for a command, given its full [`CredentialRequest`]. + /// + /// The default implementation ignores the metadata and delegates to + /// [`get_credential`](AuthProvider::get_credential). Providers that act on + /// command metadata — such as an OAuth provider performing scope step-up + /// from [`CommandMeta::scopes`] — override this. The framework calls this + /// method (not `get_credential`) when resolving credentials, so an override + /// receives the command's metadata. + async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { + self.get_credential(req.env, req.command, req.tier).await + } + /// Returns cached credential status for one environment. async fn status(&self, env: &str) -> Result; diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 83c2e32..1b7ea43 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -39,6 +39,7 @@ use std::{ collections::HashMap, + io::IsTerminal, net::{SocketAddr, TcpListener}, sync::Arc, time::Duration, @@ -54,7 +55,7 @@ use sha2::{Digest, Sha256}; use tokio::sync::RwLock; use zeroize::{Zeroize, ZeroizeOnDrop}; -use crate::{Credential, Result, auth::AuthProvider, error::CliCoreError}; +use crate::{Credential, Result, auth::AuthProvider, auth::CredentialRequest, error::CliCoreError}; const REDIRECT_PORT_DEFAULT: u16 = 7443; const TOKEN_EXPIRY_BUFFER_SECS: i64 = 30; @@ -535,12 +536,18 @@ impl PkceAuthProvider { } async fn run_pkce_flow(&self, env: &str) -> Result { + self.run_pkce_flow_with(env, &self.scopes).await + } + + /// Runs the browser PKCE flow requesting exactly `scopes` (used both for the + /// default login and for scope step-up, which requests a wider union). + async fn run_pkce_flow_with(&self, env: &str, scopes: &[String]) -> Result { let (code_verifier, code_challenge) = pkce_challenge(); let state = random_state(); let client_id = self.effective_client_id(); let auth_url = self.effective_auth_url(); let redirect_uri = self.effective_redirect_uri(); - let scope = self.scopes.join(" "); + let scope = scopes.join(" "); let auth_params = [ ("response_type", "code"), @@ -648,6 +655,54 @@ impl AuthProvider for PkceAuthProvider { Ok(self.build_credential(env, &token)) } + async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { + let env = req.env; + let token = self.resolve_token(env).await?; + let required = &req.meta.scopes; + if required.is_empty() { + return Ok(self.build_credential(env, &token)); + } + + // The access token is a JWT carrying its granted scopes; if it already + // covers everything the command needs, no re-auth is required. + let granted = scopes_from_jwt(&token.access_token); + let missing: Vec<&str> = required + .iter() + .filter(|scope| !granted.iter().any(|have| have == *scope)) + .map(String::as_str) + .collect(); + if missing.is_empty() { + return Ok(self.build_credential(env, &token)); + } + + // Step-up means re-consent: the authorization server has no silent + // scope-expansion grant, so widening scopes requires a fresh browser + // flow. Fail fast in non-interactive contexts rather than hang on the + // local callback timeout. + if !std::io::stderr().is_terminal() { + let missing_list = missing.join(", "); + return Err(CliCoreError::message(format!( + "access token for {env:?} is missing required scope(s): {missing_list}; \ + run `auth login --scope {missing_list}` in an interactive terminal" + ))); + } + + // Request the union (defaults ∪ already-granted ∪ required) so step-up + // never drops scopes acquired by an earlier login or step-up. + let mut union = self.scopes.clone(); + for scope in granted.iter().chain(required.iter()) { + if !union.iter().any(|have| have == scope) { + union.push(scope.clone()); + } + } + let new_token = self.run_pkce_flow_with(env, &union).await?; + self.delete_token_from_keychain(env).await; + self.cache.write().await.remove(env); + self.save_token_to_keychain(env, &new_token).await?; + self.store_cached_token(env, new_token.clone()).await; + Ok(self.build_credential(env, &new_token)) + } + async fn status(&self, env: &str) -> Result { let Some(token) = self.load_token_from_keychain(env).await else { return Err(CliCoreError::message(format!( @@ -1030,6 +1085,21 @@ fn decode_jwt_claims(token: &str) -> Option> { serde_json::from_slice(&bytes).ok() } +/// Reads the space-delimited `scope` claim from a JWT access token. +/// +/// Returns an empty list for opaque (non-JWT) tokens or tokens without a +/// `scope` claim, which forces scope step-up to treat them as missing scopes. +fn scopes_from_jwt(token: &str) -> Vec { + decode_jwt_claims(token) + .and_then(|claims| { + claims + .get("scope") + .and_then(Value::as_str) + .map(|scope| scope.split_whitespace().map(str::to_owned).collect()) + }) + .unwrap_or_default() +} + /// Returns the first claim value that is a non-empty string, in priority order. fn extract_identity(claims: &Map, priority: &[String]) -> String { priority @@ -1155,6 +1225,69 @@ mod tests { ); } + #[test] + fn scopes_from_jwt_parses_scope_claim() { + let token = make_jwt(&json!({ "scope": "a b c" })); + assert_eq!(scopes_from_jwt(&token), vec!["a", "b", "c"]); + } + + #[test] + fn scopes_from_jwt_empty_for_opaque_or_missing() { + assert!(scopes_from_jwt("opaque-token").is_empty()); + let no_scope = make_jwt(&json!({ "sub": "user" })); + assert!(scopes_from_jwt(&no_scope).is_empty()); + } + + /// When the cached token's JWT already covers the required scopes, + /// `get_credential_for` must return it without starting a PKCE flow. + #[tokio::test] + async fn get_credential_for_uses_cached_token_when_scopes_covered() { + let provider = test_provider(); + let token = valid_token(&make_jwt(&json!({ + "scope": "apps.app-registry:read apps.app-registry:write", + "sub": "user-1", + }))); + provider.store_cached_token("dev", token).await; + + let meta = crate::middleware::CommandMeta { + scopes: vec!["apps.app-registry:read".to_owned()], + ..crate::middleware::CommandMeta::default() + }; + let req = CredentialRequest { + env: "dev", + command: "app:list", + tier: "read", + meta: &meta, + }; + let credential = provider + .get_credential_for(&req) + .await + .expect("cached token covers required scopes"); + assert_eq!(credential.sub, "user-1"); + } + + /// With no required scopes, `get_credential_for` behaves like + /// `get_credential` and returns the cached token unchanged. + #[tokio::test] + async fn get_credential_for_no_scopes_returns_cached() { + let provider = test_provider(); + provider + .store_cached_token("dev", valid_token("opaque")) + .await; + let meta = crate::middleware::CommandMeta::default(); + let req = CredentialRequest { + env: "dev", + command: "app:list", + tier: "read", + meta: &meta, + }; + let credential = provider + .get_credential_for(&req) + .await + .expect("no scopes required"); + assert_eq!(credential.token, "opaque"); + } + #[test] fn redirect_uri_default_uses_127_0_0_1_and_redirect_port() { let provider = test_provider().with_redirect_port(9000); diff --git a/src/command.rs b/src/command.rs index d5a7ec3..b71056e 100644 --- a/src/command.rs +++ b/src/command.rs @@ -144,6 +144,25 @@ impl CommandContext { pub async fn try_credential(&self) -> Result> { self.credential.try_resolve().await } + + /// Resolves a credential that additionally covers `extra` scopes, on top of + /// the command's declared scopes. + /// + /// Use this when the required scopes are only known at runtime (for example + /// a generic API caller that derives scopes from the target endpoint). A + /// scope-aware auth provider re-authenticates when the cached token does not + /// already cover the requested set. + /// + /// Convenience wrapper over + /// [`self.credential.resolve_with_scopes()`](CredentialResolver::resolve_with_scopes). + /// + /// # Errors + /// + /// Returns an error when the command is marked `no_auth`, or when the auth + /// provider fails to produce a credential. + pub async fn credential_with_scopes(&self, extra: &[String]) -> Result { + self.credential.resolve_with_scopes(extra).await + } } /// Declarative leaf command metadata and parser arguments. @@ -310,6 +329,23 @@ impl CommandSpec { self } + /// Declares the OAuth scopes this command requires. + /// + /// Sugar over [`with_auth_metadata`](CommandSpec::with_auth_metadata) with the + /// `"scopes"` key (whitespace-joined). The scopes surface on + /// [`CommandMeta::scopes`](crate::CommandMeta) and reach the auth provider via + /// [`CredentialRequest`](crate::CredentialRequest); a provider that supports + /// scope step-up re-authenticates when the cached token lacks them. + #[must_use] + pub fn with_scopes(self, scopes: &[impl AsRef]) -> Self { + let joined = scopes + .iter() + .map(AsRef::as_ref) + .collect::>() + .join(" "); + self.with_auth_metadata("scopes", joined) + } + /// Adds a `clap` argument or option to this command. #[must_use] pub fn with_arg(mut self, arg: Arg) -> Self { diff --git a/src/lib.rs b/src/lib.rs index e9ad245..90cc9f5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -88,9 +88,9 @@ pub mod transport; pub mod tree; pub use auth::{ - AuthLoginResult, AuthProvider, AuthStatusEntry, CACHE_TTL, Credential, Dispatcher, - SingleProvider, StatusEntry, auth_command_group, login_and_build, logout_result, status_result, - to_status_entry, + AuthLoginResult, AuthProvider, AuthStatusEntry, CACHE_TTL, Credential, CredentialRequest, + Dispatcher, SingleProvider, StatusEntry, auth_command_group, login_and_build, + login_and_build_with_scopes, logout_result, status_result, to_status_entry, }; pub use cli::{ ApplyFlags, BuildInfo, Cli, CliConfig, CliRunOutput, ExtraSearchDocs, InitDeps, diff --git a/src/middleware.rs b/src/middleware.rs index 9175a73..76d3da7 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -8,10 +8,10 @@ use std::{ use async_trait::async_trait; use serde::{Deserialize, Serialize}; use serde_json::{Map, Value, json}; -use tokio::sync::OnceCell; +use tokio::sync::{Mutex, OnceCell}; use crate::{ - CommandResult, Credential, Dispatcher, Result, SchemaRegistry, Tier, + CommandResult, Credential, CredentialRequest, Dispatcher, Result, SchemaRegistry, Tier, error::{CliCoreError, exit_code_for_error}, output::{ Envelope, HumanViewRegistry, OutputFormat, PipelineOpts, apply_pipeline, @@ -142,9 +142,25 @@ struct ResolverInner { command_path: String, tier: String, no_auth: bool, + /// Static command metadata; `meta.scopes` are always requested. + meta: CommandMeta, + /// Authoritative resolved credential plus the scopes it was requested with. + /// Serializes concurrent resolution and lets scope step-up replace a + /// previously-resolved (narrower) credential. + state: Mutex, + /// Write-once mirror of the first resolved credential so [`CredentialResolver::peek`] + /// can lend a reference without holding a lock. Identity is stable across + /// step-up, so a later wider credential need not replace it for + /// audit/activity purposes. cell: OnceCell, } +#[derive(Debug, Default)] +struct ResolveState { + credential: Option, + requested: Vec, +} + impl std::fmt::Debug for CredentialResolver { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { formatter @@ -165,6 +181,7 @@ impl CredentialResolver { command_path: String, tier: String, no_auth: bool, + meta: CommandMeta, ) -> Self { Self { inner: Arc::new(ResolverInner { @@ -174,6 +191,8 @@ impl CredentialResolver { command_path, tier, no_auth, + meta, + state: Mutex::new(ResolveState::default()), cell: OnceCell::new(), }), } @@ -192,27 +211,79 @@ impl CredentialResolver { "command is marked no_auth and has no credential", )); } + self.resolve_scopes(&[]).await + } + + /// Resolves a credential that additionally covers `extra` scopes (on top of + /// the command's declared [`CommandMeta::scopes`]). + /// + /// Used by handlers whose required scopes are only known at runtime (for + /// example a generic `api call` that derives scopes from the target + /// endpoint). A scope-aware auth provider re-authenticates when the cached + /// token does not already cover the requested set. + /// + /// # Errors + /// + /// Returns an error when the command is marked + /// [`no_auth`](crate::CommandSpec::no_auth), or when the auth provider fails + /// to produce a credential. + pub async fn resolve_with_scopes(&self, extra: &[String]) -> Result { + if self.inner.no_auth { + return Err(CliCoreError::message( + "command is marked no_auth and has no credential", + )); + } + self.resolve_scopes(extra).await + } + + /// Shared resolution: returns the memoized credential when it already covers + /// the wanted scopes, otherwise (re)authenticates requesting the union and + /// updates the memoized credential. + async fn resolve_scopes(&self, extra: &[String]) -> Result { let inner = &self.inner; + let mut want = inner.meta.scopes.clone(); + for scope in extra { + if !want.contains(scope) { + want.push(scope.clone()); + } + } + + let mut state = inner.state.lock().await; + if let Some(credential) = &state.credential + && want.iter().all(|scope| state.requested.contains(scope)) + { + return Ok(credential.clone()); + } + + let mut requested = state.requested.clone(); + for scope in &want { + if !requested.contains(scope) { + requested.push(scope.clone()); + } + } + let meta = CommandMeta { + dry_run_prompt: inner.meta.dry_run_prompt, + auth_metadata: inner.meta.auth_metadata.clone(), + scopes: requested.clone(), + }; + let req = CredentialRequest { + env: &inner.env, + command: &inner.command_path, + tier: &inner.tier, + meta: &meta, + }; let credential = inner - .cell - .get_or_try_init(async || { - inner - .auth - .get_credential( - &inner.provider, - &inner.env, - &inner.command_path, - &inner.tier, - ) - .await - // Mark resolution failures so the engine can classify them as - // `auth-error` based on the error a handler actually returns, - // rather than tracking a separate side-channel flag that could - // go stale if the handler swallows the failure. - .map_err(|source| auth_resolution_error(&inner.provider, source)) - }) - .await?; - Ok(credential.clone()) + .auth + .get_credential_for(&inner.provider, &req) + .await + // Mark resolution failures so the engine can classify them as + // `auth-error` based on the error a handler actually returns. + .map_err(|source| auth_resolution_error(&inner.provider, source))?; + state.credential = Some(credential.clone()); + state.requested = requested; + // Mirror the first resolution for `peek`; ignored once already set. + drop(inner.cell.set(credential.clone())); + Ok(credential) } /// Resolves the credential when one is available. @@ -468,6 +539,7 @@ impl Middleware { command_path.to_owned(), tier_text, no_auth, + meta.clone(), ); if no_auth diff --git a/tests/foundation.rs b/tests/foundation.rs index 19335d4..278dff9 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -3124,6 +3124,16 @@ fn command_spec_metadata_matches_legacy_annotation_resolver_behavior() { assert_eq!(meta.scopes, vec!["read:apps", "write:apps"]); } +#[test] +fn command_spec_with_scopes_round_trips_through_metadata() { + let spec = CommandSpec::new("get", "Get").with_scopes(&["commerce.business:read", "x:y"]); + + let meta = spec.metadata(); + + assert_eq!(meta.auth_metadata["scopes"], "commerce.business:read x:y"); + assert_eq!(meta.scopes, vec!["commerce.business:read", "x:y"]); +} + #[test] fn command_spec_metadata_leaves_provider_unset_by_default() { let spec = CommandSpec::new("list", "List"); @@ -6097,6 +6107,58 @@ async fn middleware_run_does_not_override_explicit_env_arg() { assert_eq!(authz.args().await[0].get("env"), Some(&json!("staging"))); } +#[tokio::test] +async fn middleware_passes_command_scopes_to_provider_and_supports_step_up() { + let recorded = Arc::new(Mutex::new(Vec::>::new())); + let mut middleware = Middleware::new(); + middleware.auth.register(Arc::new(RecordingScopeProvider { + scopes: Arc::clone(&recorded), + })); + middleware.default_auth_provider = "primary".to_owned(); + middleware.app_id = "test-app".to_owned(); + middleware.output_format = "json".to_owned(); + middleware.env = "prod".to_owned(); + + middleware + .run( + middleware_request( + CommandMeta { + dry_run_prompt: false, + auth_metadata: BTreeMap::new(), + scopes: vec!["base:read".to_owned()], + }, + "things:list", + value_map([]), + value_map([]), + "", + false, + ), + async |credential: CredentialResolver| { + // Static command scopes reach the provider. + credential.resolve().await.expect("resolve"); + // A runtime-required scope triggers a re-resolution requesting + // the union of declared + extra scopes. + credential + .resolve_with_scopes(&["extra:write".to_owned()]) + .await + .expect("resolve with scopes"); + // Already-covered scopes do not re-call the provider. + credential + .resolve_with_scopes(&["extra:write".to_owned()]) + .await + .expect("resolve with covered scopes"); + Ok(CommandResult::new(json!({}))) + }, + ) + .await + .expect("middleware success should render"); + + let calls = recorded.lock().await.clone(); + assert_eq!(calls.len(), 2, "third request was already covered"); + assert_eq!(calls[0], vec!["base:read"]); + assert_eq!(calls[1], vec!["base:read", "extra:write"]); +} + #[tokio::test] async fn middleware_fixed_env_overrides_only_auth_env_preserves_legacy() { let captured_env = Arc::new(Mutex::new(Vec::new())); @@ -9545,6 +9607,55 @@ impl AuthProvider for RecordingEnvProvider { } } +/// Records the `meta.scopes` of every `get_credential_for` call, so tests can +/// assert that command scopes (and runtime step-up scopes) reach the provider. +#[derive(Debug)] +struct RecordingScopeProvider { + scopes: Arc>>>, +} + +#[async_trait] +impl AuthProvider for RecordingScopeProvider { + fn name(&self) -> &str { + "primary" + } + + async fn get_credential(&self, env: &str, _command: &str, _tier: &str) -> Result { + // Reached only if the framework bypasses get_credential_for; record an + // empty scope set so such a regression is visible. + self.scopes.lock().await.push(Vec::new()); + Ok(Credential { + token: "token".to_owned(), + env: env.to_owned(), + ..Credential::default() + }) + } + + async fn get_credential_for( + &self, + req: &cli_engine::CredentialRequest<'_>, + ) -> Result { + self.scopes.lock().await.push(req.meta.scopes.clone()); + Ok(Credential { + token: "token".to_owned(), + env: req.env.to_owned(), + ..Credential::default() + }) + } + + async fn status(&self, env: &str) -> Result { + self.get_credential(env, "", "").await + } + + async fn logout(&self, _env: &str) -> Result<()> { + Ok(()) + } + + async fn list_environments(&self) -> Result> { + Ok(Vec::new()) + } +} + #[derive(Debug)] struct FailingProvider; From 326af240bdae9343e33b70a1a4babeaa6c9f2850 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 08:53:53 -0700 Subject: [PATCH 02/14] fix: address Copilot review on scope step-up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- src/auth/commands.rs | 9 ++- src/auth/pkce.rs | 133 ++++++++++++++++++++++++++++--------------- 2 files changed, 92 insertions(+), 50 deletions(-) diff --git a/src/auth/commands.rs b/src/auth/commands.rs index e12ba99..c2f3182 100644 --- a/src/auth/commands.rs +++ b/src/auth/commands.rs @@ -1,4 +1,4 @@ -use clap::Arg; +use clap::{Arg, ArgAction}; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; @@ -54,8 +54,11 @@ pub fn auth_command_group(default_provider: &str, registered_names: &[String]) - .long("scope") .short('s') .value_name("SCOPE") - .num_args(0..) - .help("Additional OAuth scope to request (repeatable)"), + // One scope per occurrence, repeatable: `--scope a --scope b`. + // `ArgAction::Append` requires a value, so a bare `--scope` + // is rejected rather than silently doing nothing. + .action(ArgAction::Append) + .help("Additional OAuth scope to request (repeatable, one per flag)"), ), async |context| { let provider = string_arg(&context.args, "provider"); diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 1b7ea43..265656a 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -510,13 +510,24 @@ impl PkceAuthProvider { } async fn resolve_token(&self, env: &str) -> Result { - if let Some(token) = self.cached_token(env).await { + if let Some(token) = self.existing_token(env).await? { return Ok(token); } + self.reauthenticate(env, &self.scopes).await + } + + /// Returns a usable token from the in-memory cache, keychain, or a refresh — + /// **without** launching an interactive PKCE flow. `None` means the caller + /// must authenticate. Keeping this flow-free lets `get_credential_for` decide + /// the scope set for a single login instead of authenticating twice. + async fn existing_token(&self, env: &str) -> Result> { + if let Some(token) = self.cached_token(env).await { + return Ok(Some(token)); + } if let Some(token) = self.load_token_from_keychain(env).await { if token.is_valid() { self.store_cached_token(env, token.clone()).await; - return Ok(token); + return Ok(Some(token)); } if let Some(refresh_token) = token.refresh_token.as_deref() && let Ok(mut refreshed) = self.refresh_access_token(refresh_token).await @@ -526,19 +537,23 @@ impl PkceAuthProvider { } self.save_token_to_keychain(env, &refreshed).await?; self.store_cached_token(env, refreshed.clone()).await; - return Ok(refreshed); + return Ok(Some(refreshed)); } } - let token = self.run_pkce_flow(env).await?; + Ok(None) + } + + /// Runs a fresh interactive PKCE flow requesting exactly `scopes`, replacing + /// any stored token for `env`. + async fn reauthenticate(&self, env: &str, scopes: &[String]) -> Result { + let token = self.run_pkce_flow_with(env, scopes).await?; + self.delete_token_from_keychain(env).await; + self.cache.write().await.remove(env); self.save_token_to_keychain(env, &token).await?; self.store_cached_token(env, token.clone()).await; Ok(token) } - async fn run_pkce_flow(&self, env: &str) -> Result { - self.run_pkce_flow_with(env, &self.scopes).await - } - /// Runs the browser PKCE flow requesting exactly `scopes` (used both for the /// default login and for scope step-up, which requests a wider union). async fn run_pkce_flow_with(&self, env: &str, scopes: &[String]) -> Result { @@ -657,50 +672,52 @@ impl AuthProvider for PkceAuthProvider { async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { let env = req.env; - let token = self.resolve_token(env).await?; let required = &req.meta.scopes; - if required.is_empty() { - return Ok(self.build_credential(env, &token)); - } - - // The access token is a JWT carrying its granted scopes; if it already - // covers everything the command needs, no re-auth is required. - let granted = scopes_from_jwt(&token.access_token); - let missing: Vec<&str> = required - .iter() - .filter(|scope| !granted.iter().any(|have| have == *scope)) - .map(String::as_str) - .collect(); - if missing.is_empty() { - return Ok(self.build_credential(env, &token)); - } - // Step-up means re-consent: the authorization server has no silent - // scope-expansion grant, so widening scopes requires a fresh browser - // flow. Fail fast in non-interactive contexts rather than hang on the - // local callback timeout. - if !std::io::stderr().is_terminal() { - let missing_list = missing.join(", "); - return Err(CliCoreError::message(format!( - "access token for {env:?} is missing required scope(s): {missing_list}; \ - run `auth login --scope {missing_list}` in an interactive terminal" - ))); - } + // Look for a usable token WITHOUT launching a flow, so we can pick the + // scope set for a single login rather than authenticating twice (e.g. + // `auth login --scope X` logs out first; resolving defaults and then + // stepping up would open the browser twice). + if let Some(token) = self.existing_token(env).await? { + // The access token is a JWT carrying its granted scopes; if it + // already covers everything the command needs, no re-auth is needed. + let granted = scopes_from_jwt(&token.access_token); + let missing: Vec<&str> = required + .iter() + .filter(|scope| !granted.iter().any(|have| have == *scope)) + .map(String::as_str) + .collect(); + if missing.is_empty() { + return Ok(self.build_credential(env, &token)); + } - // Request the union (defaults ∪ already-granted ∪ required) so step-up - // never drops scopes acquired by an earlier login or step-up. - let mut union = self.scopes.clone(); - for scope in granted.iter().chain(required.iter()) { - if !union.iter().any(|have| have == scope) { - union.push(scope.clone()); + // Step-up means re-consent: the authorization server has no silent + // scope-expansion grant. Fail fast in non-interactive contexts + // rather than hang on the local callback timeout. + if !std::io::stderr().is_terminal() { + let display = missing.join(", "); + let hint = missing + .iter() + .map(|scope| format!("--scope {scope}")) + .collect::>() + .join(" "); + return Err(CliCoreError::message(format!( + "access token for {env:?} is missing required scope(s): {display}; \ + run `auth login {hint}` in an interactive terminal" + ))); } + + // Union (defaults ∪ already-granted ∪ required) so step-up never + // drops scopes acquired by an earlier login or step-up. + let union = union_scopes(&self.scopes, &granted, required); + let token = self.reauthenticate(env, &union).await?; + return Ok(self.build_credential(env, &token)); } - let new_token = self.run_pkce_flow_with(env, &union).await?; - self.delete_token_from_keychain(env).await; - self.cache.write().await.remove(env); - self.save_token_to_keychain(env, &new_token).await?; - self.store_cached_token(env, new_token.clone()).await; - Ok(self.build_credential(env, &new_token)) + + // No usable token: authenticate once, requesting defaults ∪ required. + let union = union_scopes(&self.scopes, &[], required); + let token = self.reauthenticate(env, &union).await?; + Ok(self.build_credential(env, &token)) } async fn status(&self, env: &str) -> Result { @@ -1085,6 +1102,17 @@ fn decode_jwt_claims(token: &str) -> Option> { serde_json::from_slice(&bytes).ok() } +/// Returns `defaults ∪ granted ∪ required`, order-preserving and de-duplicated. +fn union_scopes(defaults: &[String], granted: &[String], required: &[String]) -> Vec { + let mut union = defaults.to_vec(); + for scope in granted.iter().chain(required.iter()) { + if !union.contains(scope) { + union.push(scope.clone()); + } + } + union +} + /// Reads the space-delimited `scope` claim from a JWT access token. /// /// Returns an empty list for opaque (non-JWT) tokens or tokens without a @@ -1231,6 +1259,17 @@ mod tests { assert_eq!(scopes_from_jwt(&token), vec!["a", "b", "c"]); } + #[test] + fn union_scopes_dedupes_and_preserves_order() { + let defaults = vec!["a".to_owned(), "b".to_owned()]; + let granted = vec!["b".to_owned(), "c".to_owned()]; + let required = vec!["c".to_owned(), "d".to_owned()]; + assert_eq!( + super::union_scopes(&defaults, &granted, &required), + vec!["a", "b", "c", "d"] + ); + } + #[test] fn scopes_from_jwt_empty_for_opaque_or_missing() { assert!(scopes_from_jwt("opaque-token").is_empty()); From 0ade42fb8c32099c30a1eabe333bbccf2d07d645 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 08:58:41 -0700 Subject: [PATCH 03/14] fix: drop redundant super:: qualification in union_scopes test Resolves the Rust CI failure (unused_qualifications under -D warnings). Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 265656a..933693b 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -1265,7 +1265,7 @@ mod tests { let granted = vec!["b".to_owned(), "c".to_owned()]; let required = vec!["c".to_owned(), "d".to_owned()]; assert_eq!( - super::union_scopes(&defaults, &granted, &required), + union_scopes(&defaults, &granted, &required), vec!["a", "b", "c", "d"] ); } From 60ae55600fc7122ac70a8ad790663bf47657c0fc Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 09:43:49 -0700 Subject: [PATCH 04/14] docs: clarify CredentialResolver re-resolves on scope step-up 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 --- src/middleware.rs | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/middleware.rs b/src/middleware.rs index 76d3da7..20c1b76 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -118,17 +118,23 @@ impl AuthRequirement { /// Resolves the credential for a single command invocation, memoizing the result. /// -/// Resolution — including any interactive browser/OAuth flow — runs at most once: -/// a handler and an authorizer that both ask share a single resolution, and the -/// engine resolves it up front for [`AuthRequirement::Required`] commands. For -/// [`Optional`](AuthRequirement::Optional) commands resolution is deferred until a -/// handler or authorizer calls [`resolve`](Self::resolve) or -/// [`try_resolve`](Self::try_resolve), and `--schema`/`--dry-run` short-circuit -/// before any resolution happens. +/// Resolution — including any interactive browser/OAuth flow — runs once for a +/// given scope set: a handler and an authorizer that both ask share a single +/// resolution, and the engine resolves it up front for +/// [`AuthRequirement::Required`] commands. For [`Optional`](AuthRequirement::Optional) +/// commands resolution is deferred until a handler or authorizer calls +/// [`resolve`](Self::resolve) or [`try_resolve`](Self::try_resolve), and +/// `--schema`/`--dry-run` short-circuit before any resolution happens. /// -/// The resolved credential is memoized: a handler and an authorizer that both -/// ask share a single resolution. Clones share the same underlying state, so the -/// engine can observe (via [`peek`](Self::peek)) whatever a handler resolved. +/// [`resolve_with_scopes`](Self::resolve_with_scopes) may trigger an *additional* +/// resolution when it needs scopes the memoized credential does not yet cover +/// (OAuth scope step-up); a scope-aware provider then re-authenticates for the +/// wider set. Resolutions are serialized, so concurrent callers never launch +/// overlapping interactive flows. +/// +/// The resolved credential is memoized: callers that need no new scopes share a +/// single resolution. Clones share the same underlying state, so the engine can +/// observe (via [`peek`](Self::peek)) whatever a handler resolved. #[derive(Clone)] pub struct CredentialResolver { inner: Arc, From 6515f33adae07c5a9d477e557bae5744773d1367 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 09:52:26 -0700 Subject: [PATCH 05/14] fix: filter empty scope tokens; soften peek identity doc 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 --- src/auth/commands.rs | 6 +++++- src/middleware.rs | 8 +++++--- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/auth/commands.rs b/src/auth/commands.rs index c2f3182..730dc99 100644 --- a/src/auth/commands.rs +++ b/src/auth/commands.rs @@ -123,9 +123,13 @@ fn string_arg(args: &serde_json::Map, name: &str) -> String { /// JSON array (multiple values) or a single string. fn string_vec_arg(args: &serde_json::Map, name: &str) -> Vec { match args.get(name) { + // Drop empty strings: an empty scope token is never valid and only + // produces confusing auth-server errors. Some(Value::Array(items)) => items .iter() - .filter_map(|item| item.as_str().map(str::to_owned)) + .filter_map(Value::as_str) + .filter(|value| !value.is_empty()) + .map(str::to_owned) .collect(), Some(Value::String(value)) if !value.is_empty() => vec![value.clone()], _ => Vec::new(), diff --git a/src/middleware.rs b/src/middleware.rs index 20c1b76..eb3cea8 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -155,9 +155,11 @@ struct ResolverInner { /// previously-resolved (narrower) credential. state: Mutex, /// Write-once mirror of the first resolved credential so [`CredentialResolver::peek`] - /// can lend a reference without holding a lock. Identity is stable across - /// step-up, so a later wider credential need not replace it for - /// audit/activity purposes. + /// can lend a reference without holding a lock. `peek` (used for audit/activity + /// identity) therefore reflects the *first* resolved credential and is not + /// replaced by a later step-up. This assumes step-up re-authenticates the same + /// user — the expected case, though a browser flow could in principle return a + /// different account. cell: OnceCell, } From 7301629b76cf8cd4eb6106eceb460427dc6a0f4d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 10:03:53 -0700 Subject: [PATCH 06/14] fix: robust TTY detection + keep CommandMeta scopes/auth_metadata consistent 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 --- src/auth/dispatcher.rs | 6 ++---- src/auth/pkce.rs | 10 ++++++++-- src/middleware.rs | 24 +++++++++++++++++++----- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/auth/dispatcher.rs b/src/auth/dispatcher.rs index 67418e6..1a21211 100644 --- a/src/auth/dispatcher.rs +++ b/src/auth/dispatcher.rs @@ -109,10 +109,8 @@ impl Dispatcher { if let Err(err) = provider.logout(env).await { tracing::debug!(provider = name, error = %err, "ignoring logout error before login"); } - let meta = CommandMeta { - scopes: additional_scopes.to_vec(), - ..CommandMeta::default() - }; + let mut meta = CommandMeta::default(); + meta.set_scopes(additional_scopes.to_vec()); let req = CredentialRequest { env, command: "", diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 933693b..9c9c399 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -693,8 +693,14 @@ impl AuthProvider for PkceAuthProvider { // Step-up means re-consent: the authorization server has no silent // scope-expansion grant. Fail fast in non-interactive contexts - // rather than hang on the local callback timeout. - if !std::io::stderr().is_terminal() { + // rather than hang on the local callback timeout. Treat the session + // as interactive if any stdio stream is a TTY, so redirecting one + // (e.g. capturing stderr to a log) doesn't block a user who can + // still complete the browser flow. + let interactive = std::io::stdin().is_terminal() + || std::io::stdout().is_terminal() + || std::io::stderr().is_terminal(); + if !interactive { let display = missing.join(", "); let hint = missing .iter() diff --git a/src/middleware.rs b/src/middleware.rs index eb3cea8..1d3881e 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -57,6 +57,23 @@ impl CommandMeta { pub fn fixed_env(&self) -> Option<&str> { self.auth_metadata.get("fixed_env").map(String::as_str) } + + /// Sets the OAuth scopes, keeping [`scopes`](CommandMeta::scopes) and + /// `auth_metadata["scopes"]` consistent. + /// + /// `scopes` is documented as derived from `auth_metadata["scopes"]`, so any + /// code that synthesizes or widens scopes (e.g. runtime step-up) should use + /// this rather than assigning the field directly, so metadata-aware providers + /// reading `auth_metadata` see the same set. An empty list removes the key. + pub fn set_scopes(&mut self, scopes: Vec) { + if scopes.is_empty() { + self.auth_metadata.remove("scopes"); + } else { + self.auth_metadata + .insert("scopes".to_owned(), scopes.join(" ")); + } + self.scopes = scopes; + } } /// Declares whether a command requires an authenticated credential. @@ -269,11 +286,8 @@ impl CredentialResolver { requested.push(scope.clone()); } } - let meta = CommandMeta { - dry_run_prompt: inner.meta.dry_run_prompt, - auth_metadata: inner.meta.auth_metadata.clone(), - scopes: requested.clone(), - }; + let mut meta = inner.meta.clone(); + meta.set_scopes(requested.clone()); let req = CredentialRequest { env: &inner.env, command: &inner.command_path, From 508c31beec5b16bfc8755719eea1aaef7edff120 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 10:22:35 -0700 Subject: [PATCH 07/14] fix: clear empty scopes key; align test + decode_jwt_claims doc 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 --- src/auth/pkce.rs | 12 ++++++++---- src/command.rs | 11 +++++++++-- tests/foundation.rs | 15 +++------------ 3 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 9c9c399..2f188c3 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -1096,10 +1096,14 @@ struct TokenResponse { /// Decodes the claims (payload) segment of a JWT **without verifying the /// signature**. /// -/// The returned claims are used only to display a human-readable identity in -/// `auth status` and audit logs — never for trust or authorization decisions, so -/// signature verification is intentionally skipped. Opaque (non-JWT) tokens and -/// any decode/parse failure yield `None`, leaving the identity blank. +/// The returned claims are used to display a human-readable identity in +/// `auth status` and audit logs, and (via [`scopes_from_jwt`]) to decide whether +/// scope step-up needs a fresh login. These are convenience/optimization reads, +/// **not** trust or authorization decisions — the authorization server remains +/// the source of truth for granted scopes — so signature verification is +/// intentionally skipped. Opaque (non-JWT) tokens and any decode/parse failure +/// yield `None`, leaving the identity blank (and treating scopes as absent, which +/// just forces a re-auth). fn decode_jwt_claims(token: &str) -> Option> { // A JWT is `header.payload.signature`; the payload is the middle segment, // base64url-encoded without padding. diff --git a/src/command.rs b/src/command.rs index b71056e..3812075 100644 --- a/src/command.rs +++ b/src/command.rs @@ -337,13 +337,20 @@ impl CommandSpec { /// [`CredentialRequest`](crate::CredentialRequest); a provider that supports /// scope step-up re-authenticates when the cached token lacks them. #[must_use] - pub fn with_scopes(self, scopes: &[impl AsRef]) -> Self { + pub fn with_scopes(mut self, scopes: &[impl AsRef]) -> Self { let joined = scopes .iter() .map(AsRef::as_ref) .collect::>() .join(" "); - self.with_auth_metadata("scopes", joined) + // Mirror `CommandMeta::set_scopes`: an empty list clears the key rather + // than leaving an empty-but-present `auth_metadata["scopes"]`. + if joined.is_empty() { + self.auth_metadata.remove("scopes"); + } else { + self.auth_metadata.insert("scopes".to_owned(), joined); + } + self } /// Adds a `clap` argument or option to this command. diff --git a/tests/foundation.rs b/tests/foundation.rs index 278dff9..2faa138 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -6119,20 +6119,11 @@ async fn middleware_passes_command_scopes_to_provider_and_supports_step_up() { middleware.output_format = "json".to_owned(); middleware.env = "prod".to_owned(); + let mut meta = CommandMeta::default(); + meta.set_scopes(vec!["base:read".to_owned()]); middleware .run( - middleware_request( - CommandMeta { - dry_run_prompt: false, - auth_metadata: BTreeMap::new(), - scopes: vec!["base:read".to_owned()], - }, - "things:list", - value_map([]), - value_map([]), - "", - false, - ), + middleware_request(meta, "things:list", value_map([]), value_map([]), "", false), async |credential: CredentialResolver| { // Static command scopes reach the provider. credential.resolve().await.expect("resolve"); From 8d425551e60c761864ea6aebd6d71a1503a5499b Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 10:28:38 -0700 Subject: [PATCH 08/14] fix: include --env in non-interactive step-up login hint Per Copilot review: `auth login` requires --env, so the suggested command now reads `auth login --env --scope ...` and is copy-pasteable. Co-Authored-By: Claude Opus 4.8 --- src/auth/pkce.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 2f188c3..3ac3faf 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -709,7 +709,7 @@ impl AuthProvider for PkceAuthProvider { .join(" "); return Err(CliCoreError::message(format!( "access token for {env:?} is missing required scope(s): {display}; \ - run `auth login {hint}` in an interactive terminal" + run `auth login --env {env} {hint}` in an interactive terminal" ))); } From 364405d4b16ed6cdfc4d5be765763b7d0547337f Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 10:37:20 -0700 Subject: [PATCH 09/14] fix: save new token before evicting old in reauthenticate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/auth/pkce.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 3ac3faf..63791c8 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -547,8 +547,10 @@ impl PkceAuthProvider { /// any stored token for `env`. async fn reauthenticate(&self, env: &str, scopes: &[String]) -> Result { let token = self.run_pkce_flow_with(env, scopes).await?; - self.delete_token_from_keychain(env).await; - self.cache.write().await.remove(env); + // Persist first — the keychain write overwrites the existing entry for + // this env — and only update the in-memory cache after a successful + // save. This avoids destroying a still-valid token if the save fails + // (e.g. keychain unavailable and file fallback disabled). self.save_token_to_keychain(env, &token).await?; self.store_cached_token(env, token.clone()).await; Ok(token) @@ -1298,10 +1300,8 @@ mod tests { }))); provider.store_cached_token("dev", token).await; - let meta = crate::middleware::CommandMeta { - scopes: vec!["apps.app-registry:read".to_owned()], - ..crate::middleware::CommandMeta::default() - }; + let mut meta = crate::middleware::CommandMeta::default(); + meta.set_scopes(vec!["apps.app-registry:read".to_owned()]); let req = CredentialRequest { env: "dev", command: "app:list", From 114590ed5b7068a6d1cb7d1ada81ad9b18f4283d Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Wed, 10 Jun 2026 11:01:15 -0700 Subject: [PATCH 10/14] refactor: make CredentialRequest non_exhaustive with a constructor 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. --- src/auth/dispatcher.rs | 7 +------ src/auth/mod.rs | 41 +++++++++++++++++++++++++++++++++++++++++ src/middleware.rs | 7 +------ 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/src/auth/dispatcher.rs b/src/auth/dispatcher.rs index 1a21211..12e6ba0 100644 --- a/src/auth/dispatcher.rs +++ b/src/auth/dispatcher.rs @@ -111,12 +111,7 @@ impl Dispatcher { } let mut meta = CommandMeta::default(); meta.set_scopes(additional_scopes.to_vec()); - let req = CredentialRequest { - env, - command: "", - tier: "", - meta: &meta, - }; + let req = CredentialRequest::new(env, "", "", &meta); provider.get_credential_for(&req).await } diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 67937e4..7bcc347 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -44,7 +44,13 @@ use crate::middleware::CommandMeta; /// [`CommandMeta`], so a provider can read richer metadata — for example an /// OAuth provider reading [`CommandMeta::scopes`] to decide whether the cached /// token is sufficient. Providers that do not need metadata can ignore it. +/// +/// Marked `#[non_exhaustive]` because the framework constructs it (providers only +/// read it) and more request fields may be added over time; build one with +/// [`CredentialRequest::new`] rather than a struct literal so adding a field is +/// not a breaking change for downstream crates. #[derive(Clone, Copy, Debug)] +#[non_exhaustive] pub struct CredentialRequest<'req> { /// Target environment name. pub env: &'req str, @@ -56,6 +62,24 @@ pub struct CredentialRequest<'req> { pub meta: &'req CommandMeta, } +impl<'req> CredentialRequest<'req> { + /// Creates a request from the routing fields and command metadata. + #[must_use] + pub fn new( + env: &'req str, + command: &'req str, + tier: &'req str, + meta: &'req CommandMeta, + ) -> Self { + Self { + env, + command, + tier, + meta, + } + } +} + #[async_trait] /// Named auth provider used by middleware and transport injectors. /// @@ -89,3 +113,20 @@ pub trait AuthProvider: Send + Sync + std::fmt::Debug { /// Lists environments with cached credentials. async fn list_environments(&self) -> Result>; } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn credential_request_new_sets_all_fields() { + let meta = CommandMeta::default(); + let req = CredentialRequest::new("dev", "app:list", "read", &meta); + assert_eq!(req.env, "dev"); + assert_eq!(req.command, "app:list"); + assert_eq!(req.tier, "read"); + // `Copy` is preserved (using `req` after copying it must compile). + let copy = req; + assert_eq!(copy.env, req.env); + } +} diff --git a/src/middleware.rs b/src/middleware.rs index 1d3881e..4ab2549 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -288,12 +288,7 @@ impl CredentialResolver { } let mut meta = inner.meta.clone(); meta.set_scopes(requested.clone()); - let req = CredentialRequest { - env: &inner.env, - command: &inner.command_path, - tier: &inner.tier, - meta: &meta, - }; + let req = CredentialRequest::new(&inner.env, &inner.command_path, &inner.tier, &meta); let credential = inner .auth .get_credential_for(&inner.provider, &req) From 548b988577a09e26f1719bf3031ac15664cdcb6f Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Wed, 10 Jun 2026 11:07:23 -0700 Subject: [PATCH 11/14] fix: make OAuth scope coverage robust for opaque and scp tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/auth/pkce.rs | 309 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 251 insertions(+), 58 deletions(-) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 63791c8..01e4bb8 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -68,6 +68,17 @@ struct StoredToken { access_token: String, expires_at: i64, refresh_token: Option, + /// Scopes the token was obtained with (granted by the authorization server, + /// or the requested set when the server does not echo `scope`). Lets scope + /// coverage work for opaque access tokens and IdPs that do not expose scopes + /// in the access token itself. Not secret, so excluded from zeroization. + /// + /// `#[serde(default)]` keeps tokens written before this field was added + /// loadable from the keychain (they decode with an empty set, falling back to + /// the JWT `scope`/`scp` claim as before). + #[serde(default)] + #[zeroize(skip)] + scopes: Vec, } impl std::fmt::Debug for StoredToken { @@ -83,6 +94,7 @@ impl std::fmt::Debug for StoredToken { &"None" }, ) + .field("scopes", &self.scopes) .finish() } } @@ -530,7 +542,9 @@ impl PkceAuthProvider { return Ok(Some(token)); } if let Some(refresh_token) = token.refresh_token.as_deref() - && let Ok(mut refreshed) = self.refresh_access_token(refresh_token).await + && let Ok(mut refreshed) = self + .refresh_access_token(refresh_token, &token.scopes) + .await { if refreshed.refresh_token.is_none() { refreshed.refresh_token = Some(refresh_token.to_owned()); @@ -546,7 +560,7 @@ impl PkceAuthProvider { /// Runs a fresh interactive PKCE flow requesting exactly `scopes`, replacing /// any stored token for `env`. async fn reauthenticate(&self, env: &str, scopes: &[String]) -> Result { - let token = self.run_pkce_flow_with(env, scopes).await?; + let token = self.run_pkce_flow_with(scopes).await?; // Persist first — the keychain write overwrites the existing entry for // this env — and only update the in-memory cache after a successful // save. This avoids destroying a still-valid token if the save fails @@ -558,7 +572,7 @@ impl PkceAuthProvider { /// Runs the browser PKCE flow requesting exactly `scopes` (used both for the /// default login and for scope step-up, which requests a wider union). - async fn run_pkce_flow_with(&self, env: &str, scopes: &[String]) -> Result { + async fn run_pkce_flow_with(&self, scopes: &[String]) -> Result { let (code_verifier, code_challenge) = pkce_challenge(); let state = random_state(); let client_id = self.effective_client_id(); @@ -595,7 +609,7 @@ impl PkceAuthProvider { let code = wait_for_callback(listener, &state, &callback_path, Duration::from_secs(120)).await?; - self.exchange_code_for_token(&code, &code_verifier, env) + self.exchange_code_for_token(&code, &code_verifier, scopes) .await } @@ -603,7 +617,7 @@ impl PkceAuthProvider { &self, code: &str, code_verifier: &str, - env: &str, + requested_scopes: &[String], ) -> Result { let redirect_uri = self.effective_redirect_uri(); let client_id = self.effective_client_id(); @@ -631,10 +645,14 @@ impl PkceAuthProvider { ))); } - parse_token_response(response, env).await + parse_token_response(response, requested_scopes).await } - async fn refresh_access_token(&self, refresh_token: &str) -> Result { + async fn refresh_access_token( + &self, + refresh_token: &str, + prior_scopes: &[String], + ) -> Result { let client_id = self.effective_client_id(); let token_url = self.effective_token_url(); let params = [ @@ -657,7 +675,7 @@ impl PkceAuthProvider { ))); } - parse_token_response(response, "").await + parse_token_response(response, prior_scopes).await } } @@ -681,45 +699,23 @@ impl AuthProvider for PkceAuthProvider { // `auth login --scope X` logs out first; resolving defaults and then // stepping up would open the browser twice). if let Some(token) = self.existing_token(env).await? { - // The access token is a JWT carrying its granted scopes; if it - // already covers everything the command needs, no re-auth is needed. - let granted = scopes_from_jwt(&token.access_token); - let missing: Vec<&str> = required - .iter() - .filter(|scope| !granted.iter().any(|have| have == *scope)) - .map(String::as_str) - .collect(); - if missing.is_empty() { - return Ok(self.build_credential(env, &token)); - } - - // Step-up means re-consent: the authorization server has no silent - // scope-expansion grant. Fail fast in non-interactive contexts - // rather than hang on the local callback timeout. Treat the session - // as interactive if any stdio stream is a TTY, so redirecting one - // (e.g. capturing stderr to a log) doesn't block a user who can - // still complete the browser flow. - let interactive = std::io::stdin().is_terminal() - || std::io::stdout().is_terminal() - || std::io::stderr().is_terminal(); - if !interactive { - let display = missing.join(", "); - let hint = missing - .iter() - .map(|scope| format!("--scope {scope}")) - .collect::>() - .join(" "); - return Err(CliCoreError::message(format!( - "access token for {env:?} is missing required scope(s): {display}; \ - run `auth login --env {env} {hint}` in an interactive terminal" - ))); + // Decide based on what the token grants (JWT claim plus the scopes it + // was obtained with). Step-up means re-consent: the authorization + // server has no silent scope-expansion grant, so in non-interactive + // contexts we fail fast rather than hang on the callback timeout. + let granted = granted_scopes(&token); + match plan_step_up(&self.scopes, &granted, required, session_is_interactive()) { + StepUp::Covered => return Ok(self.build_credential(env, &token)), + StepUp::MissingNonInteractive(missing) => { + return Err(missing_scope_error(env, &missing)); + } + // Union (defaults ∪ already-granted ∪ required) so step-up never + // drops scopes acquired by an earlier login or step-up. + StepUp::Reauthenticate(union) => { + let token = self.reauthenticate(env, &union).await?; + return Ok(self.build_credential(env, &token)); + } } - - // Union (defaults ∪ already-granted ∪ required) so step-up never - // drops scopes acquired by an earlier login or step-up. - let union = union_scopes(&self.scopes, &granted, required); - let token = self.reauthenticate(env, &union).await?; - return Ok(self.build_credential(env, &token)); } // No usable token: authenticate once, requesting defaults ∪ required. @@ -1093,6 +1089,8 @@ struct TokenResponse { access_token: String, expires_in: Option, refresh_token: Option, + /// Space-delimited scopes the server actually granted, when it echoes them. + scope: Option, } /// Decodes the claims (payload) segment of a JWT **without verifying the @@ -1125,19 +1123,112 @@ fn union_scopes(defaults: &[String], granted: &[String], required: &[String]) -> union } -/// Reads the space-delimited `scope` claim from a JWT access token. +/// Reads the granted scopes from a JWT access token. /// -/// Returns an empty list for opaque (non-JWT) tokens or tokens without a -/// `scope` claim, which forces scope step-up to treat them as missing scopes. +/// OAuth uses a space-delimited `scope` string (RFC), but some IdPs (e.g. Azure +/// AD) use `scp`, and either may be encoded as a JSON array — so all of those +/// forms are accepted. Returns an empty list for opaque (non-JWT) tokens or +/// tokens without a recognized scope claim; coverage then falls back to the +/// scopes recorded on the [`StoredToken`] (see [`granted_scopes`]). fn scopes_from_jwt(token: &str) -> Vec { - decode_jwt_claims(token) - .and_then(|claims| { - claims - .get("scope") - .and_then(Value::as_str) - .map(|scope| scope.split_whitespace().map(str::to_owned).collect()) - }) - .unwrap_or_default() + let Some(claims) = decode_jwt_claims(token) else { + return Vec::new(); + }; + for key in ["scope", "scp"] { + if let Some(value) = claims.get(key) { + let scopes = scopes_from_claim(value); + if !scopes.is_empty() { + return scopes; + } + } + } + Vec::new() +} + +/// Parses a scope claim that may be a space-delimited string or a JSON array of +/// (possibly space-delimited) strings. +fn scopes_from_claim(value: &Value) -> Vec { + match value { + Value::String(scope) => scope.split_whitespace().map(str::to_owned).collect(), + Value::Array(items) => items + .iter() + .filter_map(Value::as_str) + .flat_map(str::split_whitespace) + .map(str::to_owned) + .collect(), + _ => Vec::new(), + } +} + +/// All scopes an access token is known to carry: the JWT `scope`/`scp` claim +/// plus the scopes recorded when the token was obtained. The recorded scopes +/// make coverage work for opaque tokens and IdPs that omit scopes from the +/// access token. +fn granted_scopes(token: &StoredToken) -> Vec { + let mut scopes = scopes_from_jwt(&token.access_token); + for scope in &token.scopes { + if !scopes.contains(scope) { + scopes.push(scope.clone()); + } + } + scopes +} + +/// The action scope step-up should take for a token, given what it already +/// grants and what the command requires. Pure so the decision is unit-testable +/// without real TTY detection or a browser flow. +#[derive(Debug, PartialEq, Eq)] +enum StepUp { + /// The token already covers every required scope. + Covered, + /// Re-authenticate requesting this scope set (defaults ∪ granted ∪ required). + Reauthenticate(Vec), + /// Scopes are missing but the session is non-interactive, so step-up cannot + /// prompt; carries the missing scopes for the error message. + MissingNonInteractive(Vec), +} + +fn plan_step_up( + defaults: &[String], + granted: &[String], + required: &[String], + interactive: bool, +) -> StepUp { + let missing: Vec = required + .iter() + .filter(|scope| !granted.iter().any(|have| have == *scope)) + .cloned() + .collect(); + if missing.is_empty() { + StepUp::Covered + } else if interactive { + StepUp::Reauthenticate(union_scopes(defaults, granted, required)) + } else { + StepUp::MissingNonInteractive(missing) + } +} + +/// Treats the session as interactive if any stdio stream is a TTY, so +/// redirecting one (e.g. capturing stderr) does not block a user who can still +/// complete the browser flow. +fn session_is_interactive() -> bool { + std::io::stdin().is_terminal() + || std::io::stdout().is_terminal() + || std::io::stderr().is_terminal() +} + +/// Error returned when required scopes are missing and step-up cannot prompt. +fn missing_scope_error(env: &str, missing: &[String]) -> CliCoreError { + let display = missing.join(", "); + let hint = missing + .iter() + .map(|scope| format!("--scope {scope}")) + .collect::>() + .join(" "); + CliCoreError::message(format!( + "access token for {env:?} is missing required scope(s): {display}; \ + run `auth login --env {env} {hint}` in an interactive terminal" + )) } /// Returns the first claim value that is a non-empty string, in priority order. @@ -1150,17 +1241,35 @@ fn extract_identity(claims: &Map, priority: &[String]) -> String .to_owned() } -async fn parse_token_response(response: reqwest::Response, _env: &str) -> Result { +async fn parse_token_response( + response: reqwest::Response, + requested_scopes: &[String], +) -> Result { let body: TokenResponse = response .json() .await .map_err(|err| CliCoreError::message(format!("failed to parse token response: {err}")))?; let expires_in = body.expires_in.unwrap_or(3600); let expires_at = Utc::now().timestamp() + expires_in; + // Record what the token grants: the server's echoed `scope` when present, + // otherwise the scopes we asked for. This is the coverage signal for opaque + // tokens, which carry no readable scope claim. + let scopes = body + .scope + .as_deref() + .map(|scope| { + scope + .split_whitespace() + .map(str::to_owned) + .collect::>() + }) + .filter(|scopes| !scopes.is_empty()) + .unwrap_or_else(|| requested_scopes.to_vec()); Ok(StoredToken { access_token: body.access_token, expires_at, refresh_token: body.refresh_token, + scopes, }) } @@ -1223,6 +1332,18 @@ mod tests { access_token: access_token.to_owned(), expires_at: Utc::now().timestamp() + 3600, refresh_token: None, + scopes: Vec::new(), + } + } + + fn token_with_scopes(access_token: &str, scopes: &[&str]) -> StoredToken { + // No struct-update from `valid_token`: StoredToken is `Drop` + // (ZeroizeOnDrop), so fields cannot be moved out of another instance. + StoredToken { + access_token: access_token.to_owned(), + expires_at: Utc::now().timestamp() + 3600, + refresh_token: None, + scopes: scopes.iter().map(|s| (*s).to_owned()).collect(), } } @@ -1232,6 +1353,7 @@ mod tests { // Older than the expiry buffer so is_valid() returns false. expires_at: Utc::now().timestamp() - TOKEN_EXPIRY_BUFFER_SECS - 1, refresh_token: None, + scopes: Vec::new(), } } @@ -1271,6 +1393,77 @@ mod tests { assert_eq!(scopes_from_jwt(&token), vec!["a", "b", "c"]); } + #[test] + fn scopes_from_jwt_parses_scp_and_array_claims() { + // Azure-style `scp` array. + let scp = make_jwt(&json!({ "scp": ["a", "b"] })); + assert_eq!(scopes_from_jwt(&scp), vec!["a", "b"]); + // `scope` encoded as an array. + let array = make_jwt(&json!({ "scope": ["a", "b c"] })); + assert_eq!(scopes_from_jwt(&array), vec!["a", "b", "c"]); + // Empty `scope` falls through to `scp`. + let mixed = make_jwt(&json!({ "scope": "", "scp": ["x"] })); + assert_eq!(scopes_from_jwt(&mixed), vec!["x"]); + } + + #[test] + fn granted_scopes_uses_recorded_scopes_for_opaque_token() { + // An opaque (non-JWT) token carries no readable claim, so coverage comes + // from the scopes recorded when it was obtained. + let token = token_with_scopes("opaque-token", &["a", "b"]); + assert_eq!(granted_scopes(&token), vec!["a", "b"]); + } + + #[test] + fn plan_step_up_covers_reauths_and_fails_non_interactive() { + let defaults = vec!["base".to_owned()]; + let granted = vec!["base".to_owned(), "read".to_owned()]; + let read = vec!["read".to_owned()]; + let write = vec!["write".to_owned()]; + + // Already covered. + assert_eq!( + plan_step_up(&defaults, &granted, &read, true), + StepUp::Covered + ); + // Missing + interactive → reauth requesting the union. + assert_eq!( + plan_step_up(&defaults, &granted, &write, true), + StepUp::Reauthenticate(vec![ + "base".to_owned(), + "read".to_owned(), + "write".to_owned() + ]) + ); + // Missing + non-interactive → fail fast, carrying the missing scopes. + assert_eq!( + plan_step_up(&defaults, &granted, &write, false), + StepUp::MissingNonInteractive(vec!["write".to_owned()]) + ); + } + + /// An opaque cached token whose recorded scopes cover the requirement is + /// returned without starting a flow — proving coverage no longer depends on + /// a readable JWT scope claim. + #[tokio::test] + async fn get_credential_for_uses_recorded_scopes_for_opaque_token() { + let provider = test_provider(); + provider + .store_cached_token("dev", token_with_scopes("opaque-token", &["read", "write"])) + .await; + + let meta = crate::middleware::CommandMeta { + scopes: vec!["read".to_owned()], + ..crate::middleware::CommandMeta::default() + }; + let req = CredentialRequest::new("dev", "app:list", "read", &meta); + let credential = provider + .get_credential_for(&req) + .await + .expect("recorded scopes cover the requirement"); + assert_eq!(credential.token, "opaque-token"); + } + #[test] fn union_scopes_dedupes_and_preserves_order() { let defaults = vec!["a".to_owned(), "b".to_owned()]; From ce569c0258834c0759ee8cead83ed8e07124fa4d Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Wed, 10 Jun 2026 11:08:35 -0700 Subject: [PATCH 12/14] fix: error when step-up cannot obtain the required scopes 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. --- src/auth/pkce.rs | 49 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index 01e4bb8..e16e9cf 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -713,6 +713,7 @@ impl AuthProvider for PkceAuthProvider { // drops scopes acquired by an earlier login or step-up. StepUp::Reauthenticate(union) => { let token = self.reauthenticate(env, &union).await?; + ensure_granted(env, &token, required)?; return Ok(self.build_credential(env, &token)); } } @@ -721,6 +722,7 @@ impl AuthProvider for PkceAuthProvider { // No usable token: authenticate once, requesting defaults ∪ required. let union = union_scopes(&self.scopes, &[], required); let token = self.reauthenticate(env, &union).await?; + ensure_granted(env, &token, required)?; Ok(self.build_credential(env, &token)) } @@ -1217,6 +1219,33 @@ fn session_is_interactive() -> bool { || std::io::stderr().is_terminal() } +/// Confirms a freshly (re)authenticated token actually grants `required`. +/// +/// Re-consent does not guarantee the authorization server grants every requested +/// scope (it may decline by policy). When the difference is detectable — the +/// token is a JWT exposing its scopes, or the token response echoed a narrower +/// `scope` — return a clear error instead of handing back an under-scoped token +/// that the API would later reject with a 403, and instead of re-prompting in a +/// loop the server will keep refusing. (For opaque tokens whose grant the server +/// does not echo, the recorded scopes equal what was requested, so an undetected +/// decline still surfaces downstream as a 403.) +fn ensure_granted(env: &str, token: &StoredToken, required: &[String]) -> Result<()> { + let granted = granted_scopes(token); + let missing: Vec = required + .iter() + .filter(|scope| !granted.iter().any(|have| have == *scope)) + .cloned() + .collect(); + if missing.is_empty() { + Ok(()) + } else { + Err(CliCoreError::message(format!( + "authorization server did not grant required scope(s) for {env:?}: {}", + missing.join(", ") + ))) + } +} + /// Error returned when required scopes are missing and step-up cannot prompt. fn missing_scope_error(env: &str, missing: &[String]) -> CliCoreError { let display = missing.join(", "); @@ -1414,6 +1443,26 @@ mod tests { assert_eq!(granted_scopes(&token), vec!["a", "b"]); } + #[test] + fn ensure_granted_rejects_a_token_missing_required_scopes() { + let required = vec!["a".to_owned(), "b".to_owned()]; + // JWT that exposes only `a` → `b` is detectably not granted. + let jwt = valid_token(&make_jwt(&json!({ "scope": "a" }))); + let err = ensure_granted("dev", &jwt, &required).expect_err("b is not granted"); + assert!( + err.to_string().contains("did not grant required scope(s)"), + "{err}" + ); + assert!(err.to_string().contains('b'), "{err}"); + + // A token granting both passes. + let ok = valid_token(&make_jwt(&json!({ "scope": "a b" }))); + ensure_granted("dev", &ok, &required).expect("both granted"); + // Recorded scopes (opaque token) also satisfy the check. + let opaque = token_with_scopes("opaque", &["a", "b"]); + ensure_granted("dev", &opaque, &required).expect("recorded scopes granted"); + } + #[test] fn plan_step_up_covers_reauths_and_fails_non_interactive() { let defaults = vec!["base".to_owned()]; From 01a7186e36bc8077380eb8c53cd634e8efc3b64a Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Wed, 10 Jun 2026 11:11:08 -0700 Subject: [PATCH 13/14] fix: abort scope step-up that authenticates a different identity `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. --- src/middleware.rs | 32 +++++++++++++++-- tests/foundation.rs | 86 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/src/middleware.rs b/src/middleware.rs index 4ab2549..36ba0b4 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -174,9 +174,10 @@ struct ResolverInner { /// Write-once mirror of the first resolved credential so [`CredentialResolver::peek`] /// can lend a reference without holding a lock. `peek` (used for audit/activity /// identity) therefore reflects the *first* resolved credential and is not - /// replaced by a later step-up. This assumes step-up re-authenticates the same - /// user — the expected case, though a browser flow could in principle return a - /// different account. + /// replaced by a later step-up. That is sound because step-up is required to + /// re-authenticate the *same* identity: [`resolve_scopes`](CredentialResolver::resolve_scopes) + /// aborts if a step-up returns a different account, so the mirrored identity + /// always matches the identity that performed every action in the command. cell: OnceCell, } @@ -296,6 +297,20 @@ impl CredentialResolver { // Mark resolution failures so the engine can classify them as // `auth-error` based on the error a handler actually returns. .map_err(|source| auth_resolution_error(&inner.provider, source))?; + // Guard against a step-up that re-authenticates as a *different* identity. + // `peek` (audit/activity identity) reflects the first resolution, so a + // silent account switch would misattribute the elevated action. Abort + // rather than proceed under a mismatched identity. + if let Some(previous) = &state.credential { + let previous_key = identity_key(previous); + let new_key = identity_key(&credential); + if !previous_key.is_empty() && !new_key.is_empty() && previous_key != new_key { + return Err(CliCoreError::message(format!( + "scope step-up authenticated as a different identity \ + (was {previous_key:?}, now {new_key:?}); aborting" + ))); + } + } state.credential = Some(credential.clone()); state.requested = requested; // Mirror the first resolution for `peek`; ignored once already set. @@ -344,6 +359,17 @@ fn auth_resolution_error(provider: &str, source: CliCoreError) -> CliCoreError { } } +/// Stable identity discriminator for a credential: the subject (`sub`) when set, +/// otherwise the human identity. Empty when the provider exposes neither, in +/// which case the step-up identity guard cannot (and does not) compare. +fn identity_key(credential: &Credential) -> &str { + if credential.sub.is_empty() { + credential.identity.as_str() + } else { + credential.sub.as_str() + } +} + #[async_trait] /// Authorization hook called before business logic. /// diff --git a/tests/foundation.rs b/tests/foundation.rs index 2faa138..c9700b2 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -6150,6 +6150,41 @@ async fn middleware_passes_command_scopes_to_provider_and_supports_step_up() { assert_eq!(calls[1], vec!["base:read", "extra:write"]); } +#[tokio::test] +async fn middleware_aborts_step_up_that_switches_identity() { + let mut middleware = Middleware::new(); + middleware + .auth + .register(Arc::new(SwitchingIdentityProvider { + calls: Arc::new(Mutex::new(0)), + })); + middleware.default_auth_provider = "primary".to_owned(); + middleware.app_id = "test-app".to_owned(); + middleware.output_format = "json".to_owned(); + middleware.env = "prod".to_owned(); + + let mut meta = CommandMeta::default(); + meta.set_scopes(vec!["base:read".to_owned()]); + middleware + .run( + middleware_request(meta, "things:list", value_map([]), value_map([]), "", false), + async |credential: CredentialResolver| { + // First resolution authenticates as user-a (also the `peek` identity). + credential.resolve().await.expect("first resolve"); + // Step-up forces a fresh resolution; the provider returns user-b, + // so the engine must refuse rather than misattribute the action. + let err = credential + .resolve_with_scopes(&["extra:write".to_owned()]) + .await + .expect_err("identity switch during step-up must abort"); + assert!(err.to_string().contains("different identity"), "{err}"); + Ok(CommandResult::new(json!({}))) + }, + ) + .await + .expect("middleware renders"); +} + #[tokio::test] async fn middleware_fixed_env_overrides_only_auth_env_preserves_legacy() { let captured_env = Arc::new(Mutex::new(Vec::new())); @@ -9647,6 +9682,57 @@ impl AuthProvider for RecordingScopeProvider { } } +/// Returns identity `user-a` on its first credential call and `user-b` after, +/// so a step-up re-resolution observes a different account. +#[derive(Debug)] +struct SwitchingIdentityProvider { + calls: Arc>, +} + +impl SwitchingIdentityProvider { + async fn next_credential(&self, env: &str) -> Credential { + let mut calls = self.calls.lock().await; + *calls += 1; + let sub = if *calls == 1 { "user-a" } else { "user-b" }; + Credential { + token: "token".to_owned(), + env: env.to_owned(), + sub: sub.to_owned(), + ..Credential::default() + } + } +} + +#[async_trait] +impl AuthProvider for SwitchingIdentityProvider { + fn name(&self) -> &str { + "primary" + } + + async fn get_credential(&self, env: &str, _command: &str, _tier: &str) -> Result { + Ok(self.next_credential(env).await) + } + + async fn get_credential_for( + &self, + req: &cli_engine::CredentialRequest<'_>, + ) -> Result { + Ok(self.next_credential(req.env).await) + } + + async fn status(&self, env: &str) -> Result { + self.get_credential(env, "", "").await + } + + async fn logout(&self, _env: &str) -> Result<()> { + Ok(()) + } + + async fn list_environments(&self) -> Result> { + Ok(Vec::new()) + } +} + #[derive(Debug)] struct FailingProvider; From db142c3d6dbf41325dbaa6c698d152b5940f0f1c Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Wed, 10 Jun 2026 11:15:16 -0700 Subject: [PATCH 14/14] docs: document HTTP step-up ordering for the bearer injector 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. --- src/command.rs | 6 ++++++ src/middleware.rs | 11 +++++++++++ src/transport/injector.rs | 7 +++++++ 3 files changed, 24 insertions(+) diff --git a/src/command.rs b/src/command.rs index 3812075..145e658 100644 --- a/src/command.rs +++ b/src/command.rs @@ -156,6 +156,12 @@ impl CommandContext { /// Convenience wrapper over /// [`self.credential.resolve_with_scopes()`](CredentialResolver::resolve_with_scopes). /// + /// If the handler also issues HTTP requests through the transport bearer + /// injector, call this **before** the first request: the injector resolves + /// and caches a scope-unaware token, so stepping up afterwards would not + /// affect requests it already authorized. See + /// [`CredentialResolver::resolve_with_scopes`] for the full ordering note. + /// /// # Errors /// /// Returns an error when the command is marked `no_auth`, or when the auth diff --git a/src/middleware.rs b/src/middleware.rs index 36ba0b4..59888a1 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -248,6 +248,17 @@ impl CredentialResolver { /// endpoint). A scope-aware auth provider re-authenticates when the cached /// token does not already cover the requested set. /// + /// # Ordering with the transport injector + /// + /// The HTTP transport's bearer injector resolves its token through the + /// provider's scope-*unaware* path and caches the first token it sees for the + /// injector's lifetime. So when a handler both steps up scopes and makes HTTP + /// calls through that injector, call `resolve_with_scopes` (or + /// [`CommandContext::credential_with_scopes`](crate::CommandContext::credential_with_scopes)) + /// **before** the first request: that populates the provider cache with the + /// wider-scoped token, which the injector then picks up. Resolving after the + /// injector's first `inject` would send the narrower token. + /// /// # Errors /// /// Returns an error when the command is marked diff --git a/src/transport/injector.rs b/src/transport/injector.rs index 6251b72..1ade0ea 100644 --- a/src/transport/injector.rs +++ b/src/transport/injector.rs @@ -172,6 +172,13 @@ impl AuthInjector for ProviderBearerInjector { async fn inject(&self, request: &mut reqwest::Request) -> Result<()> { let mut cached = self.token.lock().await; if cached.as_deref().is_none_or(str::is_empty) { + // Scope-unaware on purpose: this fetches whatever token the provider + // has for `env` (no command scopes) and caches it for the injector's + // lifetime. A handler needing OAuth scope step-up over HTTP must + // resolve the wider scopes first (CredentialResolver::resolve_with_scopes), + // which populates the provider cache so the token fetched here already + // covers them; resolving after the first inject would send the + // narrower token. let credential = self .provider .get_credential(&self.env, "", "")