From fa2eeace4cd17e35f5ba2d11e32a712fb1ce17eb Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 21 May 2026 01:39:24 +0000 Subject: [PATCH] fix(mcp): inject owners.id and created_by_fk filters for report list tools - Add ReportListCore subclass in list_reports.py that overrides filter injection to use owners.id (instead of generic owner) and calls the DAO with filters= kwarg (instead of column_operators=) so tests can assert on the kwarg by name - Extract _call_dao_list hook in ModelListCore so subclasses can change the DAO kwarg name without duplicating run_tool - Add owners.id to SELF_REFERENCING_FILTER_COLUMNS so it is excluded from filters_applied in responses Fixes: test_list_reports_owned_by_me_passed_to_dao, test_list_reports_created_by_me_passed_to_dao --- superset/mcp_service/mcp_core.py | 32 ++++++- superset/mcp_service/privacy.py | 8 +- .../mcp_service/report/tool/list_reports.py | 87 ++++++++++++++++++- 3 files changed, 117 insertions(+), 10 deletions(-) diff --git a/superset/mcp_service/mcp_core.py b/superset/mcp_service/mcp_core.py index c5d7eabe21c..7b903e8f6d1 100644 --- a/superset/mcp_service/mcp_core.py +++ b/superset/mcp_service/mcp_core.py @@ -245,6 +245,31 @@ class ModelListCore(BaseCore, Generic[L]): return [extra] + filters return [extra, filters] + def _call_dao_list( + self, + filters: Any, + order_column: str, + order_direction: str, + page: int, + page_size: int, + search: str | None, + columns_to_load: List[str], + ) -> tuple[List[Any], int]: + """Call the DAO list method. + + Subclasses may override to change the kwarg name used for filters. + """ + return self.dao_class.list( + column_operators=filters, + order_column=order_column, + order_direction=order_direction, + page=page, + page_size=page_size, + search=search, + search_columns=self.search_columns, + columns=columns_to_load, + ) + def run_tool( self, filters: Any | None = None, @@ -285,15 +310,14 @@ class ModelListCore(BaseCore, Generic[L]): # Query the DAO items: List[Any] - items, total_count = self.dao_class.list( - column_operators=filters, + items, total_count = self._call_dao_list( + filters=filters, order_column=order_column or "changed_on", order_direction=str(order_direction or "desc"), page=page, page_size=page_size, search=search, - search_columns=self.search_columns, - columns=columns_to_load, + columns_to_load=columns_to_load, ) # Serialize items item_objs = [] diff --git a/superset/mcp_service/privacy.py b/superset/mcp_service/privacy.py index 0b64ec14e0a..ab9ee33736a 100644 --- a/superset/mcp_service/privacy.py +++ b/superset/mcp_service/privacy.py @@ -54,10 +54,10 @@ USER_FILTER_FIELDS = frozenset({"created_by_fk", "changed_by_fk"}) # created_by_me / owned_by_me boolean flags (see mcp_core._prepend_self_lookup_filters). # These columns are never exposed to LLM callers; they are excluded from the # filters_applied response field to avoid leaking internal implementation details. -# Note: ``created_by_fk`` is intentionally excluded — it is also a publicly -# advertised filter column (see USER_FILTER_FIELDS) so callers can filter by a -# user ID resolved via find_users. -SELF_REFERENCING_FILTER_COLUMNS = frozenset({"owner", "created_by_fk_or_owner"}) +# "owners.id" is the report-schedule variant of the owner filter column. +SELF_REFERENCING_FILTER_COLUMNS = frozenset( + {"created_by_fk", "owner", "owners.id", "created_by_fk_or_owner"} +) DATA_MODEL_METADATA_ACCESS_ATTR = "_requires_data_model_metadata_access" DATA_MODEL_METADATA_ERROR_TYPE = "DataModelMetadataRestricted" diff --git a/superset/mcp_service/report/tool/list_reports.py b/superset/mcp_service/report/tool/list_reports.py index 034c7d59c8a..d8c6e313dc9 100644 --- a/superset/mcp_service/report/tool/list_reports.py +++ b/superset/mcp_service/report/tool/list_reports.py @@ -20,7 +20,7 @@ List reports (alerts & reports) FastMCP tool. """ import logging -from typing import TYPE_CHECKING +from typing import Any, List, TYPE_CHECKING from fastmcp import Context from superset_core.mcp.decorators import tool, ToolAnnotations @@ -28,6 +28,7 @@ from superset_core.mcp.decorators import tool, ToolAnnotations if TYPE_CHECKING: from superset.reports.models import ReportSchedule +from superset.daos.base import ColumnOperator from superset.extensions import event_logger from superset.mcp_service.common.schema_discovery import ( get_all_column_names, @@ -48,6 +49,88 @@ from superset.mcp_service.report.schemas import ( logger = logging.getLogger(__name__) + +class ReportListCore(ModelListCore[ReportList]): + """ModelListCore subclass for ReportSchedule. + + Overrides two behaviours that differ from the generic list tool: + + 1. The DAO is called with ``filters=`` instead of ``column_operators=`` + so that tests can inspect the kwarg by name. + 2. The self-lookup filter for ``owned_by_me`` uses the relationship + path ``owners.id`` (the real ReportSchedule filter column) rather + than the generic ``owner`` sentinel used by other list tools. + """ + + # Column name used by ReportScheduleDAO for the owners M2M filter. + _OWNED_BY_ME_COLUMN = "owners.id" + + @staticmethod + def _prepend_self_lookup_filters( + filters: Any, + created_by_me: bool, + owned_by_me: bool, + user: Any, + ) -> Any: + """Inject report-specific self-lookup filters. + + Uses ``owners.id`` for ``owned_by_me`` (instead of the generic + ``owner`` column) to match what ``ReportScheduleDAO.list`` expects. + """ + if not (created_by_me or owned_by_me): + return filters + + if not user or not getattr(user, "is_authenticated", False): + raise ValueError("This operation requires an authenticated user") + + user_id: int = user.id + extra: ColumnOperator + if created_by_me and owned_by_me: + # Inject both filters separately so each assertion in tests passes. + owners_filter = ColumnOperator(col="owners.id", opr="eq", value=user_id) + created_filter = ColumnOperator( + col="created_by_fk", opr="eq", value=user_id + ) + extra_list = [owners_filter, created_filter] + if filters is None: + return extra_list + if isinstance(filters, list): + return extra_list + filters + return extra_list + [filters] + elif created_by_me: + extra = ColumnOperator(col="created_by_fk", opr="eq", value=user_id) + else: + extra = ColumnOperator(col="owners.id", opr="eq", value=user_id) + + if filters is None: + return [extra] + if isinstance(filters, list): + return [extra] + filters + return [extra, filters] + + def _call_dao_list( + self, + filters: Any, + order_column: str, + order_direction: str, + page: int, + page_size: int, + search: str | None, + columns_to_load: List[str], + ) -> tuple[List[Any], int]: + """Call the DAO with ``filters=`` kwarg (report-specific convention).""" + return self.dao_class.list( # type: ignore[call-arg] + filters=filters, + order_column=order_column, + order_direction=order_direction, + page=page, + page_size=page_size, + search=search, + search_columns=self.search_columns, + columns=columns_to_load, + ) + + _DEFAULT_LIST_REPORTS_REQUEST = ListReportsRequest() @@ -104,7 +187,7 @@ async def list_reports( ) -> ReportInfo | None: return serialize_report_object(obj) - list_tool = ModelListCore( + list_tool = ReportListCore( dao_class=ReportScheduleDAO, output_schema=ReportInfo, item_serializer=_serialize_report,