From fdceb0f4a27109dff0fd5f3f32df994831b6c414 Mon Sep 17 00:00:00 2001 From: Stefano Amorelli Date: Thu, 4 Jun 2026 20:58:35 +0300 Subject: [PATCH] feat(auth): accumulate client-side scopes during step-up authorization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SEP-2350 [1] clarifies that scope accumulation is a client-side responsibility: during re-authorization the client requests the union of its previously requested scopes and the newly challenged scopes, because servers report only the scopes needed for the current operation in 403 / insufficient_scope challenges (RFC 6750 §3.1), not the union of everything granted so far. select_base_scopes returned a single source, so a 403 challenge replaced the previously requested scopes instead of widening them, dropping prior permissions across step-up rounds. I make it union the previously requested scopes, the WWW-Authenticate challenge, and the protected resource metadata scopes (RFC 9728), treating each server-reported set as an operational requirement for the current operation rather than an exclusive directive; AS metadata and caller defaults only seed the request when nothing has been requested or challenged yet. exchange_code_for_token treats an explicit scope list as authoritative, so a server may still narrow the grant. When the server omits scope it has granted exactly what the client requested (RFC 6749 §5.1), so the grant has to fall back to the scopes requested in this round, not the previously granted set; otherwise a step-up that the server confirms by omitting scope would silently drop the just-added permission and the client would loop on the same 403. The widened request was not persisted anywhere the exchange could read it, so I record it on StoredAuthorizationState per authorization (defaulting empty for states stored before this field existed) and resolve the grant from there. This addresses review feedback [2] that the earlier fallback returned the previous grant rather than the request. Deduplication preserves first-seen order for stable, testable output. Tests cover multi-round accumulation, dedup, resource-metadata unioning, and grant resolution when the response omits scope. Implements [3]. [1]: https://github.com/modelcontextprotocol/modelcontextprotocol/blob/main/docs/specification/draft/basic/authorization.mdx#L682 [2]: https://github.com/modelcontextprotocol/rust-sdk/pull/888#pullrequestreview-4454854582 [3]: https://github.com/modelcontextprotocol/rust-sdk/issues/877 Signed-off-by: Stefano Amorelli --- crates/rmcp/src/transport/auth.rs | 209 ++++++++++++++++++++++++++---- 1 file changed, 183 insertions(+), 26 deletions(-) diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index 3c0b5583..29cf57bc 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -160,6 +160,10 @@ pub struct StoredAuthorizationState { pub pkce_verifier: String, pub csrf_token: String, pub created_at: u64, + /// scopes requested in this round, used to resolve the grant when the token response omits + /// `scope` (RFC 6749 §5.1) + #[serde(default)] + pub requested_scopes: Vec, } impl std::fmt::Debug for StoredAuthorizationState { @@ -168,6 +172,7 @@ impl std::fmt::Debug for StoredAuthorizationState { .field("pkce_verifier", &"[REDACTED]") .field("csrf_token", &"[REDACTED]") .field("created_at", &self.created_at) + .field("requested_scopes", &self.requested_scopes) .finish() } } @@ -208,9 +213,16 @@ impl StoredAuthorizationState { .duration_since(std::time::UNIX_EPOCH) .map(|d| d.as_secs()) .unwrap_or(0), + requested_scopes: Vec::new(), } } + /// record the scopes requested in this authorization round (SEP-2350) + pub fn with_requested_scopes(mut self, scopes: Vec) -> Self { + self.requested_scopes = scopes; + self + } + pub fn into_pkce_verifier(self) -> PkceCodeVerifier { PkceCodeVerifier::new(self.pkce_verifier) } @@ -1026,8 +1038,9 @@ impl AuthorizationManager { let (auth_url, csrf_token) = auth_request.url(); - // store pkce verifier for later use via state store - let stored_state = StoredAuthorizationState::new(&pkce_verifier, &csrf_token); + // store pkce verifier and the requested scopes for later use via state store + let stored_state = StoredAuthorizationState::new(&pkce_verifier, &csrf_token) + .with_requested_scopes(scopes.iter().map(|s| s.to_string()).collect()); self.state_store .save(csrf_token.secret(), stored_state) .await?; @@ -1042,11 +1055,33 @@ impl AuthorizationManager { /// compute the union of current scopes and required scopes fn compute_scope_union(current: &[String], required: &str) -> Vec { - let mut scope_set: std::collections::HashSet = current.iter().cloned().collect(); - for scope in required.split_whitespace() { - scope_set.insert(scope.to_string()); + let mut scopes = current.to_vec(); + scopes.extend(required.split_whitespace().map(|s| s.to_string())); + Self::dedup_scopes(scopes) + } + + /// deduplicate scopes preserving first-seen order (SEP-2350: stable for testability) + fn dedup_scopes(scopes: Vec) -> Vec { + let mut seen = std::collections::HashSet::new(); + scopes + .into_iter() + .filter(|s| seen.insert(s.clone())) + .collect() + } + + /// resolve the granted scope set from a token response (SEP-2350, RFC 6749 §5.1): an explicit + /// `scope` may narrow the grant; an omitted one means the request was granted in full, so fall + /// back to the requested scopes (or the previously granted set when none were recorded). + fn resolve_granted_scopes( + response_scopes: Option>, + requested_scopes: &[String], + current_scopes: &[String], + ) -> Vec { + match response_scopes { + Some(scopes) => scopes, + None if !requested_scopes.is_empty() => requested_scopes.to_vec(), + None => current_scopes.to_vec(), } - scope_set.into_iter().collect() } /// check if a scope upgrade is possible and allowed @@ -1069,35 +1104,42 @@ impl AuthorizationManager { scopes } - /// select scopes based on SEP-835 priority: - /// 1. scope from WWW-Authenticate header (argument or stored from initial 401 probe) - /// 2. scopes_supported from protected resource metadata (RFC 9728) - /// 3. scopes_supported from authorization server metadata - /// 4. provided default scopes + /// select scopes following SEP-2350: re-authorization requests the union of the + /// previously requested scopes and the newly challenged scopes. Server-reported + /// scopes (WWW-Authenticate challenge, protected resource metadata) are operational + /// requirements for the current operation, never an exclusive directive, so they + /// accumulate rather than replace. The AS metadata and caller defaults only seed the + /// request when nothing has been requested or challenged yet. fn select_base_scopes( &self, www_authenticate_scope: Option<&str>, default_scopes: &[&str], ) -> Vec { - if let Some(scope) = www_authenticate_scope { - return scope.split_whitespace().map(|s| s.to_string()).collect(); + let mut accumulated: Vec = Vec::new(); + + // previously requested scopes + if let Ok(guard) = self.current_scopes.try_read() { + accumulated.extend(guard.iter().cloned()); } - // use scopes from initial 401 WWW-Authenticate header + // newly challenged scopes for the current operation (RFC 6750 §3.1) + if let Some(scope) = www_authenticate_scope { + accumulated.extend(scope.split_whitespace().map(|s| s.to_string())); + } if let Ok(guard) = self.www_auth_scopes.try_read() { - if !guard.is_empty() { - return guard.clone(); - } + accumulated.extend(guard.iter().cloned()); } - // use scopes_supported from protected resource metadata (RFC 9728) + // scopes required for the current operation per protected resource metadata (RFC 9728) if let Ok(guard) = self.resource_scopes.try_read() { - if !guard.is_empty() { - return guard.clone(); - } + accumulated.extend(guard.iter().cloned()); + } + + if !accumulated.is_empty() { + return Self::dedup_scopes(accumulated); } - // use scopes_supported from authorization server metadata + // nothing requested or challenged yet: seed from AS metadata, then caller defaults if let Some(metadata) = &self.metadata { if let Some(scopes_supported) = &metadata.scopes_supported { if !scopes_supported.is_empty() { @@ -1187,6 +1229,9 @@ impl AuthorizationManager { // Delete state after retrieval (one-time use) self.state_store.delete(csrf_token).await?; + // capture requested scopes before the state is consumed + let requested_scopes = stored_state.requested_scopes.clone(); + // Reconstruct the PKCE verifier let pkce_verifier = stored_state.into_pkce_verifier(); @@ -1225,10 +1270,14 @@ impl AuthorizationManager { debug!("exchange token result: {:?}", token_result); - let granted_scopes: Vec = token_result + // SEP-2350: an omitted `scope` means the grant equals the request (RFC 6749 §5.1). + let response_scopes = token_result .scopes() - .map(|scopes| scopes.iter().map(|s| s.to_string()).collect()) - .unwrap_or_default(); + .map(|scopes| scopes.iter().map(|s| s.to_string()).collect()); + let granted_scopes = { + let current = self.current_scopes.read().await; + Self::resolve_granted_scopes(response_scopes, &requested_scopes, ¤t) + }; *self.current_scopes.write().await = granted_scopes.clone(); *self.scope_upgrade_attempts.write().await = 0; @@ -2915,13 +2964,23 @@ mod tests { fn test_stored_authorization_state_serialization() { let pkce = PkceCodeVerifier::new("my-verifier".to_string()); let csrf = CsrfToken::new("my-csrf".to_string()); - let state = StoredAuthorizationState::new(&pkce, &csrf); + let state = StoredAuthorizationState::new(&pkce, &csrf) + .with_requested_scopes(vec!["read".to_string(), "write".to_string()]); let json = serde_json::to_string(&state).unwrap(); let deserialized: StoredAuthorizationState = serde_json::from_str(&json).unwrap(); assert_eq!(deserialized.pkce_verifier, "my-verifier"); assert_eq!(deserialized.csrf_token, "my-csrf"); + assert_eq!(deserialized.requested_scopes, vec!["read", "write"]); + } + + #[test] + fn stored_authorization_state_defaults_requested_scopes_when_absent() { + let json = r#"{"pkce_verifier":"v","csrf_token":"c","created_at":1}"#; + let state: StoredAuthorizationState = serde_json::from_str(json).unwrap(); + + assert!(state.requested_scopes.is_empty()); } #[test] @@ -3506,6 +3565,104 @@ mod tests { assert!(scopes.contains(&"email".to_string())); } + // -- SEP-2350: client-side scope accumulation in step-up authorization -- + + #[tokio::test] + async fn select_scopes_unions_challenge_with_previously_requested() { + let mgr = manager_with_metadata(None).await; + *mgr.current_scopes.write().await = vec!["read".to_string()]; + + let scopes = mgr.select_scopes(Some("write"), &[]); + + assert_eq!(scopes, vec!["read".to_string(), "write".to_string()]); + } + + #[tokio::test] + async fn select_scopes_does_not_replace_previously_requested_with_challenge() { + let mgr = manager_with_metadata(None).await; + *mgr.current_scopes.write().await = vec!["read".to_string(), "profile".to_string()]; + + let scopes = mgr.select_scopes(Some("write"), &[]); + + assert!(scopes.contains(&"read".to_string())); + assert!(scopes.contains(&"profile".to_string())); + assert!(scopes.contains(&"write".to_string())); + } + + #[tokio::test] + async fn select_scopes_accumulates_across_multiple_step_up_rounds() { + let mgr = manager_with_metadata(None).await; + *mgr.current_scopes.write().await = vec!["read".to_string()]; + + // round one: server challenges for "write" + let round_one = mgr.select_scopes(Some("write"), &[]); + assert_eq!(round_one, vec!["read".to_string(), "write".to_string()]); + *mgr.current_scopes.write().await = round_one; + + // round two: server challenges for "admin", earlier scopes are retained + let round_two = mgr.select_scopes(Some("admin"), &[]); + assert_eq!( + round_two, + vec!["read".to_string(), "write".to_string(), "admin".to_string()] + ); + } + + #[tokio::test] + async fn select_scopes_deduplicates_challenge_already_requested() { + let mgr = manager_with_metadata(None).await; + *mgr.current_scopes.write().await = vec!["read".to_string(), "write".to_string()]; + + let scopes = mgr.select_scopes(Some("write admin"), &[]); + + assert_eq!( + scopes, + vec!["read".to_string(), "write".to_string(), "admin".to_string()] + ); + } + + #[tokio::test] + async fn select_scopes_unions_resource_metadata_as_operational_requirement() { + let mgr = manager_with_metadata(None).await; + *mgr.current_scopes.write().await = vec!["read".to_string()]; + *mgr.resource_scopes.write().await = vec!["profile".to_string()]; + + let scopes = mgr.select_scopes(Some("write"), &[]); + + assert!(scopes.contains(&"read".to_string())); + assert!(scopes.contains(&"write".to_string())); + assert!(scopes.contains(&"profile".to_string())); + } + + #[test] + fn resolve_granted_scopes_uses_requested_when_response_omits_scope() { + let granted = AuthorizationManager::resolve_granted_scopes( + None, + &["read".to_string(), "write".to_string()], + &["read".to_string()], + ); + + assert_eq!(granted, vec!["read".to_string(), "write".to_string()]); + } + + #[test] + fn resolve_granted_scopes_honors_explicit_server_downgrade() { + let granted = AuthorizationManager::resolve_granted_scopes( + Some(vec!["read".to_string()]), + &["read".to_string(), "write".to_string()], + &["read".to_string()], + ); + + assert_eq!(granted, vec!["read".to_string()]); + } + + #[test] + fn resolve_granted_scopes_falls_back_to_current_when_nothing_requested() { + let granted = + AuthorizationManager::resolve_granted_scopes(None, &[], &["read".to_string()]); + + assert_eq!(granted, vec!["read".to_string()]); + } + #[tokio::test] async fn add_offline_access_if_supported_works_with_explicit_scopes() { let mgr = manager_with_metadata(Some(AuthorizationMetadata {