Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ ipython_config.py
# pyenv
# For a library or package, you might want to ignore these files since the code is
# intended to run in multiple environments; otherwise, check them in:
# .python-version
.python-version

# pipenv
# According to pypa/pipenv#598, it is recommended to include Pipfile.lock in version control.
Expand Down
15 changes: 12 additions & 3 deletions src/mcp/client/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -478,9 +478,18 @@ async def _handle_oauth_metadata_response(self, response: httpx.Response) -> Non
content = await response.aread()
metadata = OAuthMetadata.model_validate_json(content)
self.context.oauth_metadata = metadata
# Apply default scope if needed
if self.context.client_metadata.scope is None and metadata.scopes_supported is not None:
self.context.client_metadata.scope = " ".join(metadata.scopes_supported)

# Only set scope if client_metadata.scope is None
if self.context.client_metadata.scope is None:
# Priority 1: Use PRM's scopes_supported if available
if (
self.context.protected_resource_metadata is not None
and self.context.protected_resource_metadata.scopes_supported is not None
):
self.context.client_metadata.scope = " ".join(self.context.protected_resource_metadata.scopes_supported)
# Priority 2: Fall back to OAuth metadata scopes if available
elif metadata.scopes_supported is not None:
self.context.client_metadata.scope = " ".join(metadata.scopes_supported)
Copy link
Member

Choose a reason for hiding this comment

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

see this latest change to the spec:

When implementing authorization flows, MCP clients SHOULD follow the principle of least privilege by requesting
only the scopes necessary for their intended operations. During the initial authorization handshake, MCP clients
SHOULD follow this priority order for scope selection:

  1. Use scope parameter from the initial WWW-Authenticate header in the 401 response, if provided
  2. If scope is not available, use all scopes defined in scopes_supported from the Protected Resource Metadata document, omitting the scope parameter if scopes_supported is undefined.

This approach accommodates the general-purpose nature of MCP clients, which typically lack domain-specific knowledge to make informed decisions about individual scope selection. Requesting all available scopes allows the authorization server and end-user to determine appropriate permissions during the consent process.

Are you able to update this PR to:

  1. Get scope from www-authenticate
  2. Omit scope if not present in PRM? (currently impl falls back to scopes_supported from AS metadata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Although the complete priority order is:

  1. Explicitly configured scope in the client
  2. www-authenticate scope
  3. PRM scope
  4. Omit

Copy link
Member

Choose a reason for hiding this comment

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

Hm I don't think (1) is correct there. If we always use what's configured on the client, there's no path for upgrading scope if the initial request didn't have the correct scope.

We had some discussion in this across a few PR's (example) and in discord about similar issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to this spec https://modelcontextprotocol.io/specification/draft/basic/authorization#scope-challenge-handling the step-up only happens when the server returns 403. The changes in this PR only take effect upon a 401.

I can definitely update the 401 path by not prioritizing the configured scope on the client, but the step-up logic remains unhandled in the MCP client in this SDK. I can create a separate PR for that if that makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can make it part of this PR too considering this is already a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

working on adding step-up auth flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


async def async_auth_flow(self, request: httpx.Request) -> AsyncGenerator[httpx.Request, httpx.Response]:
"""HTTPX auth flow integration."""
Expand Down
133 changes: 133 additions & 0 deletions tests/client/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,63 @@ async def callback_handler() -> tuple[str, str | None]:
)


@pytest.fixture
def oauth_provider_without_scope(oauth_provider: OAuthClientProvider) -> OAuthClientProvider:
"""Create OAuth provider without predefined scope."""
oauth_provider.context.client_metadata.scope = None
return oauth_provider


@pytest.fixture
def oauth_metadata_response():
"""Common OAuth metadata response with scopes."""
return httpx.Response(
200,
content=(
b'{"issuer": "https://auth.example.com", '
b'"authorization_endpoint": "https://auth.example.com/authorize", '
b'"token_endpoint": "https://auth.example.com/token", '
b'"registration_endpoint": "https://auth.example.com/register", '
b'"scopes_supported": ["read", "write", "admin"]}'
),
)


@pytest.fixture
def prm_metadata():
"""PRM metadata with scopes."""
return ProtectedResourceMetadata(
resource=AnyHttpUrl("https://api.example.com/v1/mcp"),
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
scopes_supported=["resource:read", "resource:write"],
)


@pytest.fixture
def prm_metadata_without_scopes():
"""PRM metadata without scopes."""
return ProtectedResourceMetadata(
resource=AnyHttpUrl("https://api.example.com/v1/mcp"),
authorization_servers=[AnyHttpUrl("https://auth.example.com")],
scopes_supported=None,
)


@pytest.fixture
def oauth_metadata_response_without_scopes():
"""OAuth metadata response without scopes."""
return httpx.Response(
200,
content=(
b'{"issuer": "https://auth.example.com", '
b'"authorization_endpoint": "https://auth.example.com/authorize", '
b'"token_endpoint": "https://auth.example.com/token", '
b'"registration_endpoint": "https://auth.example.com/register"}'
# No scopes_supported field
),
)


class TestPKCEParameters:
"""Test PKCE parameter generation."""

Expand Down Expand Up @@ -391,6 +448,82 @@ async def test_handle_metadata_response_success(self, oauth_provider: OAuthClien
assert oauth_provider.context.oauth_metadata is not None
assert str(oauth_provider.context.oauth_metadata.issuer) == "https://auth.example.com/"

@pytest.mark.anyio
async def test_prioritize_prm_scopes_over_oauth_metadata(
self,
oauth_provider_without_scope: OAuthClientProvider,
oauth_metadata_response: httpx.Response,
prm_metadata: ProtectedResourceMetadata,
):
"""Test that PRM scopes are prioritized over auth server metadata scopes."""
provider = oauth_provider_without_scope

# Set up PRM metadata with specific scopes
provider.context.protected_resource_metadata = prm_metadata

# Process the OAuth metadata
await provider._handle_oauth_metadata_response(oauth_metadata_response)

# Verify that PRM scopes are used (not OAuth metadata scopes)
assert provider.context.client_metadata.scope == "resource:read resource:write"

@pytest.mark.anyio
async def test_fallback_to_oauth_metadata_scopes_when_no_prm_scopes(
self,
oauth_provider_without_scope: OAuthClientProvider,
oauth_metadata_response: httpx.Response,
prm_metadata_without_scopes: ProtectedResourceMetadata,
):
"""Test fallback to OAuth metadata scopes when PRM has no scopes."""
provider = oauth_provider_without_scope

# Set up PRM metadata without scopes
provider.context.protected_resource_metadata = prm_metadata_without_scopes

# Process the OAuth metadata
await provider._handle_oauth_metadata_response(oauth_metadata_response)

# Verify that OAuth metadata scopes are used as fallback
assert provider.context.client_metadata.scope == "read write admin"

@pytest.mark.anyio
async def test_no_scope_changes_when_both_missing(
self,
oauth_provider_without_scope: OAuthClientProvider,
prm_metadata_without_scopes: ProtectedResourceMetadata,
oauth_metadata_response_without_scopes: httpx.Response,
):
"""Test that no scope changes occur when both PRM and OAuth metadata lack scopes."""
provider = oauth_provider_without_scope

# Set up PRM metadata without scopes
provider.context.protected_resource_metadata = prm_metadata_without_scopes

# Process the OAuth metadata
await provider._handle_oauth_metadata_response(oauth_metadata_response_without_scopes)

# Verify that scope remains None
assert provider.context.client_metadata.scope is None

@pytest.mark.anyio
async def test_preserve_existing_client_scope(
self,
oauth_provider: OAuthClientProvider,
oauth_metadata_response: httpx.Response,
prm_metadata: ProtectedResourceMetadata,
):
"""Test that existing client scope is preserved regardless of metadata."""
provider = oauth_provider

# Set up PRM metadata with scopes
provider.context.protected_resource_metadata = prm_metadata

# Process the OAuth metadata
await provider._handle_oauth_metadata_response(oauth_metadata_response)

# Verify that predefined scope is preserved
assert provider.context.client_metadata.scope == "read write"

@pytest.mark.anyio
async def test_register_client_request(self, oauth_provider: OAuthClientProvider):
"""Test client registration request building."""
Expand Down
Loading