Copilot commented on code in PR #11622:
URL: https://github.com/apache/gravitino/pull/11622#discussion_r3401197399
##########
mcp-server/mcp_server/server.py:
##########
@@ -73,14 +119,17 @@ def _create_gravitino_mcp(setting: Setting) -> FastMCP:
def _parse_mcp_url(url: str) -> ():
Review Comment:
The return type annotation `-> ()` is not meaningful and defeats
type-checking; this function always returns a 3-tuple (host, port, path).
##########
mcp-server/mcp_server/core/context.py:
##########
@@ -19,11 +19,52 @@
from mcp_server.core.setting import Setting
+def _get_request_authorization() -> str:
+ """Return the raw ``Authorization`` header of the current HTTP request.
+
+ The header is forwarded to Gravitino verbatim so the auth scheme chosen by
+ the agent (``Basic`` for simple auth, ``Bearer`` for OAuth2, ``Negotiate``
+ for Kerberos) is preserved. Returns an empty string in stdio mode or when
+ the header is absent.
+ """
+ try:
+ from fastmcp.server.dependencies import get_http_request
+
+ return get_http_request().headers.get("authorization", "")
+ except Exception: # noqa: BLE001 – stdio mode or missing request context
+ return ""
Review Comment:
`_get_request_authorization()` currently swallows all exceptions and turns
them into "no auth". That can mask real runtime errors; only `LookupError` is
expected when there is no HTTP request context (stdio mode).
##########
mcp-server/mcp_server/server.py:
##########
@@ -21,18 +21,64 @@
from typing import AsyncIterator
from urllib.parse import urlparse
+import mcp.types as mt
from fastmcp import FastMCP
from fastmcp.server.middleware.error_handling import ErrorHandlingMiddleware
from fastmcp.server.middleware.logging import (
LoggingMiddleware,
)
+from fastmcp.server.middleware.middleware import (
+ CallNext,
+ Middleware,
+ MiddlewareContext,
+)
from fastmcp.server.middleware.timing import TimingMiddleware
+from fastmcp.tools.base import ToolResult
+from mcp_server.core import audit
from mcp_server.core.context import GravitinoContext
from mcp_server.core.setting import Setting
from mcp_server.tools import load_tools
+def _get_principal_from_request() -> str:
+ """Derive a display principal from the current HTTP request's
Authorization header.
+
+ Returns "anonymous" in stdio mode or when no token is present.
+ """
+ try:
+ from fastmcp.server.dependencies import get_http_request
+
+ authorization = get_http_request().headers.get("authorization", "")
+ return audit._extract_principal(authorization)
+ except Exception: # noqa: BLE001 – stdio mode or no request context
+ return "anonymous"
Review Comment:
Catching `Exception` here will silently hide unexpected bugs (e.g.,
attribute errors) and turn them into an "anonymous" principal. Since
stdio/no-request context is expected to raise `LookupError`, it’s safer to only
handle that case.
##########
mcp-server/mcp_server/core/context.py:
##########
@@ -19,11 +19,52 @@
from mcp_server.core.setting import Setting
+def _get_request_authorization() -> str:
+ """Return the raw ``Authorization`` header of the current HTTP request.
+
+ The header is forwarded to Gravitino verbatim so the auth scheme chosen by
+ the agent (``Basic`` for simple auth, ``Bearer`` for OAuth2, ``Negotiate``
+ for Kerberos) is preserved. Returns an empty string in stdio mode or when
+ the header is absent.
+ """
+ try:
+ from fastmcp.server.dependencies import get_http_request
+
+ return get_http_request().headers.get("authorization", "")
+ except Exception: # noqa: BLE001 – stdio mode or missing request context
+ return ""
+
+
class GravitinoContext:
def __init__(self, setting: Setting):
- self.gravitino_client = RESTClientFactory.create_rest_client(
- setting.metalake, setting.gravitino_uri
+ self._setting = setting
+ # Static startup identity: the --token CLI value is treated as a Bearer
+ # token (OAuth2). Used in stdio mode or as the fallback when an HTTP
+ # request carries no Authorization header.
+ default_authorization = (
+ f"Bearer {setting.token}" if setting.token else ""
+ )
+ self._default_client = RESTClientFactory.create_rest_client(
+ setting.metalake, setting.gravitino_uri, default_authorization
)
def rest_client(self):
- return self.gravitino_client
+ """Return a REST client carrying the correct identity for this request.
+
+ In HTTP transport mode the incoming request's ``Authorization`` header
is
+ forwarded verbatim to Gravitino, taking priority over the static
startup
+ token. This keeps concurrent sessions with different principals fully
+ isolated — one principal's identity never leaks into another's calls.
+
+ Falls back to the shared default client (static startup token) when:
+ - running in stdio mode (no HTTP request context), or
+ - the incoming request carries no Authorization header.
+ """
+ authorization = _get_request_authorization()
+ if authorization:
+ return RESTClientFactory.create_rest_client(
+ self._setting.metalake,
+ self._setting.gravitino_uri,
+ authorization,
+ )
+ return self._default_client
Review Comment:
In HTTP mode, `rest_client()` creates a brand-new REST client (and thus a
new `httpx.AsyncClient` connection pool) on every tool call whenever an
Authorization header is present. `PlainRESTClientOperation` does not expose any
close/cleanup hook, so this can leak sockets and grow resource usage under load.
##########
mcp-server/mcp_server/core/setting.py:
##########
@@ -33,3 +33,19 @@ class Setting:
tags: Set[str] = field(default_factory=set)
transport: str = DefaultSetting.default_transport
mcp_url: str = DefaultSetting.default_mcp_url
+ # Bearer token forwarded to Gravitino on every request.
+ # Empty string means anonymous (no Authorization header sent).
+ token: str = ""
Review Comment:
The comment on `Setting.token` says it is forwarded to Gravitino "on every
request", but with per-request identity forwarding it is only the
default/fallback when a request does not carry its own Authorization header
(and in stdio mode). Keeping this comment accurate avoids future misuse.
##########
mcp-server/mcp_server/main.py:
##########
@@ -83,17 +93,44 @@ def _parse_args():
parser.add_argument(
"--transport",
type=str,
- choices=["stdio", "http"],
+ choices=["stdio", "http", "streamable-http"],
default=DefaultSetting.default_transport,
- help=f"Transport protocol type: stdio (local), http (Streamable HTTP).
"
+ help="Transport protocol type: stdio (local), http / streamable-http "
+ "(networked Streamable HTTP; the two names are equivalent). "
f"(default: {DefaultSetting.default_transport})",
)
parser.add_argument(
"--mcp-url",
type=str,
default=DefaultSetting.default_mcp_url,
- help=f"The url of MCP server if using http transport. (default:
{DefaultSetting.default_mcp_url})",
+ help="The url of MCP server if using http transport, http:// or
https://. "
+ f"(default: {DefaultSetting.default_mcp_url})",
+ )
+
+ parser.add_argument(
+ "--token",
+ type=str,
+ default=os.environ.get("GRAVITINO_TOKEN", ""),
+ help="Bearer token forwarded as Authorization header to Gravitino on
every request. "
+ "Can also be set via the GRAVITINO_TOKEN environment variable. "
+ "When omitted, requests are sent without authentication.",
+ )
Review Comment:
The `--token` help text says the token is forwarded "on every request", but
in HTTP transport it is only used as a fallback when the incoming request has
no Authorization header (per-request identity takes priority). This is
user-facing CLI documentation and should reflect the actual behavior.
##########
mcp-server/tests/unit/tools/test_tag.py:
##########
@@ -99,14 +99,13 @@ async def _test_disassociate_tag_from_metadata(mcp_server):
asyncio.run(_test_disassociate_tag_from_metadata(self.mcp))
- def test_destructive_tag_tools_disabled_by_default(self):
- async def _test_destructive_tag_tools_disabled_by_default(mcp_server):
- tool_names = {tool.name for tool in await mcp_server.list_tools()}
+ def test_write_tag_tools_enabled_and_protected_by_authz(self):
+ """Write tools are exposed; authorization is enforced by Gravitino,
not by hiding."""
Review Comment:
This test/docstring asserts that authorization is enforced, but the test
only verifies tool exposure (presence in `list_tools`). Consider narrowing the
docstring (or extending the test) so it matches what is actually validated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
