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. +Authentication and authorization hooks for MCP tools. -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 +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. + + 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. Supports both sync and async tool functions. - - TODO (future PR): Add permission checking - TODO (future PR): Add JWT scope validation - TODO (future PR): Add comprehensive audit logging """ 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 f09d8f89fdf..7cec6e9e71f 100644 --- a/superset/mcp_service/chart/tool/generate_chart.py +++ b/superset/mcp_service/chart/tool/generate_chart.py @@ -118,7 +118,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 9239a278d9e..96e7b1845ad 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 95d0af23ff3..6c60214f705 100644 --- a/superset/mcp_service/chart/tool/get_chart_info.py +++ b/superset/mcp_service/chart/tool/get_chart_info.py @@ -56,7 +56,7 @@ def _get_cached_form_data(form_data_key: str) -> str | None: return None -@tool(tags=["discovery"]) +@tool(tags=["discovery"], class_permission_name="Chart") @parse_request(GetChartInfoRequest) async def get_chart_info( request: GetChartInfoRequest, ctx: Context diff --git a/superset/mcp_service/chart/tool/get_chart_preview.py b/superset/mcp_service/chart/tool/get_chart_preview.py index 6ee3ece4df4..579a2d748f5 100644 --- a/superset/mcp_service/chart/tool/get_chart_preview.py +++ b/superset/mcp_service/chart/tool/get_chart_preview.py @@ -2065,7 +2065,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 a3a72ad8166..3dacb3a7ec8 100644 --- a/superset/mcp_service/chart/tool/list_charts.py +++ b/superset/mcp_service/chart/tool/list_charts.py @@ -61,7 +61,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 b0a099888e1..7abc1ef9da3 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 7ff3d2bc84a..117a92a6331 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 2ffec57f267..19f17c95a5e 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 @@ -304,7 +304,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 8126ee387ca..61285c1a874 100644 --- a/superset/mcp_service/dashboard/tool/generate_dashboard.py +++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py @@ -177,7 +177,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 eaab375d6c8..78bfec2794a 100644 --- a/superset/mcp_service/dashboard/tool/list_dashboards.py +++ b/superset/mcp_service/dashboard/tool/list_dashboards.py @@ -62,7 +62,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 9404b93179d..1bdbcb381fd 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 b9a79b54b28..88fad7d4414 100644 --- a/superset/mcp_service/sql_lab/tool/execute_sql.py +++ b/superset/mcp_service/sql_lab/tool/execute_sql.py @@ -50,7 +50,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 d27b970d609..d9dc6bcd564 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