fix(auth): add an SDK path for pre-registered OAuth clients#994
Conversation
github-actions
Bot
added
T-core
| /// dynamic client registration and URL-based client IDs. | ||
| /// | ||
| /// The manager must already have discovered authorization server metadata. | ||
| pub async fn with_preregistered_client( |
There was a problem hiding this comment.
The same impl currently exposes new and for_scope_upgrade. While with_* is valid Rust naming, the three constructors do not seem to communicate an obvious shared convention. I was just thinking we could clean up the API, since v3 allows us to make breaking changes.
There was a problem hiding this comment.
Yes, good point, we should take the opportunity. Probably best in a follow-up PR - what do you think? What's your preferred convention?
There was a problem hiding this comment.
My preference would be a single AuthorizationSession::new(...) plus chained builder methods for the optional parts. Same shape as OAuthClientConfig.
There was a problem hiding this comment.
Oh, I found this: https://rust-lang.github.io/api-guidelines/naming.html Just FYI
| /// out of band and already holds a `client_id` (and optionally a | ||
| /// `client_secret`). If `config.scopes` is empty, scopes are selected | ||
| /// from the discovered authorization server metadata. | ||
| pub async fn start_authorization_with_preregistered_client( |
There was a problem hiding this comment.
Could we add targeted tests using the existing recording HTTP client to verify that this path skips the registration endpoint, handles default and explicit scopes, and etc? Those tests would isolate the new orchestration from the broader conformance harness. e.g.
rust-sdk/crates/rmcp/src/transport/auth.rs
Lines 3870 to 3871 in 98bb263
| /// out of band and already holds a `client_id` (and optionally a | ||
| /// `client_secret`). If `config.scopes` is empty, scopes are selected | ||
| /// from the discovered authorization server metadata. | ||
| pub async fn start_authorization_with_preregistered_client( |
There was a problem hiding this comment.
The draft authorization spec recommends the priority pre-registered client information → CIMD → DCR, but the current API exposes these choices through separate entry points. In particular, start_authorization() passes no metadata URL and therefore cannot take the preferred CIMD path, while start_authorization_with_metadata_url() is the method that actually implements CIMD with DCR fallback. This PR understandably extends that pattern with another mechanism-specific method, but we might want to consolidates these paths behind one start_authorization API. This is out of scope and non-blocking for this PR. Let me know what you think, and we can follow up on this separately.
There was a problem hiding this comment.
That makes sense to me. Probably a good thing to do jointly along with cleaning up the API as you suggested in the above comment.
Opened: #1006
There was a problem hiding this comment.
@jamadeo Sounds good! Let's fast-follow on this so we can include it in v3 alongside the other breaking changes.
... and exercise it in conformance tests
0a82f65 to
9e87e9b
Compare
| let placeholder = self.placeholder().await?; | ||
| if let OAuthState::Unauthorized(mut manager) = std::mem::replace(self, placeholder) { | ||
| debug!("start discovery"); | ||
| let metadata = manager.discover_metadata().await?; |
There was a problem hiding this comment.
Should the same recovery pattern be applied to the existing paths too?
2 participants
... and exercise it in conformance tests
fixes #984
Motivation and Context
The sdk didn't have a high-level path for preregistered oauth clients, so the conformance client implemented it in a more custom way, which didn't pick up the
iss-parameter preservation.How Has This Been Tested?
Ran the conformance client.
Breaking Changes
None
Types of changes
Checklist
Additional context