You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by vi...@apache.org on 2026/09/16 20:00:44 UTC
(superset) 15/34: fix(mcp): refuse certification changes on externally managed dashboards (#44094)
This is an automated email from the ASF dual-hosted git repository.
villebro pushed a commit to branch 7.0
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 9f708957922bf943cc386d06139b5f0347d4bdd7
Author: Mike Bridge <mi...@preset.io>
AuthorDate: Mon Sep 14 16:10:17 2026 -0600
fix(mcp): refuse certification changes on externally managed dashboards (#44094)
Co-authored-by: Claude Fable 5 <no...@anthropic.com>
(cherry picked from commit d3906aabf364e3b7efde3c1dbb44abcd69c55857)
---
superset/mcp_service/dashboard/schemas.py | 14 +++-
.../tool/manage_dashboard_certification.py | 25 +++++++
.../tool/test_manage_dashboard_certification.py | 79 ++++++++++++++++++++++
3 files changed, 117 insertions(+), 1 deletion(-)
diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py
index df0c757dc61..4ee2b3cb91e 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -1070,7 +1070,19 @@ class DashboardMutationErrorFields(BaseModel):
error: str | None = Field(None, description="Error message, if operation failed")
permission_denied: bool = Field(
default=False,
- description=("True when the user lacks edit rights on the target dashboard."),
+ description=(
+ "True when the user lacks edit rights on the target dashboard. "
+ "Remediation: ask the user to grant access; do not retry as-is."
+ ),
+ )
+ managed_externally: bool = Field(
+ default=False,
+ description=(
+ "True when the mutation was refused because the dashboard is "
+ "managed externally. Structural, not an access denial: granting "
+ "permissions cannot resolve it and the call should not be "
+ "retried — the entity's source of truth lives outside Superset."
+ ),
)
diff --git a/superset/mcp_service/dashboard/tool/manage_dashboard_certification.py b/superset/mcp_service/dashboard/tool/manage_dashboard_certification.py
index 0c7ebedc6bd..67582394d93 100644
--- a/superset/mcp_service/dashboard/tool/manage_dashboard_certification.py
+++ b/superset/mcp_service/dashboard/tool/manage_dashboard_certification.py
@@ -66,6 +66,12 @@ def manage_dashboard_certification(
clear it, or pass a value to set it. Certification surfaces as a badge
next to the dashboard title in the UI.
+ Externally managed dashboards refuse set/clear — the response carries
+ ``managed_externally=True``; the refusal is structural (the badge is
+ owned by the external system), so granting permissions cannot resolve
+ it and the call should not be retried. The no-field inspect call
+ still returns current values for such dashboards.
+
Example::
manage_dashboard_certification(request={
@@ -92,6 +98,25 @@ def manage_dashboard_certification(
warnings=["No fields provided; dashboard unchanged."],
)
+ # Externally managed dashboards refuse certification CHANGES (the
+ # no-field inspect path above still returns current values): their
+ # source of truth lives outside Superset, so a badge set here would be
+ # overwritten (or drift from the certifying system) on the next
+ # external sync. This tool writes by direct attribute assignment +
+ # commit rather than through a command, so no command-layer check can
+ # protect it — the refusal must live in the tool itself, after the
+ # editorship check, where a caller with no edit rights keeps getting
+ # the plain editorship denial.
+ if dashboard.is_managed_externally:
+ return ManageDashboardCertificationResponse(
+ managed_externally=True,
+ error=(
+ f"Dashboard '{dashboard.dashboard_title}' (ID: {dashboard.id}) "
+ "is managed externally; its certification is owned by the "
+ "external system and cannot be changed here."
+ ),
+ )
+
changed_fields: list[str] = []
warnings: list[str] = []
# Captured before commit so the final response never has to dereference
diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py b/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py
index 6c924a2bb2a..76a8b18432e 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_manage_dashboard_certification.py
@@ -22,6 +22,7 @@ from unittest.mock import Mock, patch
import pytest
from fastmcp import Client
+from fastmcp.client.client import CallToolResult
from superset.utils import json
@@ -34,6 +35,7 @@ def _mock_dashboard(
slug: str | None = "test-slug",
certified_by: str | None = None,
certification_details: str | None = None,
+ is_managed_externally: bool = False,
) -> Mock:
dashboard: Mock = Mock()
dashboard.id = id
@@ -41,10 +43,87 @@ def _mock_dashboard(
dashboard.slug = slug
dashboard.certified_by = certified_by
dashboard.certification_details = certification_details
+ # Declared explicitly: a bare Mock attribute is truthy, which would
+ # trip the managed-externally refusal in every test (real dashboards
+ # default the flag to False).
+ dashboard.is_managed_externally = is_managed_externally
return dashboard
class TestManageDashboardCertification:
+ @pytest.mark.parametrize("field", ["certified_by", "certification_details"])
+ @pytest.mark.parametrize("value", ["Data Platform Team", ""])
+ @patch(DAO_GET)
+ @patch("superset.extensions.db.session")
+ @pytest.mark.asyncio
+ async def test_refuses_externally_managed_dashboard(
+ self,
+ mock_session: Mock,
+ mock_get: Mock,
+ mcp_server: object,
+ field: str,
+ value: str,
+ ) -> None:
+ """sc-120051: certification changes on a managed dashboard are refused.
+
+ This tool writes by direct attribute assignment + commit rather
+ than through a command, so no command-layer check can protect it —
+ the refusal lives in the tool, and nothing may be assigned or
+ committed on the refused path. Parametrized across BOTH badge
+ fields and both set and clear, so gating one field but not the
+ other cannot pass."""
+ dash: Mock = _mock_dashboard(is_managed_externally=True)
+ mock_get.return_value = dash
+
+ async with Client(mcp_server) as client:
+ result: CallToolResult = await client.call_tool(
+ "manage_dashboard_certification",
+ {"request": {"identifier": 42, field: value}},
+ )
+
+ payload: dict[str, Any] = json.loads(result.content[0].text)
+ # Structural refusal, NOT an ACL denial: the flags must not be
+ # conflated — permission_denied's remediation ("ask for access")
+ # can never resolve a managed-externally refusal.
+ assert payload["managed_externally"] is True
+ assert payload["permission_denied"] is False
+ assert "managed externally" in payload["error"]
+ assert dash.certified_by is None
+ assert dash.certification_details is None
+ mock_session.commit.assert_not_called()
+
+ @patch(DAO_GET)
+ @patch("superset.extensions.db.session")
+ @pytest.mark.asyncio
+ async def test_no_fields_on_managed_dashboard_still_inspects(
+ self, mock_session: Mock, mock_get: Mock, mcp_server: object
+ ) -> None:
+ """The no-field inspect path is not gated: reading changes nothing.
+
+ The refusal covers CHANGES only; a no-field call on a managed
+ dashboard returns current values (which the caller can read via
+ get_dashboard_info anyway) instead of a misleading denial."""
+ dash: Mock = _mock_dashboard(
+ certified_by="External Certifier",
+ certification_details="Synced from the source of truth",
+ is_managed_externally=True,
+ )
+ mock_get.return_value = dash
+
+ async with Client(mcp_server) as client:
+ result: CallToolResult = await client.call_tool(
+ "manage_dashboard_certification",
+ {"request": {"identifier": 42}},
+ )
+
+ payload: dict[str, Any] = json.loads(result.content[0].text)
+ assert payload.get("permission_denied") is not True
+ assert payload.get("managed_externally") is not True
+ assert payload["certified_by"] == "External Certifier"
+ assert payload["certification_details"] == "Synced from the source of truth"
+ assert payload["changed_fields"] == []
+ mock_session.commit.assert_not_called()
+
@patch(DAO_GET)
@patch("superset.extensions.db.session")
@pytest.mark.asyncio