From 0bd7ea30de4e30a3ddd598246c81f29514ce7382 Mon Sep 17 00:00:00 2001 From: "jaehoon.kim" Date: Tue, 23 Jun 2026 15:52:54 +0900 Subject: [PATCH 1/4] fix(mcp): serialize dashboard role permissions --- superset/mcp_service/dashboard/schemas.py | 33 +++++++++++++++++-- .../dashboard/tool/test_dashboard_tools.py | 19 +++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index 73a57a6e292e..f5a7cece62b2 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -79,6 +79,7 @@ model_validator, PositiveInt, ) +from sqlalchemy.orm.exc import DetachedInstanceError if TYPE_CHECKING: from superset.models.dashboard import Dashboard @@ -152,15 +153,41 @@ def serialize_role_object(role: Any) -> RoleInfo | None: if not role: return None + try: + raw_permissions = getattr(role, "permissions", None) + except DetachedInstanceError: + raw_permissions = None + + permissions: list[str] | None = None + if raw_permissions is not None: + permissions = [] + try: + for permission in raw_permissions: + permission_name = _serialize_permission_name(permission) + if permission_name is not None: + permissions.append(permission_name) + except (DetachedInstanceError, TypeError): + permissions = [] + return RoleInfo( id=getattr(role, "id", None), name=getattr(role, "name", None), - permissions=[perm.name for perm in getattr(role, "permissions", [])] - if hasattr(role, "permissions") - else None, + permissions=permissions, ) +def _serialize_permission_name(permission: Any) -> str | None: + if (name := getattr(permission, "name", None)) is not None: + return str(name) + + permission_name = getattr(getattr(permission, "permission", None), "name", None) + view_menu_name = getattr(getattr(permission, "view_menu", None), "name", None) + if permission_name and view_menu_name: + return f"{permission_name} on {view_menu_name}" + + return None + + class DashboardFilter(ColumnOperator): """ Filter object for dashboard listing. diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py index 3cabe42f471e..f54015138ebb 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py @@ -21,6 +21,7 @@ import logging from importlib import import_module +from types import SimpleNamespace from unittest.mock import Mock, patch import pytest @@ -31,6 +32,7 @@ from superset.mcp_service.app import mcp from superset.mcp_service.dashboard.schemas import ( ListDashboardsRequest, + serialize_role_object, ) from superset.mcp_service.dashboard.tool.get_dashboard_info import ( _refresh_request_user_for_permalink_access, @@ -134,6 +136,23 @@ async def test_list_dashboards_basic(mock_list, mcp_server): assert "slug" in data["columns_loaded"] +def test_dashboard_role_serializer_serializes_permission_view_names(): + permission_view = SimpleNamespace( + permission=SimpleNamespace(name="can_read"), + view_menu=SimpleNamespace(name="Dashboard"), + ) + role = SimpleNamespace(id=1, name="Gamma", permissions=[permission_view]) + + role_info = serialize_role_object(role) + + assert role_info is not None + assert role_info.model_dump() == { + "id": 1, + "name": "Gamma", + "permissions": ["can_read on Dashboard"], + } + + @patch("superset.daos.dashboard.DashboardDAO.list") @pytest.mark.asyncio async def test_list_dashboards_with_filters(mock_list, mcp_server): From f1ecd71f24097c2f870c2b29aaad993bf5a3c8c9 Mon Sep 17 00:00:00 2001 From: "jaehoon.kim" Date: Thu, 25 Jun 2026 13:15:47 +0900 Subject: [PATCH 2/4] chore(mcp): address dashboard role review --- superset/mcp_service/dashboard/schemas.py | 4 ++++ .../mcp_service/dashboard/tool/test_dashboard_tools.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index f5a7cece62b2..fee514cb160c 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -177,6 +177,10 @@ def serialize_role_object(role: Any) -> RoleInfo | None: def _serialize_permission_name(permission: Any) -> str | None: + """Return direct permission names or FAB permission/view pairs. + + Returns None when neither representation is available. + """ if (name := getattr(permission, "name", None)) is not None: return str(name) diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py index f54015138ebb..27d64068fb50 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py @@ -136,7 +136,7 @@ async def test_list_dashboards_basic(mock_list, mcp_server): assert "slug" in data["columns_loaded"] -def test_dashboard_role_serializer_serializes_permission_view_names(): +def test_dashboard_role_serializer_serializes_permission_view_names() -> None: permission_view = SimpleNamespace( permission=SimpleNamespace(name="can_read"), view_menu=SimpleNamespace(name="Dashboard"), From 75f8215015c9625ea2a1b88271c9c1eb358f26b4 Mon Sep 17 00:00:00 2001 From: "jaehoon.kim" Date: Thu, 25 Jun 2026 13:20:53 +0900 Subject: [PATCH 3/4] fix(mcp): preserve valid role permissions --- superset/mcp_service/dashboard/schemas.py | 23 ++++++++++++---- .../dashboard/tool/test_dashboard_tools.py | 27 +++++++++++++++++++ 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py index fee514cb160c..63eea1ea3836 100644 --- a/superset/mcp_service/dashboard/schemas.py +++ b/superset/mcp_service/dashboard/schemas.py @@ -162,12 +162,25 @@ def serialize_role_object(role: Any) -> RoleInfo | None: if raw_permissions is not None: permissions = [] try: - for permission in raw_permissions: + permission_iterator = iter(raw_permissions) + except TypeError: + permission_iterator = iter(()) + + while True: + try: + permission = next(permission_iterator) + except StopIteration: + break + except (DetachedInstanceError, TypeError): + break + + try: permission_name = _serialize_permission_name(permission) - if permission_name is not None: - permissions.append(permission_name) - except (DetachedInstanceError, TypeError): - permissions = [] + except (DetachedInstanceError, TypeError): + continue + + if permission_name is not None: + permissions.append(permission_name) return RoleInfo( id=getattr(role, "id", None), diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py index 27d64068fb50..8bf234402c59 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py @@ -28,6 +28,7 @@ from fastmcp import Client from fastmcp.exceptions import ToolError from flask import g +from sqlalchemy.orm.exc import DetachedInstanceError from superset.mcp_service.app import mcp from superset.mcp_service.dashboard.schemas import ( @@ -153,6 +154,32 @@ def test_dashboard_role_serializer_serializes_permission_view_names() -> None: } +def test_dashboard_role_serializer_skips_bad_permission_items() -> None: + class DetachedPermission: + @property + def name(self) -> str: + raise DetachedInstanceError("detached permission") + + permission_view = SimpleNamespace( + permission=SimpleNamespace(name="can_read"), + view_menu=SimpleNamespace(name="Dashboard"), + ) + role = SimpleNamespace( + id=1, + name="Gamma", + permissions=[ + permission_view, + DetachedPermission(), + SimpleNamespace(name="can_write"), + ], + ) + + role_info = serialize_role_object(role) + + assert role_info is not None + assert role_info.permissions == ["can_read on Dashboard", "can_write"] + + @patch("superset.daos.dashboard.DashboardDAO.list") @pytest.mark.asyncio async def test_list_dashboards_with_filters(mock_list, mcp_server): From 84bbc705277d78a4f63a181cbf66c7f86beb6f27 Mon Sep 17 00:00:00 2001 From: "jaehoon.kim" Date: Thu, 25 Jun 2026 16:49:29 +0900 Subject: [PATCH 4/4] test(mcp): cover dashboard role permission edge cases --- .../dashboard/tool/test_dashboard_tools.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py index 8bf234402c59..858ed44d1b6a 100644 --- a/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py +++ b/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py @@ -180,6 +180,66 @@ def name(self) -> str: assert role_info.permissions == ["can_read on Dashboard", "can_write"] +def test_dashboard_role_serializer_handles_detached_permissions() -> None: + class DetachedRole: + id = 1 + name = "Gamma" + + @property + def permissions(self) -> list[object]: + raise DetachedInstanceError("detached permissions") + + role_info = serialize_role_object(DetachedRole()) + + assert role_info is not None + assert role_info.permissions is None + + +def test_dashboard_role_serializer_handles_non_iterable_permissions() -> None: + role = SimpleNamespace(id=1, name="Gamma", permissions=object()) + + role_info = serialize_role_object(role) + + assert role_info is not None + assert role_info.permissions == [] + + +def test_dashboard_role_serializer_stops_on_detached_permission_iterator() -> None: + class DetachedPermissionIterator: + def __iter__(self) -> "DetachedPermissionIterator": + return self + + def __next__(self) -> object: + raise DetachedInstanceError("detached permission iterator") + + role = SimpleNamespace( + id=1, + name="Gamma", + permissions=DetachedPermissionIterator(), + ) + + role_info = serialize_role_object(role) + + assert role_info is not None + assert role_info.permissions == [] + + +def test_dashboard_role_serializer_skips_unnamed_permission_items() -> None: + role = SimpleNamespace( + id=1, + name="Gamma", + permissions=[ + SimpleNamespace(permission=SimpleNamespace(name="can_read")), + SimpleNamespace(view_menu=SimpleNamespace(name="Dashboard")), + ], + ) + + role_info = serialize_role_object(role) + + assert role_info is not None + assert role_info.permissions == [] + + @patch("superset.daos.dashboard.DashboardDAO.list") @pytest.mark.asyncio async def test_list_dashboards_with_filters(mock_list, mcp_server):