mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-11 22:51:28 +00:00
fix(mcp): classify the user-subject reload's errors like the key path (503 outage, 401 missing)
_reload_admitted_user mirrored only part of _reload_admitted_key's error contract: it caught ProxyException and HTTPException but had no arm for anything else, so a transient DB outage surfaced as an opaque 500 instead of the retryable 503 the key path guarantees, and a missing user surfaced as a 500 too. The missing-user case is the subtle one: get_user_object raises a bare Exception for a deleted user (not a ProxyException like get_key_object does for a missing key), so the ProxyException/HTTPException clause never caught it and the user_object-is-None branch it was supposed to hit is unreachable on the production path. Add the same except-Exception arm the key path uses, with the one deliberate difference the differing get_user_object contract requires: a database-service-unavailable error still raises the retryable 503, while a missing user or any other non-outage resolution failure fails closed as a 401 rather than propagating as a 500. The regression tests now drive the real behavior (get_user_object raising) rather than a None return that never happens in production, and cover both the 503 outage and the 401 missing-user paths.
This commit is contained in:
parent
02e9c5631a
commit
f96899ae2b
2 changed files with 39 additions and 8 deletions
|
|
@ -605,8 +605,14 @@ class MCPRequestHandler:
|
|||
caller's centralized policy gate then enforces the user's live budget and org state,
|
||||
and a SCIM-deactivated owner fails closed here exactly as the key path enforces it. No
|
||||
team is bound; a user may belong to many teams or none, so the envelope grants the
|
||||
user's own access rather than silently selecting one team's scope. A missing user
|
||||
fails closed with a 401 rather than admitting an unresolved identity."""
|
||||
user's own access rather than silently selecting one team's scope.
|
||||
|
||||
Error handling mirrors the key path's retryable-503 contract, with one deliberate
|
||||
difference: ``get_key_object`` raises a ``ProxyException`` for a missing key, but
|
||||
``get_user_object`` raises a bare ``Exception`` for a missing user (it does not surface as a
|
||||
``ProxyException``/``HTTPException``). So a transient DB outage still surfaces as a retryable
|
||||
503 via ``_raise_503_if_db_unavailable``, while a missing user, or any other non-outage
|
||||
resolution failure, fails closed as a 401 rather than propagating as an opaque 500."""
|
||||
from litellm.proxy.auth.auth_checks import get_user_object
|
||||
from litellm.proxy.proxy_server import prisma_client, user_api_key_cache
|
||||
|
||||
|
|
@ -621,6 +627,9 @@ class MCPRequestHandler:
|
|||
)
|
||||
except (ProxyException, HTTPException):
|
||||
raise HTTPException(status_code=401, detail="Invalid or expired credential") from None
|
||||
except Exception as e: # noqa: BLE001 # DB outage -> retryable 503; a missing user (bare Exception) or any other resolution failure -> fail closed 401, never an opaque 500
|
||||
MCPRequestHandler._raise_503_if_db_unavailable(e)
|
||||
raise HTTPException(status_code=401, detail="Invalid or expired credential") from None
|
||||
if user_object is None:
|
||||
raise HTTPException(status_code=401, detail="Invalid or expired credential")
|
||||
if isinstance(user_object.metadata, dict) and user_object.metadata.get("scim_active") is False:
|
||||
|
|
|
|||
|
|
@ -5117,8 +5117,10 @@ class TestMCPDcrBridgeDelegateAdmission:
|
|||
}
|
||||
|
||||
async def test_user_subject_envelope_missing_user_fails_closed_401(self):
|
||||
"""A user_id envelope whose user has since been deleted must fail closed: get_user_object
|
||||
resolves None, so admission 401s instead of admitting an unresolved identity."""
|
||||
"""A user_id envelope whose user has since been deleted must fail closed with a 401, not a 500.
|
||||
get_user_object raises a bare Exception for a missing user (it does not return None on the
|
||||
production path), so the reload must catch it and fail closed rather than let it propagate as an
|
||||
opaque 500. Regression for the missing-user path surfacing as a 500."""
|
||||
envelope = self._mint_bridge_envelope(user_id="ghost-user")
|
||||
scope = {
|
||||
"type": "http",
|
||||
|
|
@ -5129,7 +5131,7 @@ class TestMCPDcrBridgeDelegateAdmission:
|
|||
with (
|
||||
patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr,
|
||||
patch("litellm.proxy.proxy_server.master_key", self._MASTER_KEY),
|
||||
self._patch_user_reload(return_value=None),
|
||||
self._patch_user_reload(side_effect=Exception("user not found")),
|
||||
):
|
||||
mock_mgr.get_mcp_server_by_name.return_value = self._bridge_delegate_server()
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
|
|
@ -5137,6 +5139,28 @@ class TestMCPDcrBridgeDelegateAdmission:
|
|||
|
||||
assert exc_info.value.status_code == 401
|
||||
|
||||
async def test_user_subject_envelope_db_outage_is_retryable_503(self):
|
||||
"""A transient database outage while reloading the envelope's user is a retryable 503, not an
|
||||
opaque 500, matching the key path's contract so an interactive DCR client retries instead of
|
||||
treating a live identity as invalid. Regression for the user reload dropping the 503 arm."""
|
||||
envelope = self._mint_bridge_envelope(user_id="sso-user-7")
|
||||
scope = {
|
||||
"type": "http",
|
||||
"method": "POST",
|
||||
"path": "/mcp/bridge_delegate_server",
|
||||
"headers": [(b"authorization", f"Bearer {envelope}".encode("latin-1"))],
|
||||
}
|
||||
with (
|
||||
patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr,
|
||||
patch("litellm.proxy.proxy_server.master_key", self._MASTER_KEY),
|
||||
self._patch_user_reload(side_effect=ConnectionError("auth database unreachable")),
|
||||
):
|
||||
mock_mgr.get_mcp_server_by_name.return_value = self._bridge_delegate_server()
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
await MCPRequestHandler.process_mcp_request(scope)
|
||||
|
||||
assert exc_info.value.status_code == 503
|
||||
|
||||
async def test_user_subject_envelope_scim_deactivated_user_fails_closed_401(self):
|
||||
"""SCIM-deactivating the envelope's user revokes it immediately: the reloaded user carries
|
||||
scim_active False, so admission 401s rather than letting an offboarded user keep tool access
|
||||
|
|
@ -5151,9 +5175,7 @@ class TestMCPDcrBridgeDelegateAdmission:
|
|||
with (
|
||||
patch("litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager") as mock_mgr,
|
||||
patch("litellm.proxy.proxy_server.master_key", self._MASTER_KEY),
|
||||
self._patch_user_reload(
|
||||
return_value=MagicMock(user_id="offboarded-user", metadata={"scim_active": False})
|
||||
),
|
||||
self._patch_user_reload(return_value=MagicMock(user_id="offboarded-user", metadata={"scim_active": False})),
|
||||
):
|
||||
mock_mgr.get_mcp_server_by_name.return_value = self._bridge_delegate_server()
|
||||
with pytest.raises(HTTPException) as exc_info:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue