mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-11 22:51:28 +00:00
fix(mcp): align token_url suppression with has_client_credentials property
Use oauth2_flow=client_credentials as the sole signal for 2LO suppression instead of client_id+client_secret presence, matching MCPServer.has_client_credentials. - Fixes P0: 3LO servers with client_id+client_secret now keep discovered token_url so exchange_token_with_server() can complete the auth-code exchange - Fixes P1: _is_client_credentials_flow now mirrors has_client_credentials — both require explicit oauth2_flow opt-in, not field presence - Fixes P1: 2LO integration test now sets oauth2_flow=client_credentials so has_client_credentials assertion passes - Updates 3LO test to assert token_url IS populated from discovery and that authorization_url/scopes also survive
This commit is contained in:
parent
e70ad1b980
commit
09884d84c0
2 changed files with 27 additions and 20 deletions
|
|
@ -299,15 +299,17 @@ class MCPServerManager:
|
|||
mcp_oauth_metadata.authorization_url if mcp_oauth_metadata else None
|
||||
)
|
||||
_explicit_token_url = server_config.get("token_url")
|
||||
_has_client_creds = bool(
|
||||
server_config.get("client_id") and server_config.get("client_secret")
|
||||
# Use the same signal as MCPServer.has_client_credentials: explicit opt-in
|
||||
# via oauth2_flow="client_credentials". Presence of client_id/client_secret
|
||||
# alone is ambiguous — both 2LO and 3LO flows use them.
|
||||
_is_client_credentials_flow = (
|
||||
server_config.get("oauth2_flow") == "client_credentials"
|
||||
)
|
||||
_discovered_token_url = mcp_oauth_metadata.token_url if mcp_oauth_metadata else None
|
||||
if not _explicit_token_url and _has_client_creds and _discovered_token_url:
|
||||
if not _explicit_token_url and _is_client_credentials_flow and _discovered_token_url:
|
||||
verbose_logger.warning(
|
||||
"MCP server '%s': auto-discovered token_url '%s' ignored because "
|
||||
"client_id + client_secret are set without an explicit token_url, "
|
||||
"which signals a 3LO (authorization_code) flow. "
|
||||
"oauth2_flow=client_credentials is set without an explicit token_url. "
|
||||
"Set token_url explicitly in config to use 2LO (client_credentials).",
|
||||
server_id,
|
||||
_discovered_token_url,
|
||||
|
|
|
|||
|
|
@ -1211,14 +1211,15 @@ class TestMCPServerManager:
|
|||
assert server.has_client_credentials is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_pkce_creds_without_explicit_token_url_stays_3lo(self):
|
||||
async def test_client_creds_without_explicit_token_url_stays_3lo(self):
|
||||
"""
|
||||
Regression test for: MCP OAuth 3LO fails when token_url is auto-discovered.
|
||||
Regression test: client_id + client_secret without oauth2_flow=client_credentials
|
||||
must stay in the 3LO (authorization_code) path.
|
||||
|
||||
When client_id + client_secret are configured but token_url is absent (3LO intent),
|
||||
auto-discovered token_url must NOT be stored on the server — otherwise
|
||||
has_client_credentials returns True and incorrectly triggers the
|
||||
client_credentials (2LO) grant, leaving the tool set empty
|
||||
- has_client_credentials must be False (no explicit opt-in via oauth2_flow)
|
||||
- token_url MUST be populated from discovery so exchange_token_with_server()
|
||||
can complete the auth-code exchange (raises HTTP 400 if token_url is None)
|
||||
- authorization_url and scopes must also be populated from discovery
|
||||
"""
|
||||
manager = MCPServerManager()
|
||||
|
||||
|
|
@ -1239,22 +1240,25 @@ class TestMCPServerManager:
|
|||
"auth_type": MCPAuth.oauth2,
|
||||
"client_id": "my-client-id",
|
||||
"client_secret": "my-client-secret",
|
||||
# Intentionally no token_url — signals 3LO (user-authorised) flow
|
||||
# No oauth2_flow — signals 3LO (user-authorised) flow
|
||||
}
|
||||
}
|
||||
|
||||
await manager.load_servers_from_config(config)
|
||||
|
||||
server = next(iter(manager.config_mcp_servers.values()))
|
||||
# token_url must NOT be populated from discovery when PKCE creds are present
|
||||
assert server.token_url is None, (
|
||||
"Auto-discovered token_url should not be set when client_id+client_secret "
|
||||
"are configured without an explicit token_url (3LO intent)"
|
||||
)
|
||||
assert server.has_client_credentials is False, (
|
||||
"has_client_credentials must be False for 3LO servers so the "
|
||||
"client_credentials grant is not triggered"
|
||||
"has_client_credentials must be False when oauth2_flow is not set to "
|
||||
"client_credentials — presence of client_id/secret alone is ambiguous"
|
||||
)
|
||||
# token_url must be populated from discovery so the 3LO auth-code exchange works
|
||||
assert server.token_url == "https://discovered.example.com/token", (
|
||||
"Auto-discovered token_url must be preserved for 3LO servers — "
|
||||
"exchange_token_with_server() raises HTTP 400 if token_url is None"
|
||||
)
|
||||
# authorization_url and scopes must also survive from discovery
|
||||
assert server.authorization_url == "https://discovered.example.com/auth"
|
||||
assert server.scopes == ["read"]
|
||||
assert server.needs_user_oauth_token is True
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -1284,7 +1288,8 @@ class TestMCPServerManager:
|
|||
"auth_type": MCPAuth.oauth2,
|
||||
"client_id": "my-client-id",
|
||||
"client_secret": "my-client-secret",
|
||||
"token_url": "https://explicit.example.com/token", # 2LO intent
|
||||
"token_url": "https://explicit.example.com/token",
|
||||
"oauth2_flow": "client_credentials", # explicit 2LO opt-in
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue