(superset) 21/30: fix(mcp): address dpgaspar review — imports, types, exception scope

[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)