Skip to content

fix(oauth): The dynamic client registration should be optional #463

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −19
Open

fix(oauth): The dynamic client registration should be optional #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 15 additions & 18 deletions crates/rmcp/src/transport/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use reqwest::{Client as HttpClient, IntoUrl, StatusCode, Url, header::AUTHORIZAT
use serde::{Deserialize, Serialize};
use thiserror::Error;
use tokio::sync::{Mutex, RwLock};
use tracing::{debug, error};
use tracing::{debug, error, warn};

const DEFAULT_EXCHANGE_URL: &str = "http://localhost";

Expand Down Expand Up @@ -102,7 +102,7 @@ pub enum AuthError {
pub struct AuthorizationMetadata {
pub authorization_endpoint: String,
pub token_endpoint: String,
pub registration_endpoint: String,
pub registration_endpoint: Option<String>,
pub issuer: Option<String>,
pub jwks_uri: Option<String>,
pub scopes_supported: Option<Vec<String>>,
Expand Down Expand Up @@ -273,7 +273,7 @@ impl AuthorizationManager {
return Ok(metadata);
}

debug!("No valid .well-known endpoint found, falling back to default endpoints");
warn!("No valid .well-known endpoint found, falling back to default endpoints");

// fallback to default endpoints
let mut auth_base = self.base_url.clone();
Expand All @@ -290,7 +290,7 @@ impl AuthorizationManager {
Ok(AuthorizationMetadata {
authorization_endpoint: create_endpoint("authorize"),
token_endpoint: create_endpoint("token"),
registration_endpoint: create_endpoint("register"),
registration_endpoint: None,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional
image
.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)

issuer: None,
jwks_uri: None,
scopes_supported: None,
Expand Down Expand Up @@ -323,12 +323,10 @@ impl AuthorizationManager {
let token_url = TokenUrl::new(metadata.token_endpoint.clone())
.map_err(|e| AuthError::OAuthError(format!("Invalid token URL: {}", e)))?;

// debug!("token url: {:?}", token_url);
let client_id = ClientId::new(config.client_id);
let redirect_url = RedirectUrl::new(config.redirect_uri.clone())
.map_err(|e| AuthError::OAuthError(format!("Invalid re URL: {}", e)))?;

debug!("client_id: {:?}", client_id);
let mut client_builder = BasicClient::new(client_id.clone())
.set_auth_uri(auth_url)
.set_token_uri(token_url)
Expand All @@ -349,14 +347,16 @@ impl AuthorizationManager {
redirect_uri: &str,
) -> Result<OAuthClientConfig, AuthError> {
if self.metadata.is_none() {
error!("No authorization support detected");
return Err(AuthError::NoAuthorizationSupport);
}

let metadata = self.metadata.as_ref().unwrap();
let registration_url = metadata.registration_endpoint.clone();
let Some(registration_url) = metadata.registration_endpoint.as_ref() else {
return Err(AuthError::RegistrationFailed(
"Dynamic client registration not supported".to_string(),
));
};

debug!("registration url: {:?}", registration_url);
// prepare registration request
let registration_request = ClientRegistrationRequest {
client_name: name.to_string(),
Expand All @@ -369,8 +369,6 @@ impl AuthorizationManager {
response_types: vec!["code".to_string()],
};

debug!("registration request: {:?}", registration_request);

let response = match self
.http_client
.post(registration_url)
Expand All @@ -380,7 +378,6 @@ impl AuthorizationManager {
{
Ok(response) => response,
Err(e) => {
error!("Registration request failed: {}", e);
return Err(AuthError::RegistrationFailed(format!(
"HTTP request error: {}",
e
Expand All @@ -395,7 +392,6 @@ impl AuthorizationManager {
Err(_) => "cannot get error details".to_string(),
};

error!("Registration failed: HTTP {} - {}", status, error_text);
return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
Expand All @@ -406,7 +402,6 @@ impl AuthorizationManager {
let reg_response = match response.json::<ClientRegistrationResponse>().await {
Ok(response) => response,
Err(e) => {
error!("Failed to parse registration response: {}", e);
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
Expand Down Expand Up @@ -471,7 +466,6 @@ impl AuthorizationManager {
pkce_verifier,
csrf_token,
});
debug!("set authorization state: {:?}", self.state.read().await);

Ok(auth_url.to_string())
}
Expand Down Expand Up @@ -625,9 +619,9 @@ impl AuthorizationSession {
redirect_uri: &str,
client_name: Option<&str>,
) -> Result<Self, AuthError> {
// set redirect uri
// Default client config
let config = OAuthClientConfig {
client_id: "mcp-client".to_string(), // temporary id, will be updated by dynamic registration
client_id: "mcp-client".to_string(),
client_secret: None,
scopes: scopes.iter().map(|s| s.to_string()).collect(),
redirect_uri: redirect_uri.to_string(),
Expand All @@ -640,7 +634,10 @@ impl AuthorizationSession {
{
Ok(config) => config,
Err(e) => {
eprintln!("Dynamic registration failed: {}", e);
warn!(
"Dynamic registration failed: {}, fallback to default config",
e
);
// fallback to default config
config
}
Expand Down
2 changes: 1 addition & 1 deletion examples/servers/src/complex_auth_sse.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,7 +525,7 @@ async fn oauth_authorization_server() -> impl IntoResponse {
authorization_endpoint: format!("http://{}/oauth/authorize", BIND_ADDRESS),
token_endpoint: format!("http://{}/oauth/token", BIND_ADDRESS),
scopes_supported: Some(vec!["profile".to_string(), "email".to_string()]),
registration_endpoint: format!("http://{}/oauth/register", BIND_ADDRESS),
registration_endpoint: Some(format!("http://{}/oauth/register", BIND_ADDRESS)),
issuer: Some(BIND_ADDRESS.to_string()),
jwks_uri: Some(format!("http://{}/oauth/jwks", BIND_ADDRESS)),
additional_fields,
Expand Down
Loading