Skip to content

Commit 43a217b

Browse files
committed
fix(auth): enforce is_active check + extend DB-backed proxy auth to primary MCP path
Second follow-up addressing findings from a deeper security + design review: SECURITY FIX (critical): - _authenticate_proxy_user() now enforces user_info.is_active, matching the JWT path's check in _enforce_revocation_and_active_user (:398-399). Without this, a disabled user - including a disabled admin - could still authenticate via trusted-proxy mode and keep their pre-disable authorizations. Raises 401 'Account disabled' for inactive users. PRIMARY MCP TRANSPORT GAP (critical): - streamablehttp_transport._set_proxy_user_context() previously hardcoded teams=[] and is_admin=False, so on the primary MCP entry point (_StreamableHttpAuthHandler.authenticate) proxy-authenticated users still received public-only access. Only the stateful-session fallback path at :1953 went through the enriched helper. - Rewritten as an async function that performs the same DB lookup, team/admin resolution, is_active enforcement, and platform-admin bootstrap as the REST helper. Returns None on success or {detail, headers} on failure so the caller can send a 401 via self._send_error(). - Call site at _StreamableHttpAuthHandler.authenticate awaits the helper and sends 401 on rejection. DISPATCH-KEY FIX (S1/S2): - Proxy payload now includes token_use='session' so downstream dispatchers (main.py:2870, streamablehttp_transport.py:1998) route through resolve_session_teams() - the canonical 'single policy point' per AGENTS.md - rather than treating the proxy payload as an API-token payload with embedded teams. DOCS + TEST HYGIENE (S3/S4a): - require_auth docstring updated to describe the DB-enriched proxy path, bootstrap flow, and request.state caching. - Misleading test test_jwt_auth_with_proxy_enabled renamed to test_mcp_client_auth_mode_skips_proxy_path with accurate docstring and comment explaining why 'anonymous' is the expected outcome. - Stale comment 'Proxy auth path - identical to require_auth' updated to accurately describe the shared-helper relationship. DENY-PATH REGRESSION TESTS (AGENTS.md security invariant): - test_proxy_auth_disabled_user_raises_401: non-admin disabled user -> 401 - test_proxy_auth_disabled_admin_raises_401: disabled admin -> 401 (no elevation via is_admin=True on a deactivated record) - test_proxy_auth_payload_has_token_use_session: dispatch-key contract - 4 pre-existing tests updated to supply DB mocks instead of relying on the old unvalidated proxy shape. Signed-off-by: Jonathan Springer <jps@s390x.com>
1 parent 9ab37cc commit 43a217b

4 files changed

Lines changed: 262 additions & 35 deletions

File tree

mcpgateway/transports/streamablehttp_transport.py

Lines changed: 68 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -4527,23 +4527,71 @@ def _set_user_identity_from_dict(ctx: dict[str, Any]) -> None:
45274527
)
45284528

45294529

