Resolve user ID from configurable HTTP header#1620
Resolve user ID from configurable HTTP header#1620ericevans-nv wants to merge 1 commit intoNVIDIA:developfrom
Conversation
WalkthroughAdds optional header-based authentication to the MCP client: a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MCP as MCPFunctionGroup
participant Config as MCPClientConfig
participant Context as Context
participant JWT as JWT Decoder
participant Session as Session Router
Client->>MCP: _response_fn(request)
MCP->>Config: is auth_token_header set?
alt Header-based auth enabled
MCP->>Context: read HTTP headers
Context-->>MCP: headers
MCP->>JWT: decode/extract token from header
JWT-->>MCP: return user_id (claims.sub)
MCP->>Session: route by user_id
else Header-based auth disabled
MCP->>MCP: _get_session_id_from_context()
MCP->>Session: route by session_id
end
Session-->>MCP: session selected
MCP-->>Client: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ericevans-nv
changed the title
Signed-off-by: Eric Evans <194135482+ericevans-nv@users.noreply.github.com>
ericevans-nv
force-pushed
the
feat/mcp-client-header-auth-support
branch
from
738c3c8 to
f52d269
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py (1)
256-286: Add exception handling for consistency with_get_session_id_from_context.
_get_session_id_from_contextwraps its entire body intry/except: return None, gracefully degrading to the "temporarily unavailable" path on any unexpected error (e.g. no active context)._resolve_user_id_from_auth_headerhas no such guard: if_Ctx.get()raises, the exception propagates through_response_fn's outerexcept Exception as eandstr(e)is returned to the caller — potentially exposing internal error details and diverging from the cookie path's behavior.🔧 Proposed fix
def _resolve_user_id_from_auth_header(self) -> str | None: ... if not self._client_config or not self._client_config.auth_token_header: return None + try: from nat.builder.context import Context as _Ctx from nat.runtime.connection_auth import resolve_user_id headers = getattr(_Ctx.get().metadata, "headers", None) if headers is None: return None raw_value: str | None = headers.get(self._client_config.auth_token_header) if not raw_value: return None user_id: str | None = resolve_user_id(raw_value, {}) if not user_id: logger.warning("Could not resolve user ID from header '%s'", self._client_config.auth_token_header) return None logger.debug("Resolved user_id '%s' from header '%s'", truncate_session_id(user_id), self._client_config.auth_token_header) return user_id + except Exception: + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py` around lines 256 - 286, _wrap the body of _resolve_user_id_from_auth_header in a try/except that catches Exception and returns None (mirroring _get_session_id_from_context) so any unexpected errors (e.g. _Ctx.get() raising) do not propagate; locate the method _resolve_user_id_from_auth_header and enclose the existing logic that references _Ctx.get(), headers, and resolve_user_id(...) in this guard, ensuring on exception you return None to maintain the same graceful "temporarily unavailable" behavior as _get_session_id_from_context and avoid leaking internal errors via _response_fn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py`:
- Line 187: Add an inline Ruff S106 suppression on the MCPClientConfig
invocation that sets auth_token_header to "Authorization" in the test (the line
calling MCPClientConfig(server=server_cfg, auth_token_header="Authorization")).
Specifically, update the test line where MCPClientConfig is constructed to
include a local noqa/ruff: noqa-style suppression for S106 so Ruff recognizes
this as a false positive for the auth_token_header parameter; keep the
suppression inline next to the auth_token_header usage to limit scope to this
expression.
- Line 21: The test imports PyJWT via "import jwt" (used in
test_resolve_user_id_from_auth_header) but relies on it transitively; add an
explicit test dependency on pyjwt (matching the required version, e.g., ~=2.11)
to packages/nvidia_nat_mcp/pyproject.toml under the
[tool.poetry.dev-dependencies] or test requirements so the test no longer
depends on a transitive package and remains stable if nvidia-nat-core removes
PyJWT.
---
Nitpick comments:
In `@packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py`:
- Around line 256-286: _wrap the body of _resolve_user_id_from_auth_header in a
try/except that catches Exception and returns None (mirroring
_get_session_id_from_context) so any unexpected errors (e.g. _Ctx.get() raising)
do not propagate; locate the method _resolve_user_id_from_auth_header and
enclose the existing logic that references _Ctx.get(), headers, and
resolve_user_id(...) in this guard, ensuring on exception you return None to
maintain the same graceful "temporarily unavailable" behavior as
_get_session_id_from_context and avoid leaking internal errors via _response_fn.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py (1)
178-191: Missing failure-path coverage for_resolve_user_id_from_auth_header.The test covers the happy path but leaves the following paths untested:
- Header configured but absent from the request (should return
Nonegracefully).- JWT is malformed / not decodable.
_Ctx.get()raises an exception (exercises the missing guard flagged inclient_impl.py).- Empty claims dict returned by
resolve_user_id(e.g., noname/subclaim present).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py` around lines 178 - 191, Add negative-path tests for MCPFunctionGroup._resolve_user_id_from_auth_header: write test cases that (1) configure auth_token_header but provide no such header in ctx.metadata.headers and assert it returns None, (2) supply a malformed/undecodable JWT in the Authorization header and assert it returns None, (3) patch nat.builder.context.Context.get (mock_context_cls.get) to raise an exception and assert the method handles it and returns None, and (4) supply a valid JWT whose claims are empty or missing "name"/"sub" and assert the result is None; use the same test harness pattern as the existing test (patch Context, set mock_context_cls.get.return_value = ctx or side_effect to raise, construct MCPClientConfig/MCPFunctionGroup) and keep assertions explicit for each failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py`:
- Around line 256-280: The method _resolve_user_id_from_auth_header should
mirror _get_session_id_from_context by guarding the entire body in a try/except
Exception that returns None on error; wrap calls to _Ctx.get() and
resolve_user_id(...) in that try block so any exception (e.g., Context.get()
when no request context) is swallowed and the function returns None instead of
letting the exception propagate to _response_fn and leak to callers; keep
existing logging for unresolved user_id but do not log exceptions here—just
return None.
- Around line 268-272: The code is currently decoding JWTs without signature
verification via resolve_user_id(user_auth_header, {}), which allows forged
headers to impersonate users; update the MCPClientConfig to include verification
material (a JWKS URL or public key / verification options) and change
client_impl.py to pass those verification options into resolve_user_id (or call
a verified decode function such as decode_jwt_payload with the provided
key/JWKS) instead of an empty dict; alternatively, if you truly rely on a
trusted gateway, add an explicit comment and config flag (e.g.,
require_trusted_gateway=True) documenting the trust boundary and fail fast when
no verification material is supplied to auth_token_header handling.
- Line 272: The current session routing uses resolve_user_id (used to set
user_id in client_impl.py) which prefers the "name" JWT claim and can cause
collisions; update resolve_user_id to prefer the standard unique "sub" claim
first and only fall back to "name", "email", or "preferred_username" to ensure
per-user MCP client sessions are uniquely routed, and update any
docstring/comments for resolve_user_id and the user_id usage in client_impl.py
to reflect this change.
---
Duplicate comments:
In `@packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py`:
- Line 187: The test sets auth_token_header as a literal in the MCPClientConfig
instantiation (client_cfg = MCPClientConfig(server=server_cfg,
auth_token_header="Authorization")), which Ruff flags as S106; suppress this
false positive by appending a noqa S106 comment to that expression (add "# noqa:
S106" to the same line where auth_token_header is passed) so the linter ignores
the hardcoded-string check for this test helper.
- Line 21: The file contains a duplicated explicit import of the PyJWT package
("import jwt")—remove the redundant import from
tests/client/test_mcp_client_impl.py (the lone "import jwt" statement) so the
test reuses the existing jwt import/fixture or dependency declared elsewhere;
ensure no other references break and that any needed jwt usage relies on the
shared import or test utilities instead of adding a new explicit dependency.
---
Nitpick comments:
In `@packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py`:
- Around line 178-191: Add negative-path tests for
MCPFunctionGroup._resolve_user_id_from_auth_header: write test cases that (1)
configure auth_token_header but provide no such header in ctx.metadata.headers
and assert it returns None, (2) supply a malformed/undecodable JWT in the
Authorization header and assert it returns None, (3) patch
nat.builder.context.Context.get (mock_context_cls.get) to raise an exception and
assert the method handles it and returns None, and (4) supply a valid JWT whose
claims are empty or missing "name"/"sub" and assert the result is None; use the
same test harness pattern as the existing test (patch Context, set
mock_context_cls.get.return_value = ctx or side_effect to raise, construct
MCPClientConfig/MCPFunctionGroup) and keep assertions explicit for each failure
path.
| def _resolve_user_id_from_auth_header(self) -> str | None: | ||
| """Extract a JWT from the configured HTTP header and resolve the user ID from its claims.""" | ||
| if not self._client_config or not self._client_config.auth_token_header: | ||
| return None | ||
|
|
||
| from nat.builder.context import Context as _Ctx | ||
| from nat.runtime.connection_auth import resolve_user_id | ||
|
|
||
| headers = getattr(_Ctx.get().metadata, "headers", None) | ||
| if headers is None: | ||
| return None | ||
|
|
||
| user_auth_header: str | None = headers.get(self._client_config.auth_token_header) | ||
| if not user_auth_header: | ||
| return None | ||
|
|
||
| user_id: str | None = resolve_user_id(user_auth_header, {}) | ||
| if not user_id: | ||
| logger.warning("Could not resolve user ID from header '%s'", self._client_config.auth_token_header) | ||
| return None | ||
|
|
||
| logger.debug("Resolved user_id '%s' from header '%s'", | ||
| truncate_session_id(user_id), | ||
| self._client_config.auth_token_header) | ||
| return user_id |
There was a problem hiding this comment.
_resolve_user_id_from_auth_header missing exception guard — inconsistent with sibling method.
_get_session_id_from_context wraps its entire body in try/except Exception: return None to silently degrade when the context is unavailable. _resolve_user_id_from_auth_header has no such guard, so if _Ctx.get() throws (e.g., when called outside a request context), the exception escapes to _response_fn's outer except Exception as e: handler, returning the raw exception string to the caller and logging it at WARNING level instead of silently returning None.
🔧 Proposed fix
def _resolve_user_id_from_auth_header(self) -> str | None:
"""Extract a JWT from the configured HTTP header and resolve the user ID from its claims."""
if not self._client_config or not self._client_config.auth_token_header:
return None
+ try:
from nat.builder.context import Context as _Ctx
from nat.runtime.connection_auth import resolve_user_id
headers = getattr(_Ctx.get().metadata, "headers", None)
if headers is None:
return None
user_auth_header: str | None = headers.get(self._client_config.auth_token_header)
if not user_auth_header:
return None
user_id: str | None = resolve_user_id(user_auth_header, {})
if not user_id:
logger.warning("Could not resolve user ID from header '%s'", self._client_config.auth_token_header)
return None
logger.debug("Resolved user_id '%s' from header '%s'",
truncate_session_id(user_id),
self._client_config.auth_token_header)
return user_id
+ except Exception:
+ logger.debug("Failed to resolve user ID from auth header", exc_info=True)
+ return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py` around
lines 256 - 280, The method _resolve_user_id_from_auth_header should mirror
_get_session_id_from_context by guarding the entire body in a try/except
Exception that returns None on error; wrap calls to _Ctx.get() and
resolve_user_id(...) in that try block so any exception (e.g., Context.get()
when no request context) is swallowed and the function returns None instead of
letting the exception propagate to _response_fn and leak to callers; keep
existing logging for unresolved user_id but do not log exceptions here—just
return None.
| if not user_auth_header: | ||
| return None | ||
|
|
||
| user_id: str | None = resolve_user_id(user_auth_header, {}) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the resolve_user_id function and examine its implementation
rg -n "def resolve_user_id" --type py -A 20Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 2161
🏁 Script executed:
#!/bin/bash
# Find the test file and examine the test at line 191
fd -e py "test" --type f | xargs rg -l "_resolve_user_id_from_auth_header\|resolve_user_id" | head -5Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Find test files related to client_impl
find . -name "*test*.py" -type f | xargs rg -l "client_impl\|resolve_user_id" 2>/dev/null | head -10Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Read the client_impl.py file to see the test and context
cat -n packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py | sed -n '180,200p'Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 1256
🏁 Script executed:
#!/bin/bash
# Search for test files related to connection_auth
find . -name "*test*.py" -o -name "*_test.py" | head -20Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 1532
🏁 Script executed:
#!/bin/bash
# Search for "Alice" in the codebase to find the test mentioned
rg "Alice" --type py -B 3 -A 3Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 19203
🏁 Script executed:
#!/bin/bash
# Examine the test file to understand the implementation
cat -n packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py | head -100Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 4588
🏁 Script executed:
#!/bin/bash
# Find the test_resolve_user_id_from_auth_header test
rg -n "test_resolve_user_id_from_auth_header" packages/nvidia_nat_mcp/tests/client/test_mcp_client_impl.py -A 15Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 878
🏁 Script executed:
#!/bin/bash
# Find _resolve_user_id_from_auth_header in client_impl.py
rg -n "_resolve_user_id_from_auth_header\|resolve_user_id" packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py -B 2 -A 10Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Read the client_impl.py file around line 272
cat -n packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py | sed -n '265,280p'Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 807
🏁 Script executed:
#!/bin/bash
# Find the method that calls _resolve_user_id_from_auth_header
rg -n "_resolve_user_id_from_auth_header" packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py -B 30 | head -50Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 2685
🏁 Script executed:
#!/bin/bash
# Find where _resolve_user_id_from_auth_header is called
rg -n "_resolve_user_id_from_auth_header()" packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py -B 2 -A 2Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 708
🏁 Script executed:
#!/bin/bash
# Read more context around the session routing
cat -n packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py | sed -n '540,570p'Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 2085
🏁 Script executed:
#!/bin/bash
# Verify the complete priority order in resolve_user_id
cat -n packages/nvidia_nat_core/src/nat/runtime/connection_auth.py | sed -n '74,93p'Repository: NVIDIA/NeMo-Agent-Toolkit
Length of output: 1041
Confirm session routing uses unique JWT claim for data isolation.
The code uses resolve_user_id, which prioritizes the "name" claim over "sub" when routing to per-user MCP client sessions (line 546). This creates a collision risk: two users with the same display name will be routed to the same session, violating data isolation.
While the function falls back through "email" and "preferred_username" before "sub", the primary reliance on "name" is not documented. Either confirm this is intentional and document why it is safe in your deployment context, or switch the priority to prefer "sub" (the JWT standard unique subject identifier) with "name" as a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nvidia_nat_mcp/src/nat/plugins/mcp/client/client_impl.py` at line
272, The current session routing uses resolve_user_id (used to set user_id in
client_impl.py) which prefers the "name" JWT claim and can cause collisions;
update resolve_user_id to prefer the standard unique "sub" claim first and only
fall back to "name", "email", or "preferred_username" to ensure per-user MCP
client sessions are uniquely routed, and update any docstring/comments for
resolve_user_id and the user_id usage in client_impl.py to reflect this change.
willkill07
left a comment
willkill07
left a comment
There was a problem hiding this comment.
Documentation should be added/updated with a clear security warning. This is an attack vector that has to be well documented.
| def _resolve_user_id_from_auth_header(self) -> str | None: | ||
| """Extract a JWT from the configured HTTP header and resolve the user ID from its claims.""" | ||
| if not self._client_config or not self._client_config.auth_token_header: | ||
| return None | ||
|
|
||
| from nat.builder.context import Context as _Ctx | ||
| from nat.runtime.connection_auth import resolve_user_id | ||
|
|
||
| headers = getattr(_Ctx.get().metadata, "headers", None) | ||
| if headers is None: | ||
| return None | ||
|
|
||
| user_auth_header: str | None = headers.get(self._client_config.auth_token_header) | ||
| if not user_auth_header: | ||
| return None | ||
|
|
||
| user_id: str | None = resolve_user_id(user_auth_header, {}) | ||
| if not user_id: | ||
| logger.warning("Could not resolve user ID from header '%s'", self._client_config.auth_token_header) | ||
| return None | ||
|
|
||
| logger.debug("Resolved user_id '%s' from header '%s'", | ||
| truncate_session_id(user_id), | ||
| self._client_config.auth_token_header) | ||
| return user_id |
There was a problem hiding this comment.
| def _resolve_user_id_from_auth_header(self) -> str | None: | |
| """Extract a JWT from the configured HTTP header and resolve the user ID from its claims.""" | |
| if not self._client_config or not self._client_config.auth_token_header: | |
| return None | |
| from nat.builder.context import Context as _Ctx | |
| from nat.runtime.connection_auth import resolve_user_id | |
| headers = getattr(_Ctx.get().metadata, "headers", None) | |
| if headers is None: | |
| return None | |
| user_auth_header: str | None = headers.get(self._client_config.auth_token_header) | |
| if not user_auth_header: | |
| return None | |
| user_id: str | None = resolve_user_id(user_auth_header, {}) | |
| if not user_id: | |
| logger.warning("Could not resolve user ID from header '%s'", self._client_config.auth_token_header) | |
| return None | |
| logger.debug("Resolved user_id '%s' from header '%s'", | |
| truncate_session_id(user_id), | |
| self._client_config.auth_token_header) | |
| return user_id | |
| def _resolve_user_id_from_auth_header(self) -> str | None: | |
| """Extract a JWT from the configured HTTP header and resolve the user ID from its claims.""" | |
| if not self._client_config or not self._client_config.auth_token_header: | |
| return None | |
| try: | |
| from nat.builder.context import Context as _Ctx | |
| from nat.runtime.connection_auth import resolve_user_id | |
| headers = getattr(_Ctx.get().metadata, "headers", None) | |
| if headers is None: | |
| return None | |
| user_auth_header: str | None = headers.get(self._client_config.auth_token_header) | |
| if not user_auth_header: | |
| return None | |
| user_id: str | None = resolve_user_id(user_auth_header, {}) | |
| if not user_id: | |
| logger.warning("Could not resolve user ID from header '%s'", self._client_config.auth_token_header) | |
| return None | |
| logger.debug("Resolved user_id '%s' from header '%s'", | |
| truncate_session_id(user_id), | |
| self._client_config.auth_token_header) | |
| return user_id | |
| except Exception: | |
| logger.debug("Failed to resolve user ID from auth header", exc_info=True) | |
| return None |
There was a problem hiding this comment.
I suggest, wrapping this block in try/except Exception: return None so we match _get_session_id_from_context and don’t leak exceptions (and internal errors) to the caller via _response_fn.
Optional: log at debug with exc_info=True in the except.
|
|
||
| assert group._resolve_user_id_from_auth_header() == "Alice" | ||
|
|
||
|
|
There was a problem hiding this comment.
| @patch("nat.builder.context.Context") | |
| def test_resolve_user_id_from_auth_header_missing_header_returns_none(mock_context_cls): | |
| """When auth_token_header is set but the header is absent, returns None.""" | |
| ctx = MagicMock() | |
| ctx.metadata.headers = {} | |
| mock_context_cls.get.return_value = ctx | |
| server_cfg = MCPServerConfig(transport="streamable-http", url="http://localhost:9901/mcp") | |
| client_cfg = MCPClientConfig(server=server_cfg, auth_token_header="Authorization") | |
| group = MCPFunctionGroup(config=client_cfg) | |
| group._client_config = client_cfg | |
| assert group._resolve_user_id_from_auth_header() is None | |
| @patch("nat.builder.context.Context") | |
| def test_resolve_user_id_from_auth_header_context_raises_returns_none(mock_context_cls): | |
| """When Context.get() raises, returns None instead of propagating.""" | |
| mock_context_cls.get.side_effect = RuntimeError("No context") | |
| server_cfg = MCPServerConfig(transport="streamable-http", url="http://localhost:9901/mcp") | |
| client_cfg = MCPClientConfig(server=server_cfg, auth_token_header="Authorization") | |
| group = MCPFunctionGroup(config=client_cfg) | |
| group._client_config = client_cfg | |
| assert group._resolve_user_id_from_auth_header() is None | |
| @patch("nat.runtime.connection_auth.resolve_user_id") | |
| @patch("nat.builder.context.Context") | |
| def test_resolve_user_id_from_auth_header_malformed_jwt_returns_none(mock_context_cls, mock_resolve_user_id): | |
| """When the header value is invalid, returns None (e.g. decode error or no user).""" | |
| mock_resolve_user_id.return_value = None | |
| ctx = MagicMock() | |
| ctx.metadata.headers = {"Authorization": "Bearer not-a-valid-jwt"} | |
| mock_context_cls.get.return_value = ctx | |
| server_cfg = MCPServerConfig(transport="streamable-http", url="http://localhost:9901/mcp") | |
| client_cfg = MCPClientConfig(server=server_cfg, auth_token_header="Authorization") | |
| group = MCPFunctionGroup(config=client_cfg) | |
| group._client_config = client_cfg | |
| assert group._resolve_user_id_from_auth_header() is None |
There was a problem hiding this comment.
I suggest we add tests for: (1) header configured but missing → None, (2) Context.get() raises → None, (3) invalid token / resolve_user_id returns None or raises → None. Patch Context at nat.builder.context.Context; for (3) patch resolve_user_id at nat.runtime.connection_auth.resolve_user_id so the test is self-contained.
| auth_token_header: str | None = Field( | ||
| default=None, description="HTTP header name containing a token used for user identification.") |
There was a problem hiding this comment.
| auth_token_header: str | None = Field( | |
| default=None, description="HTTP header name containing a token used for user identification.") | |
| auth_token_header: str | None = Field( | |
| default=None, | |
| description="HTTP header name containing a token used for user identification. " | |
| "When set, the header value is used to resolve the session user ID (e.g. from JWT claims). " | |
| "Only set this when the header is set by a trusted upstream (e.g. gateway); no verification is performed here.") |
| """ | ||
| session_aware_tools: bool = Field(default=True, | ||
| description="Session-aware tools are created if True. Defaults to True.") | ||
| auth_token_header: str | None = Field( |
There was a problem hiding this comment.
@ericevans-nv Thanks for this change. I think we can simplify/unify user-id resolution by first using Context.get().user_id (already populated during request/session handling) and only falling back to header parsing when it’s missing.
There was a problem hiding this comment.
I missed that this PR touches only shared MCPFunctionGroup. per_user_mcp_client_function_group already uses Context.get().user_id, which is the right source of truth.
I’d suggest aligning shared MCP routing to the same pattern and removing auth_token_header from MCP config to avoid duplicate JWT parsing logic in the plugin.
There was a problem hiding this comment.
The auth_token_header config serves a different purpose: it allows deployers to specify a custom header name containing the JWT, since the UI does not send nat-session cookies over HTTP and the header name is only known to the deployer. Context.user_id is populated exclusively from the Authorization header or the nat-session cookie, so it won't have the correct value when a non-standard header is used. That said, I agree we should align the pattern, I'll update the PR so the custom header resolution works for both the shared MCPFunctionGroup and per_user_mcp_client_function_group.
|
Closing this PR. After discussion with the team, HTTP headers will always send JWTs in the standard Authorization header, which the session layer already handles. The custom header support isn't needed at this time. If a non-standard header requirement comes up in the future, we can revisit. |
4 participants
Description
Adds
auth_token_headerconfig field toMCPClientConfigthat specifies an HTTP header containing a JWT for user identification. When set, the user ID is extracted from JWT claims and used as the per-user session key, as an alternative to thenat-sessioncookie. Authentication with the MCP server still proceeds through the normal OAuth flow.This supports deployments where an upstream service sends the user's identity as a JWT in an HTTP header (e.g.,
Authorization) rather than a session cookie, such as HTTP streaming scenarios.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Tests