You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2026/05/27 16:09:43 UTC
(superset) branch oss-39604 created (now 60c1fd93882)
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a change to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
at 60c1fd93882 fix(mcp): replace MCPJWTVerifier with JWTVerifier after browser-hello revert
This branch includes the following new commits:
new 20ab9277ab3 fix(mcp): create ApiKey permissions on init and support API keys with JWT auth
new 34c76e54598 fix(mcp): wire composite verifier and add ApiKey permission sync
new 9bc0065646f fix(mcp): add type annotations to test fixtures and parameters
new 6e362f95039 fix(mcp): remove prefixes from log to satisfy CodeQL
new 86587178401 fix(mcp): validate API keys via FastMCP AccessToken and lock down ApiKey perms
new e67c7cd5986 refactor(mcp): hoist API key auth imports to module top
new 90969bff322 fix(security): drop redundant explicit ApiKey perm creation
new 2297603a9b2 refactor(mcp): hoist JWT verifier imports to module top
new 190e75e8688 Potential fix for pull request finding
new 5966d689576 fix(mcp): fix stale patch target in auth tests and update stale docstring
new 3b622e8bf2b refactor(mcp): extract duplicated app context + sm setup into helper
new ce6f4c22f6e fix(mcp): harden auth — PermissionError propagation, passthrough client_id guard, fail-closed on missing token
new c4377b524f0 refactor(mcp): delegate load_user_with_relationships to SecurityManager.find_user_with_relationships
new 6b0fda96bf4 fix(mcp): fix stale patch target in auth tests and update stale docstring
new a33d5cf815f fix(mcp): validate api_key_prefixes in CompositeTokenVerifier — filter empty/non-string entries
new dc17712916b fix(mcp): fix stale patch target in auth tests and update stale docstring
new 3d817bb23be fix(mcp): normalize FAB_API_KEY_PREFIXES from config before passing to CompositeTokenVerifier
new bfad0590049 fix(mcp): address CodeQL security warnings and add ApiKey RBAC regression test
new 776fa9f1d5f fix(mcp): remove sensitive values from log calls to satisfy CodeQL
new c57a4988f12 fix(mcp): use class-bound attribute in joinedload for group roles
new dc3ce1eec8b fix(mcp): address dpgaspar review — imports, types, exception scope
new 6e205dea133 fix(mcp): remove sensitive values from log calls to satisfy CodeQL
new 8e4f7aa2bd2 fix(mcp): update security_manager patch target in RBAC tests
new 28bf9717cf6 fix(mcp): address Codex review — error class, fail-open, DRY permission logic
new 29c5c466455 fix(mcp): update security_manager patch target in tool-search tests
new 8165dbb0eab fix(mcp): broaden _log_user_resolution_failure type hint
new 0d1a4217616 fix(mcp): fix MCPPermissionDeniedError handler order and visibility test patch targets
new 51b20579230 fix(mcp): remove exc_info=True from tool-visibility debug log to prevent traceback-based credential leak
new a9459e87716 fix(mcp): use consistent filter() style for email lookup in find_user_with_relationships
new 60c1fd93882 fix(mcp): replace MCPJWTVerifier with JWTVerifier after browser-hello revert
The 30 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 18/30: fix(mcp): address CodeQL security warnings and add ApiKey RBAC regression test
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit bfad05900491e81d71e5f58baafbbf3afc8366d3
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu May 14 23:37:10 2026 +0000
fix(mcp): address CodeQL security warnings and add ApiKey RBAC regression test
- Remove JWT-extracted username from ValueError message in auth.py to
avoid CodeQL py/clear-text-logging-sensitive-data; log at DEBUG instead
- Log count of invalid FAB_API_KEY_PREFIXES entries rather than values to
avoid the same CodeQL rule in composite_token_verifier.py
- Add regression test asserting "ApiKey" in ADMIN_ONLY_VIEW_MENUS so a
future rename cannot silently re-open the FAB ApiKeyApi to non-Admin roles
---
superset/mcp_service/auth.py | 7 +++++--
superset/mcp_service/composite_token_verifier.py | 6 +++++-
tests/unit_tests/security/test_granular_export_permissions.py | 11 +++++++++++
3 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 22e82e038fe..61361657000 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -305,9 +305,12 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None:
if not user:
# Fail closed: JWT says this user should exist but they don't.
# Do NOT fall through to MCP_DEV_USERNAME or stale g.user.
+ # Avoid echoing the JWT-extracted username in the exception message
+ # (CodeQL py/clear-text-logging-sensitive-data).
+ logger.debug("JWT-authenticated user not found in database (identity from JWT)")
raise ValueError(
- f"JWT authenticated user '{username}' not found in Superset database. "
- f"Ensure the user exists before granting MCP access."
+ "JWT authenticated user not found in Superset database. "
+ "Ensure the user exists before granting MCP access."
)
return user
diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py
index 8889d66af42..72b22df60aa 100644
--- a/superset/mcp_service/composite_token_verifier.py
+++ b/superset/mcp_service/composite_token_verifier.py
@@ -73,8 +73,12 @@ class CompositeTokenVerifier(TokenVerifier):
]
invalid = [p for p in api_key_prefixes if p not in valid]
if invalid:
+ # Log count only — actual values may be config secrets
+ # (CodeQL py/clear-text-logging-sensitive-data).
logger.warning(
- "FAB_API_KEY_PREFIXES contains invalid entries (ignored): %r", invalid
+ "FAB_API_KEY_PREFIXES has %d invalid entries (empty/non-string)"
+ " — ignored",
+ len(invalid),
)
self._api_key_prefixes = tuple(valid)
diff --git a/tests/unit_tests/security/test_granular_export_permissions.py b/tests/unit_tests/security/test_granular_export_permissions.py
index 6db5e9712b2..af703debc64 100644
--- a/tests/unit_tests/security/test_granular_export_permissions.py
+++ b/tests/unit_tests/security/test_granular_export_permissions.py
@@ -91,6 +91,17 @@ def test_is_gamma_pvm_excludes_export_image(app_context: None) -> None:
assert sm._is_gamma_pvm(pvm) is False
+def test_api_key_view_menu_is_admin_only() -> None:
+ """Regression test: 'ApiKey' must be in ADMIN_ONLY_VIEW_MENUS.
+
+ FAB registers an ApiKeyApi blueprint when FAB_API_KEY_ENABLED=True.
+ Without this guard any Gamma user could reach the API key management
+ endpoints. A rename or removal of the entry would silently re-open
+ that access hole.
+ """
+ assert "ApiKey" in SupersetSecurityManager.ADMIN_ONLY_VIEW_MENUS
+
+
def test_is_gamma_pvm_allows_copy_clipboard(app_context: None) -> None:
"""Verify _is_gamma_pvm returns True for can_copy_clipboard."""
from superset.extensions import appbuilder
(superset) 05/30: fix(mcp): validate API keys via FastMCP AccessToken and lock down ApiKey perms
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 865871784019aad0d7549cf4e70ce445761030d2
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 8 14:26:05 2026 -0400
fix(mcp): validate API keys via FastMCP AccessToken and lock down ApiKey perms
Three independent bugs let MCP requests presenting Bearer tokens with the
sst_ prefix authenticate as MCP_DEV_USERNAME without any validation under
streamable-http:
1. _resolve_user_from_api_key read the token from flask.request.headers,
but the streamable-http transport never pushes a Flask request context
— has_request_context() was always False, so the function returned
None before validating, falling through to the dev-user fallback.
Now reads the token from FastMCP's per-request AccessToken (which the
CompositeTokenVerifier already populated) and fails closed when the
key is invalid.
2. CompositeTokenVerifier was only installed when MCP_AUTH_ENABLED=True.
With FAB_API_KEY_ENABLED=True alone, no transport-level verifier
existed at all. The factory now builds an API-key-only verifier in
that case (jwt_verifier=None) that rejects non-API-key Bearer tokens
at the transport instead of silently accepting them.
3. The pass-through AccessToken was minted with scopes=[], which would
make FastMCP's RequireAuthMiddleware 403 every API-key request when
MCP_REQUIRED_SCOPES is non-empty. Pass-through now propagates
self.required_scopes.
Also addresses Daniel's review comment on superset/security/manager.py:
adds "ApiKey" to ADMIN_ONLY_VIEW_MENUS so the FAB ApiKeyApi PVMs are
gated to Admin instead of leaking to Alpha and Gamma.
Renames the pass-through claim from _api_key_passthrough to the
namespaced _superset_mcp_api_key_passthrough (exported as
API_KEY_PASSTHROUGH_CLAIM) so a custom claim from an external IdP can't
accidentally divert a JWT into the API-key validation path.
Tests updated to mock get_access_token instead of app.test_request_context
(the simulated Flask context was the reason the prior tests passed while
production failed). New tests cover API-key-only verifier mode, scope
propagation on pass-through, and the namespaced-claim isolation.
---
superset/mcp_service/auth.py | 61 +++--
superset/mcp_service/composite_token_verifier.py | 36 ++-
superset/mcp_service/mcp_config.py | 141 +++++++-----
superset/mcp_service/server.py | 8 +-
superset/security/manager.py | 4 +
tests/unit_tests/mcp_service/test_auth_api_key.py | 253 +++++++++++++--------
.../mcp_service/test_composite_token_verifier.py | 53 ++++-
7 files changed, 365 insertions(+), 191 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 5320821d88a..04b2e802a66 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -288,8 +288,12 @@ def _resolve_user_from_jwt_context(app: Any) -> User | 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).
+ from superset.mcp_service.composite_token_verifier import (
+ API_KEY_PASSTHROUGH_CLAIM,
+ )
+
claims = getattr(access_token, "claims", None)
- if isinstance(claims, dict) and claims.get("_api_key_passthrough"):
+ if isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM):
logger.debug("API key pass-through token detected, deferring to API key auth")
return None
@@ -323,37 +327,53 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None:
def _resolve_user_from_api_key(app: Any) -> User | None:
"""
- Resolve the current user from an API key in the Authorization header.
+ Resolve the current user from an API key passed via Bearer token.
- Uses FAB SecurityManager's API key validation. Only attempts when
- FAB_API_KEY_ENABLED is True and a request context is active.
+ Reads the token from FastMCP's per-request ``AccessToken`` (set by
+ ``CompositeTokenVerifier`` when a Bearer token matches an API key
+ prefix). The streamable-http transport does not push a Flask request
+ context, so we cannot rely on ``flask.request`` headers — the verifier
+ already saw the token and stashed it on the ``AccessToken``.
Returns:
- User object with relationships loaded, or None if no API key present
- or API key auth is not enabled/available.
+ User object with relationships loaded, or None if no API key
+ pass-through token is present or API key auth is not enabled.
Raises:
- PermissionError: If an API key is present but invalid/expired,
- or if validation is not available in this FAB version.
+ PermissionError: If an API key pass-through token is present but
+ invalid/expired (fail closed — do NOT fall through to weaker
+ auth sources like ``MCP_DEV_USERNAME``), or if validation is
+ not available in this FAB version.
"""
- if not app.config.get("FAB_API_KEY_ENABLED", False) or not has_request_context():
+ if not app.config.get("FAB_API_KEY_ENABLED", False):
return None
- sm = app.appbuilder.sm
- # extract_api_key_from_request is FAB's method for reading
- # the Bearer token from the Authorization header and matching prefixes.
- # Not all FAB versions include this method, so guard with hasattr.
- if not hasattr(sm, "extract_api_key_from_request"):
- logger.debug(
- "FAB SecurityManager does not have extract_api_key_from_request; "
- "API key authentication is not available in this FAB version"
- )
+ try:
+ from fastmcp.server.dependencies import get_access_token
+ except ImportError:
+ logger.debug("fastmcp.server.dependencies not available, skipping API key auth")
+ return None
+
+ access_token = get_access_token()
+ if access_token is None:
return None
- api_key_string = sm.extract_api_key_from_request()
- if api_key_string is None:
+ # Only validate tokens that the CompositeTokenVerifier flagged as
+ # API key pass-throughs. Plain JWTs were already validated by the JWT
+ # verifier and resolved in _resolve_user_from_jwt_context.
+ from superset.mcp_service.composite_token_verifier import (
+ API_KEY_PASSTHROUGH_CLAIM,
+ )
+
+ claims = getattr(access_token, "claims", None)
+ if not (isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM)):
+ return None
+
+ api_key_string = getattr(access_token, "token", None)
+ if not api_key_string:
return None
+ sm = app.appbuilder.sm
if not hasattr(sm, "validate_api_key"):
logger.warning(
"FAB SecurityManager does not have validate_api_key; "
@@ -535,7 +555,6 @@ def _setup_user_context() -> User | None:
# tool calls when no per-request middleware refreshes it.
# Only clear in app-context-only mode; preserve g.user when
# a request context is active (external middleware set it).
- from flask import has_request_context
if not has_request_context():
g.pop("user", None)
diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py
index e4f5c6a5c43..e404e364373 100644
--- a/superset/mcp_service/composite_token_verifier.py
+++ b/superset/mcp_service/composite_token_verifier.py
@@ -22,7 +22,9 @@ Routes Bearer tokens to the appropriate verifier based on prefix:
- Tokens matching FAB_API_KEY_PREFIXES (e.g. ``sst_``) are passed through
to the Flask layer where ``_resolve_user_from_api_key()`` handles
actual validation via FAB SecurityManager.
-- All other tokens are delegated to the wrapped JWT verifier.
+- All other tokens are delegated to the wrapped JWT verifier (when one is
+ configured); when no JWT verifier is configured, non-API-key tokens are
+ rejected at the transport layer.
"""
import logging
@@ -32,6 +34,12 @@ from fastmcp.server.auth.providers.jwt import TokenVerifier
logger = logging.getLogger(__name__)
+# Namespaced claim that flags an AccessToken as an API-key pass-through.
+# Namespacing avoids collision with custom claims an external IdP might
+# happen to mint on a JWT — a plain ``_api_key_passthrough`` claim could
+# be silently misidentified as a Superset API-key request.
+API_KEY_PASSTHROUGH_CLAIM = "_superset_mcp_api_key_passthrough"
+
class CompositeTokenVerifier(TokenVerifier):
"""Routes Bearer tokens between API key pass-through and JWT verification.
@@ -43,18 +51,21 @@ class CompositeTokenVerifier(TokenVerifier):
Args:
jwt_verifier: The wrapped JWT verifier for non-API-key tokens.
+ When ``None``, only API-key tokens are accepted; all other
+ Bearer tokens are rejected at the transport layer (used when
+ ``MCP_AUTH_ENABLED=False`` but ``FAB_API_KEY_ENABLED=True``).
api_key_prefixes: List of prefixes that identify API key tokens
(e.g. ``["sst_"]``).
"""
def __init__(
self,
- jwt_verifier: TokenVerifier,
+ jwt_verifier: TokenVerifier | None,
api_key_prefixes: list[str],
) -> None:
super().__init__(
base_url=getattr(jwt_verifier, "base_url", None),
- required_scopes=jwt_verifier.required_scopes,
+ required_scopes=getattr(jwt_verifier, "required_scopes", None) or [],
)
self._jwt_verifier = jwt_verifier
self._api_key_prefixes = tuple(api_key_prefixes)
@@ -66,15 +77,28 @@ class CompositeTokenVerifier(TokenVerifier):
AccessToken with a ``_api_key_passthrough`` claim. The Flask-layer
``_resolve_user_from_api_key()`` performs the real validation.
- Otherwise, delegate to the wrapped JWT verifier.
+ Otherwise, delegate to the wrapped JWT verifier when one is
+ configured; if no JWT verifier is configured, reject the token.
"""
if any(token.startswith(prefix) for prefix in self._api_key_prefixes):
logger.debug("API key token detected (prefix match), passing through")
+ # Populate ``scopes`` from ``self.required_scopes`` so FastMCP's
+ # ``RequireAuthMiddleware`` (transport-layer scope check) is
+ # satisfied for API-key requests. Without this, MCP_REQUIRED_SCOPES
+ # being non-empty would 403 every API-key call before
+ # ``_resolve_user_from_api_key`` even runs.
return AccessToken(
token=token,
client_id="api_key",
- scopes=[],
- claims={"_api_key_passthrough": True},
+ scopes=list(self.required_scopes or []),
+ claims={API_KEY_PASSTHROUGH_CLAIM: True},
+ )
+
+ if self._jwt_verifier is None:
+ logger.debug(
+ "Bearer token does not match any API key prefix and no JWT "
+ "verifier is configured; rejecting"
)
+ return None
return await self._jwt_verifier.verify_token(token)
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index a0d9ae89336..837b092ecf2 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -304,71 +304,96 @@ MCP_TOOL_SEARCH_CONFIG: Dict[str, Any] = {
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"
- 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)
+ 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:
- # Default JWTVerifier: minimal logging, generic error responses.
- from fastmcp.server.auth.providers.jwt import JWTVerifier
-
- auth_provider = JWTVerifier(**common_kwargs)
-
- # Wrap with CompositeTokenVerifier when API key auth is enabled
- # so that API key tokens (e.g. sst_...) pass through the transport
- # layer instead of being rejected by the JWT verifier.
- if app.config.get("FAB_API_KEY_ENABLED", False):
- from superset.mcp_service.composite_token_verifier import (
- CompositeTokenVerifier,
- )
-
- api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
- auth_provider = CompositeTokenVerifier(
- jwt_verifier=auth_provider,
- api_key_prefixes=api_key_prefixes,
- )
- logger.info("API key auth enabled for MCP")
-
- 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
+ try:
+ jwt_verifier = _build_jwt_verifier(
+ app=app,
+ jwks_uri=jwks_uri,
+ public_key=public_key,
+ secret=secret,
+ )
+ except Exception:
+ # Do not log the exception — it may contain secrets
+ logger.error("Failed to create MCP JWT verifier")
+ if not api_key_enabled:
+ return None
+
+ if api_key_enabled:
+ from superset.mcp_service.composite_token_verifier import (
+ CompositeTokenVerifier,
+ )
+
+ api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
+ logger.info("API key auth enabled for MCP")
+ return CompositeTokenVerifier(
+ jwt_verifier=jwt_verifier,
+ api_key_prefixes=api_key_prefixes,
+ )
+
+ return jwt_verifier
+
+
+def _build_jwt_verifier(
+ app: Flask,
+ jwks_uri: Optional[str],
+ public_key: Optional[str],
+ secret: Optional[str],
+) -> Any:
+ """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", []),
+ }
+
+ # 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.
+ from superset.mcp_service.jwt_verifier import DetailedJWTVerifier
+
+ return DetailedJWTVerifier(**common_kwargs)
+
+ # MCPJWTVerifier: minimal logging + browser-friendly error page.
+ from superset.mcp_service.jwt_verifier import MCPJWTVerifier
+
+ return MCPJWTVerifier(**common_kwargs)
def default_user_resolver(app: Any, access_token: Any) -> str | None:
diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py
index dc4bffbb598..ab6fb04a2ca 100644
--- a/superset/mcp_service/server.py
+++ b/superset/mcp_service/server.py
@@ -665,7 +665,9 @@ def _create_auth_provider(flask_app: Any) -> Any | None:
"""Create an auth provider from Flask app config.
Tries MCP_AUTH_FACTORY first, then falls back to the default factory
- when MCP_AUTH_ENABLED is True.
+ when either ``MCP_AUTH_ENABLED`` (JWT auth) or ``FAB_API_KEY_ENABLED``
+ (API key auth) is True. The default factory builds a
+ ``CompositeTokenVerifier`` that handles either or both auth modes.
"""
auth_provider = None
if auth_factory := flask_app.config.get("MCP_AUTH_FACTORY"):
@@ -678,7 +680,9 @@ def _create_auth_provider(flask_app: Any) -> Any | None:
except Exception:
# Do not log the exception — it may contain secrets
logger.error("Failed to create auth provider from MCP_AUTH_FACTORY")
- elif flask_app.config.get("MCP_AUTH_ENABLED", False):
+ elif flask_app.config.get("MCP_AUTH_ENABLED", False) or flask_app.config.get(
+ "FAB_API_KEY_ENABLED", False
+ ):
from superset.mcp_service.mcp_config import (
create_default_mcp_auth_factory,
)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 44c23f86597..19a8cf8c522 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -462,6 +462,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"PermissionViewMenu",
"ViewMenu",
"User",
+ # FAB ApiKeyApi blueprint (active when FAB_API_KEY_ENABLED=True).
+ # Listed unconditionally — harmless when the feature is off because
+ # no PVMs exist under this view menu.
+ "ApiKey",
} | USER_MODEL_VIEWS
ALPHA_ONLY_VIEW_MENUS = {
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index b53e624e83a..11717ae0edc 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -15,8 +15,15 @@
# specific language governing permissions and limitations
# under the License.
-"""Tests for API key authentication in get_user_from_request()."""
+"""Tests for API key authentication in get_user_from_request().
+The streamable-http transport does not push a Flask request context, so
+``_resolve_user_from_api_key`` reads the token from FastMCP's per-request
+``AccessToken`` (populated by ``CompositeTokenVerifier``) rather than from
+``flask.request``. These tests mock ``get_access_token`` accordingly.
+"""
+
+from collections.abc import Generator
from unittest.mock import MagicMock, patch
import pytest
@@ -27,17 +34,34 @@ from superset.mcp_service.auth import (
_resolve_user_from_jwt_context,
get_user_from_request,
)
+from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM
@pytest.fixture
-def mock_user():
+def mock_user() -> MagicMock:
user = MagicMock()
user.username = "api_key_user"
return user
+def _passthrough_access_token(token: str) -> MagicMock:
+ """Build an AccessToken matching what CompositeTokenVerifier emits."""
+ access_token = MagicMock()
+ access_token.token = token
+ access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True}
+ return access_token
+
+
+def _patch_access_token(access_token: MagicMock | None):
+ """Patch get_access_token where _resolve_user_from_api_key imports it."""
+ return patch(
+ "fastmcp.server.dependencies.get_access_token",
+ return_value=access_token,
+ )
+
+
@pytest.fixture
-def _enable_api_keys(app):
+def _enable_api_keys(app: SupersetApp) -> Generator[None, None, None]:
"""Enable FAB API key auth and clear MCP_DEV_USERNAME so the API key
path is exercised instead of falling through to the dev-user fallback."""
app.config["FAB_API_KEY_ENABLED"] = True
@@ -49,7 +73,7 @@ def _enable_api_keys(app):
@pytest.fixture
-def _disable_api_keys(app):
+def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]:
app.config["FAB_API_KEY_ENABLED"] = False
old_dev = app.config.pop("MCP_DEV_USERNAME", None)
yield
@@ -62,20 +86,22 @@ def _disable_api_keys(app):
@pytest.mark.usefixtures("_enable_api_keys")
-def test_valid_api_key_returns_user(app, mock_user) -> None:
- """A valid API key should authenticate and return the user."""
+def test_valid_api_key_returns_user(app: SupersetApp, mock_user: MagicMock) -> None:
+ """A valid API key pass-through token should authenticate and return the user."""
mock_sm = MagicMock()
- mock_sm.extract_api_key_from_request.return_value = "sst_abc123"
mock_sm.validate_api_key.return_value = mock_user
- with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}):
+ with app.app_context():
g.user = None
app.appbuilder = MagicMock()
app.appbuilder.sm = mock_sm
- with patch(
- "superset.mcp_service.auth.load_user_with_relationships",
- return_value=mock_user,
+ with (
+ _patch_access_token(_passthrough_access_token("sst_abc123")),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=mock_user,
+ ),
):
result = get_user_from_request()
@@ -83,54 +109,61 @@ def test_valid_api_key_returns_user(app, mock_user) -> None:
mock_sm.validate_api_key.assert_called_once_with("sst_abc123")
-# -- Invalid API key -> PermissionError --
+# -- Invalid API key -> PermissionError (does not silently fall back) --
@pytest.mark.usefixtures("_enable_api_keys")
-def test_invalid_api_key_raises(app) -> None:
- """An invalid API key should raise PermissionError."""
+def test_invalid_api_key_raises(app: SupersetApp) -> None:
+ """An invalid API key pass-through token should raise PermissionError
+ (fail closed — do NOT fall through to MCP_DEV_USERNAME)."""
mock_sm = MagicMock()
- mock_sm.extract_api_key_from_request.return_value = "sst_bad_key"
mock_sm.validate_api_key.return_value = None
- with app.test_request_context(headers={"Authorization": "Bearer sst_bad_key"}):
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
+ # The dangerous fallthrough scenario: dev username IS set, but the
+ # request presented an invalid API key. The dev fallback must not
+ # mask the rejection.
+ app.config["MCP_DEV_USERNAME"] = "admin"
+ try:
+ with app.app_context():
+ g.user = None
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm = mock_sm
- with pytest.raises(PermissionError, match="Invalid or expired API key"):
- get_user_from_request()
+ with _patch_access_token(_passthrough_access_token("sst_bad_key")):
+ with pytest.raises(PermissionError, match="Invalid or expired API key"):
+ get_user_from_request()
+ finally:
+ app.config.pop("MCP_DEV_USERNAME", None)
# -- API key disabled -> falls through to next auth method --
@pytest.mark.usefixtures("_disable_api_keys")
-def test_api_key_disabled_skips_auth(app) -> None:
- """When FAB_API_KEY_ENABLED is False, API key auth is skipped entirely."""
+def test_api_key_disabled_skips_auth(app: SupersetApp) -> None:
+ """When FAB_API_KEY_ENABLED is False, API key auth is skipped entirely
+ even if an AccessToken is present."""
mock_sm = MagicMock()
- with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}):
+ with app.app_context():
g.user = None
app.appbuilder = MagicMock()
app.appbuilder.sm = mock_sm
- # Without API key auth or MCP_DEV_USERNAME, should raise ValueError
- # about no authenticated user (not about invalid API key)
- with pytest.raises(ValueError, match="No authenticated user found"):
- get_user_from_request()
+ with _patch_access_token(_passthrough_access_token("sst_abc123")):
+ with pytest.raises(ValueError, match="No authenticated user found"):
+ get_user_from_request()
- # SecurityManager API key methods should never be called
- mock_sm.extract_api_key_from_request.assert_not_called()
+ mock_sm.validate_api_key.assert_not_called()
-# -- No request context -> API key auth skipped --
+# -- No AccessToken -> API key auth skipped --
@pytest.mark.usefixtures("_enable_api_keys")
-def test_no_request_context_skips_api_key_auth(app) -> None:
- """Without a request context, API key auth should be skipped
- (e.g., during MCP tool discovery with only an app context)."""
+def test_no_access_token_skips_api_key_auth(app: SupersetApp) -> None:
+ """Without a FastMCP AccessToken (e.g., MCP_AUTH_ENABLED=False and no
+ auth provider installed), API key auth is skipped."""
mock_sm = MagicMock()
with app.app_context():
@@ -138,20 +171,20 @@ def test_no_request_context_skips_api_key_auth(app) -> None:
app.appbuilder = MagicMock()
app.appbuilder.sm = mock_sm
- # Explicitly mock has_request_context to False because the test
- # framework's app fixture may implicitly provide a request context.
- with patch("superset.mcp_service.auth.has_request_context", return_value=False):
+ with _patch_access_token(None):
with pytest.raises(ValueError, match="No authenticated user found"):
get_user_from_request()
- mock_sm.extract_api_key_from_request.assert_not_called()
+ mock_sm.validate_api_key.assert_not_called()
# -- g.user fallback when no higher-priority auth succeeds --
@pytest.mark.usefixtures("_disable_api_keys")
-def test_g_user_fallback_when_no_jwt_or_api_key(app, mock_user) -> None:
+def test_g_user_fallback_when_no_jwt_or_api_key(
+ app: SupersetApp, mock_user: MagicMock
+) -> None:
"""When no JWT or API key auth succeeds and MCP_DEV_USERNAME is not set,
g.user (set by external middleware) is used as fallback."""
with app.test_request_context():
@@ -162,106 +195,97 @@ def test_g_user_fallback_when_no_jwt_or_api_key(app, mock_user) -> None:
assert result.username == "api_key_user"
-# -- FAB version without extract_api_key_from_request --
-
-
-@pytest.mark.usefixtures("_enable_api_keys")
-def test_fab_without_extract_method_skips_gracefully(app) -> None:
- """If FAB SecurityManager lacks extract_api_key_from_request,
- API key auth should be skipped with a debug log, not crash."""
- mock_sm = MagicMock(spec=[]) # empty spec = no attributes
-
- with app.test_request_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
- with pytest.raises(ValueError, match="No authenticated user found"):
- get_user_from_request()
-
-
# -- FAB version without validate_api_key --
@pytest.mark.usefixtures("_enable_api_keys")
-def test_fab_without_validate_method_raises(app) -> None:
- """If FAB has extract_api_key_from_request but not validate_api_key,
- should raise PermissionError about unavailable validation."""
- mock_sm = MagicMock(spec=["extract_api_key_from_request"])
- mock_sm.extract_api_key_from_request.return_value = "sst_abc123"
+def test_fab_without_validate_method_raises(app: SupersetApp) -> None:
+ """If FAB SecurityManager lacks validate_api_key, should raise
+ PermissionError about unavailable validation."""
+ mock_sm = MagicMock(spec=[]) # empty spec = no attributes
- with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}):
+ with app.app_context():
g.user = None
app.appbuilder = MagicMock()
app.appbuilder.sm = mock_sm
- with pytest.raises(
- PermissionError, match="API key validation is not available"
- ):
- get_user_from_request()
+ with _patch_access_token(_passthrough_access_token("sst_abc123")):
+ with pytest.raises(
+ PermissionError, match="API key validation is not available"
+ ):
+ get_user_from_request()
# -- Relationship reload fallback --
@pytest.mark.usefixtures("_enable_api_keys")
-def test_relationship_reload_failure_returns_original_user(app, mock_user) -> None:
+def test_relationship_reload_failure_returns_original_user(
+ app: SupersetApp, mock_user: MagicMock
+) -> None:
"""If load_user_with_relationships fails, the original user from
validate_api_key should be returned as fallback."""
mock_sm = MagicMock()
- mock_sm.extract_api_key_from_request.return_value = "sst_abc123"
mock_sm.validate_api_key.return_value = mock_user
- with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}):
+ with app.app_context():
g.user = None
app.appbuilder = MagicMock()
app.appbuilder.sm = mock_sm
- with patch(
- "superset.mcp_service.auth.load_user_with_relationships",
- return_value=None,
+ with (
+ _patch_access_token(_passthrough_access_token("sst_abc123")),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=None,
+ ),
):
result = get_user_from_request()
assert result is mock_user
-# -- Bearer token present but not matching API key prefix --
+# -- AccessToken without passthrough claim (plain JWT) -> skip API key auth --
@pytest.mark.usefixtures("_enable_api_keys")
-def test_non_matching_bearer_token_skips_api_key_auth(app: SupersetApp) -> None:
- """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES
- (e.g., a JWT token), extract_api_key_from_request returns None and API key
- auth is skipped, falling through to the next auth method."""
+def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None:
+ """When the AccessToken is a plain JWT (no ``_api_key_passthrough`` claim),
+ API key auth is skipped — the JWT was already validated by the JWT
+ verifier and resolved in _resolve_user_from_jwt_context."""
mock_sm = MagicMock()
- mock_sm.extract_api_key_from_request.return_value = None
- with app.test_request_context(
- headers={"Authorization": "Bearer eyJhbGciOiJIUzI1NiJ9.not-an-api-key"}
- ):
+ jwt_access_token = MagicMock()
+ jwt_access_token.token = "eyJhbGciOiJIUzI1NiJ9.not-an-api-key" # noqa: S105
+ jwt_access_token.claims = {"sub": "alice"}
+
+ with app.app_context():
g.user = None
app.appbuilder = MagicMock()
app.appbuilder.sm = mock_sm
- with pytest.raises(ValueError, match="No authenticated user found"):
- get_user_from_request()
+ with _patch_access_token(jwt_access_token):
+ # _resolve_user_from_jwt_context will try to resolve the user
+ # from the JWT claims and (in this isolated unit-test setup)
+ # raise ValueError because the username is not a real user.
+ # We assert that _resolve_user_from_api_key did NOT short-circuit
+ # to the API key path.
+ with pytest.raises(ValueError, match="not found"):
+ get_user_from_request()
- # extract was called but returned None, so validate should NOT be called
- mock_sm.extract_api_key_from_request.assert_called_once()
mock_sm.validate_api_key.assert_not_called()
-# -- API key pass-through from CompositeTokenVerifier --
+# -- API key pass-through detection in JWT context resolver --
def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> None:
"""When CompositeTokenVerifier passes through an API key token,
- _resolve_user_from_jwt_context should detect the _api_key_passthrough
- claim and return None so get_user_from_request falls through to
- _resolve_user_from_api_key."""
+ _resolve_user_from_jwt_context should detect the namespaced
+ pass-through claim and return None so get_user_from_request falls
+ through to _resolve_user_from_api_key."""
mock_access_token = MagicMock()
- mock_access_token.claims = {"_api_key_passthrough": True}
+ mock_access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True}
with patch(
"fastmcp.server.dependencies.get_access_token",
@@ -272,24 +296,51 @@ def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) ->
assert result is None
+# -- Plain JWT with a colliding non-namespaced claim is NOT mistaken for API key --
+
+
+@pytest.mark.usefixtures("_enable_api_keys")
+def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path(
+ app: SupersetApp,
+) -> None:
+ """A JWT minted by an external IdP that happens to include a custom
+ ``_api_key_passthrough`` claim (legacy unnamespaced name) must NOT be
+ treated as an API-key pass-through. Only the namespaced
+ ``API_KEY_PASSTHROUGH_CLAIM`` triggers the API-key path."""
+ mock_sm = MagicMock()
+
+ rogue_token = MagicMock()
+ rogue_token.token = "eyJhbGciOiJSUzI1NiJ9.rogue_jwt" # noqa: S105
+ rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"}
+
+ with app.app_context():
+ g.user = None
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm = mock_sm
+
+ with _patch_access_token(rogue_token):
+ # JWT path tries to resolve user "alice" from DB and (in this
+ # isolated unit-test setup) raises ValueError. The assertion
+ # below confirms validate_api_key was never called — i.e., the
+ # rogue claim did NOT divert into _resolve_user_from_api_key.
+ with pytest.raises(ValueError, match="not found"):
+ get_user_from_request()
+
+ mock_sm.validate_api_key.assert_not_called()
+
+
# -- SecurityManager method name regression test --
def test_security_manager_has_expected_api_key_methods(app: SupersetApp) -> None:
- """Regression test: verify the SecurityManager method names referenced in
- auth._resolve_user_from_api_key() actually exist on the FAB SecurityManager
- class. This catches future renames before they silently break API key auth
- at runtime (see PR #39437: _extract_api_key_from_request vs
- extract_api_key_from_request)."""
+ """Regression test: verify the SecurityManager method name referenced in
+ auth._resolve_user_from_api_key() actually exists on the FAB
+ SecurityManager class. Catches future renames before they silently break
+ API key auth at runtime (see PR #39437)."""
with app.app_context():
from superset import security_manager
sm = security_manager
- assert hasattr(sm, "extract_api_key_from_request"), (
- "FAB SecurityManager is missing 'extract_api_key_from_request'. "
- "auth._resolve_user_from_api_key() references this method by name — "
- "update auth.py if the FAB API changed."
- )
assert hasattr(sm, "validate_api_key"), (
"FAB SecurityManager is missing 'validate_api_key'. "
"auth._resolve_user_from_api_key() references this method by name — "
diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
index 8f496305a1c..d493c07ea03 100644
--- a/tests/unit_tests/mcp_service/test_composite_token_verifier.py
+++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
@@ -22,7 +22,10 @@ from unittest.mock import AsyncMock, MagicMock
import pytest
from fastmcp.server.auth import AccessToken
-from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier
+from superset.mcp_service.composite_token_verifier import (
+ API_KEY_PASSTHROUGH_CLAIM,
+ CompositeTokenVerifier,
+)
@pytest.fixture
@@ -52,7 +55,7 @@ async def test_api_key_token_returns_passthrough(
assert result is not None
assert result.token == api_key
assert result.client_id == "api_key"
- assert result.claims.get("_api_key_passthrough") is True
+ assert result.claims.get(API_KEY_PASSTHROUGH_CLAIM) is True
@pytest.mark.asyncio
@@ -63,7 +66,7 @@ async def test_second_prefix_matches(
result = await composite_verifier.verify_token("pat_mytoken")
assert result is not None
- assert result.claims.get("_api_key_passthrough") is True
+ assert result.claims.get(API_KEY_PASSTHROUGH_CLAIM) is True
@pytest.mark.asyncio
@@ -109,3 +112,47 @@ async def test_api_key_does_not_call_jwt_verifier(
await composite_verifier.verify_token("sst_test_key")
mock_jwt_verifier.verify_token.assert_not_awaited()
+
+
+# -- API-key-only mode (no JWT verifier configured) --
+
+
+@pytest.mark.asyncio
+async def test_api_key_only_mode_accepts_api_keys() -> None:
+ """When jwt_verifier is None, API key tokens are still passed through."""
+ verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["sst_"])
+
+ result = await verifier.verify_token("sst_abc123")
+
+ assert result is not None
+ assert result.claims.get(API_KEY_PASSTHROUGH_CLAIM) is True
+
+
+@pytest.mark.asyncio
+async def test_api_key_only_mode_rejects_non_api_key_tokens() -> None:
+ """When jwt_verifier is None, non-API-key Bearer tokens are rejected at
+ the transport instead of being silently accepted."""
+ verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["sst_"])
+
+ result = await verifier.verify_token("eyJhbGciOiJSUzI1NiJ9.jwt_payload")
+
+ assert result is None
+
+
+@pytest.mark.asyncio
+async def test_api_key_passthrough_propagates_required_scopes() -> None:
+ """The pass-through AccessToken must carry the verifier's required_scopes
+ so FastMCP's transport-level ``RequireAuthMiddleware`` does not 403 the
+ request before ``_resolve_user_from_api_key`` runs."""
+ jwt_verifier = MagicMock()
+ jwt_verifier.required_scopes = ["read", "write"]
+ jwt_verifier.verify_token = AsyncMock()
+
+ verifier = CompositeTokenVerifier(
+ jwt_verifier=jwt_verifier, api_key_prefixes=["sst_"]
+ )
+
+ result = await verifier.verify_token("sst_abc123")
+
+ assert result is not None
+ assert result.scopes == ["read", "write"]
(superset) 27/30: fix(mcp): fix MCPPermissionDeniedError handler order and visibility test patch targets
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 0d1a42176167a1975ec919ab3eb284b62b6b7652
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 22 02:21:45 2026 +0000
fix(mcp): fix MCPPermissionDeniedError handler order and visibility test patch targets
MCPPermissionDeniedError(PermissionError) was being caught by the generic
PermissionError branch in _handle_error before reaching its own handler,
so the ToolError message was sanitized to "You don't have access to this
resource." instead of the structured permission message.
Move the MCPPermissionDeniedError branch above PermissionError so the
subclass is matched first.
Also fix four visibility test patch targets: auth.py imports
security_manager at module level via `from superset import security_manager`,
so tests must patch `superset.mcp_service.auth.security_manager` (not
`superset.security_manager`) to intercept calls inside auth.py.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/middleware.py | 8 +++++---
tests/unit_tests/mcp_service/test_auth_rbac.py | 8 ++++----
2 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py
index 685022c3a9f..02688a761f6 100644
--- a/superset/mcp_service/middleware.py
+++ b/superset/mcp_service/middleware.py
@@ -632,6 +632,11 @@ class GlobalErrorHandlerMiddleware(Middleware):
elif isinstance(error, HTTPException):
# HTTP errors from screenshot endpoints or API calls
raise ToolError(f"Service error in {tool_name}: {error.detail}") from error
+ elif isinstance(error, MCPPermissionDeniedError):
+ # MCP RBAC permission denied — convert to structured ToolError.
+ # Must come before the generic PermissionError branch because
+ # MCPPermissionDeniedError inherits from PermissionError.
+ raise ToolError(str(error)) from error
elif isinstance(error, PermissionError):
# Permission/authorization errors
raise ToolError(
@@ -648,9 +653,6 @@ class GlobalErrorHandlerMiddleware(Middleware):
raise ToolError(
f"Invalid request for {tool_name}: {_sanitize_error_for_logging(error)}"
) from error
- elif isinstance(error, MCPPermissionDeniedError):
- # MCP RBAC permission denied — convert to structured ToolError
- raise ToolError(str(error)) from error
elif isinstance(error, (ForbiddenError, SupersetSecurityException)):
# Superset access denied — agent tried a tool it can't use
raise ToolError(
diff --git a/tests/unit_tests/mcp_service/test_auth_rbac.py b/tests/unit_tests/mcp_service/test_auth_rbac.py
index 64a9684eaec..55a97dacea7 100644
--- a/tests/unit_tests/mcp_service/test_auth_rbac.py
+++ b/tests/unit_tests/mcp_service/test_auth_rbac.py
@@ -280,7 +280,7 @@ def test_visibility_allowed_tool(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
- with patch("superset.security_manager", mock_sm):
+ with patch("superset.mcp_service.auth.security_manager", mock_sm):
result = is_tool_visible_to_current_user(tool)
assert result is True
@@ -295,7 +295,7 @@ def test_visibility_denied_tool(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=False)
- with patch("superset.security_manager", mock_sm):
+ with patch("superset.mcp_service.auth.security_manager", mock_sm):
result = is_tool_visible_to_current_user(tool)
assert result is False
@@ -312,7 +312,7 @@ def test_visibility_data_model_metadata_denied(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
with (
- patch("superset.security_manager", mock_sm),
+ patch("superset.mcp_service.auth.security_manager", mock_sm),
patch(
"superset.mcp_service.privacy.user_can_view_data_model_metadata",
return_value=False,
@@ -334,7 +334,7 @@ def test_visibility_data_model_metadata_allowed(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
with (
- patch("superset.security_manager", mock_sm),
+ patch("superset.mcp_service.auth.security_manager", mock_sm),
patch(
"superset.mcp_service.privacy.user_can_view_data_model_metadata",
return_value=True,
(superset) 06/30: refactor(mcp): hoist API key auth imports to module top
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit e67c7cd59865967b30570796ec06096306ef6508
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 8 14:36:16 2026 -0400
refactor(mcp): hoist API key auth imports to module top
The API_KEY_PASSTHROUGH_CLAIM constant in auth.py and CompositeTokenVerifier
in mcp_config.py have no circular-import or optional-dependency reason to
be imported inline. Moved them to module top.
---
superset/mcp_service/auth.py | 10 ++--------
superset/mcp_service/mcp_config.py | 5 +----
2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 04b2e802a66..94b223aec08 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -51,6 +51,8 @@ from typing import Any, Callable, TYPE_CHECKING, TypeVar
from flask import current_app, g, has_app_context, has_request_context
from flask_appbuilder.security.sqla.models import Group, User
+from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM
+
if TYPE_CHECKING:
from superset.connectors.sqla.models import SqlaTable
from superset.mcp_service.chart.chart_utils import DatasetValidationResult
@@ -288,10 +290,6 @@ def _resolve_user_from_jwt_context(app: Any) -> User | 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).
- from superset.mcp_service.composite_token_verifier import (
- API_KEY_PASSTHROUGH_CLAIM,
- )
-
claims = getattr(access_token, "claims", None)
if isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM):
logger.debug("API key pass-through token detected, deferring to API key auth")
@@ -361,10 +359,6 @@ def _resolve_user_from_api_key(app: Any) -> User | None:
# Only validate tokens that the CompositeTokenVerifier flagged as
# API key pass-throughs. Plain JWTs were already validated by the JWT
# verifier and resolved in _resolve_user_from_jwt_context.
- from superset.mcp_service.composite_token_verifier import (
- API_KEY_PASSTHROUGH_CLAIM,
- )
-
claims = getattr(access_token, "claims", None)
if not (isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM)):
return None
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index 837b092ecf2..0694fc90975 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -22,6 +22,7 @@ from typing import Any, Dict, Optional
from flask import Flask
+from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier
from superset.mcp_service.constants import (
DEFAULT_TOKEN_LIMIT,
DEFAULT_WARN_THRESHOLD_PCT,
@@ -343,10 +344,6 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
return None
if api_key_enabled:
- from superset.mcp_service.composite_token_verifier import (
- CompositeTokenVerifier,
- )
-
api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
logger.info("API key auth enabled for MCP")
return CompositeTokenVerifier(
(superset) 23/30: fix(mcp): update security_manager patch target in RBAC tests
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 8e4f7aa2bd28e08d023fd98d2698c718233c80b7
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sat May 16 22:01:28 2026 +0000
fix(mcp): update security_manager patch target in RBAC tests
Moving security_manager to a module-level import in auth.py means
patch('superset.security_manager') no longer intercepts calls inside
auth.py — the name is bound at import time. Patch where it is used:
'superset.mcp_service.auth.security_manager'.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
tests/unit_tests/mcp_service/test_auth_rbac.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_auth_rbac.py b/tests/unit_tests/mcp_service/test_auth_rbac.py
index 3949203282a..64a9684eaec 100644
--- a/tests/unit_tests/mcp_service/test_auth_rbac.py
+++ b/tests/unit_tests/mcp_service/test_auth_rbac.py
@@ -122,7 +122,7 @@ def test_check_tool_permission_granted(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
- with patch("superset.security_manager", mock_sm):
+ with patch("superset.mcp_service.auth.security_manager", mock_sm):
result = check_tool_permission(func)
assert result is True
@@ -136,7 +136,7 @@ def test_check_tool_permission_denied(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=False)
- with patch("superset.security_manager", mock_sm):
+ with patch("superset.mcp_service.auth.security_manager", mock_sm):
result = check_tool_permission(func)
assert result is False
@@ -151,7 +151,7 @@ def test_check_tool_permission_default_method_is_read(app_context) -> None:
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
- with patch("superset.security_manager", mock_sm):
+ with patch("superset.mcp_service.auth.security_manager", mock_sm):
result = check_tool_permission(func)
assert result is True
(superset) 12/30: fix(mcp): harden auth — PermissionError propagation, passthrough client_id guard, fail-closed on missing token
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit ce6f4c22f6eb34d6353f9afac2d860b6af01f233
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed May 13 21:26:42 2026 +0000
fix(mcp): harden auth — PermissionError propagation, passthrough client_id guard, fail-closed on missing token
- _tool_allowed_for_current_user (server.py): catch PermissionError
alongside ValueError so invalid API keys return False instead of
propagating through the tool-search permission filter
- _setup_user_context (auth.py): catch PermissionError alongside
ValueError so g.user is cleared and the error is logged consistently
regardless of which failure type get_user_from_request() raises
- _resolve_user_from_api_key (auth.py): require client_id=="api_key"
(set by CompositeTokenVerifier) in addition to API_KEY_PASSTHROUGH_CLAIM
to prevent an external IdP JWT that happens to include the claim name
from being misclassified as an API-key pass-through (DoS vector)
- _resolve_user_from_jwt_context (auth.py): same client_id guard so
a rogue-claim JWT continues through JWT resolution instead of deferring
to the API-key path (which would raise PermissionError for the user)
- _resolve_user_from_api_key (auth.py): raise PermissionError (not
return None) when the pass-through claim is present but the raw token
is absent — fail closed rather than falling through to weaker auth
- Tests: set client_id="api_key" on _passthrough_access_token helper;
update test_jwt_context_with_api_key_passthrough_returns_none docstring;
add test for namespaced claim on non-API-key client_id being ignored
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/auth.py | 27 +++++++++++++++++---
tests/unit_tests/mcp_service/test_auth_api_key.py | 30 +++++++++++++++++++++--
2 files changed, 51 insertions(+), 6 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 94b223aec08..620763e5285 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -290,10 +290,21 @@ def _resolve_user_from_jwt_context(app: Any) -> User | 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):
- logger.debug("API key pass-through token detected, deferring to API key auth")
- return None
+ 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(
+ "Ignoring %s claim on non-API-key token (client_id=%r); processing as JWT",
+ API_KEY_PASSTHROUGH_CLAIM,
+ getattr(access_token, "client_id", None),
+ )
# Use configurable resolver or default
from superset.mcp_service.mcp_config import default_user_resolver
@@ -362,10 +373,18 @@ def _resolve_user_from_api_key(app: Any) -> User | None:
claims = getattr(access_token, "claims", None)
if not (isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM)):
return None
+ # Defense-in-depth: require client_id=="api_key" (set by CompositeTokenVerifier)
+ # to guard against rogue external IdP JWTs that include the passthrough claim.
+ if getattr(access_token, "client_id", None) != "api_key":
+ return None
api_key_string = getattr(access_token, "token", None)
if not api_key_string:
- return None
+ # Passthrough claim is set but the raw token is absent — fail closed
+ # rather than silently falling through to weaker auth sources.
+ raise PermissionError(
+ "API key pass-through token is missing the raw token value."
+ )
sm = app.appbuilder.sm
if not hasattr(sm, "validate_api_key"):
@@ -592,7 +611,7 @@ def _setup_user_context() -> User | None:
logger.error("DB connection failed on retry during user setup: %s", e)
_cleanup_session_on_error()
raise
- except ValueError as e:
+ except (ValueError, PermissionError) as e:
# User resolution failed — fail closed. Do not fall back to
# g.user from middleware, as that could allow a request to
# proceed as a different user in multi-tenant deployments.
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index ab74cb2236b..21d060441ff 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -49,6 +49,7 @@ def _passthrough_access_token(token: str) -> MagicMock:
"""Build an AccessToken matching what CompositeTokenVerifier emits."""
access_token = MagicMock()
access_token.token = token
+ access_token.client_id = "api_key"
access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True}
return access_token
@@ -265,9 +266,10 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None:
def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> None:
"""When CompositeTokenVerifier passes through an API key token,
_resolve_user_from_jwt_context should detect the namespaced
- pass-through claim and return None so get_user_from_request falls
- through to _resolve_user_from_api_key."""
+ pass-through claim AND client_id=="api_key" and return None so
+ get_user_from_request falls through to _resolve_user_from_api_key."""
mock_access_token = MagicMock()
+ mock_access_token.client_id = "api_key"
mock_access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True}
with patch(
@@ -279,6 +281,30 @@ def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) ->
assert result is None
+def test_namespaced_claim_without_api_key_client_id_is_ignored(
+ app: SupersetApp,
+) -> None:
+ """An external IdP JWT that includes the namespaced API_KEY_PASSTHROUGH_CLAIM
+ but does NOT have client_id=='api_key' must NOT divert into the API-key path.
+ The client_id guard prevents misclassification / DoS for affected JWT users."""
+ mock_sm = MagicMock()
+
+ rogue_token = MagicMock()
+ rogue_token.token = "eyJhbGciOiJSUzI1NiJ9.idp_jwt_with_rogue_claim" # noqa: S105
+ rogue_token.client_id = "some-idp-client"
+ rogue_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True, "sub": "alice"}
+
+ with _mock_sm_ctx(app, mock_sm):
+ with _patch_access_token(rogue_token):
+ # JWT path tries to resolve user "alice" from DB and raises
+ # ValueError in this isolated unit-test setup.
+ # validate_api_key must NOT be called — the rogue claim was ignored.
+ with pytest.raises(ValueError, match="not found"):
+ get_user_from_request()
+
+ mock_sm.validate_api_key.assert_not_called()
+
+
# -- Plain JWT with a colliding non-namespaced claim is NOT mistaken for API key --
(superset) 17/30: fix(mcp): normalize FAB_API_KEY_PREFIXES from config before passing to CompositeTokenVerifier
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3d817bb23be2b9641c8b402ec17c48d95c29b73c
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu May 14 19:08:34 2026 +0000
fix(mcp): normalize FAB_API_KEY_PREFIXES from config before passing to CompositeTokenVerifier
A plain string value (e.g. FAB_API_KEY_PREFIXES = "sst_") would iterate
as individual characters ['s','s','t','_'], matching far too many tokens.
Wrap strings in a list at the config-read boundary so CompositeTokenVerifier
always receives a proper sequence regardless of how the config is set.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/mcp_config.py | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index dc16b248c54..24f20504d83 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -346,7 +346,13 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
return None
if api_key_enabled:
- api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
+ 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:
+ api_key_prefixes = list(raw_prefixes)
logger.info("API key auth enabled for MCP")
return CompositeTokenVerifier(
jwt_verifier=jwt_verifier,
(superset) 10/30: fix(mcp): fix stale patch target in auth tests and update stale docstring
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 5966d689576cbf79a9c25eb10152007f84734e1e
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed May 13 06:34:46 2026 +0000
fix(mcp): fix stale patch target in auth tests and update stale docstring
- Use superset.mcp_service.auth.has_request_context as patch target in
test_mcp_auth_hook_clears_stale_g_user tests; patching flask.has_request_context
has no effect on the module-level import already bound in auth.py
- Update test_jwt_access_token_skips_api_key_auth docstring to reference
API_KEY_PASSTHROUGH_CLAIM instead of the legacy _api_key_passthrough name
- Add noqa: BLE001 to broad exception catch in mcp_config.py to document
that the wide catch is intentional (JWT libs raise many types, secrets guard)
---
superset/mcp_service/mcp_config.py | 4 ++--
tests/unit_tests/mcp_service/test_auth_api_key.py | 2 +-
tests/unit_tests/mcp_service/test_auth_user_resolution.py | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index 307045b3a8c..dc16b248c54 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -339,8 +339,8 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
public_key=public_key,
secret=secret,
)
- except Exception:
- # Do not log the exception — it may contain secrets
+ 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
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index 11717ae0edc..546f789d498 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -250,7 +250,7 @@ def test_relationship_reload_failure_returns_original_user(
@pytest.mark.usefixtures("_enable_api_keys")
def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None:
- """When the AccessToken is a plain JWT (no ``_api_key_passthrough`` claim),
+ """When the AccessToken is a plain JWT (no API_KEY_PASSTHROUGH_CLAIM),
API key auth is skipped — the JWT was already validated by the JWT
verifier and resolved in _resolve_user_from_jwt_context."""
mock_sm = MagicMock()
diff --git a/tests/unit_tests/mcp_service/test_auth_user_resolution.py b/tests/unit_tests/mcp_service/test_auth_user_resolution.py
index 34669e51d1e..9142cee8253 100644
--- a/tests/unit_tests/mcp_service/test_auth_user_resolution.py
+++ b/tests/unit_tests/mcp_service/test_auth_user_resolution.py
@@ -285,7 +285,7 @@ def test_mcp_auth_hook_clears_stale_g_user(app) -> None:
# framework's autouse app_context fixture may implicitly provide
# a request context in some CI environments.
with (
- patch("flask.has_request_context", return_value=False),
+ patch("superset.mcp_service.auth.has_request_context", return_value=False),
patch(
"superset.mcp_service.auth.get_user_from_request",
side_effect=lambda: _assert_cleared_then_return(),
@@ -324,7 +324,7 @@ def test_mcp_auth_hook_clears_stale_g_user_async(app) -> None:
with app.app_context():
g.user = stale_user
with (
- patch("flask.has_request_context", return_value=False),
+ patch("superset.mcp_service.auth.has_request_context", return_value=False),
patch(
"superset.mcp_service.auth.get_user_from_request",
side_effect=lambda: _assert_cleared_then_return(),
(superset) 19/30: fix(mcp): remove sensitive values from log calls to satisfy CodeQL
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 776fa9f1d5feb10349965c86415c0126ede88a85
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 15 00:02:31 2026 +0000
fix(mcp): remove sensitive values from log calls to satisfy CodeQL
- Drop g.user.username from the permission-denied warning
(CodeQL py/clear-text-logging-sensitive-data flags .username)
- Replace the parametrized debug log that passed API_KEY_PASSTHROUGH_CLAIM
(variable name contains KEY) with a static message
---
superset/mcp_service/auth.py | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 61361657000..68b3fd6db68 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -142,16 +142,14 @@ def check_tool_permission(func: Callable[..., Any], *, log_denial: bool = True)
if not has_permission:
if log_denial:
logger.warning(
- "Permission denied for user %s: %s on %s (tool: %s)",
- g.user.username,
+ "Permission denied: %s on %s (tool: %s)",
permission_str,
class_permission_name,
func.__name__,
)
else:
logger.debug(
- "Tool hidden for user %s: %s on %s (tool: %s)",
- g.user.username,
+ "Tool hidden: %s on %s (tool: %s)",
permission_str,
class_permission_name,
func.__name__,
@@ -280,9 +278,8 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None:
)
return None
logger.debug(
- "Ignoring %s claim on non-API-key token (client_id=%r); processing as JWT",
- API_KEY_PASSTHROUGH_CLAIM,
- getattr(access_token, "client_id", None),
+ "API key passthrough claim present but client_id is not 'api_key';"
+ " processing as JWT"
)
# Use configurable resolver or default
(superset) 26/30: fix(mcp): broaden _log_user_resolution_failure type hint
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 8165dbb0eabd67e67fc83903bfbccf65221ad74d
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 22 02:13:39 2026 +0000
fix(mcp): broaden _log_user_resolution_failure type hint
mypy correctly flags that the caller can pass either ValueError or
PermissionError; widen the type hint to match.
---
superset/mcp_service/auth.py | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index c9686c39834..a75e9a83c90 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -519,14 +519,14 @@ def check_chart_data_access(chart: Any) -> "DatasetValidationResult":
return validate_chart_dataset(chart, check_access=True)
-def _log_user_resolution_failure(exc: ValueError) -> None:
- """Log a user-resolution ValueError at the appropriate level.
+def _log_user_resolution_failure(exc: ValueError | PermissionError) -> None:
+ """Log a user-resolution failure at the appropriate level.
"No authenticated user found" is expected in unauthenticated/dev
deployments (no JWT, no API key, no MCP_DEV_USERNAME configured) and
during tools/list scanning — log at DEBUG to avoid ERROR noise.
- All other ValueErrors (e.g. dev username not in DB) are genuine
- credential failures and are logged at ERROR.
+ All other failures (e.g. dev username not in DB, permission denied) are
+ genuine credential failures and are logged at ERROR.
"""
if "No authenticated user found" in str(exc):
logger.debug("MCP: no auth source configured, unauthenticated request")
(superset) 30/30: fix(mcp): replace MCPJWTVerifier with JWTVerifier after browser-hello revert
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 60c1fd93882326a34235b1ae5c392ca59ef8638c
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed May 27 16:09:20 2026 +0000
fix(mcp): replace MCPJWTVerifier with JWTVerifier after browser-hello revert
MCPJWTVerifier was removed in the master revert of the browser-friendly
hello page (#40467). Update mcp_config.py to import only DetailedJWTVerifier
and use JWTVerifier directly for the non-debug code path, matching the
current jwt_verifier.py interface.
---
superset/mcp_service/mcp_config.py | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index 0fe43e2be6d..6e93e9f7644 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -29,7 +29,7 @@ from superset.mcp_service.constants import (
DEFAULT_TOKEN_LIMIT,
DEFAULT_WARN_THRESHOLD_PCT,
)
-from superset.mcp_service.jwt_verifier import DetailedJWTVerifier, MCPJWTVerifier
+from superset.mcp_service.jwt_verifier import DetailedJWTVerifier
logger = logging.getLogger(__name__)
@@ -404,8 +404,8 @@ def _build_jwt_verifier(
# RFC 6750 Section 3.1.
return DetailedJWTVerifier(**common_kwargs)
- # MCPJWTVerifier: minimal logging + browser-friendly error page.
- return MCPJWTVerifier(**common_kwargs)
+ # Default JWTVerifier: minimal logging, generic error responses.
+ return JWTVerifier(**common_kwargs)
def default_user_resolver(app: Any, access_token: Any) -> str | None:
(superset) 03/30: fix(mcp): add type annotations to test fixtures and parameters
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 9bc0065646ff88110637207b6533e04fcb926969
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Apr 23 20:18:44 2026 -0400
fix(mcp): add type annotations to test fixtures and parameters
Address code review feedback: add explicit type annotations
to all new test function parameters and fixture return types.
---
tests/unit_tests/mcp_service/test_auth_api_key.py | 7 ++++---
.../mcp_service/test_composite_token_verifier.py | 20 +++++++++++++-------
2 files changed, 17 insertions(+), 10 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index 7fa9b293481..b53e624e83a 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -22,6 +22,7 @@ from unittest.mock import MagicMock, patch
import pytest
from flask import g
+from superset.app import SupersetApp
from superset.mcp_service.auth import (
_resolve_user_from_jwt_context,
get_user_from_request,
@@ -229,7 +230,7 @@ def test_relationship_reload_failure_returns_original_user(app, mock_user) -> No
@pytest.mark.usefixtures("_enable_api_keys")
-def test_non_matching_bearer_token_skips_api_key_auth(app) -> None:
+def test_non_matching_bearer_token_skips_api_key_auth(app: SupersetApp) -> None:
"""When a Bearer token is present but does not match FAB_API_KEY_PREFIXES
(e.g., a JWT token), extract_api_key_from_request returns None and API key
auth is skipped, falling through to the next auth method."""
@@ -254,7 +255,7 @@ def test_non_matching_bearer_token_skips_api_key_auth(app) -> None:
# -- API key pass-through from CompositeTokenVerifier --
-def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None:
+def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> None:
"""When CompositeTokenVerifier passes through an API key token,
_resolve_user_from_jwt_context should detect the _api_key_passthrough
claim and return None so get_user_from_request falls through to
@@ -274,7 +275,7 @@ def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None:
# -- SecurityManager method name regression test --
-def test_security_manager_has_expected_api_key_methods(app) -> None:
+def test_security_manager_has_expected_api_key_methods(app: SupersetApp) -> None:
"""Regression test: verify the SecurityManager method names referenced in
auth._resolve_user_from_api_key() actually exist on the FAB SecurityManager
class. This catches future renames before they silently break API key auth
diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
index 537fe4d8f07..8f496305a1c 100644
--- a/tests/unit_tests/mcp_service/test_composite_token_verifier.py
+++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
@@ -26,7 +26,7 @@ from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier
@pytest.fixture
-def mock_jwt_verifier():
+def mock_jwt_verifier() -> MagicMock:
verifier = MagicMock()
verifier.required_scopes = []
verifier.verify_token = AsyncMock()
@@ -34,7 +34,7 @@ def mock_jwt_verifier():
@pytest.fixture
-def composite_verifier(mock_jwt_verifier):
+def composite_verifier(mock_jwt_verifier: MagicMock) -> CompositeTokenVerifier:
return CompositeTokenVerifier(
jwt_verifier=mock_jwt_verifier,
api_key_prefixes=["sst_", "pat_"],
@@ -42,7 +42,9 @@ def composite_verifier(mock_jwt_verifier):
@pytest.mark.asyncio
-async def test_api_key_token_returns_passthrough(composite_verifier) -> None:
+async def test_api_key_token_returns_passthrough(
+ composite_verifier: CompositeTokenVerifier,
+) -> None:
"""Tokens matching an API key prefix return a pass-through AccessToken."""
api_key = "sst_abc123secret" # noqa: S105
result = await composite_verifier.verify_token(api_key)
@@ -54,7 +56,9 @@ async def test_api_key_token_returns_passthrough(composite_verifier) -> None:
@pytest.mark.asyncio
-async def test_second_prefix_matches(composite_verifier) -> None:
+async def test_second_prefix_matches(
+ composite_verifier: CompositeTokenVerifier,
+) -> None:
"""All configured prefixes are checked, not just the first."""
result = await composite_verifier.verify_token("pat_mytoken")
@@ -64,7 +68,7 @@ async def test_second_prefix_matches(composite_verifier) -> None:
@pytest.mark.asyncio
async def test_jwt_token_delegates_to_wrapped_verifier(
- composite_verifier, mock_jwt_verifier
+ composite_verifier: CompositeTokenVerifier, mock_jwt_verifier: MagicMock
) -> None:
"""Non-API-key tokens are delegated to the wrapped JWT verifier."""
jwt_token = "eyJhbGciOiJSUzI1NiJ9.jwt_payload" # noqa: S105
@@ -85,7 +89,9 @@ async def test_jwt_token_delegates_to_wrapped_verifier(
@pytest.mark.asyncio
-async def test_invalid_jwt_returns_none(composite_verifier, mock_jwt_verifier) -> None:
+async def test_invalid_jwt_returns_none(
+ composite_verifier: CompositeTokenVerifier, mock_jwt_verifier: MagicMock
+) -> None:
"""When the JWT verifier rejects a token, None is returned."""
mock_jwt_verifier.verify_token.return_value = None
@@ -97,7 +103,7 @@ async def test_invalid_jwt_returns_none(composite_verifier, mock_jwt_verifier) -
@pytest.mark.asyncio
async def test_api_key_does_not_call_jwt_verifier(
- composite_verifier, mock_jwt_verifier
+ composite_verifier: CompositeTokenVerifier, mock_jwt_verifier: MagicMock
) -> None:
"""API key tokens bypass the JWT verifier entirely."""
await composite_verifier.verify_token("sst_test_key")
(superset) 02/30: fix(mcp): wire composite verifier and add ApiKey permission sync
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 34c76e54598f6720929bacd15588b74cceac9b0e
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Apr 23 19:52:17 2026 -0400
fix(mcp): wire composite verifier and add ApiKey permission sync
Wire CompositeTokenVerifier into create_default_mcp_auth_factory,
add _api_key_passthrough detection in _resolve_user_from_jwt_context,
create ApiKey permissions in create_custom_permissions, and update
test_auth_api_key with pass-through and non-matching prefix tests.
---
superset/mcp_service/auth.py | 8 +++
superset/mcp_service/mcp_config.py | 15 +++++
superset/security/manager.py | 9 +++
tests/unit_tests/mcp_service/test_auth_api_key.py | 82 ++++++++++++++++++-----
4 files changed, 98 insertions(+), 16 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 69d8e625531..5320821d88a 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -285,6 +285,14 @@ 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).
+ claims = getattr(access_token, "claims", None)
+ if isinstance(claims, dict) and claims.get("_api_key_passthrough"):
+ logger.debug("API key pass-through token detected, deferring to API key auth")
+ return None
+
# Use configurable resolver or default
from superset.mcp_service.mcp_config import default_user_resolver
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index d12b44bbc87..d38a5d0eca3 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -348,6 +348,21 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
auth_provider = JWTVerifier(**common_kwargs)
+ # Wrap with CompositeTokenVerifier when API key auth is enabled
+ # so that API key tokens (e.g. sst_...) pass through the transport
+ # layer instead of being rejected by the JWT verifier.
+ if app.config.get("FAB_API_KEY_ENABLED", False):
+ from superset.mcp_service.composite_token_verifier import (
+ CompositeTokenVerifier,
+ )
+
+ api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
+ auth_provider = CompositeTokenVerifier(
+ jwt_verifier=auth_provider,
+ api_key_prefixes=api_key_prefixes,
+ )
+ logger.info("API key auth enabled for MCP (prefixes: %s)", api_key_prefixes)
+
return auth_provider
except Exception:
# Do not log the exception — it may contain the HS256 secret
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 9a5055d43fe..44c23f86597 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1422,6 +1422,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
self.add_permission_view_menu("can_tag", "Chart")
self.add_permission_view_menu("can_tag", "Dashboard")
+ # API Key permissions (FAB's ApiKeyApi blueprint).
+ # Superset uses AppBuilder(update_perms=False) so FAB skips
+ # permission creation during blueprint registration. Create them
+ # explicitly here so that ``superset init`` picks them up and
+ # sync_role_definitions assigns them to the Admin role.
+ if current_app.config.get("FAB_API_KEY_ENABLED", False):
+ for perm in ("can_list", "can_create", "can_get", "can_delete"):
+ self.add_permission_view_menu(perm, "ApiKey")
+
def create_missing_perms(self) -> None:
"""
Creates missing FAB permissions for datasources, schemas and metrics.
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index 6a0bcab6719..7fa9b293481 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -22,7 +22,10 @@ from unittest.mock import MagicMock, patch
import pytest
from flask import g
-from superset.mcp_service.auth import get_user_from_request
+from superset.mcp_service.auth import (
+ _resolve_user_from_jwt_context,
+ get_user_from_request,
+)
@pytest.fixture
@@ -222,25 +225,72 @@ def test_relationship_reload_failure_returns_original_user(app, mock_user) -> No
assert result is mock_user
+# -- Bearer token present but not matching API key prefix --
+
+
+@pytest.mark.usefixtures("_enable_api_keys")
+def test_non_matching_bearer_token_skips_api_key_auth(app) -> None:
+ """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES
+ (e.g., a JWT token), extract_api_key_from_request returns None and API key
+ auth is skipped, falling through to the next auth method."""
+ mock_sm = MagicMock()
+ mock_sm.extract_api_key_from_request.return_value = None
+
+ with app.test_request_context(
+ headers={"Authorization": "Bearer eyJhbGciOiJIUzI1NiJ9.not-an-api-key"}
+ ):
+ g.user = None
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm = mock_sm
+
+ with pytest.raises(ValueError, match="No authenticated user found"):
+ get_user_from_request()
+
+ # extract was called but returned None, so validate should NOT be called
+ mock_sm.extract_api_key_from_request.assert_called_once()
+ mock_sm.validate_api_key.assert_not_called()
+
+
+# -- API key pass-through from CompositeTokenVerifier --
+
+
+def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None:
+ """When CompositeTokenVerifier passes through an API key token,
+ _resolve_user_from_jwt_context should detect the _api_key_passthrough
+ claim and return None so get_user_from_request falls through to
+ _resolve_user_from_api_key."""
+ mock_access_token = MagicMock()
+ mock_access_token.claims = {"_api_key_passthrough": True}
+
+ with patch(
+ "fastmcp.server.dependencies.get_access_token",
+ return_value=mock_access_token,
+ ):
+ result = _resolve_user_from_jwt_context(app)
+
+ assert result is None
+
+
# -- SecurityManager method name regression test --
-def test_security_manager_has_expected_api_key_methods() -> None:
+def test_security_manager_has_expected_api_key_methods(app) -> None:
"""Regression test: verify the SecurityManager method names referenced in
auth._resolve_user_from_api_key() actually exist on the FAB SecurityManager
class. This catches future renames before they silently break API key auth
- at runtime (SC-99414: _extract_api_key_from_request vs
+ at runtime (see PR #39437: _extract_api_key_from_request vs
extract_api_key_from_request)."""
- from superset import security_manager
-
- sm = security_manager
- assert hasattr(sm, "extract_api_key_from_request"), (
- "FAB SecurityManager is missing 'extract_api_key_from_request'. "
- "auth._resolve_user_from_api_key() references this method by name — "
- "update auth.py if the FAB API changed."
- )
- assert hasattr(sm, "validate_api_key"), (
- "FAB SecurityManager is missing 'validate_api_key'. "
- "auth._resolve_user_from_api_key() references this method by name — "
- "update auth.py if the FAB API changed."
- )
+ with app.app_context():
+ from superset import security_manager
+
+ sm = security_manager
+ assert hasattr(sm, "extract_api_key_from_request"), (
+ "FAB SecurityManager is missing 'extract_api_key_from_request'. "
+ "auth._resolve_user_from_api_key() references this method by name — "
+ "update auth.py if the FAB API changed."
+ )
+ assert hasattr(sm, "validate_api_key"), (
+ "FAB SecurityManager is missing 'validate_api_key'. "
+ "auth._resolve_user_from_api_key() references this method by name — "
+ "update auth.py if the FAB API changed."
+ )
(superset) 14/30: fix(mcp): fix stale patch target in auth tests and update stale docstring
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6b0fda96bf48d9e502852ab3352fd1f4aad863a4
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed May 13 23:04:02 2026 +0000
fix(mcp): fix stale patch target in auth tests and update stale docstring
_mock_sm_ctx now sets find_user_with_relationships.return_value = None so
JWT/dev-user lookups that delegate through the (now refactored)
load_user_with_relationships → security_manager.find_user_with_relationships
path behave as "user not found" in unit tests that don't patch the DB — matching
the behavior of the previous direct db.session.query() implementation.
Without this, tests that expected ValueError("not found") received a truthy
MagicMock() from the unspecified mock method, causing them to fail.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
tests/unit_tests/mcp_service/test_auth_api_key.py | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index 961d41fbbc6..e4279ffcd9d 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -86,7 +86,14 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]:
@contextmanager
def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock):
- """Push an app context with g.user cleared and appbuilder.sm mocked."""
+ """Push an app context with g.user cleared and appbuilder.sm mocked.
+
+ Defaults find_user_with_relationships to None so JWT/dev-user lookups
+ that hit the SM (via load_user_with_relationships) behave as "user not
+ found" without a real DB, matching the pre-refactor db.session behavior.
+ Tests that need a specific return value should set it on mock_sm directly.
+ """
+ mock_sm.find_user_with_relationships.return_value = None
with app.app_context():
g.user = None
app.appbuilder = MagicMock()
(superset) 22/30: fix(mcp): remove sensitive values from log calls to satisfy CodeQL
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6e205dea13370388a882bb9c4956a798f950170a
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sat May 16 21:19:22 2026 +0000
fix(mcp): remove sensitive values from log calls to satisfy CodeQL
Replace user.username and email values in logger calls with non-PII
identifiers (user id integer) or remove the value entirely, so CodeQL
py/clear-text-logging-sensitive-data does not flag them.
---
superset/mcp_service/auth.py | 4 ++--
superset/security/manager.py | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 8fa6795acfc..de6227a1e5b 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -383,10 +383,10 @@ def _resolve_user_from_api_key(app: Any) -> User | None:
user_with_rels = load_user_with_relationships(username=user.username)
if user_with_rels is None:
logger.warning(
- "Failed to reload API key user %s with relationships; "
+ "Failed to reload API key user id=%s with relationships; "
"using original user object which may have lazy-loaded "
"relationships",
- user.username,
+ getattr(user, "id", "?"),
)
return user
return user_with_rels
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 6a7c8ce2f05..67d1bcfd53c 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -3206,7 +3206,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
.one_or_none()
)
except MultipleResultsFound:
- logger.error("Multiple results found for user %s", username)
+ logger.error("Multiple results found for username lookup")
return None
if email:
try:
@@ -3217,7 +3217,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
.one_or_none()
)
except MultipleResultsFound:
- logger.error("Multiple results found for user with email %s", email)
+ logger.error("Multiple results found for email lookup")
return None
return None
(superset) 01/30: fix(mcp): create ApiKey permissions on init and support API keys with JWT auth
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 20ab9277ab3601dc4b73a3eda441a0d893c013ef
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Apr 23 19:51:51 2026 -0400
fix(mcp): create ApiKey permissions on init and support API keys with JWT auth
Two fixes for MCP API key authentication:
1. superset init now creates ApiKey FAB permissions (can_list, can_create,
can_get, can_delete) when FAB_API_KEY_ENABLED=True. Previously, because
Superset uses AppBuilder(update_perms=False), FAB skipped permission
creation during blueprint registration and superset init never picked
them up, causing 403 errors on /api/v1/security/api_keys/.
2. CompositeTokenVerifier allows API key tokens (e.g. sst_...) to coexist
with JWT auth on the MCP transport layer. Previously, when
MCP_AUTH_ENABLED=True, the JWTVerifier rejected all non-JWT Bearer
tokens at the transport layer before they could reach the Flask-level
_resolve_user_from_api_key() handler. The composite verifier detects
API key prefixes and passes them through with a marker claim, letting
the existing auth priority chain handle validation.
---
superset/mcp_service/composite_token_verifier.py | 80 ++++++++++++++++
.../mcp_service/test_composite_token_verifier.py | 105 +++++++++++++++++++++
2 files changed, 185 insertions(+)
diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py
new file mode 100644
index 00000000000..e4f5c6a5c43
--- /dev/null
+++ b/superset/mcp_service/composite_token_verifier.py
@@ -0,0 +1,80 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Composite token verifier for MCP authentication.
+
+Routes Bearer tokens to the appropriate verifier based on prefix:
+- Tokens matching FAB_API_KEY_PREFIXES (e.g. ``sst_``) are passed through
+ to the Flask layer where ``_resolve_user_from_api_key()`` handles
+ actual validation via FAB SecurityManager.
+- All other tokens are delegated to the wrapped JWT verifier.
+"""
+
+import logging
+
+from fastmcp.server.auth import AccessToken
+from fastmcp.server.auth.providers.jwt import TokenVerifier
+
+logger = logging.getLogger(__name__)
+
+
+class CompositeTokenVerifier(TokenVerifier):
+ """Routes Bearer tokens between API key pass-through and JWT verification.
+
+ API key tokens (identified by prefix) are accepted at the transport layer
+ with a marker claim so that ``_resolve_user_from_jwt_context()`` can
+ detect them and fall through to ``_resolve_user_from_api_key()`` for
+ actual validation.
+
+ Args:
+ jwt_verifier: The wrapped JWT verifier for non-API-key tokens.
+ api_key_prefixes: List of prefixes that identify API key tokens
+ (e.g. ``["sst_"]``).
+ """
+
+ def __init__(
+ self,
+ jwt_verifier: TokenVerifier,
+ api_key_prefixes: list[str],
+ ) -> None:
+ super().__init__(
+ base_url=getattr(jwt_verifier, "base_url", None),
+ required_scopes=jwt_verifier.required_scopes,
+ )
+ self._jwt_verifier = jwt_verifier
+ self._api_key_prefixes = tuple(api_key_prefixes)
+
+ async def verify_token(self, token: str) -> AccessToken | None:
+ """Verify a Bearer token.
+
+ If the token starts with an API key prefix, return a pass-through
+ AccessToken with a ``_api_key_passthrough`` claim. The Flask-layer
+ ``_resolve_user_from_api_key()`` performs the real validation.
+
+ Otherwise, delegate to the wrapped JWT verifier.
+ """
+ if any(token.startswith(prefix) for prefix in self._api_key_prefixes):
+ logger.debug("API key token detected (prefix match), passing through")
+ return AccessToken(
+ token=token,
+ client_id="api_key",
+ scopes=[],
+ claims={"_api_key_passthrough": True},
+ )
+
+ return await self._jwt_verifier.verify_token(token)
diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
new file mode 100644
index 00000000000..537fe4d8f07
--- /dev/null
+++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
@@ -0,0 +1,105 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for CompositeTokenVerifier."""
+
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from fastmcp.server.auth import AccessToken
+
+from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier
+
+
+@pytest.fixture
+def mock_jwt_verifier():
+ verifier = MagicMock()
+ verifier.required_scopes = []
+ verifier.verify_token = AsyncMock()
+ return verifier
+
+
+@pytest.fixture
+def composite_verifier(mock_jwt_verifier):
+ return CompositeTokenVerifier(
+ jwt_verifier=mock_jwt_verifier,
+ api_key_prefixes=["sst_", "pat_"],
+ )
+
+
+@pytest.mark.asyncio
+async def test_api_key_token_returns_passthrough(composite_verifier) -> None:
+ """Tokens matching an API key prefix return a pass-through AccessToken."""
+ api_key = "sst_abc123secret" # noqa: S105
+ result = await composite_verifier.verify_token(api_key)
+
+ assert result is not None
+ assert result.token == api_key
+ assert result.client_id == "api_key"
+ assert result.claims.get("_api_key_passthrough") is True
+
+
+@pytest.mark.asyncio
+async def test_second_prefix_matches(composite_verifier) -> None:
+ """All configured prefixes are checked, not just the first."""
+ result = await composite_verifier.verify_token("pat_mytoken")
+
+ assert result is not None
+ assert result.claims.get("_api_key_passthrough") is True
+
+
+@pytest.mark.asyncio
+async def test_jwt_token_delegates_to_wrapped_verifier(
+ composite_verifier, mock_jwt_verifier
+) -> None:
+ """Non-API-key tokens are delegated to the wrapped JWT verifier."""
+ jwt_token = "eyJhbGciOiJSUzI1NiJ9.jwt_payload" # noqa: S105
+ jwt_result = AccessToken(
+ token=jwt_token,
+ client_id="oauth_client",
+ scopes=["read"],
+ claims={"sub": "user1"},
+ )
+ mock_jwt_verifier.verify_token.return_value = jwt_result
+
+ result = await composite_verifier.verify_token("eyJhbGciOiJSUzI1NiJ9.jwt_payload")
+
+ assert result is jwt_result
+ mock_jwt_verifier.verify_token.assert_awaited_once_with(
+ "eyJhbGciOiJSUzI1NiJ9.jwt_payload"
+ )
+
+
+@pytest.mark.asyncio
+async def test_invalid_jwt_returns_none(composite_verifier, mock_jwt_verifier) -> None:
+ """When the JWT verifier rejects a token, None is returned."""
+ mock_jwt_verifier.verify_token.return_value = None
+
+ result = await composite_verifier.verify_token("not_a_valid_token")
+
+ assert result is None
+ mock_jwt_verifier.verify_token.assert_awaited_once()
+
+
+@pytest.mark.asyncio
+async def test_api_key_does_not_call_jwt_verifier(
+ composite_verifier, mock_jwt_verifier
+) -> None:
+ """API key tokens bypass the JWT verifier entirely."""
+ await composite_verifier.verify_token("sst_test_key")
+
+ mock_jwt_verifier.verify_token.assert_not_awaited()
(superset) 25/30: fix(mcp): update security_manager patch target in tool-search tests
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 29c5c4664554ba7b98231acdf6450c6770b4e678
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sat May 16 23:27:54 2026 +0000
fix(mcp): update security_manager patch target in tool-search tests
_tool_allowed_for_current_user now delegates to check_tool_permission
(auth.py) which uses the module-level security_manager binding.
patch('superset.security_manager') no longer intercepts those calls;
update the 4 affected tests to patch the correct location:
'superset.mcp_service.auth.security_manager'.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
tests/unit_tests/mcp_service/test_tool_search_transform.py | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_tool_search_transform.py b/tests/unit_tests/mcp_service/test_tool_search_transform.py
index b8f301c9692..5042af23e3e 100644
--- a/tests/unit_tests/mcp_service/test_tool_search_transform.py
+++ b/tests/unit_tests/mcp_service/test_tool_search_transform.py
@@ -869,7 +869,7 @@ def test_tool_search_permission_filter_hides_disallowed_tools():
with app.app_context():
g.user = SimpleNamespace(username="viewer")
with patch(
- "superset.security_manager", new_callable=MagicMock
+ "superset.mcp_service.auth.security_manager", new_callable=MagicMock
) as security_manager:
security_manager.can_access.side_effect = [True, False]
@@ -970,7 +970,9 @@ def test_tool_search_permission_filter_still_applies_rbac_to_metadata_tools() ->
"superset.mcp_service.privacy.user_can_view_data_model_metadata",
return_value=True,
),
- patch("superset.security_manager", new_callable=Mock) as security_manager,
+ patch(
+ "superset.mcp_service.auth.security_manager", new_callable=Mock
+ ) as security_manager,
):
security_manager.can_access.return_value = False
result = _filter_tools_by_current_user_permission([metadata, public])
@@ -997,7 +999,9 @@ def test_tool_search_permission_filter_resolves_user_from_request() -> None:
"superset.mcp_service.auth.get_user_from_request",
return_value=SimpleNamespace(username="viewer"),
),
- patch("superset.security_manager", new_callable=Mock) as security_manager,
+ patch(
+ "superset.mcp_service.auth.security_manager", new_callable=Mock
+ ) as security_manager,
):
security_manager.can_access.return_value = True
result = _filter_tools_by_current_user_permission([protected])
@@ -1023,7 +1027,9 @@ def test_tool_search_permission_filter_keeps_get_schema_visible_without_metadata
"superset.mcp_service.privacy.user_can_view_data_model_metadata",
return_value=False,
),
- patch("superset.security_manager", new_callable=Mock) as security_manager,
+ patch(
+ "superset.mcp_service.auth.security_manager", new_callable=Mock
+ ) as security_manager,
):
security_manager.can_access.return_value = True
result = _filter_tools_by_current_user_permission([schema_tool])
(superset) 04/30: fix(mcp): remove prefixes from log to satisfy CodeQL
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6e362f95039929cf20d1f5f14bf4c013f1f0c3ec
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Apr 23 20:29:08 2026 -0400
fix(mcp): remove prefixes from log to satisfy CodeQL
Remove API key prefixes from log message to avoid CodeQL
false positive about clear-text logging of sensitive data.
---
superset/mcp_service/mcp_config.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index d38a5d0eca3..a0d9ae89336 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -361,7 +361,7 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
jwt_verifier=auth_provider,
api_key_prefixes=api_key_prefixes,
)
- logger.info("API key auth enabled for MCP (prefixes: %s)", api_key_prefixes)
+ logger.info("API key auth enabled for MCP")
return auth_provider
except Exception:
(superset) 11/30: refactor(mcp): extract duplicated app context + sm setup into helper
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3b622e8bf2b490d61d41714f0bdc734b015e2575
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed May 13 19:57:41 2026 +0000
refactor(mcp): extract duplicated app context + sm setup into helper
Add _mock_sm_ctx() context manager to eliminate repeated boilerplate
(g.user = None / app.appbuilder = MagicMock() / appbuilder.sm = mock_sm)
across seven API key auth unit tests.
---
tests/unit_tests/mcp_service/test_auth_api_key.py | 59 ++++++++---------------
1 file changed, 19 insertions(+), 40 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index 546f789d498..ab74cb2236b 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -24,6 +24,7 @@ The streamable-http transport does not push a Flask request context, so
"""
from collections.abc import Generator
+from contextlib import contextmanager
from unittest.mock import MagicMock, patch
import pytest
@@ -82,6 +83,16 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]:
app.config["MCP_DEV_USERNAME"] = old_dev
+@contextmanager
+def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock):
+ """Push an app context with g.user cleared and appbuilder.sm mocked."""
+ with app.app_context():
+ g.user = None
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm = mock_sm
+ yield
+
+
# -- Valid API key -> user loaded --
@@ -91,11 +102,7 @@ def test_valid_api_key_returns_user(app: SupersetApp, mock_user: MagicMock) -> N
mock_sm = MagicMock()
mock_sm.validate_api_key.return_value = mock_user
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with (
_patch_access_token(_passthrough_access_token("sst_abc123")),
patch(
@@ -124,11 +131,7 @@ def test_invalid_api_key_raises(app: SupersetApp) -> None:
# mask the rejection.
app.config["MCP_DEV_USERNAME"] = "admin"
try:
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with _patch_access_token(_passthrough_access_token("sst_bad_key")):
with pytest.raises(PermissionError, match="Invalid or expired API key"):
get_user_from_request()
@@ -145,11 +148,7 @@ def test_api_key_disabled_skips_auth(app: SupersetApp) -> None:
even if an AccessToken is present."""
mock_sm = MagicMock()
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with _patch_access_token(_passthrough_access_token("sst_abc123")):
with pytest.raises(ValueError, match="No authenticated user found"):
get_user_from_request()
@@ -166,11 +165,7 @@ def test_no_access_token_skips_api_key_auth(app: SupersetApp) -> None:
auth provider installed), API key auth is skipped."""
mock_sm = MagicMock()
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with _patch_access_token(None):
with pytest.raises(ValueError, match="No authenticated user found"):
get_user_from_request()
@@ -204,11 +199,7 @@ def test_fab_without_validate_method_raises(app: SupersetApp) -> None:
PermissionError about unavailable validation."""
mock_sm = MagicMock(spec=[]) # empty spec = no attributes
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with _patch_access_token(_passthrough_access_token("sst_abc123")):
with pytest.raises(
PermissionError, match="API key validation is not available"
@@ -228,11 +219,7 @@ def test_relationship_reload_failure_returns_original_user(
mock_sm = MagicMock()
mock_sm.validate_api_key.return_value = mock_user
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with (
_patch_access_token(_passthrough_access_token("sst_abc123")),
patch(
@@ -259,11 +246,7 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None:
jwt_access_token.token = "eyJhbGciOiJIUzI1NiJ9.not-an-api-key" # noqa: S105
jwt_access_token.claims = {"sub": "alice"}
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with _patch_access_token(jwt_access_token):
# _resolve_user_from_jwt_context will try to resolve the user
# from the JWT claims and (in this isolated unit-test setup)
@@ -313,11 +296,7 @@ def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path(
rogue_token.token = "eyJhbGciOiJSUzI1NiJ9.rogue_jwt" # noqa: S105
rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"}
- with app.app_context():
- g.user = None
- app.appbuilder = MagicMock()
- app.appbuilder.sm = mock_sm
-
+ with _mock_sm_ctx(app, mock_sm):
with _patch_access_token(rogue_token):
# JWT path tries to resolve user "alice" from DB and (in this
# isolated unit-test setup) raises ValueError. The assertion
(superset) 07/30: fix(security): drop redundant explicit ApiKey perm creation
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 90969bff3224df6365dcb806618358f8b28a2245
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 8 14:47:58 2026 -0400
fix(security): drop redundant explicit ApiKey perm creation
``superset init`` calls ``appbuilder.add_permissions(update_perms=True)``
before ``sync_role_definitions()`` (cli/main.py:84), which forces FAB to
walk all registered baseviews — including ``ApiKeyApi`` (registered when
``FAB_API_KEY_ENABLED=True``) — and create their PVMs via
``add_permissions_view``. The explicit ``add_permission_view_menu`` calls
in ``create_custom_permissions`` were redundant.
With ``"ApiKey"`` already in ``ADMIN_ONLY_VIEW_MENUS``, the role
predicate ``_is_admin_only`` gates the auto-created PVMs to Admin.
Per Daniel Gaspar's review: "Adding ApiKey to ADMIN_ONLY_VIEW_MENUS
should just work when FAB_API_KEY_ENABLED is True".
---
superset/security/manager.py | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 19a8cf8c522..5da6e9ced97 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1426,15 +1426,6 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
self.add_permission_view_menu("can_tag", "Chart")
self.add_permission_view_menu("can_tag", "Dashboard")
- # API Key permissions (FAB's ApiKeyApi blueprint).
- # Superset uses AppBuilder(update_perms=False) so FAB skips
- # permission creation during blueprint registration. Create them
- # explicitly here so that ``superset init`` picks them up and
- # sync_role_definitions assigns them to the Admin role.
- if current_app.config.get("FAB_API_KEY_ENABLED", False):
- for perm in ("can_list", "can_create", "can_get", "can_delete"):
- self.add_permission_view_menu(perm, "ApiKey")
-
def create_missing_perms(self) -> None:
"""
Creates missing FAB permissions for datasources, schemas and metrics.
(superset) 29/30: fix(mcp): use consistent filter() style for email lookup in find_user_with_relationships
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit a9459e87716510f72d0b15b43dfe90aa0c416f86
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Tue May 26 17:27:58 2026 +0000
fix(mcp): use consistent filter() style for email lookup in find_user_with_relationships
Use `.filter(self.user_model.email == email)` to match the explicit
column-attribute style used for username lookups in the same method.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/security/manager.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 10fb388a359..7e4938c2c31 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -3217,7 +3217,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
return (
self.session.query(self.user_model)
.options(*eager)
- .filter_by(email=email)
+ .filter(self.user_model.email == email)
.one_or_none()
)
except MultipleResultsFound:
(superset) 28/30: fix(mcp): remove exc_info=True from tool-visibility debug log to prevent traceback-based credential leak
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 51b2057923024a1d50dbbb7333c40665472190dc
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 22 18:14:36 2026 +0000
fix(mcp): remove exc_info=True from tool-visibility debug log to prevent traceback-based credential leak
CodeQL py/clear-text-logging-sensitive-data (alert #2283) flagged this path
because exc_info=True includes the full exception traceback, which can expose
sensitive local variables (tokens, API keys) from frames on the call stack.
The message alone is sufficient for debugging.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/auth.py | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index a75e9a83c90..ff8abf53729 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -211,9 +211,7 @@ def is_tool_visible_to_current_user(tool: Any) -> bool:
return check_tool_permission(tool_func, log_denial=False)
except (AttributeError, RuntimeError, ValueError):
- logger.debug(
- "Could not evaluate tool visibility for current user", exc_info=True
- )
+ logger.debug("Could not evaluate tool visibility for current user")
return False
(superset) 21/30: fix(mcp): address dpgaspar review — imports, types, exception scope
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit dc3ce1eec8b76dfae01380d71cf5cc5b4aa36786
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sat May 16 21:15:58 2026 +0000
fix(mcp): address dpgaspar review — imports, types, exception scope
- auth.py: hoist security_manager, current_app, and default_user_resolver
to module level; restore user id (not username) in permission-denied log
so security audits remain useful without triggering CodeQL
- mcp_config.py: narrow _build_jwt_verifier return type to JWTVerifier;
replace bare except Exception with except (ValueError, JoseError) per
authlib's exception hierarchy; annotate raw_prefixes as str|Sequence[str]
to satisfy strict mypy; import JoseError and Sequence at module level
- security/manager.py: move `func as sa_func` import to module level
---
superset/mcp_service/auth.py | 20 ++++++--------------
superset/mcp_service/mcp_config.py | 13 ++++++++-----
superset/security/manager.py | 4 +---
3 files changed, 15 insertions(+), 22 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 68b3fd6db68..8fa6795acfc 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -51,7 +51,9 @@ from typing import Any, Callable, TYPE_CHECKING, TypeVar
from flask import current_app, g, has_app_context, has_request_context
from flask_appbuilder.security.sqla.models import User
+from superset import security_manager
from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM
+from superset.mcp_service.mcp_config import default_user_resolver
if TYPE_CHECKING:
from superset.connectors.sqla.models import SqlaTable
@@ -109,13 +111,9 @@ def check_tool_permission(func: Callable[..., Any], *, log_denial: bool = True)
True if user has permission or no permission is required.
"""
try:
- from flask import current_app
-
if not current_app.config.get("MCP_RBAC_ENABLED", True):
return True
- from superset import security_manager
-
if not hasattr(g, "user") or not g.user:
if log_denial:
logger.warning(
@@ -142,14 +140,16 @@ def check_tool_permission(func: Callable[..., Any], *, log_denial: bool = True)
if not has_permission:
if log_denial:
logger.warning(
- "Permission denied: %s on %s (tool: %s)",
+ "Permission denied for user id=%s: %s on %s (tool: %s)",
+ getattr(g.user, "id", "?"),
permission_str,
class_permission_name,
func.__name__,
)
else:
logger.debug(
- "Tool hidden: %s on %s (tool: %s)",
+ "Tool hidden for user id=%s: %s on %s (tool: %s)",
+ getattr(g.user, "id", "?"),
permission_str,
class_permission_name,
func.__name__,
@@ -230,8 +230,6 @@ def load_user_with_relationships(
if not username and not email:
raise ValueError("Either username or email must be provided")
- from superset import security_manager
-
return security_manager.find_user_with_relationships(username=username, email=email)
@@ -283,8 +281,6 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None:
)
# Use configurable resolver or default
- from superset.mcp_service.mcp_config import default_user_resolver
-
resolver = app.config.get("MCP_USER_RESOLVER", default_user_resolver)
username = resolver(app, access_token)
@@ -417,8 +413,6 @@ def get_user_from_request() -> User:
Raises:
ValueError: If user cannot be authenticated or found
"""
- from flask import current_app
-
# Priority 1: JWT context (per-request safe via ContextVar)
if (jwt_user := _resolve_user_from_jwt_context(current_app)) is not None:
return jwt_user
@@ -484,8 +478,6 @@ def has_dataset_access(dataset: "SqlaTable") -> bool:
Returns False on any error to fail securely.
"""
try:
- from superset import security_manager
-
# Check if user has read access to the dataset
if hasattr(g, "user") and g.user:
# Use Superset's security manager to check dataset access
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index 24f20504d83..0c312cc420c 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -18,8 +18,9 @@
import logging
import secrets
-from typing import Any, Dict, Optional
+from typing import Any, Dict, Optional, Sequence
+from authlib.jose.errors import JoseError
from fastmcp.server.auth.providers.jwt import JWTVerifier
from flask import Flask
@@ -339,18 +340,20 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
public_key=public_key,
secret=secret,
)
- except Exception: # noqa: BLE001 — JWT lib raises many types; broad catch intentional
+ 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:
- raw_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
+ 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.
if isinstance(raw_prefixes, str):
- api_key_prefixes = [raw_prefixes]
+ api_key_prefixes: list[str] = [raw_prefixes]
else:
api_key_prefixes = list(raw_prefixes)
logger.info("API key auth enabled for MCP")
@@ -367,7 +370,7 @@ def _build_jwt_verifier(
jwks_uri: Optional[str],
public_key: Optional[str],
secret: Optional[str],
-) -> Any:
+) -> JWTVerifier:
"""Construct the JWT verifier from configured keys/secret."""
debug_errors = app.config.get("MCP_JWT_DEBUG_ERRORS", False)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index adb452a9d3b..6a7c8ce2f05 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -50,7 +50,7 @@ from flask_appbuilder.security.views import (
from flask_babel import lazy_gettext as _
from flask_login import AnonymousUserMixin, LoginManager
from jwt.api_jwt import _jwt_global_obj
-from sqlalchemy import and_, inspect, or_
+from sqlalchemy import and_, func as sa_func, inspect, or_
from sqlalchemy.engine.base import Connection
from sqlalchemy.orm import eagerload, joinedload
from sqlalchemy.orm.exc import MultipleResultsFound
@@ -3190,8 +3190,6 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
if username:
try:
if self.auth_username_ci:
- from sqlalchemy import func as sa_func
-
return (
self.session.query(self.user_model)
.options(*eager)
(superset) 16/30: fix(mcp): fix stale patch target in auth tests and update stale docstring
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit dc17712916bf319ca4e5f33dc12b2d8df02218a9
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu May 14 19:05:42 2026 +0000
fix(mcp): fix stale patch target in auth tests and update stale docstring
- Remove mock_sm.find_user_with_relationships.return_value = None from
_mock_sm_ctx: load_user_with_relationships delegates to the global
security_manager (not app.appbuilder.sm), so setting it on mock_sm had
no effect and broke MagicMock(spec=[]) tests.
- Add _patch_load_user_not_found() helper that patches
superset.mcp_service.auth.load_user_with_relationships directly.
- Apply it to the 3 JWT-path tests that expect ValueError("not found"):
test_jwt_access_token_skips_api_key_auth,
test_namespaced_claim_without_api_key_client_id_is_ignored,
test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
tests/unit_tests/mcp_service/test_auth_api_key.py | 43 ++++++++++++-----------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index e4279ffcd9d..afe5a53ac5b 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -86,14 +86,7 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]:
@contextmanager
def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock):
- """Push an app context with g.user cleared and appbuilder.sm mocked.
-
- Defaults find_user_with_relationships to None so JWT/dev-user lookups
- that hit the SM (via load_user_with_relationships) behave as "user not
- found" without a real DB, matching the pre-refactor db.session behavior.
- Tests that need a specific return value should set it on mock_sm directly.
- """
- mock_sm.find_user_with_relationships.return_value = None
+ """Push an app context with g.user cleared and appbuilder.sm mocked."""
with app.app_context():
g.user = None
app.appbuilder = MagicMock()
@@ -101,6 +94,19 @@ def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock):
yield
+def _patch_load_user_not_found():
+ """Patch load_user_with_relationships to return None (user not found).
+
+ load_user_with_relationships delegates to the global security_manager
+ (not app.appbuilder.sm), so tests that need the JWT path to raise
+ ValueError("not found") must patch it directly at the module level.
+ """
+ return patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=None,
+ )
+
+
# -- Valid API key -> user loaded --
@@ -255,10 +261,9 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None:
jwt_access_token.claims = {"sub": "alice"}
with _mock_sm_ctx(app, mock_sm):
- with _patch_access_token(jwt_access_token):
- # _resolve_user_from_jwt_context will try to resolve the user
- # from the JWT claims and (in this isolated unit-test setup)
- # raise ValueError because the username is not a real user.
+ with _patch_access_token(jwt_access_token), _patch_load_user_not_found():
+ # _resolve_user_from_jwt_context resolves "alice" from JWT claims
+ # and raises ValueError because the username is not a real user.
# We assert that _resolve_user_from_api_key did NOT short-circuit
# to the API key path.
with pytest.raises(ValueError, match="not found"):
@@ -302,9 +307,9 @@ def test_namespaced_claim_without_api_key_client_id_is_ignored(
rogue_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True, "sub": "alice"}
with _mock_sm_ctx(app, mock_sm):
- with _patch_access_token(rogue_token):
- # JWT path tries to resolve user "alice" from DB and raises
- # ValueError in this isolated unit-test setup.
+ with _patch_access_token(rogue_token), _patch_load_user_not_found():
+ # JWT path resolves "alice" from claims and raises ValueError
+ # because no such user exists.
# validate_api_key must NOT be called — the rogue claim was ignored.
with pytest.raises(ValueError, match="not found"):
get_user_from_request()
@@ -330,11 +335,9 @@ def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path(
rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"}
with _mock_sm_ctx(app, mock_sm):
- with _patch_access_token(rogue_token):
- # JWT path tries to resolve user "alice" from DB and (in this
- # isolated unit-test setup) raises ValueError. The assertion
- # below confirms validate_api_key was never called — i.e., the
- # rogue claim did NOT divert into _resolve_user_from_api_key.
+ with _patch_access_token(rogue_token), _patch_load_user_not_found():
+ # JWT path resolves "alice" from claims and raises ValueError.
+ # validate_api_key must NOT be called — the rogue claim was ignored.
with pytest.raises(ValueError, match="not found"):
get_user_from_request()
(superset) 08/30: refactor(mcp): hoist JWT verifier imports to module top
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 2297603a9b2ae64af53f8ab27299f45fe81aee43
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 8 14:57:06 2026 -0400
refactor(mcp): hoist JWT verifier imports to module top
DetailedJWTVerifier and JWTVerifier have no circular-import or optional-
dependency reason to be imported inline — fastmcp is already pulled in
at module top via composite_token_verifier, and authlib is already a
hard dependency. Moving them up for consistency with the rest of the
module's imports.
---
superset/mcp_service/mcp_config.py | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index 0694fc90975..307045b3a8c 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -20,6 +20,7 @@ import logging
import secrets
from typing import Any, Dict, Optional
+from fastmcp.server.auth.providers.jwt import JWTVerifier
from flask import Flask
from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier
@@ -27,6 +28,7 @@ from superset.mcp_service.constants import (
DEFAULT_TOKEN_LIMIT,
DEFAULT_WARN_THRESHOLD_PCT,
)
+from superset.mcp_service.jwt_verifier import DetailedJWTVerifier, MCPJWTVerifier
logger = logging.getLogger(__name__)
@@ -383,13 +385,9 @@ def _build_jwt_verifier(
# 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
-
return DetailedJWTVerifier(**common_kwargs)
# MCPJWTVerifier: minimal logging + browser-friendly error page.
- from superset.mcp_service.jwt_verifier import MCPJWTVerifier
-
return MCPJWTVerifier(**common_kwargs)
(superset) 09/30: Potential fix for pull request finding
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 190e75e86883257f4a77629253f4b01ec4ed5a0a
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 8 16:04:09 2026 -0400
Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <17...@users.noreply.github.com>
---
superset/mcp_service/composite_token_verifier.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py
index e404e364373..ffba111bf48 100644
--- a/superset/mcp_service/composite_token_verifier.py
+++ b/superset/mcp_service/composite_token_verifier.py
@@ -74,7 +74,8 @@ class CompositeTokenVerifier(TokenVerifier):
"""Verify a Bearer token.
If the token starts with an API key prefix, return a pass-through
- AccessToken with a ``_api_key_passthrough`` claim. The Flask-layer
+ AccessToken with the namespaced ``API_KEY_PASSTHROUGH_CLAIM``
+ (``_superset_mcp_api_key_passthrough``). The Flask-layer
``_resolve_user_from_api_key()`` performs the real validation.
Otherwise, delegate to the wrapped JWT verifier when one is
(superset) 13/30: refactor(mcp): delegate load_user_with_relationships to SecurityManager.find_user_with_relationships
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c4377b524f08e4ae4beb06273bac7bd3b2456e88
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed May 13 22:10:36 2026 +0000
refactor(mcp): delegate load_user_with_relationships to SecurityManager.find_user_with_relationships
Fixes a gap identified in code review: the standalone load_user_with_relationships()
in auth.py duplicated SecurityManager.find_user() logic but dropped two FAB behaviors:
- auth_username_ci (case-insensitive username lookup)
- MultipleResultsFound guard (username uniqueness not guaranteed at DB level in all FAB versions)
It also hard-coded User/Group models instead of sm.user_model.
Changes:
- Add SupersetSecurityManager.find_user_with_relationships() to security/manager.py,
mirroring FAB's find_user() (auth_username_ci, MultipleResultsFound handling,
self.user_model) and adding eager loading of roles and group.roles via joinedload
- Simplify load_user_with_relationships() in auth.py to a thin delegate to the
new method, removing the duplicated query logic and raw Group/User imports
- Add regression test asserting find_user_with_relationships() exists on the SM
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/auth.py | 41 ++++------------
superset/security/manager.py | 57 ++++++++++++++++++++++-
tests/unit_tests/mcp_service/test_auth_api_key.py | 14 ++++++
3 files changed, 80 insertions(+), 32 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 620763e5285..22e82e038fe 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -49,7 +49,7 @@ from contextlib import AbstractContextManager, nullcontext
from typing import Any, Callable, TYPE_CHECKING, TypeVar
from flask import current_app, g, has_app_context, has_request_context
-from flask_appbuilder.security.sqla.models import Group, User
+from flask_appbuilder.security.sqla.models import User
from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM
@@ -217,23 +217,14 @@ def is_tool_visible_to_current_user(tool: 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.
-
- 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.
+ """Load a user with roles and group roles eagerly loaded.
- 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
@@ -241,21 +232,9 @@ def load_user_with_relationships(
if not username and not email:
raise ValueError("Either username or email must be provided")
- from sqlalchemy.orm import joinedload
-
- from superset.extensions import db
-
- query = db.session.query(User).options(
- joinedload(User.roles),
- joinedload(User.groups).joinedload(Group.roles),
- )
-
- if username:
- query = query.filter(User.username == username)
- else:
- query = query.filter(User.email == email)
+ from superset import security_manager
- return query.first()
+ return security_manager.find_user_with_relationships(username=username, email=email)
def _resolve_user_from_jwt_context(app: Any) -> User | None:
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 5da6e9ced97..982c2c46e8a 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -52,7 +52,8 @@ from flask_login import AnonymousUserMixin, LoginManager
from jwt.api_jwt import _jwt_global_obj
from sqlalchemy import and_, inspect, or_
from sqlalchemy.engine.base import Connection
-from sqlalchemy.orm import eagerload
+from sqlalchemy.orm import eagerload, joinedload
+from sqlalchemy.orm.exc import MultipleResultsFound
from sqlalchemy.orm.mapper import Mapper
from sqlalchemy.orm.query import Query as SqlaQuery
from sqlalchemy.sql import exists
@@ -3168,6 +3169,60 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
.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("roles"),
+ ]
+ if username:
+ try:
+ if self.auth_username_ci:
+ from sqlalchemy import func as sa_func
+
+ return (
+ self.session.query(self.user_model)
+ .options(*eager)
+ .filter(
+ sa_func.lower(self.user_model.username)
+ == sa_func.lower(username)
+ )
+ .one_or_none()
+ )
+ return (
+ self.session.query(self.user_model)
+ .options(*eager)
+ .filter(self.user_model.username == username)
+ .one_or_none()
+ )
+ except MultipleResultsFound:
+ logger.error("Multiple results found for user %s", username)
+ return None
+ if email:
+ try:
+ return (
+ self.session.query(self.user_model)
+ .options(*eager)
+ .filter_by(email=email)
+ .one_or_none()
+ )
+ except MultipleResultsFound:
+ logger.error("Multiple results found for user with email %s", email)
+ return None
+ return None
+
def get_anonymous_user(self) -> User:
return AnonymousUserMixin()
diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index 21d060441ff..961d41fbbc6 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -351,3 +351,17 @@ def test_security_manager_has_expected_api_key_methods(app: SupersetApp) -> None
"auth._resolve_user_from_api_key() references this method by name — "
"update auth.py if the FAB API changed."
)
+
+
+def test_security_manager_has_find_user_with_relationships(app: SupersetApp) -> None:
+ """Regression test: verify SupersetSecurityManager.find_user_with_relationships
+ exists. load_user_with_relationships() in auth.py delegates to it — a rename
+ or removal would silently break MCP user resolution at runtime."""
+ with app.app_context():
+ from superset import security_manager
+
+ assert hasattr(security_manager, "find_user_with_relationships"), (
+ "SupersetSecurityManager is missing 'find_user_with_relationships'. "
+ "auth.load_user_with_relationships() delegates to this method — "
+ "update auth.py if the method was renamed or removed."
+ )
(superset) 15/30: fix(mcp): validate api_key_prefixes in CompositeTokenVerifier — filter empty/non-string entries
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit a33d5cf815f0e276c3773c093ccd1e70edab2e91
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu May 14 16:34:36 2026 +0000
fix(mcp): validate api_key_prefixes in CompositeTokenVerifier — filter empty/non-string entries
Empty-string prefixes match every Bearer token (DoS/misclassification vector).
Non-string entries cause TypeError in str.startswith(). Filter both in __init__,
warn on invalid entries, and only store valid non-empty string prefixes.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/composite_token_verifier.py | 10 +++-
.../mcp_service/test_composite_token_verifier.py | 60 ++++++++++++++++++++++
2 files changed, 69 insertions(+), 1 deletion(-)
diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py
index ffba111bf48..8889d66af42 100644
--- a/superset/mcp_service/composite_token_verifier.py
+++ b/superset/mcp_service/composite_token_verifier.py
@@ -68,7 +68,15 @@ class CompositeTokenVerifier(TokenVerifier):
required_scopes=getattr(jwt_verifier, "required_scopes", None) or [],
)
self._jwt_verifier = jwt_verifier
- self._api_key_prefixes = tuple(api_key_prefixes)
+ valid: list[str] = [
+ p for p in api_key_prefixes if isinstance(p, str) and p.strip()
+ ]
+ invalid = [p for p in api_key_prefixes if p not in valid]
+ if invalid:
+ logger.warning(
+ "FAB_API_KEY_PREFIXES contains invalid entries (ignored): %r", invalid
+ )
+ self._api_key_prefixes = tuple(valid)
async def verify_token(self, token: str) -> AccessToken | None:
"""Verify a Bearer token.
diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
index d493c07ea03..d519b39a8a9 100644
--- a/tests/unit_tests/mcp_service/test_composite_token_verifier.py
+++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py
@@ -139,6 +139,66 @@ async def test_api_key_only_mode_rejects_non_api_key_tokens() -> None:
assert result is None
+@pytest.mark.asyncio
+async def test_empty_string_prefix_is_filtered_out() -> None:
+ """An empty-string prefix would match every Bearer token (DoS vector).
+ It must be silently dropped and never stored in _api_key_prefixes."""
+ verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=[""])
+
+ assert "" not in verifier._api_key_prefixes
+ # A plain JWT must NOT be misidentified as an API key.
+ result = await verifier.verify_token("eyJhbGciOiJSUzI1NiJ9.jwt_payload")
+ assert result is None
+
+
+@pytest.mark.asyncio
+async def test_whitespace_only_prefix_is_filtered_out() -> None:
+ """A whitespace-only prefix is also invalid and must be dropped."""
+ verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=[" "])
+
+ assert " " not in verifier._api_key_prefixes
+ result = await verifier.verify_token(" starts_with_spaces")
+ assert result is None
+
+
+@pytest.mark.asyncio
+async def test_non_string_prefix_is_filtered_out() -> None:
+ """Non-string entries (e.g. None, int) must not be stored and must not
+ cause a TypeError during verify_token."""
+ verifier = CompositeTokenVerifier(
+ jwt_verifier=None,
+ api_key_prefixes=[None, 42, "sst_"], # type: ignore[list-item]
+ )
+
+ assert None not in verifier._api_key_prefixes
+ assert 42 not in verifier._api_key_prefixes
+ assert verifier._api_key_prefixes == ("sst_",)
+
+
+@pytest.mark.asyncio
+async def test_invalid_prefixes_emit_warning(caplog: pytest.LogCaptureFixture) -> None:
+ """Invalid prefix entries must trigger a logger.warning so operators can
+ detect misconfiguration in FAB_API_KEY_PREFIXES."""
+ import logging
+
+ logger_name = "superset.mcp_service.composite_token_verifier"
+ with caplog.at_level(logging.WARNING, logger=logger_name):
+ CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["", "sst_"])
+
+ assert any("invalid" in record.message.lower() for record in caplog.records)
+
+
+@pytest.mark.asyncio
+async def test_all_invalid_prefixes_accepts_no_api_keys() -> None:
+ """When all prefixes are invalid and filtered out, no token should match
+ the API key path."""
+ verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["", " "])
+
+ assert verifier._api_key_prefixes == ()
+ result = await verifier.verify_token("sst_abc123")
+ assert result is None
+
+
@pytest.mark.asyncio
async def test_api_key_passthrough_propagates_required_scopes() -> None:
"""The pass-through AccessToken must carry the verifier's required_scopes
(superset) 20/30: fix(mcp): use class-bound attribute in joinedload for group roles
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit c57a4988f1241296388f27c36f4480ff97838e10
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Fri May 15 00:05:49 2026 +0000
fix(mcp): use class-bound attribute in joinedload for group roles
Replace string-based joinedload("roles") with the class-bound
self.group_model.roles attribute, consistent with how joinedload is
used elsewhere in Superset and forward-compatible with SQLAlchemy's
deprecation of string-based relationship names.
---
superset/security/manager.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 982c2c46e8a..adb452a9d3b 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -3185,7 +3185,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"""
eager = [
joinedload(self.user_model.roles),
- joinedload(self.user_model.groups).joinedload("roles"),
+ joinedload(self.user_model.groups).joinedload(self.group_model.roles),
]
if username:
try:
(superset) 24/30: fix(mcp): address Codex review — error class, fail-open, DRY permission logic
Posted by mi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 28bf9717cf6e5afe1cd646d0560d2d8d7b6f319a
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Sat May 16 22:15:01 2026 +0000
fix(mcp): address Codex review — error class, fail-open, DRY permission logic
- MCPPermissionDeniedError now inherits PermissionError so the middleware
classifies RBAC denials as user errors (WARNING log, "Access denied"
sanitized message) rather than server errors with full stack traces.
- Fix fail-open: FAB_API_KEY_PREFIXES with a non-iterable value (e.g. None)
no longer raises TypeError and silently disables transport auth; falls back
to default prefix ["sst_"] with a warning instead.
- Fix diagnostic message in get_user_from_request(): when FAB_API_KEY_PREFIXES
is a plain string, prefix_example was taking index [0] and showing only the
first character ("s" instead of "sst_").
- DRY: _tool_allowed_for_current_user in server.py now delegates the RBAC
check to check_tool_permission (auth.py) instead of duplicating the config
flag, permission-attr lookup, and security_manager.can_access call.
- Document that MCP_REQUIRED_SCOPES is not enforced for API-key auth (FAB
keys have no scopes; RBAC is enforced via check_tool_permission instead).
- Add FAB upgrade note to find_user_with_relationships docstring: the method
mirrors FAB's find_user() internals and should be reviewed on FAB upgrades.
Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
superset/mcp_service/auth.py | 16 +++++++++++++---
superset/mcp_service/composite_token_verifier.py | 4 ++++
superset/mcp_service/mcp_config.py | 10 +++++++++-
superset/security/manager.py | 4 ++++
4 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index de6227a1e5b..c9686c39834 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -70,8 +70,13 @@ CLASS_PERMISSION_ATTR = "_class_permission_name"
METHOD_PERMISSION_ATTR = "_method_permission_name"
-class MCPPermissionDeniedError(Exception):
- """Raised when user lacks required RBAC permission for an MCP tool."""
+class MCPPermissionDeniedError(PermissionError):
+ """Raised when user lacks required RBAC permission for an MCP tool.
+
+ Inherits from ``PermissionError`` so the middleware classifies denials as
+ user errors (HTTP 403 / WARNING log / "Access denied" sanitized message)
+ rather than unexpected server errors.
+ """
def __init__(
self,
@@ -451,7 +456,12 @@ def get_user_from_request() -> User:
"g.user was not set by external middleware",
]
configured_prefixes = current_app.config.get("FAB_API_KEY_PREFIXES", ["sst_"])
- prefix_example = configured_prefixes[0] if configured_prefixes else "sst_"
+ if isinstance(configured_prefixes, str):
+ prefix_example = configured_prefixes
+ elif configured_prefixes:
+ prefix_example = configured_prefixes[0]
+ else:
+ prefix_example = "sst_"
raise ValueError(
"No authenticated user found. Tried:\n"
+ "\n".join(f" - {d}" for d in details)
diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py
index 72b22df60aa..f2a4d9bed86 100644
--- a/superset/mcp_service/composite_token_verifier.py
+++ b/superset/mcp_service/composite_token_verifier.py
@@ -100,6 +100,10 @@ class CompositeTokenVerifier(TokenVerifier):
# satisfied for API-key requests. Without this, MCP_REQUIRED_SCOPES
# being non-empty would 403 every API-key call before
# ``_resolve_user_from_api_key`` even runs.
+ #
+ # NOTE: ``MCP_REQUIRED_SCOPES`` is intentionally not enforced for
+ # API-key auth — FAB API keys do not carry scopes. Authorization is
+ # enforced downstream via ``check_tool_permission`` (RBAC).
return AccessToken(
token=token,
client_id="api_key",
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index 0c312cc420c..0fe43e2be6d 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -352,10 +352,18 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]:
)
# 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:
- api_key_prefixes = list(raw_prefixes)
+ 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,
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 67d1bcfd53c..10fb388a359 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -3182,6 +3182,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
``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.
+
+ FAB does not expose an eager-loading option on ``find_user``, so the
+ query logic is mirrored here with joinedload options added. Review this
+ method when upgrading FAB to ensure it stays in sync with upstream.
"""
eager = [
joinedload(self.user_model.roles),