bito-code-review[bot] commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3350594605
##########
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
Review Comment:
<!-- Bito Reply -->
The addition of four dedicated unit tests in `test_mcp_config.py`
effectively addresses the reviewer's concern regarding the lack of test
coverage for the `get_mcp_api_key_enabled` function. These tests cover the
required scenarios: explicit overrides, the fallback to `FAB_API_KEY_ENABLED`,
and the default behavior when both are absent.
##########
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
Review Comment:
<!-- Bito Reply -->
The reviewer has identified that the refactored
`create_default_mcp_auth_factory` function lacks direct unit tests, despite
handling multiple authentication modes (JWT, API-key, and composite). The
reviewer suggests that comprehensive unit tests covering success paths, error
scenarios, and edge cases should be added to comply with project standards for
new features.
--
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]
