dpgaspar commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3247557060
##########
superset/mcp_service/auth.py:
##########
@@ -218,6 +198,24 @@ def _resolve_user_from_jwt_context(app: Any) -> User |
None:
if access_token is None:
return None
+ # API key pass-through: CompositeTokenVerifier accepted this token
+ # at the transport layer but defers actual validation to
+ # _resolve_user_from_api_key() (priority 2 in get_user_from_request).
+ # Require client_id=="api_key" (set by CompositeTokenVerifier) in addition
+ # to the claim so that an external IdP JWT that happens to include the
+ # claim name is not misclassified as an API-key pass-through.
+ claims = getattr(access_token, "claims", None)
+ if isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM):
+ if getattr(access_token, "client_id", None) == "api_key":
+ logger.debug(
+ "API key pass-through token detected, deferring to API key
auth"
+ )
+ return None
+ logger.debug(
+ "API key passthrough claim present but client_id is not 'api_key';"
+ " processing as JWT"
+ )
+
# Use configurable resolver or default
from superset.mcp_service.mcp_config import default_user_resolver
Review Comment:
nit: can this be imported at the module level?
##########
superset/mcp_service/auth.py:
##########
@@ -148,45 +149,24 @@ def check_tool_permission(func: Callable[..., Any]) ->
bool:
def load_user_with_relationships(
username: str | None = None, email: str | None = None
) -> User | None:
- """
- Load a user with all relationships needed for permission checks.
-
- This function eagerly loads User.roles, User.groups, and Group.roles
- to prevent detached instance errors when the session is closed/rolled back.
+ """Load a user with roles and group roles eagerly loaded.
- IMPORTANT: Always use this function instead of security_manager.find_user()
- when loading users for MCP tool execution. The find_user() method doesn't
- eagerly load Group.roles, causing "detached instance" errors when
permission
- checks access group.roles after the session is rolled back.
-
- Args:
- username: The username to look up (optional if email provided)
- email: The email to look up (optional if username provided)
-
- Returns:
- User object with relationships loaded, or None if not found
+ Delegates to :meth:`SupersetSecurityManager.find_user_with_relationships`,
+ which mirrors FAB's ``find_user`` (including ``auth_username_ci`` and
+ ``MultipleResultsFound`` handling) while adding eager loading of
+ ``User.roles`` and ``User.groups.roles`` to prevent detached-instance
+ errors when the SQLAlchemy session is closed or rolled back after the
+ lookup — as happens in MCP tool-execution contexts.
Raises:
ValueError: If neither username nor email is provided
"""
if not username and not email:
raise ValueError("Either username or email must be provided")
- from sqlalchemy.orm import joinedload
+ from superset import security_manager
Review Comment:
nit: Can we import the security_manager at the module level? we can access
it also with `current_app.appbuilder.sm`
##########
superset/mcp_service/mcp_config.py:
##########
@@ -20,12 +20,15 @@
import secrets
from typing import Any, Dict, Optional
+from fastmcp.server.auth.providers.jwt import JWTVerifier
Review Comment:
Will this import make `fastmcp` a required dependency?
##########
superset/mcp_service/auth.py:
##########
@@ -131,8 +133,7 @@ def check_tool_permission(func: Callable[..., Any]) -> bool:
if not has_permission:
logger.warning(
- "Permission denied for user %s: %s on %s (tool: %s)",
- g.user.username,
+ "Permission denied: %s on %s (tool: %s)",
Review Comment:
Why is the current user being removed from the log, it's super useful for
debugging or security audits
##########
superset/mcp_service/mcp_config.py:
##########
@@ -291,56 +294,94 @@
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 ``FAB_API_KEY_ENABLED=True`` (API-key-only verifier that
+ rejects all non-API-key Bearer tokens at the transport).
+ """
+ auth_enabled = app.config.get("MCP_AUTH_ENABLED", False)
+ api_key_enabled = app.config.get("FAB_API_KEY_ENABLED", False)
- if not (jwks_uri or public_key or secret):
- logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret
configured")
+ 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)
+ try:
+ jwt_verifier = _build_jwt_verifier(
+ app=app,
+ jwks_uri=jwks_uri,
+ public_key=public_key,
+ secret=secret,
+ )
+ except Exception: # noqa: BLE001 — JWT lib raises many types;
broad catch intentional
+ # 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:
+ raw_prefixes = 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.
+ if isinstance(raw_prefixes, str):
+ api_key_prefixes = [raw_prefixes]
else:
- # Default JWTVerifier: minimal logging, generic error responses.
- from fastmcp.server.auth.providers.jwt import JWTVerifier
+ api_key_prefixes = list(raw_prefixes)
+ logger.info("API key auth enabled for MCP")
+ return CompositeTokenVerifier(
+ jwt_verifier=jwt_verifier,
+ api_key_prefixes=api_key_prefixes,
+ )
- auth_provider = JWTVerifier(**common_kwargs)
+ return jwt_verifier
- 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
+
+def _build_jwt_verifier(
+ app: Flask,
+ jwks_uri: Optional[str],
+ public_key: Optional[str],
+ secret: Optional[str],
+) -> Any:
Review Comment:
nit: can we declare the return type as `JWTVerifier`
##########
superset/mcp_service/mcp_config.py:
##########
@@ -291,56 +294,94 @@
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 ``FAB_API_KEY_ENABLED=True`` (API-key-only verifier that
+ rejects all non-API-key Bearer tokens at the transport).
+ """
+ auth_enabled = app.config.get("MCP_AUTH_ENABLED", False)
+ api_key_enabled = app.config.get("FAB_API_KEY_ENABLED", False)
- if not (jwks_uri or public_key or secret):
- logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret
configured")
+ 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)
+ try:
+ jwt_verifier = _build_jwt_verifier(
+ app=app,
+ jwks_uri=jwks_uri,
+ public_key=public_key,
+ secret=secret,
+ )
+ except Exception: # noqa: BLE001 — JWT lib raises many types;
broad catch intentional
+ # Do not log the exception — it may contain secrets (e.g., key
material)
+ logger.error("Failed to create MCP JWT verifier")
Review Comment:
nit: FastMCP's JWTVerifier uses authlib under the hood, and authlib has a
clean exception hierarchy:
JoseError (base)
├── BadSignatureError
├── DecodeError
├── ExpiredTokenError
└── ...
The JWTVerifier.__init__() constructor can realistically raise:
- JoseError (and subclasses) — key parsing, JOSE protocol issues
- ValueError — invalid configuration (bad algorithm, malformed URIs, etc.)
Superset's own DetailedJWTVerifier already imports JoseError as the base
class in superset/mcp_service/jwt_verifier.py:37:
``` python
from authlib.jose.errors import (
BadSignatureError,
DecodeError,
ExpiredTokenError,
JoseError, # <-- base exception, already used in this codebase
)
```
So the catch should be:
``` python
except (ValueError, JoseError):
logger.error("Failed to create MCP JWT verifier")
if not api_key_enabled:
return None
```
The comment "JWT lib raises many types; broad catch intentional" isn't
accurate — authlib has JoseError as a proper parent class specifically for this
purpose. A bare except Exception swallows things
like TypeError, ImportError, or MemoryError that should propagate.
##########
superset/security/manager.py:
##########
@@ -3164,6 +3169,60 @@ def get_user_by_username(self, username: str) ->
Optional[User]:
.one_or_none()
)
+ def find_user_with_relationships(
+ self,
+ username: Optional[str] = None,
+ email: Optional[str] = None,
+ ) -> Optional[User]:
+ """Find a user with roles and group roles eagerly loaded.
+
+ Mirrors FAB's ``SecurityManager.find_user``
+ (including ``auth_username_ci`` case-insensitive handling and
+ ``MultipleResultsFound`` guard) and additionally eager-loads
+ ``User.roles`` and ``User.groups.roles`` to prevent detached-instance
+ errors when the SQLAlchemy session is closed or rolled back after the
+ lookup — as happens in MCP tool-execution contexts.
+ """
+ eager = [
+ joinedload(self.user_model.roles),
+
joinedload(self.user_model.groups).joinedload(self.group_model.roles),
+ ]
+ if username:
+ try:
+ if self.auth_username_ci:
+ from sqlalchemy import func as sa_func
Review Comment:
nit: Let's move this import to the module level
--
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]
