aminghadersohi commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3350594350
##########
superset/mcp_service/mcp_config.py:
##########
@@ -303,57 +314,142 @@
}
+def get_mcp_api_key_enabled(app: Flask, *, startup_warning: bool = False) ->
bool:
+ """Return whether API key auth is enabled for the MCP transport.
+
+ Prefers ``MCP_API_KEY_ENABLED`` when explicitly set; falls back to
+ ``FAB_API_KEY_ENABLED``. When ``startup_warning=True`` and the value
+ is inherited from ``FAB_API_KEY_ENABLED``, logs a warning so operators
+ know a FAB config change now also affects the MCP transport.
+ """
+ if (mcp_setting := app.config.get("MCP_API_KEY_ENABLED", None)) is not
None:
+ return bool(mcp_setting)
+ fab_enabled = bool(app.config.get("FAB_API_KEY_ENABLED", False))
+ if startup_warning and fab_enabled:
+ logger.warning(
+ "MCP API key auth is enabled via FAB_API_KEY_ENABLED=True. "
+ "Set MCP_API_KEY_ENABLED=True to silence this warning or "
+ "MCP_API_KEY_ENABLED=False to disable API keys on the MCP "
+ "transport without affecting the FAB REST API."
+ )
+ return fab_enabled
+
+
def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
- """Default MCP auth factory using app.config values."""
- if not app.config.get("MCP_AUTH_ENABLED", False):
- return None
+ """Default MCP auth factory using app.config values.
- jwks_uri = app.config.get("MCP_JWKS_URI")
- public_key = app.config.get("MCP_JWT_PUBLIC_KEY")
- secret = app.config.get("MCP_JWT_SECRET")
+ Returns an auth provider when ``MCP_AUTH_ENABLED=True`` (JWT verifier,
+ optionally wrapped with ``CompositeTokenVerifier`` for API keys) or
+ when only ``MCP_API_KEY_ENABLED=True`` (or ``FAB_API_KEY_ENABLED=True``
+ as a fallback) — API-key-only verifier that rejects all non-API-key
+ Bearer tokens at the transport.
- if not (jwks_uri or public_key or secret):
- logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret
configured")
+ ``MCP_API_KEY_ENABLED=None`` (default) defers to ``FAB_API_KEY_ENABLED``
+ and logs a startup warning when that setting is True, so operators are
+ aware that a FAB config change now also affects the MCP transport.
+ """
+ auth_enabled = app.config.get("MCP_AUTH_ENABLED", False)
+ api_key_enabled = get_mcp_api_key_enabled(app, startup_warning=True)
+
+ if not (auth_enabled or api_key_enabled):
return None
- try:
- debug_errors = app.config.get("MCP_JWT_DEBUG_ERRORS", False)
+ jwt_verifier: Any | None = None
- common_kwargs: dict[str, Any] = {
- "issuer": app.config.get("MCP_JWT_ISSUER"),
- "audience": app.config.get("MCP_JWT_AUDIENCE"),
- "required_scopes": app.config.get("MCP_REQUIRED_SCOPES", []),
- }
+ if auth_enabled:
+ jwks_uri = app.config.get("MCP_JWKS_URI")
+ public_key = app.config.get("MCP_JWT_PUBLIC_KEY")
+ secret = app.config.get("MCP_JWT_SECRET")
- # For HS256 (symmetric), use the secret as the public_key parameter
- if app.config.get("MCP_JWT_ALGORITHM") == "HS256" and secret:
- common_kwargs["public_key"] = secret
- common_kwargs["algorithm"] = "HS256"
+ if not (jwks_uri or public_key or secret):
+ logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret
configured")
+ if not api_key_enabled:
+ return None
else:
- # For RS256 (asymmetric), use public key or JWKS
- common_kwargs["jwks_uri"] = jwks_uri
- common_kwargs["public_key"] = public_key
- common_kwargs["algorithm"] = app.config.get("MCP_JWT_ALGORITHM",
"RS256")
-
- if debug_errors:
- # DetailedJWTVerifier: detailed server-side logging of JWT
- # validation failures. HTTP responses are always generic per
- # RFC 6750 Section 3.1.
- from superset.mcp_service.jwt_verifier import DetailedJWTVerifier
-
- auth_provider = DetailedJWTVerifier(**common_kwargs)
- else:
- # MCPJWTVerifier: minimal logging + browser-friendly error page.
- from superset.mcp_service.jwt_verifier import MCPJWTVerifier
-
- auth_provider = MCPJWTVerifier(**common_kwargs)
+ try:
+ jwt_verifier = _build_jwt_verifier(
+ app=app,
+ jwks_uri=jwks_uri,
+ public_key=public_key,
+ secret=secret,
+ )
+ except (ValueError, JoseError):
+ # Do not log the exception — it may contain secrets (e.g., key
material)
+ logger.error("Failed to create MCP JWT verifier")
+ if not api_key_enabled:
+ return None
+
+ if api_key_enabled:
+ return _build_composite_verifier(app, jwt_verifier)
+
+ return jwt_verifier
+
+
+def _build_composite_verifier(app: Flask, jwt_verifier: Any) ->
CompositeTokenVerifier:
+ """Build a CompositeTokenVerifier with API key prefixes from config."""
+ if required_scopes := app.config.get("MCP_REQUIRED_SCOPES", []):
+ logger.debug(
+ "MCP_REQUIRED_SCOPES is configured but API key tokens bypass "
+ "scope enforcement. API key holders gain access regardless of "
+ "MCP_REQUIRED_SCOPES=%r.",
+ required_scopes,
+ )
+ raw_prefixes: str | Sequence[str] = app.config.get("FAB_API_KEY_PREFIXES",
["sst_"])
+ # Normalize: a plain string (e.g. "sst_") would iterate as characters;
+ # wrap it in a list so CompositeTokenVerifier receives a proper sequence.
+ # Guard against non-iterable config values (e.g. None, integers) that
+ # would raise TypeError and cause _create_auth_provider to fail open.
+ if isinstance(raw_prefixes, str):
+ api_key_prefixes: list[str] = [raw_prefixes]
+ else:
+ try:
+ api_key_prefixes = list(raw_prefixes)
+ except TypeError:
+ logger.warning(
+ "FAB_API_KEY_PREFIXES must be a string or list; using default"
+ )
+ api_key_prefixes = ["sst_"]
+ logger.info("API key auth enabled for MCP")
+ return CompositeTokenVerifier(
+ jwt_verifier=jwt_verifier,
+ api_key_prefixes=api_key_prefixes,
+ app=app,
+ )
+
+
+def _build_jwt_verifier(
+ app: Flask,
+ jwks_uri: Optional[str],
+ public_key: Optional[str],
+ secret: Optional[str],
+) -> JWTVerifier:
+ """Construct the JWT verifier from configured keys/secret."""
+ debug_errors = app.config.get("MCP_JWT_DEBUG_ERRORS", False)
+
+ common_kwargs: Dict[str, Any] = {
+ "issuer": app.config.get("MCP_JWT_ISSUER"),
+ "audience": app.config.get("MCP_JWT_AUDIENCE"),
+ "required_scopes": app.config.get("MCP_REQUIRED_SCOPES", []),
+ }
- return auth_provider
- except Exception:
- # Do not log the exception — it may contain the HS256 secret
- # from common_kwargs["public_key"]
- logger.error("Failed to create MCP auth provider")
- return None
+ # For HS256 (symmetric), use the secret as the public_key parameter
+ if app.config.get("MCP_JWT_ALGORITHM") == "HS256" and secret:
+ common_kwargs["public_key"] = secret
+ common_kwargs["algorithm"] = "HS256"
+ else:
+ # For RS256 (asymmetric), use public key or JWKS
+ common_kwargs["jwks_uri"] = jwks_uri
+ common_kwargs["public_key"] = public_key
+ common_kwargs["algorithm"] = app.config.get("MCP_JWT_ALGORITHM",
"RS256")
+
+ if debug_errors:
+ # DetailedJWTVerifier: detailed server-side logging of JWT
+ # validation failures. HTTP responses are always generic per
+ # RFC 6750 Section 3.1.
+ return DetailedJWTVerifier(**common_kwargs)
+
+ # MCPJWTVerifier: minimal logging + browser-friendly error page.
+ return MCPJWTVerifier(**common_kwargs)
Review Comment:
`_build_jwt_verifier` is a thin wrapper around FastMCP's `JWTVerifier`
constructor — adding unit tests would require mocking external library
internals, which is fragile. The function is exercised end-to-end by the JWT
path in `create_default_mcp_auth_factory`, which is already covered in
`test_mcp_server.py` (the
`test_init_fastmcp_server_applies_auth_to_global_instance` test verifies
`create_default_mcp_auth_factory` is invoked and returns the right factory).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