4530-
def _set_proxy_user_context(proxy_user: str) -> None:
4531-
"""Set user context for a proxy-authenticated request (no team context, non-admin).
4530+
async def _set_proxy_user_context(proxy_user: str) -> dict[str, Any] | None:
4531+
"""Authenticate a proxy-identified user and set per-request transport context.
4532+
4533+
Performs a DB lookup via EmailAuthService, resolves team/admin state via
4534+
:func:`mcpgateway.auth._resolve_teams_from_db`, enforces ``is_active``, and
4535+
handles the platform-admin bootstrap (``REQUIRE_USER_IN_DB=False`` + email
4536+
matches ``settings.platform_admin_email``). On success, sets
4537+
``user_context_var``, user identity, and trace context. Mirrors the REST
4538+
``_authenticate_proxy_user`` helper in ``verify_credentials.py`` so that
4539+
trusted-proxy MCP clients receive the same DB-backed team/admin resolution
4540+
as REST admin/API callers (fixes #4262 on the primary MCP transport path).
45324541
45334542
Args:
4534-
proxy_user: Email address of the proxy-authenticated user.
4543+
proxy_user: Email address supplied by the trusted upstream proxy via
4544+
``settings.proxy_user_header``.
4545+
4546+
Returns:
4547+
``None`` on success. On failure, returns a dict with ``detail`` (str)
4548+
and optional ``headers`` (dict) suitable for passing to
4549+
``_StreamableHttpAuthHandler._send_error`` to produce a 401 response.
45354550
"""
4536-
_proxy_ctx: dict[str, Any] = {
4537-
"email": proxy_user,
4538-
"teams": [],
4539-
"is_authenticated": True,
4540-
"is_admin": False,
4541-
"permission_is_admin": False,
4542-
"auth_method": "proxy",
4543-
}
4544-
user_context_var.set(_proxy_ctx)
4545-
_set_user_identity_from_dict(_proxy_ctx)
4546-
set_trace_context_from_teams([], user_email=proxy_user, is_admin=False, auth_method="proxy")
4551+
# First-Party
4552+
from mcpgateway.auth import _resolve_teams_from_db # pylint: disable=import-outside-toplevel
4553+
from mcpgateway.db import get_db # pylint: disable=import-outside-toplevel
4554+
from mcpgateway.services.email_auth_service import EmailAuthService # pylint: disable=import-outside-toplevel
4555+
4556+
db = next(get_db())
4557+
try:
4558+
auth_service = EmailAuthService(db)
4559+
user_info = await auth_service.get_user_by_email(proxy_user)
4560+
4561+
if user_info:
4562+
# Enforce account-active check (matches JWT path in _enforce_revocation_and_active_user).
4563+
# A disabled user - including a disabled admin - must not be able to authenticate via
4564+
# trusted-proxy mode and inherit their pre-disable authorizations.
4565+
if not user_info.is_active:
4566+
return {"detail": "Account disabled", "headers": {"WWW-Authenticate": "Bearer"}}
4567+
4568+
token_teams = await _resolve_teams_from_db(proxy_user, user_info)
4569+
is_admin = user_info.is_admin
4570+
else:
4571+
platform_admin_email = getattr(settings, "platform_admin_email", "admin@example.com")
4572+
if not settings.require_user_in_db and proxy_user == platform_admin_email:
4573+
token_teams = None # Admin bypass
4574+
is_admin = True
4575+
else:
4576+
return {"detail": "User not found in database", "headers": {"WWW-Authenticate": "Bearer"}}
4577+
4578+
_proxy_ctx: dict[str, Any] = {
4579+
"email": proxy_user,
4580+
"teams": token_teams, # None for admin bypass, [] for public-only, or list of team IDs
4581+
"is_authenticated": True,
4582+
"is_admin": is_admin,
4583+
"permission_is_admin": is_admin,
4584+
"auth_method": "proxy",
4585+
"token_use": "session", # nosec B105 - Not a password; JWT claim type. DB-backed team resolution.
4586+
}
4587+
user_context_var.set(_proxy_ctx)
4588+
_set_user_identity_from_dict(_proxy_ctx)
4589+
# For trace context, admin bypass (teams=None) is represented as [] to match the existing
4590+
# pre-authentication contract of set_trace_context_from_teams.
4591+
set_trace_context_from_teams(token_teams or [], user_email=proxy_user, is_admin=is_admin, auth_method="proxy")
4592+
return None
4593+
finally:
4594+
db.close()
45474595

45484596

45494597
def get_streamable_http_auth_context() -> dict[str, Any]:
@@ -4673,8 +4721,12 @@ async def authenticate(self) -> bool:
46734721

46744722
# Determine authentication strategy based on settings
46754723
if proxy_trusted and proxy_user:
4676-
_set_proxy_user_context(proxy_user)
4677-
return True # Trusted proxy supplied user
4724+
# DB-backed authentication of the proxy-supplied identity; returns None on success
4725+
# or {"detail": ..., "headers": ...} on failure (unknown user, disabled user).
4726+
proxy_error = await _set_proxy_user_context(proxy_user)
4727+
if proxy_error:
4728+
return await self._send_error(**proxy_error)
4729+
return True # Trusted proxy supplied valid, active user
46784730

46794731
# --- Standard JWT authentication flow (client auth enabled) ---
46804732
token: str | None = None

mcpgateway/utils/verify_credentials.py

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -442,6 +442,16 @@ async def _authenticate_proxy_user(request: Request, proxy_user: str) -> dict:
442442
user_info = await auth_service.get_user_by_email(proxy_user)
443443

444444
if user_info:
445+
# Enforce account-active check (matches the JWT path in _enforce_revocation_and_active_user:398-399).
446+
# Without this, a disabled user - including a disabled admin - could authenticate via trusted-proxy
447+
# mode and inherit their pre-disable authorizations.
448+
if not user_info.is_active:
449+
raise HTTPException(
450+
status_code=status.HTTP_401_UNAUTHORIZED,
451+
detail="Account disabled",
452+
headers={"WWW-Authenticate": "Bearer"},
453+
)
454+
445455
# Resolve teams from DB (returns None for admin bypass, [] for no teams, or list of team IDs)
446456
token_teams = await _resolve_teams_from_db(proxy_user, user_info)
447457
payload = {
@@ -451,6 +461,10 @@ async def _authenticate_proxy_user(request: Request, proxy_user: str) -> dict:
451461
"is_admin": user_info.is_admin,
452462
"teams": token_teams, # None for admin bypass, [] for public-only, or list of team IDs
453463
"email": proxy_user,
464+
# token_use: "session" signals DB-backed team resolution to downstream dispatchers
465+
# (main.py:2870, streamablehttp_transport.py:1998) so they route via resolve_session_teams
466+
# rather than treating the proxy payload as an API-token payload with embedded teams.
467+
"token_use": "session", # nosec B105 - Not a password; JWT claim type
454468
}
455469
else:
456470
# User not in DB - handle based on REQUIRE_USER_IN_DB setting
@@ -464,6 +478,7 @@ async def _authenticate_proxy_user(request: Request, proxy_user: str) -> dict:
464478
"is_admin": True,
465479
"teams": None, # Admin bypass
466480
"email": proxy_user,
481+
"token_use": "session", # nosec B105 - Not a password; JWT claim type
467482
}
468483
else:
469484
raise HTTPException(
@@ -483,10 +498,22 @@ async def require_auth(request: Request, credentials: Optional[HTTPAuthorization
483498
"""Require authentication via JWT token or proxy headers.
484499
485500
FastAPI dependency that checks for authentication via:
486-
1. Proxy headers (if mcp_client_auth_enabled=false and trust_proxy_auth=true)
501+
1. Proxy headers (if mcp_client_auth_enabled=false and is_proxy_auth_trust_active())
487502
2. JWT token in Authorization header (Bearer scheme)
488503
3. JWT token in cookies
489504
505+
Proxy authentication path (see :func:`_authenticate_proxy_user`):
506+
When configured, the proxy-supplied user identity is looked up in
507+
the DB via ``EmailAuthService`` and their team/admin context is
508+
resolved. The resulting enriched payload
509+
(``sub``, ``source``, ``token``, ``is_admin``, ``teams``, ``email``)
510+
is cached on ``request.state._jwt_verified_payload`` so downstream
511+
middleware and handlers get the same shape used for JWT-authenticated
512+
requests. When ``REQUIRE_USER_IN_DB=False`` and the proxy user matches
513+
``settings.platform_admin_email``, a platform-admin bootstrap payload
514+
(``is_admin=True``, ``teams=None``) is returned without requiring a
515+
DB record; otherwise an unknown proxy user raises 401.
516+
490517
If authentication is required but no token is provided, raises an HTTP 401 error.
491518
492519
Args:
@@ -496,11 +523,13 @@ async def require_auth(request: Request, credentials: Optional[HTTPAuthorization
496523
497524
Returns:
498525
str | dict: The verified credentials payload if authenticated,
499-
proxy user if proxy auth enabled, or "anonymous" if authentication is not required.
526+
the enriched proxy payload if proxy auth succeeded, or
527+
``"anonymous"`` if authentication is not required.
500528
501529
Raises:
502530
HTTPException: 401 status if authentication is required but no valid
503-
token is provided.
531+
token is provided, or if a proxy-identified user is unknown and
532+
the platform-admin bootstrap conditions do not apply.
504533
505534
Examples:
506535
>>> from mcpgateway.utils import verify_credentials as vc

tests/unit/mcpgateway/transports/test_streamablehttp_transport.py

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
import json
2828
from types import SimpleNamespace
2929
from typing import List
30-
from unittest.mock import AsyncMock, MagicMock, patch
30+
from unittest.mock import AsyncMock, MagicMock, Mock, patch
3131

3232
# Third-Party
3333
from fastapi import HTTPException
@@ -4871,7 +4871,7 @@ async def failing_get_db():
48714871

48724872
@pytest.mark.asyncio
48734873
async def test_streamable_http_auth_proxy_user_when_client_auth_disabled(monkeypatch):
4874-
"""Test auth sets user context for proxy user when client auth disabled (lines 1740-1750)."""
4874+
"""Proxy user with valid DB record authenticates and gets DB-backed team/admin context."""
48754875
monkeypatch.setattr("mcpgateway.transports.streamablehttp_transport.settings.mcp_client_auth_enabled", False)
48764876
monkeypatch.setattr("mcpgateway.transports.streamablehttp_transport.settings.trust_proxy_auth", True)
48774877
monkeypatch.setattr("mcpgateway.transports.streamablehttp_transport.settings.trust_proxy_auth_dangerously", True)
@@ -4888,7 +4888,22 @@ async def test_streamable_http_auth_proxy_user_when_client_auth_disabled(monkeyp
48884888
async def send(msg):
48894889
sent.append(msg)
48904890

4891-
result = await streamable_http_auth(scope, None, send)
4891+
mock_user = Mock()
4892+
mock_user.is_admin = False
4893+
mock_user.is_active = True
4894+
mock_user.email = "proxy_user@example.com"
4895+
4896+
with patch("mcpgateway.db.get_db") as mock_get_db, patch(
4897+
"mcpgateway.services.email_auth_service.EmailAuthService"
4898+
) as mock_auth_service, patch(
4899+
"mcpgateway.auth._resolve_teams_from_db", new_callable=AsyncMock
4900+
) as mock_resolve_teams:
4901+
mock_get_db.return_value = iter([Mock()])
4902+
mock_auth_service.return_value.get_user_by_email = AsyncMock(return_value=mock_user)
4903+
mock_resolve_teams.return_value = []
4904+
4905+
result = await streamable_http_auth(scope, None, send)
4906+
48924907
assert result is True
48934908
assert sent == [] # No 401 sent
48944909

@@ -4924,7 +4939,22 @@ async def test_streamable_http_auth_proxy_user_with_bearer_header(monkeypatch):
49244939
async def send(msg):
49254940
sent.append(msg)
49264941

4927-
result = await streamable_http_auth(scope, None, send)
4942+
mock_user = Mock()
4943+
mock_user.is_admin = False
4944+
mock_user.is_active = True
4945+
mock_user.email = "proxy_fallback@example.com"
4946+
4947+
with patch("mcpgateway.db.get_db") as mock_get_db, patch(
4948+
"mcpgateway.services.email_auth_service.EmailAuthService"
4949+
) as mock_auth_service, patch(
4950+
"mcpgateway.auth._resolve_teams_from_db", new_callable=AsyncMock
4951+
) as mock_resolve_teams:
4952+
mock_get_db.return_value = iter([Mock()])
4953+
mock_auth_service.return_value.get_user_by_email = AsyncMock(return_value=mock_user)
4954+
mock_resolve_teams.return_value = []
4955+
4956+
result = await streamable_http_auth(scope, None, send)
4957+
49284958
assert result is True
49294959
assert sent == []
49304960

@@ -4960,7 +4990,22 @@ async def fake_verify(token):
49604990
async def send(msg):
49614991
sent.append(msg)
49624992

4963-
result = await streamable_http_auth(scope, None, send)
4993+
mock_user = Mock()
4994+
mock_user.is_admin = False
4995+
mock_user.is_active = True
4996+
mock_user.email = "proxy_user@example.com"
4997+
4998+
with patch("mcpgateway.db.get_db") as mock_get_db, patch(
4999+
"mcpgateway.services.email_auth_service.EmailAuthService"
5000+
) as mock_auth_service, patch(
5001+
"mcpgateway.auth._resolve_teams_from_db", new_callable=AsyncMock
5002+
) as mock_resolve_teams:
5003+
mock_get_db.return_value = iter([Mock()])
5004+
mock_auth_service.return_value.get_user_by_email = AsyncMock(return_value=mock_user)
5005+
mock_resolve_teams.return_value = []
5006+
5007+
result = await streamable_http_auth(scope, None, send)
5008+
49645009
assert result is True
49655010

49665011
user_ctx = tr.user_context_var.get()

0 commit comments

Comments
 (0)