This is an automated email from the ASF dual-hosted git repository. aminghadersohi pushed a commit to branch work-pr-39604 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 42e588298a2aa92de529b591fe960471b807a35c Author: Amin Ghadersohi <[email protected]> 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 9b06d5ecb41..a79a6a29884 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)
