Repository navigation
feat(auth): add initial authorization scope selection - #1352
zamoshchin-openai wants to merge 6 commits into
Conversation
github-actions
Bot
added
T-core
|
@bolinfest is on triage rotation this week and is the reviewer for this one. Automated classification did not run for this one, so it needs a full manual pass. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
|
@stefanoamorelli, was the scope union in #888 intended to apply to initial login too, or mainly to step-up authorization? For example, an initial challenge requesting This PR adds a separate initial-login selector to preserve the existing union behavior. Would you prefer that approach, or changing the existing selector? |
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub async fn select_initial_scopes(&self, default_scopes: &[&str]) -> Vec<String> { |
There was a problem hiding this comment.
OAuthState::start_authorization resolves the metadata internally, then calls select_scopes. That means callers using the state machine can't access this API without handling AuthorizationManager themselves.
There was a problem hiding this comment.
Added AuthorizationRequest::with_initial_scope_selection() so state-machine callers can opt in directly. Existing defaults and explicit scopes remain unchanged.
| scopes = self.initial_resource_scopes.read().await.clone(); | ||
| } | ||
| if scopes.is_empty() { | ||
| scopes = self.select_fallback_scopes(default_scopes); |
There was a problem hiding this comment.
Since the API is new and opt-in, is there a compatibility reason to keep the AS fallback?
There was a problem hiding this comment.
Agreed—removed the AS fallback, preserving caller defaults and refresh-token handling.
| /// Initial resource challenge only; None means the resource has not been observed yet. | ||
| initial_challenge_scopes: RwLock<Option<Vec<String>>>, | ||
| /// Protected resource scopes from the current discovery, including an empty result. | ||
| initial_resource_scopes: RwLock<Vec<String>>, |
There was a problem hiding this comment.
With two locks, select_initial_scopes could read a challenge from one discovery and resource scopes after a concurrent reset. Also, the two-line reset is duplicated in resolve_metadata and resolve_metadata_from_challenge. Would it make sense to use a single lock, so resetting and reading are each a single step?
There was a problem hiding this comment.
Grouped both fields under one lock, with a shared reset and a single read.
| assert!( | ||
| manager | ||
| .select_scopes(None, &[]) | ||
| .contains(&"metadata:read".to_string()) | ||
| ); |
There was a problem hiding this comment.
This checks a second behavior. If that regresses, it would be reported as an initial-scope failure. Moving it into its own test keeps each test focused on one assertion.
| assert!( | |
| manager | |
| .select_scopes(None, &[]) | |
| .contains(&"metadata:read".to_string()) | |
| ); |
There was a problem hiding this comment.
Moved the legacy-selector assertion into its own compatibility test, preserving coverage.
This branch has not been deployed
3 participants
An MCP server can challenge an initial request with
scope="mcp.read"while its protected-resource metadata advertisesscopes_supported: ["mcp.read", "mcp.write"]. Usingselect_scopes()for that login requests both scopes, even though the challenge only asks for read access. This can introduce unnecessary consent or aninvalid_scoperejection when the client is not permitted to request the additional scope.The MCP scope-selection guidance recommends preferring the initial challenge and using protected-resource metadata when that challenge provides no scopes. RMCP already discovers this information, but its existing selector intentionally accumulates scopes and its discovery result does not expose the separate scope sources. Clients therefore lack a way to choose initial-login precedence using the existing discovery APIs.
Add the opt-in
AuthorizationManager::select_initial_scopes()API so callers can use that precedence without implementing discovery themselves or changingselect_scopes()for existing consumers. It prefers the initial resource challenge, then protected-resource metadata, using only caller-provided defaults when neither supplies scopes. It deduplicates scopes and preservesoffline_accesshandling.AuthorizationRequest::with_initial_scope_selection()exposes the same opt-in throughOAuthStateandAuthorizationSession; automatic authorization keeps the existing policy by default, and nonempty explicit scopes retain precedence.Initial scopes are recorded separately so scope-only challenges work, later metadata challenges cannot replace the resource request's scopes, and repeated discovery does not reuse stale initial scopes.
Validation:
cargo +1.97.1 test -p rmcp --lib --features auth,client,reqwest transport::auth::tests:: --quiet: 228 passed.cargo +1.97.1 test -p rmcp --doc --features auth,client,reqwest select_initial_scopes --quiet: passed.cargo +1.96 clippy -p rmcp --all-targets --all-features -- -D warnings: passed.cargo +nightly fmt --all -- --check: passed.