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/04/17 14:36:37 UTC
(superset) 02/05: fix(mcp): prevent LLM from creating new dashboard instead of adding chart to existing one (#39353)
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 129fcb54512a3b02142bd80d57fec9a8fb6f127b
Author: Gabriel Torres Ruiz <ga...@gmail.com>
AuthorDate: Thu Apr 16 18:52:53 2026 -0300
fix(mcp): prevent LLM from creating new dashboard instead of adding chart to existing one (#39353)
(cherry picked from commit c2897312123a753a2feb217c2f73c8f1662c8255)
---
superset/mcp_service/app.py | 11 +
superset/mcp_service/dashboard/schemas.py | 9 +
.../tool/add_chart_to_existing_dashboard.py | 56 +++-
.../dashboard/tool/generate_dashboard.py | 5 +-
.../tool/test_add_chart_to_existing_dashboard.py | 297 +++++++++++++++++++++
.../dashboard/tool/test_dashboard_generation.py | 11 +-
6 files changed, 373 insertions(+), 16 deletions(-)
diff --git a/superset/mcp_service/app.py b/superset/mcp_service/app.py
index 40b5dc52815..5347f323347 100644
--- a/superset/mcp_service/app.py
+++ b/superset/mcp_service/app.py
@@ -104,6 +104,12 @@ Some tools do not use a request wrapper, so follow each tool's schema
Recommended Workflows:
+To add a chart to an existing dashboard:
+1. add_chart_to_existing_dashboard(dashboard_id, chart_id) -> updates dashboard directly
+ - If permission_denied=True is returned: inform the user they lack edit rights,
+ then ask if they want a new dashboard created instead. Only call generate_dashboard
+ after they confirm. Never silently create a new dashboard without asking first.
+
To create a chart:
1. list_datasets(request={{}}) -> find a dataset
2. get_dataset_info(request={{"identifier": <id>}})
@@ -206,6 +212,11 @@ CRITICAL RULES - NEVER VIOLATE:
- NEVER fabricate or invent URLs. ALL URLs must come from tool call results.
If you need a link, call the appropriate tool (generate_explore_link, generate_chart,
open_sql_lab_with_context, etc.) and use the URL it returns.
+- NEVER call generate_dashboard when the user wants to add a chart to an EXISTING
+ dashboard. Always use add_chart_to_existing_dashboard. Only call generate_dashboard
+ to create a brand-new dashboard, or after the user explicitly confirms they want
+ a new one (e.g., after a permission_denied=True response from
+ add_chart_to_existing_dashboard).
- To modify an existing chart's filters, metrics, or dimensions, use update_chart.
Do NOT use execute_sql for chart modifications.
- Parameter name reminders: ALWAYS use the EXACT parameter names from the tool schema.
diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py
index e61573dd7ad..6973245ad64 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -493,6 +493,15 @@ class AddChartToDashboardResponse(BaseModel):
None, description="Position information for the added chart"
)
error: str | None = Field(None, description="Error message, if operation failed")
+ permission_denied: bool = Field(
+ default=False,
+ description=(
+ "True when the operation failed because the current user does not "
+ "have edit rights on the target dashboard. When True, inform the "
+ "user and ask if they would like a new dashboard created instead. "
+ "Do NOT silently create a new dashboard — always confirm first."
+ ),
+ )
class GenerateDashboardRequest(BaseModel):
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 d3d9293ce5b..da511c29eb3 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
@@ -315,6 +315,47 @@ def _ensure_layout_structure(
layout["DASHBOARD_VERSION_KEY"] = "v2"
+def _find_and_authorize_dashboard(
+ dashboard_id: int,
+) -> tuple[Any, AddChartToDashboardResponse | None]:
+ """Return (dashboard, None) on success or (None, error_response) on failure.
+
+ Handles both the not-found case and the ownership check so the main tool
+ function doesn't need two separate branches for these pre-conditions.
+ """
+ from superset import security_manager
+ from superset.daos.dashboard import DashboardDAO
+ from superset.exceptions import SupersetSecurityException
+
+ dashboard = DashboardDAO.find_by_id(dashboard_id)
+ if not dashboard:
+ return None, AddChartToDashboardResponse(
+ dashboard=None,
+ dashboard_url=None,
+ position=None,
+ error=f"Dashboard with ID {dashboard_id} not found",
+ )
+
+ try:
+ security_manager.raise_for_ownership(dashboard)
+ except SupersetSecurityException:
+ return None, AddChartToDashboardResponse(
+ dashboard=None,
+ dashboard_url=None,
+ position=None,
+ permission_denied=True,
+ error=(
+ f"You don't have permission to edit dashboard "
+ f"'{dashboard.dashboard_title}' (ID: {dashboard_id}). "
+ "Ask the user if they would like a new dashboard "
+ "created with this chart instead, and only proceed "
+ "if they confirm."
+ ),
+ )
+
+ return dashboard, None
+
+
@tool(
tags=["mutate"],
class_permission_name="Dashboard",
@@ -333,18 +374,12 @@ def add_chart_to_existing_dashboard(
"""
try:
from superset.commands.dashboard.update import UpdateDashboardCommand
- from superset.daos.dashboard import DashboardDAO
- # Validate dashboard and chart exist
+ # Validate dashboard exists and user has edit permission
with event_logger.log_context(action="mcp.add_chart_to_dashboard.validation"):
- dashboard = DashboardDAO.find_by_id(request.dashboard_id)
- if not dashboard:
- return AddChartToDashboardResponse(
- dashboard=None,
- dashboard_url=None,
- position=None,
- error=(f"Dashboard with ID {request.dashboard_id} not found"),
- )
+ dashboard, auth_error = _find_and_authorize_dashboard(request.dashboard_id)
+ if auth_error is not None:
+ return auth_error
# Get chart object for SQLAlchemy relationships and validation
from superset import db
@@ -442,6 +477,7 @@ def add_chart_to_existing_dashboard(
# trigger lazy-loading on the same dead session.
from sqlalchemy.orm import subqueryload
+ from superset.daos.dashboard import DashboardDAO
from superset.models.dashboard import Dashboard
from superset.models.slice import Slice
diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py
index 1cec383f643..aa1bb900a31 100644
--- a/superset/mcp_service/dashboard/tool/generate_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py
@@ -189,9 +189,12 @@ def _generate_title_from_charts(chart_objects: List[Any]) -> str:
def generate_dashboard( # noqa: C901
request: GenerateDashboardRequest, ctx: Context
) -> GenerateDashboardResponse:
- """Create dashboard from chart IDs.
+ """Create a NEW dashboard from chart IDs.
IMPORTANT:
+ - Use this tool ONLY when creating a brand-new dashboard.
+ - To add a chart to an EXISTING dashboard, use add_chart_to_existing_dashboard.
+ Never use this tool as a fallback when add_chart_to_existing_dashboard fails.
- All charts must exist and be accessible to current user
- Charts arranged automatically in 2-column grid layout
diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py b/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py
new file mode 100644
index 00000000000..de8058c92de
--- /dev/null
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_add_chart_to_existing_dashboard.py
@@ -0,0 +1,297 @@
+# 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.
+
+"""
+Unit tests for add_chart_to_existing_dashboard MCP tool.
+
+Follows the same pattern used in test_dashboard_generation.py:
+- Tests run through the async MCP Client (not direct function calls)
+- Patches applied at source locations (superset.daos.dashboard.*, superset.db.*, etc.)
+- auth is mocked via the autouse mock_auth fixture (same as other tool test files)
+
+Covers:
+- Dashboard not found
+- Permission denied (user does not own the dashboard) -> permission_denied=True
+- Chart not found
+- Chart already in dashboard
+- Successful add (happy path)
+"""
+
+import logging
+from unittest.mock import Mock, patch
+
+import pytest
+from fastmcp import Client
+
+from superset.mcp_service.app import mcp
+from superset.mcp_service.chart.chart_utils import DatasetValidationResult
+
+logging.basicConfig(level=logging.DEBUG)
+logger = logging.getLogger(__name__)
+
+
+# ---------------------------------------------------------------------------
+# Fixtures
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture
+def mcp_server() -> object:
+ """Return the FastMCP app instance for use in MCP client tests."""
+ return mcp
+
+
+@pytest.fixture(autouse=True)
+def mock_auth():
+ """Mock authentication for all tests."""
+ with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
+
+
+@pytest.fixture(autouse=True)
+def mock_chart_access():
+ """Allow chart data access by default so tests focus on dashboard logic."""
+ with patch(
+ "superset.mcp_service.auth.check_chart_data_access",
+ return_value=DatasetValidationResult(
+ is_valid=True,
+ dataset_id=1,
+ dataset_name="test_dataset",
+ warnings=[],
+ error=None,
+ ),
+ ):
+ yield
+
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def _mock_chart(id: int = 10, slice_name: str = "Test Chart") -> Mock:
+ """Create a minimal mock Slice object with the given ID and name."""
+ chart = Mock()
+ chart.id = id
+ chart.slice_name = slice_name
+ chart.uuid = f"chart-uuid-{id}"
+ chart.tags = []
+ chart.owners = []
+ chart.viz_type = "table"
+ chart.datasource_name = None
+ chart.description = None
+ return chart
+
+
+def _mock_dashboard(
+ id: int = 1,
+ title: str = "Sales Dashboard",
+ slices: list[Mock] | None = None,
+) -> Mock:
+ """Create a minimal mock Dashboard object with the given ID, title and slices."""
+ dashboard = Mock()
+ dashboard.id = id
+ dashboard.dashboard_title = title
+ dashboard.slug = f"test-dashboard-{id}"
+ dashboard.description = None
+ dashboard.published = True
+ dashboard.created_on = None
+ dashboard.changed_on = None
+ dashboard.created_by_name = "test_user"
+ dashboard.changed_by_name = "test_user"
+ dashboard.uuid = f"dashboard-uuid-{id}"
+ dashboard.slices = slices or []
+ dashboard.owners = []
+ dashboard.tags = []
+ dashboard.roles = []
+ dashboard.position_json = "{}"
+ dashboard.json_metadata = None
+ dashboard.css = None
+ dashboard.certified_by = None
+ dashboard.certification_details = None
+ dashboard.is_managed_externally = False
+ dashboard.external_url = None
+ return dashboard
+
+
+# ---------------------------------------------------------------------------
+# Tests
+# ---------------------------------------------------------------------------
+
+
+@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_dashboard_not_found(mock_find_by_id: Mock, mcp_server: object) -> None:
+ """Returns a clear error when the target dashboard does not exist."""
+ mock_find_by_id.return_value = None
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard",
+ {"request": {"dashboard_id": 999, "chart_id": 10}},
+ )
+
+ assert result.structured_content["dashboard"] is None
+ assert result.structured_content["dashboard_url"] is None
+ assert result.structured_content["permission_denied"] is False
+ assert "not found" in (result.structured_content["error"] or "").lower()
+
+
+@patch("superset.security_manager.raise_for_ownership")
+@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_permission_denied(
+ mock_find_by_id: Mock, mock_raise_for_ownership: Mock, mcp_server: object
+) -> None:
+ """Returns permission_denied=True and an actionable error when the user
+ cannot edit the dashboard.
+
+ This is the core regression test for the bug fix: before the fix the tool
+ returned a generic error that caused the LLM to silently call
+ generate_dashboard instead. After the fix it returns permission_denied=True
+ with a message that explicitly tells the LLM to ask the user first.
+ """
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ dashboard = _mock_dashboard(id=1, title="Sales Dashboard")
+ mock_find_by_id.return_value = dashboard
+ mock_raise_for_ownership.side_effect = SupersetSecurityException(
+ SupersetError(
+ message="Changing this Dashboard is forbidden",
+ error_type=SupersetErrorType.GENERIC_BACKEND_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard",
+ {"request": {"dashboard_id": 1, "chart_id": 10}},
+ )
+
+ content = result.structured_content
+ assert content["dashboard"] is None
+ assert content["permission_denied"] is True, (
+ "Expected permission_denied=True so the LLM knows to ask the user "
+ "before creating a new dashboard — this is the fix for the bug"
+ )
+ assert content["error"] is not None
+ assert "Sales Dashboard" in content["error"]
+ assert "permission" in content["error"].lower()
+ assert "new dashboard" in content["error"].lower()
+
+
+@patch("superset.db.session.get")
+@patch("superset.security_manager.raise_for_ownership")
+@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_chart_not_found(
+ mock_find_by_id: Mock,
+ mock_raise_for_ownership: Mock,
+ mock_session_get: Mock,
+ mcp_server: object,
+) -> None:
+ """Returns an error when the requested chart does not exist."""
+ dashboard = _mock_dashboard()
+ mock_find_by_id.return_value = dashboard
+ mock_raise_for_ownership.return_value = None
+ mock_session_get.return_value = None # chart not found
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard",
+ {"request": {"dashboard_id": 1, "chart_id": 99}},
+ )
+
+ content = result.structured_content
+ assert content["dashboard"] is None
+ assert content["permission_denied"] is False
+ assert "99" in (content["error"] or "")
+
+
+@patch("superset.db.session.get")
+@patch("superset.security_manager.raise_for_ownership")
+@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_chart_already_in_dashboard(
+ mock_find_by_id: Mock,
+ mock_raise_for_ownership: Mock,
+ mock_session_get: Mock,
+ mcp_server: object,
+) -> None:
+ """Returns an error when the chart is already present on the dashboard."""
+ chart = _mock_chart(id=10)
+ dashboard = _mock_dashboard(slices=[chart])
+ mock_find_by_id.return_value = dashboard
+ mock_raise_for_ownership.return_value = None
+ mock_session_get.return_value = chart
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard",
+ {"request": {"dashboard_id": 1, "chart_id": 10}},
+ )
+
+ content = result.structured_content
+ assert content["dashboard"] is None
+ assert content["permission_denied"] is False
+ assert "already" in (content["error"] or "").lower()
+
+
+@patch("superset.commands.dashboard.update.UpdateDashboardCommand")
+@patch("superset.db.session.get")
+@patch("superset.security_manager.raise_for_ownership")
+@patch("superset.daos.dashboard.DashboardDAO.find_by_id")
+@pytest.mark.asyncio
+async def test_successful_add(
+ mock_find_by_id: Mock,
+ mock_raise_for_ownership: Mock,
+ mock_session_get: Mock,
+ mock_update_cmd_cls: Mock,
+ mcp_server: object,
+) -> None:
+ """Happy path: chart added, permission_denied=False, URL and position returned."""
+ chart = _mock_chart(id=10)
+ dashboard = _mock_dashboard(id=1)
+ updated_dashboard = _mock_dashboard(id=1, slices=[chart])
+
+ mock_find_by_id.side_effect = [dashboard, updated_dashboard]
+ mock_raise_for_ownership.return_value = None
+ mock_session_get.return_value = chart
+
+ mock_update_cmd = Mock()
+ mock_update_cmd.run.return_value = updated_dashboard
+ mock_update_cmd_cls.return_value = mock_update_cmd
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "add_chart_to_existing_dashboard",
+ {"request": {"dashboard_id": 1, "chart_id": 10}},
+ )
+
+ content = result.structured_content
+ assert content["error"] is None
+ assert content["permission_denied"] is False
+ assert content["dashboard_url"] is not None
+ assert "/superset/dashboard/1/" in content["dashboard_url"]
+ assert content["position"] is not None
+ assert "chart_key" in content["position"]
diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
index 04a3b5a568a..b5df3e91abc 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_generation.py
@@ -52,11 +52,12 @@ def mcp_server():
def mock_auth():
"""Mock authentication for all tests."""
with patch("superset.mcp_service.auth.get_user_from_request") as mock_get_user:
- mock_user = Mock()
- mock_user.id = 1
- mock_user.username = "admin"
- mock_get_user.return_value = mock_user
- yield mock_get_user
+ with patch("superset.security_manager.raise_for_ownership"):
+ mock_user = Mock()
+ mock_user.id = 1
+ mock_user.username = "admin"
+ mock_get_user.return_value = mock_user
+ yield mock_get_user
@pytest.fixture(autouse=True)