-
Notifications
You must be signed in to change notification settings - Fork 562
fix(auth): add an SDK path for pre-registered OAuth clients #994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2803,6 +2803,26 @@ impl AuthorizationSession { | |||||
| }) | ||||||
| } | ||||||
|
|
||||||
| /// create a session using pre-registered client credentials, skipping | ||||||
| /// dynamic client registration and URL-based client IDs. | ||||||
| /// | ||||||
| /// The manager must already have discovered authorization server metadata. | ||||||
| pub async fn with_preregistered_client( | ||||||
| mut auth_manager: AuthorizationManager, | ||||||
| config: OAuthClientConfig, | ||||||
| ) -> Result<Self, AuthError> { | ||||||
| let redirect_uri = config.redirect_uri.clone(); | ||||||
| let scopes = config.scopes.clone(); | ||||||
| auth_manager.configure_client(config)?; | ||||||
| let scope_refs: Vec<&str> = scopes.iter().map(|s| s.as_str()).collect(); | ||||||
| let auth_url = auth_manager.get_authorization_url(&scope_refs).await?; | ||||||
| Ok(Self { | ||||||
| auth_manager, | ||||||
| auth_url, | ||||||
| redirect_uri, | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| /// create session for scope upgrade flow (existing manager + pre-computed auth url) | ||||||
| pub fn for_scope_upgrade( | ||||||
| auth_manager: AuthorizationManager, | ||||||
|
|
@@ -3073,6 +3093,36 @@ impl OAuthState { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// start authorization using pre-registered client credentials, | ||||||
| /// skipping dynamic client registration. | ||||||
| /// | ||||||
| /// Use this when the client was registered with the authorization server | ||||||
| /// 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. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| pub async fn start_authorization_with_preregistered_client( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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, |
||||||
| &mut self, | ||||||
| mut config: OAuthClientConfig, | ||||||
| ) -> Result<(), AuthError> { | ||||||
| let placeholder = self.placeholder().await?; | ||||||
| if let OAuthState::Unauthorized(mut manager) = std::mem::replace(self, placeholder) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
| let metadata = manager.discover_metadata().await?; | ||||||
| manager.metadata = Some(metadata); | ||||||
| if config.scopes.is_empty() { | ||||||
| config.scopes = manager.select_scopes(None, &[]); | ||||||
| } else { | ||||||
| manager.add_offline_access_if_supported(&mut config.scopes); | ||||||
| } | ||||||
| let session = AuthorizationSession::with_preregistered_client(manager, config).await?; | ||||||
| *self = OAuthState::Session(session); | ||||||
| Ok(()) | ||||||
| } else { | ||||||
| Err(AuthError::InternalError( | ||||||
| "Already in session state".to_string(), | ||||||
| )) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// complete authorization | ||||||
| pub async fn complete_authorization(&mut self) -> Result<(), AuthError> { | ||||||
| let placeholder = self.placeholder().await?; | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same impl currently exposes
newandfor_scope_upgrade. Whilewith_*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.