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/03/20 19:43:03 UTC
(superset) 02/32: feat(mcp): implement RBAC permission checking for MCP tools (#38407)
This is an automated email from the ASF dual-hosted git repository.
michaelsmolina pushed a commit to branch 6.1
in repository https://gitbox.apache.org/repos/asf/superset.git
commit f11a4834d2b7c5117f1b036d52ac5f34dc773632
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu Mar 12 17:35:07 2026 +0100
feat(mcp): implement RBAC permission checking for MCP tools (#38407)
Co-authored-by: Claude Opus 4.6 <no...@anthropic.com>
---
superset-core/src/superset_core/mcp/decorators.py | 22 +-
superset/core/mcp/core_mcp_injection.py | 24 ++-
superset/mcp_service/auth.py | 136 +++++++++++--
superset/mcp_service/chart/tool/generate_chart.py | 2 +-
superset/mcp_service/chart/tool/get_chart_data.py | 2 +-
superset/mcp_service/chart/tool/get_chart_info.py | 113 +++--------
.../mcp_service/chart/tool/get_chart_preview.py | 2 +-
superset/mcp_service/chart/tool/list_charts.py | 2 +-
superset/mcp_service/chart/tool/update_chart.py | 2 +-
.../mcp_service/chart/tool/update_chart_preview.py | 2 +-
.../tool/add_chart_to_existing_dashboard.py | 2 +-
.../dashboard/tool/generate_dashboard.py | 2 +-
.../dashboard/tool/get_dashboard_info.py | 2 +-
.../mcp_service/dashboard/tool/list_dashboards.py | 2 +-
.../mcp_service/dataset/tool/get_dataset_info.py | 2 +-
superset/mcp_service/dataset/tool/list_datasets.py | 2 +-
.../explore/tool/generate_explore_link.py | 2 +-
superset/mcp_service/mcp_config.py | 5 +
superset/mcp_service/sql_lab/tool/execute_sql.py | 6 +-
.../sql_lab/tool/open_sql_lab_with_context.py | 2 +-
tests/unit_tests/mcp_service/conftest.py | 19 +-
tests/unit_tests/mcp_service/test_auth_rbac.py | 225 +++++++++++++++++++++
22 files changed, 460 insertions(+), 118 deletions(-)
diff --git a/superset-core/src/superset_core/mcp/decorators.py b/superset-core/src/superset_core/mcp/decorators.py
index d8c814a1acd..6ad29c9adc5 100644
--- a/superset-core/src/superset_core/mcp/decorators.py
+++ b/superset-core/src/superset_core/mcp/decorators.py
@@ -48,11 +48,14 @@ def tool(
description: str | None = None,
tags: list[str] | None = None,
protect: bool = True,
+ class_permission_name: str | None = None,
+ method_permission_name: str | None = None,
) -> Any: # Use Any to avoid mypy issues with dependency injection
"""
Decorator to register an MCP tool with optional authentication.
- This decorator combines FastMCP tool registration with optional authentication.
+ This decorator combines FastMCP tool registration with optional authentication
+ and RBAC permission checking.
Can be used as:
@tool
@@ -69,6 +72,11 @@ def tool(
description: Tool description (defaults to function docstring)
tags: List of tags for categorizing the tool (defaults to empty list)
protect: Whether to require Superset authentication (defaults to True)
+ class_permission_name: FAB view/resource name for RBAC checking
+ (e.g., "Chart", "Dashboard", "SQLLab"). When set, enables
+ permission checking via security_manager.can_access().
+ method_permission_name: FAB action name (e.g., "read", "write").
+ Defaults to "write" if tags includes "mutate", else "read".
Returns:
Decorator function that registers and wraps the tool, or the wrapped function
@@ -90,6 +98,18 @@ def tool(
def public_tool() -> str:
'''Public tool accessible without auth'''
return "Hello world"
+
+ @tool(class_permission_name="Chart") # RBAC: requires can_read on Chart
+ def list_charts() -> list:
+ '''List charts the user can access'''
+ return []
+
+ @tool( # RBAC: can_write on Chart
+ tags=["mutate"], class_permission_name="Chart",
+ )
+ def create_chart(name: str) -> dict:
+ '''Create a new chart'''
+ return {"name": name}
"""
raise NotImplementedError(
"MCP tool decorator not initialized. "
diff --git a/superset/core/mcp/core_mcp_injection.py b/superset/core/mcp/core_mcp_injection.py
index ad4a7b1211d..fa2ec23738a 100644
--- a/superset/core/mcp/core_mcp_injection.py
+++ b/superset/core/mcp/core_mcp_injection.py
@@ -60,12 +60,14 @@ def create_tool_decorator(
description: Optional[str] = None,
tags: Optional[list[str]] = None,
protect: bool = True,
+ class_permission_name: Optional[str] = None,
+ method_permission_name: Optional[str] = None,
) -> Callable[[F], F] | F:
"""
Create the concrete MCP tool decorator implementation.
- This combines FastMCP tool registration with optional Superset authentication,
- replacing the need for separate @mcp.tool and @mcp_auth_hook decorators.
+ This combines FastMCP tool registration with optional Superset authentication
+ and RBAC permission checking.
Supports both @tool and @tool() syntax.
@@ -76,6 +78,10 @@ def create_tool_decorator(
description: Tool description (defaults to function docstring)
tags: List of tags for categorization (defaults to empty list)
protect: Whether to apply Superset authentication (defaults to True)
+ class_permission_name: FAB view/resource name for RBAC
+ (e.g., "Chart", "Dashboard", "SQLLab"). Enables permission checking.
+ method_permission_name: FAB action name (e.g., "read", "write").
+ Defaults to "write" if tags has "mutate", else "read".
Returns:
Decorator that registers and wraps the tool with optional authentication,
@@ -95,6 +101,20 @@ def create_tool_decorator(
# Get prefixed ID based on ambient context
tool_name, context_type = _get_prefixed_id_with_context(base_tool_name)
+ # Store RBAC permission metadata on the function so
+ # mcp_auth_hook can read them at call time.
+ if class_permission_name:
+ from superset.mcp_service.auth import (
+ CLASS_PERMISSION_ATTR,
+ METHOD_PERMISSION_ATTR,
+ )
+
+ setattr(func, CLASS_PERMISSION_ATTR, class_permission_name)
+ actual_method = method_permission_name
+ if actual_method is None:
+ actual_method = "write" if "mutate" in tool_tags else "read"
+ setattr(func, METHOD_PERMISSION_ATTR, actual_method)
+
# Conditionally apply authentication wrapper
if protect:
from superset.mcp_service.auth import mcp_auth_hook
diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py
index 8675947097f..77544f224cb 100644
--- a/superset/mcp_service/auth.py
+++ b/superset/mcp_service/auth.py
@@ -16,15 +16,16 @@
# under the License.
"""
-Minimal authentication hooks for MCP tools.
-This is a placeholder implementation that provides basic user context.
-
-Future enhancements (to be added in separate PRs):
-- JWT token authentication and validation
-- User impersonation support
-- Permission checking with scopes
-- Comprehensive audit logging
-- Field-level permissions
+Authentication and authorization hooks for MCP tools.
+
+This module provides:
+- User authentication from JWT or configured dev user
+- RBAC permission checking aligned with Superset's REST API permissions
+- Dataset access validation
+- Session lifecycle management
+
+The RBAC enforcement mirrors Flask-AppBuilder's @protect() decorator,
+ensuring MCP tools respect the same permission model as the REST API.
"""
import logging
@@ -42,6 +43,90 @@ F = TypeVar("F", bound=Callable[..., Any])
logger = logging.getLogger(__name__)
+# Constants for RBAC permission attributes (mirrors FAB conventions)
+PERMISSION_PREFIX = "can_"
+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."""
+
+ def __init__(
+ self,
+ permission_name: str,
+ view_name: str,
+ user: str | None = None,
+ tool_name: str | None = None,
+ ):
+ self.permission_name = permission_name
+ self.view_name = view_name
+ self.user = user
+ self.tool_name = tool_name
+ message = (
+ f"Permission denied: {permission_name} on {view_name}"
+ + (f" for user {user}" if user else "")
+ + (f" (tool: {tool_name})" if tool_name else "")
+ )
+ super().__init__(message)
+
+
+def check_tool_permission(func: Callable[..., Any]) -> bool:
+ """Check if the current user has RBAC permission for an MCP tool.
+
+ Reads permission metadata stored on the function by the @tool decorator
+ and uses Superset's security_manager to verify access.
+
+ Controlled by the ``MCP_RBAC_ENABLED`` config flag (default True).
+ Set to False in superset_config.py to disable RBAC checking.
+
+ Args:
+ func: The tool function with optional permission attributes.
+
+ Returns:
+ 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:
+ logger.warning(
+ "No user context for permission check on tool: %s", func.__name__
+ )
+ return False
+
+ class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
+ if not class_permission_name:
+ # No RBAC configured for this tool; allow by default.
+ return True
+
+ method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read")
+ permission_str = f"{PERMISSION_PREFIX}{method_permission_name}"
+
+ has_permission = security_manager.can_access(
+ permission_str, class_permission_name
+ )
+
+ if not has_permission:
+ logger.warning(
+ "Permission denied for user %s: %s on %s (tool: %s)",
+ g.user.username,
+ permission_str,
+ class_permission_name,
+ func.__name__,
+ )
+
+ return has_permission
+
+ except (AttributeError, ValueError, RuntimeError) as e:
+ logger.warning("Error checking tool permission: %s", e)
+ return False
+
def load_user_with_relationships(
username: str | None = None, email: str | None = None
@@ -219,14 +304,15 @@ def mcp_auth_hook(tool_func: F) -> F: # noqa: C901
"""
Authentication and authorization decorator for MCP tools.
- This decorator pushes Flask application context and sets up g.user
- for MCP tool execution.
+ This decorator pushes Flask application context, sets up g.user,
+ and enforces RBAC permission checks for MCP tool execution.
- Supports both sync and async tool functions.
+ Permission metadata (class_permission_name, method_permission_name) is
+ stored on tool_func by the @tool decorator in core_mcp_injection.py.
+ If present, check_tool_permission() verifies the user has the required
+ FAB permission before the tool function runs.
- TODO (future PR): Add permission checking
- TODO (future PR): Add JWT scope validation
- TODO (future PR): Add comprehensive audit logging
+ Supports both sync and async tool functions.
"""
import contextlib
import functools
@@ -264,6 +350,16 @@ def mcp_auth_hook(tool_func: F) -> F: # noqa: C901
)
return await tool_func(*args, **kwargs)
+ # RBAC permission check
+ if not check_tool_permission(tool_func):
+ method_name = getattr(tool_func, METHOD_PERMISSION_ATTR, "read")
+ raise MCPPermissionDeniedError(
+ permission_name=f"{PERMISSION_PREFIX}{method_name}",
+ view_name=getattr(tool_func, CLASS_PERMISSION_ATTR, "unknown"),
+ user=user.username,
+ tool_name=tool_func.__name__,
+ )
+
try:
logger.debug(
"MCP tool call: user=%s, tool=%s",
@@ -294,6 +390,16 @@ def mcp_auth_hook(tool_func: F) -> F: # noqa: C901
)
return tool_func(*args, **kwargs)
+ # RBAC permission check
+ if not check_tool_permission(tool_func):
+ method_name = getattr(tool_func, METHOD_PERMISSION_ATTR, "read")
+ raise MCPPermissionDeniedError(
+ permission_name=f"{PERMISSION_PREFIX}{method_name}",
+ view_name=getattr(tool_func, CLASS_PERMISSION_ATTR, "unknown"),
+ user=user.username,
+ tool_name=tool_func.__name__,
+ )
+
try:
logger.debug(
"MCP tool call: user=%s, tool=%s",
diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py
index 767ff2cbe31..76ce79b5212 100644
--- a/superset/mcp_service/chart/tool/generate_chart.py
+++ b/superset/mcp_service/chart/tool/generate_chart.py
@@ -119,7 +119,7 @@ def _compile_chart(
return CompileResult(success=False, error=str(exc))
-@tool(tags=["mutate"])
+@tool(tags=["mutate"], class_permission_name="Chart")
@parse_request(GenerateChartRequest)
async def generate_chart( # noqa: C901
request: GenerateChartRequest, ctx: Context
diff --git a/superset/mcp_service/chart/tool/get_chart_data.py b/superset/mcp_service/chart/tool/get_chart_data.py
index f37d963cfac..907057d859e 100644
--- a/superset/mcp_service/chart/tool/get_chart_data.py
+++ b/superset/mcp_service/chart/tool/get_chart_data.py
@@ -63,7 +63,7 @@ def _get_cached_form_data(form_data_key: str) -> str | None:
return None
-@tool(tags=["data"])
+@tool(tags=["data"], class_permission_name="Chart")
@parse_request(GetChartDataRequest)
async def get_chart_data( # noqa: C901
request: GetChartDataRequest, ctx: Context
diff --git a/superset/mcp_service/chart/tool/get_chart_info.py b/superset/mcp_service/chart/tool/get_chart_info.py
index 7029de2ffa6..6c60214f705 100644
--- a/superset/mcp_service/chart/tool/get_chart_info.py
+++ b/superset/mcp_service/chart/tool/get_chart_info.py
@@ -56,66 +56,7 @@ def _get_cached_form_data(form_data_key: str) -> str | None:
return None
-def _build_unsaved_chart_info(form_data_key: str) -> ChartInfo | ChartError:
- """Build a ChartInfo from cached form_data when no chart identifier exists."""
- from superset.utils import json as utils_json
-
- cached_form_data = _get_cached_form_data(form_data_key)
- if not cached_form_data:
- return ChartError(
- error="No cached chart data found for form_data_key. "
- "The cache may have expired.",
- error_type="NotFound",
- )
- try:
- form_data = utils_json.loads(cached_form_data)
- except (TypeError, ValueError) as e:
- return ChartError(
- error=f"Failed to parse cached form_data: {e}",
- error_type="ParseError",
- )
- if not isinstance(form_data, dict):
- return ChartError(
- error="Cached form_data is not a valid JSON object.",
- error_type="ParseError",
- )
- return ChartInfo(
- viz_type=form_data.get("viz_type"),
- datasource_name=form_data.get("datasource_name"),
- datasource_type=form_data.get("datasource_type"),
- form_data=form_data,
- form_data_key=form_data_key,
- is_unsaved_state=True,
- )
-
-
-def _apply_unsaved_state_override(result: ChartInfo, form_data_key: str) -> None:
- """Override a ChartInfo's form_data with cached unsaved state."""
- from superset.utils import json as utils_json
-
- if cached_form_data := _get_cached_form_data(form_data_key):
- try:
- result.form_data = utils_json.loads(cached_form_data)
- result.form_data_key = form_data_key
- result.is_unsaved_state = True
-
- # Update viz_type from cached form_data if present
- if result.form_data and "viz_type" in result.form_data:
- result.viz_type = result.form_data["viz_type"]
- except (TypeError, ValueError) as e:
- logger.warning(
- "Failed to parse cached form_data: %s. "
- "Using saved chart configuration.",
- e,
- )
- else:
- logger.warning(
- "form_data_key provided but no cached data found. "
- "The cache may have expired. Using saved chart configuration."
- )
-
-
-@tool(tags=["discovery"])
+@tool(tags=["discovery"], class_permission_name="Chart")
@parse_request(GetChartInfoRequest)
async def get_chart_info(
request: GetChartInfoRequest, ctx: Context
@@ -155,28 +96,13 @@ async def get_chart_info(
"""
from superset.daos.chart import ChartDAO
from superset.models.slice import Slice
+ from superset.utils import json as utils_json
await ctx.info(
"Retrieving chart information: identifier=%s, form_data_key=%s"
% (request.identifier, request.form_data_key)
)
- # Handle unsaved chart (form_data_key only, no identifier)
- if not request.identifier and request.form_data_key:
- with event_logger.log_context(
- action="mcp.get_chart_info.unsaved_chart_from_cache"
- ):
- await ctx.info(
- "No chart identifier provided - retrieving unsaved chart from cache: "
- "form_data_key=%s" % (request.form_data_key,)
- )
- return _build_unsaved_chart_info(request.form_data_key)
-
- # At this point identifier must be set (validator ensures at least one
- # of identifier/form_data_key is provided, and the form_data_key-only
- # branch returned above).
- assert request.identifier is not None
-
# Eager load owners and tags to avoid N+1 queries during serialization
eager_options = [
subqueryload(Slice.owners),
@@ -199,14 +125,35 @@ async def get_chart_info(
if isinstance(result, ChartInfo):
# If form_data_key is provided, override form_data with cached version
if request.form_data_key:
- with event_logger.log_context(
- action="mcp.get_chart_info.unsaved_state_override"
- ):
- await ctx.info(
- "Retrieving unsaved chart state from cache: form_data_key=%s"
- % (request.form_data_key,)
+ await ctx.info(
+ "Retrieving unsaved chart state from cache: form_data_key=%s"
+ % (request.form_data_key,)
+ )
+ cached_form_data = _get_cached_form_data(request.form_data_key)
+
+ if cached_form_data:
+ try:
+ result.form_data = utils_json.loads(cached_form_data)
+ result.form_data_key = request.form_data_key
+ result.is_unsaved_state = True
+
+ # Update viz_type from cached form_data if present
+ if result.form_data and "viz_type" in result.form_data:
+ result.viz_type = result.form_data["viz_type"]
+
+ await ctx.info(
+ "Chart form_data overridden with unsaved state from cache"
+ )
+ except (TypeError, ValueError) as e:
+ await ctx.warning(
+ "Failed to parse cached form_data: %s. "
+ "Using saved chart configuration." % (str(e),)
+ )
+ else:
+ await ctx.warning(
+ "form_data_key provided but no cached data found. "
+ "The cache may have expired. Using saved chart configuration."
)
- _apply_unsaved_state_override(result, request.form_data_key)
await ctx.info(
"Chart information retrieved successfully: chart_name=%s, "
diff --git a/superset/mcp_service/chart/tool/get_chart_preview.py b/superset/mcp_service/chart/tool/get_chart_preview.py
index 1b5c9716836..d95e9500661 100644
--- a/superset/mcp_service/chart/tool/get_chart_preview.py
+++ b/superset/mcp_service/chart/tool/get_chart_preview.py
@@ -2177,7 +2177,7 @@ async def _get_chart_preview_internal( # noqa: C901
)
-@tool(tags=["data"])
+@tool(tags=["data"], class_permission_name="Chart")
@parse_request(GetChartPreviewRequest)
async def get_chart_preview(
request: GetChartPreviewRequest, ctx: Context
diff --git a/superset/mcp_service/chart/tool/list_charts.py b/superset/mcp_service/chart/tool/list_charts.py
index d65523812e9..8ef2daf91f0 100644
--- a/superset/mcp_service/chart/tool/list_charts.py
+++ b/superset/mcp_service/chart/tool/list_charts.py
@@ -62,7 +62,7 @@ SORTABLE_CHART_COLUMNS = [
]
-@tool(tags=["core"])
+@tool(tags=["core"], class_permission_name="Chart")
@parse_request(ListChartsRequest)
async def list_charts(request: ListChartsRequest, ctx: Context) -> ChartList:
"""List charts with filtering and search.
diff --git a/superset/mcp_service/chart/tool/update_chart.py b/superset/mcp_service/chart/tool/update_chart.py
index 5dd9b77d34b..d69e09693a9 100644
--- a/superset/mcp_service/chart/tool/update_chart.py
+++ b/superset/mcp_service/chart/tool/update_chart.py
@@ -45,7 +45,7 @@ from superset.utils import json
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(tags=["mutate"], class_permission_name="Chart")
@parse_request(UpdateChartRequest)
async def update_chart(
request: UpdateChartRequest, ctx: Context
diff --git a/superset/mcp_service/chart/tool/update_chart_preview.py b/superset/mcp_service/chart/tool/update_chart_preview.py
index 8e99b5e1305..f3242b14091 100644
--- a/superset/mcp_service/chart/tool/update_chart_preview.py
+++ b/superset/mcp_service/chart/tool/update_chart_preview.py
@@ -44,7 +44,7 @@ from superset.mcp_service.utils.schema_utils import parse_request
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(tags=["mutate"], class_permission_name="Chart", method_permission_name="write")
@parse_request(UpdateChartPreviewRequest)
def update_chart_preview(
request: UpdateChartPreviewRequest, ctx: Context
diff --git a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
index f324b40da17..2629aded0dc 100644
--- a/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py
@@ -305,7 +305,7 @@ def _ensure_layout_structure(
layout["DASHBOARD_VERSION_KEY"] = "v2"
-@tool(tags=["mutate"])
+@tool(tags=["mutate"], class_permission_name="Dashboard")
@parse_request(AddChartToDashboardRequest)
def add_chart_to_existing_dashboard(
request: AddChartToDashboardRequest, ctx: Context
diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py
index 1475e36c496..4bbc4450502 100644
--- a/superset/mcp_service/dashboard/tool/generate_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py
@@ -178,7 +178,7 @@ def _generate_title_from_charts(chart_objects: List[Any]) -> str:
return title
-@tool(tags=["mutate"])
+@tool(tags=["mutate"], class_permission_name="Dashboard")
@parse_request(GenerateDashboardRequest)
def generate_dashboard(
request: GenerateDashboardRequest, ctx: Context
diff --git a/superset/mcp_service/dashboard/tool/get_dashboard_info.py b/superset/mcp_service/dashboard/tool/get_dashboard_info.py
index de688ee8d8d..fa0df2bee14 100644
--- a/superset/mcp_service/dashboard/tool/get_dashboard_info.py
+++ b/superset/mcp_service/dashboard/tool/get_dashboard_info.py
@@ -59,7 +59,7 @@ def _get_permalink_state(permalink_key: str) -> DashboardPermalinkValue | None:
return None
-@tool(tags=["discovery"])
+@tool(tags=["discovery"], class_permission_name="Dashboard")
@parse_request(GetDashboardInfoRequest)
async def get_dashboard_info(
request: GetDashboardInfoRequest, ctx: Context
diff --git a/superset/mcp_service/dashboard/tool/list_dashboards.py b/superset/mcp_service/dashboard/tool/list_dashboards.py
index 67555678a3a..823533a9086 100644
--- a/superset/mcp_service/dashboard/tool/list_dashboards.py
+++ b/superset/mcp_service/dashboard/tool/list_dashboards.py
@@ -63,7 +63,7 @@ SORTABLE_DASHBOARD_COLUMNS = [
]
-@tool(tags=["core"])
+@tool(tags=["core"], class_permission_name="Dashboard")
@parse_request(ListDashboardsRequest)
async def list_dashboards(
request: ListDashboardsRequest, ctx: Context
diff --git a/superset/mcp_service/dataset/tool/get_dataset_info.py b/superset/mcp_service/dataset/tool/get_dataset_info.py
index 07333e837a5..59c3603a185 100644
--- a/superset/mcp_service/dataset/tool/get_dataset_info.py
+++ b/superset/mcp_service/dataset/tool/get_dataset_info.py
@@ -42,7 +42,7 @@ from superset.mcp_service.utils.schema_utils import parse_request
logger = logging.getLogger(__name__)
-@tool(tags=["discovery"])
+@tool(tags=["discovery"], class_permission_name="Dataset")
@parse_request(GetDatasetInfoRequest)
async def get_dataset_info(
request: GetDatasetInfoRequest, ctx: Context
diff --git a/superset/mcp_service/dataset/tool/list_datasets.py b/superset/mcp_service/dataset/tool/list_datasets.py
index 714abaa199b..4810350df1a 100644
--- a/superset/mcp_service/dataset/tool/list_datasets.py
+++ b/superset/mcp_service/dataset/tool/list_datasets.py
@@ -61,7 +61,7 @@ SORTABLE_DATASET_COLUMNS = [
]
-@tool(tags=["core"])
+@tool(tags=["core"], class_permission_name="Dataset")
@parse_request(ListDatasetsRequest)
async def list_datasets(request: ListDatasetsRequest, ctx: Context) -> DatasetList:
"""List datasets with filtering and search.
diff --git a/superset/mcp_service/explore/tool/generate_explore_link.py b/superset/mcp_service/explore/tool/generate_explore_link.py
index 81e12974824..d60c0fd4d42 100644
--- a/superset/mcp_service/explore/tool/generate_explore_link.py
+++ b/superset/mcp_service/explore/tool/generate_explore_link.py
@@ -39,7 +39,7 @@ from superset.mcp_service.chart.schemas import (
from superset.mcp_service.utils.schema_utils import parse_request
-@tool(tags=["explore"])
+@tool(tags=["explore"], class_permission_name="Explore")
@parse_request(GenerateExploreLinkRequest)
async def generate_explore_link(
request: GenerateExploreLinkRequest, ctx: Context
diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py
index dab549b9b06..75221f854ef 100644
--- a/superset/mcp_service/mcp_config.py
+++ b/superset/mcp_service/mcp_config.py
@@ -45,6 +45,10 @@ MCP_SERVICE_PORT = 5008
# MCP Debug mode - shows suppressed initialization output in stdio mode
MCP_DEBUG = False
+# MCP RBAC - when True, tools with class_permission_name are checked
+# against the FAB security_manager before execution.
+MCP_RBAC_ENABLED = True
+
# MCP JWT Debug Errors - controls server-side JWT debug logging.
# When False (default), uses the default JWTVerifier with minimal logging.
# When True, uses DetailedJWTVerifier with tiered logging:
@@ -319,6 +323,7 @@ def get_mcp_config(app_config: Dict[str, Any] | None = None) -> Dict[str, Any]:
"MCP_SERVICE_HOST": MCP_SERVICE_HOST,
"MCP_SERVICE_PORT": MCP_SERVICE_PORT,
"MCP_DEBUG": MCP_DEBUG,
+ "MCP_RBAC_ENABLED": MCP_RBAC_ENABLED,
**MCP_SESSION_CONFIG,
**MCP_CSRF_CONFIG,
}
diff --git a/superset/mcp_service/sql_lab/tool/execute_sql.py b/superset/mcp_service/sql_lab/tool/execute_sql.py
index 4093765938b..7f3164d8852 100644
--- a/superset/mcp_service/sql_lab/tool/execute_sql.py
+++ b/superset/mcp_service/sql_lab/tool/execute_sql.py
@@ -51,7 +51,11 @@ from superset.mcp_service.utils.schema_utils import parse_request
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(
+ tags=["mutate"],
+ class_permission_name="SQLLab",
+ method_permission_name="execute_sql_query",
+)
@parse_request(ExecuteSqlRequest)
async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlResponse:
"""Execute SQL query against database using the unified Database.execute() API."""
diff --git a/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py b/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py
index add63f78bab..d326cf26244 100644
--- a/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py
+++ b/superset/mcp_service/sql_lab/tool/open_sql_lab_with_context.py
@@ -38,7 +38,7 @@ from superset.mcp_service.utils.url_utils import get_superset_base_url
logger = logging.getLogger(__name__)
-@tool(tags=["explore"])
+@tool(tags=["explore"], class_permission_name="SQLLab", method_permission_name="read")
@parse_request(OpenSqlLabRequest)
def open_sql_lab_with_context(
request: OpenSqlLabRequest, ctx: Context
diff --git a/tests/unit_tests/mcp_service/conftest.py b/tests/unit_tests/mcp_service/conftest.py
index 92868309552..fbb14f270df 100644
--- a/tests/unit_tests/mcp_service/conftest.py
+++ b/tests/unit_tests/mcp_service/conftest.py
@@ -18,6 +18,21 @@
"""
MCP service test configuration.
-Tool imports are handled by app.py, not here.
-This conftest is empty to prevent test pollution.
+Disables RBAC permission checks for integration tests.
+RBAC logic is tested directly in test_auth_rbac.py.
"""
+
+import pytest
+
+
+@pytest.fixture(autouse=True)
+def disable_mcp_rbac(app):
+ """Disable RBAC permission checks for MCP integration tests.
+
+ The RBAC permission logic is tested directly in test_auth_rbac.py.
+ Integration tests use mock users that do not have real FAB roles,
+ so we disable RBAC to let them exercise tool logic.
+ """
+ app.config["MCP_RBAC_ENABLED"] = False
+ yield
+ app.config.pop("MCP_RBAC_ENABLED", None)
diff --git a/tests/unit_tests/mcp_service/test_auth_rbac.py b/tests/unit_tests/mcp_service/test_auth_rbac.py
new file mode 100644
index 00000000000..3abc87be500
--- /dev/null
+++ b/tests/unit_tests/mcp_service/test_auth_rbac.py
@@ -0,0 +1,225 @@
+# 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 MCP RBAC permission checking (auth.py)."""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+from flask import g
+
+from superset.mcp_service.auth import (
+ check_tool_permission,
+ CLASS_PERMISSION_ATTR,
+ MCPPermissionDeniedError,
+ METHOD_PERMISSION_ATTR,
+ PERMISSION_PREFIX,
+)
+
+
+@pytest.fixture(autouse=True)
+def enable_mcp_rbac(app):
+ """Re-enable RBAC for dedicated RBAC tests.
+
+ The shared conftest disables RBAC for integration tests. This fixture
+ overrides that so we can test the actual permission checking logic.
+ """
+ app.config["MCP_RBAC_ENABLED"] = True
+ yield
+ app.config.pop("MCP_RBAC_ENABLED", None)
+
+
+class _ToolFunc:
+ """Minimal callable stand-in for a tool function in tests.
+
+ Unlike MagicMock, this does NOT auto-create attributes on access,
+ so ``getattr(func, ATTR, None)`` properly returns None when the
+ attribute hasn't been set.
+ """
+
+ def __init__(self, name: str = "test_tool"):
+ self.__name__ = name
+
+ def __call__(self, *args, **kwargs):
+ pass
+
+
+def _make_tool_func(
+ class_perm: str | None = None,
+ method_perm: str | None = None,
+) -> _ToolFunc:
+ """Create a tool function stub with optional permission attributes."""
+ func = _ToolFunc()
+ if class_perm is not None:
+ setattr(func, CLASS_PERMISSION_ATTR, class_perm)
+ if method_perm is not None:
+ setattr(func, METHOD_PERMISSION_ATTR, method_perm)
+ return func
+
+
+# -- MCPPermissionDeniedError --
+
+
+def test_permission_denied_error_message_basic() -> None:
+ err = MCPPermissionDeniedError(
+ permission_name="can_read",
+ view_name="Chart",
+ )
+ assert "can_read" in str(err)
+ assert "Chart" in str(err)
+ assert err.permission_name == "can_read"
+ assert err.view_name == "Chart"
+
+
+def test_permission_denied_error_message_with_user_and_tool() -> None:
+ err = MCPPermissionDeniedError(
+ permission_name="can_write",
+ view_name="Dashboard",
+ user="alice",
+ tool_name="generate_dashboard",
+ )
+ assert "alice" in str(err)
+ assert "generate_dashboard" in str(err)
+ assert "Dashboard" in str(err)
+
+
+# -- check_tool_permission --
+
+
+def test_check_tool_permission_no_class_permission_allows(app_context) -> None:
+ """Tools without class_permission_name should be allowed by default."""
+ g.user = MagicMock(username="admin")
+ func = _make_tool_func() # no class_permission_name
+ assert check_tool_permission(func) is True
+
+
+def test_check_tool_permission_no_user_denies(app_context) -> None:
+ """If no g.user, permission check should deny."""
+ g.user = None
+ func = _make_tool_func(class_perm="Chart")
+ assert check_tool_permission(func) is False
+
+
+def test_check_tool_permission_granted(app_context) -> None:
+ """When security_manager.can_access returns True, permission is granted."""
+ g.user = MagicMock(username="admin")
+ func = _make_tool_func(class_perm="Chart", method_perm="read")
+
+ mock_sm = MagicMock()
+ mock_sm.can_access = MagicMock(return_value=True)
+ with patch("superset.security_manager", mock_sm):
+ result = check_tool_permission(func)
+
+ assert result is True
+ mock_sm.can_access.assert_called_once_with("can_read", "Chart")
+
+
+def test_check_tool_permission_denied(app_context) -> None:
+ """When security_manager.can_access returns False, permission is denied."""
+ g.user = MagicMock(username="viewer")
+ func = _make_tool_func(class_perm="Dashboard", method_perm="write")
+
+ mock_sm = MagicMock()
+ mock_sm.can_access = MagicMock(return_value=False)
+ with patch("superset.security_manager", mock_sm):
+ result = check_tool_permission(func)
+
+ assert result is False
+ mock_sm.can_access.assert_called_once_with("can_write", "Dashboard")
+
+
+def test_check_tool_permission_default_method_is_read(app_context) -> None:
+ """When no method_permission_name is set, defaults to 'read'."""
+ g.user = MagicMock(username="admin")
+ func = _make_tool_func(class_perm="Dataset")
+ # No method_perm set - should default to "read"
+
+ mock_sm = MagicMock()
+ mock_sm.can_access = MagicMock(return_value=True)
+ with patch("superset.security_manager", mock_sm):
+ result = check_tool_permission(func)
+
+ assert result is True
+ mock_sm.can_access.assert_called_once_with("can_read", "Dataset")
+
+
+def test_check_tool_permission_disabled_via_config(app_context, app) -> None:
+ """When MCP_RBAC_ENABLED is False, permission checks are skipped."""
+ g.user = MagicMock(username="viewer")
+ func = _make_tool_func(class_perm="Chart", method_perm="read")
+
+ app.config["MCP_RBAC_ENABLED"] = False
+ try:
+ assert check_tool_permission(func) is True
+ finally:
+ app.config["MCP_RBAC_ENABLED"] = True
+
+
+# -- Permission constants --
+
+
+def test_permission_prefix() -> None:
+ assert PERMISSION_PREFIX == "can_"
+
+
+def test_class_permission_attr() -> None:
+ assert CLASS_PERMISSION_ATTR == "_class_permission_name"
+
+
+def test_method_permission_attr() -> None:
+ assert METHOD_PERMISSION_ATTR == "_method_permission_name"
+
+
+# -- create_tool_decorator permission metadata --
+
+
+def test_permission_attrs_read_tag() -> None:
+ """Tags with class_permission_name set method_permission to read."""
+ func = _make_tool_func(class_perm="Chart", method_perm="read")
+ assert getattr(func, CLASS_PERMISSION_ATTR) == "Chart"
+ assert getattr(func, METHOD_PERMISSION_ATTR) == "read"
+
+
+def test_permission_attrs_write_tag() -> None:
+ """mutate tag convention → method_permission = 'write'."""
+ func = _make_tool_func(class_perm="Chart", method_perm="write")
+ assert getattr(func, CLASS_PERMISSION_ATTR) == "Chart"
+ assert getattr(func, METHOD_PERMISSION_ATTR) == "write"
+
+
+def test_permission_attrs_custom_method() -> None:
+ """Explicit method_permission_name overrides tag-based default."""
+ func = _make_tool_func(class_perm="SQLLab", method_perm="execute_sql")
+ assert getattr(func, CLASS_PERMISSION_ATTR) == "SQLLab"
+ assert getattr(func, METHOD_PERMISSION_ATTR) == "execute_sql"
+
+
+def test_no_class_permission_means_no_attrs() -> None:
+ """Without class_permission_name, no permission attrs are set."""
+ func = _make_tool_func()
+ assert not hasattr(func, CLASS_PERMISSION_ATTR)
+ assert not hasattr(func, METHOD_PERMISSION_ATTR)
+
+
+# -- Fixture --
+
+
+@pytest.fixture
+def app_context(app):
+ """Provide Flask app context for tests needing g.user."""
+ with app.app_context():
+ yield