diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index 3c0b5583..29294267 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -1042,11 +1042,18 @@ 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()); - } - scope_set.into_iter().collect() + 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() } /// check if a scope upgrade is possible and allowed @@ -1069,35 +1076,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() { @@ -1225,10 +1239,13 @@ impl AuthorizationManager { debug!("exchange token result: {:?}", token_result); - let granted_scopes: Vec = token_result - .scopes() - .map(|scopes| scopes.iter().map(|s| s.to_string()).collect()) - .unwrap_or_default(); + // SEP-2350: an explicit scope list is authoritative and may narrow the grant, but a + // server that grants the requested scopes may omit `scope` (RFC 6749 §5.1); when it + // does, retain the accumulated set rather than clearing it. + let granted_scopes: Vec = match token_result.scopes() { + Some(scopes) => scopes.iter().map(|s| s.to_string()).collect(), + None => self.current_scopes.read().await.clone(), + }; *self.current_scopes.write().await = granted_scopes.clone(); *self.scope_upgrade_attempts.write().await = 0; @@ -3506,6 +3523,74 @@ 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())); + } + #[tokio::test] async fn add_offline_access_if_supported_works_with_explicit_scopes() { let mgr = manager_with_metadata(Some(AuthorizationMetadata {