From 86575e129b72ab5bd9f23f59bb8ecad01eb55204 Mon Sep 17 00:00:00 2001 From: Luiz Otavio <45200344+luizotavio32@users.noreply.github.com> Date: Wed, 15 Apr 2026 08:16:12 -0300 Subject: [PATCH 001/261] fix(native-filters): prevent infinite recursion in filter scope tree traversal (#39355) --- .../FilterScope/utils.test.ts | 76 ++++++++++++++++++- .../FiltersConfigForm/FilterScope/utils.ts | 12 +++ 2 files changed, 87 insertions(+), 1 deletion(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.test.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.test.ts index 4ce48ed1cc7..2c73e920512 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.test.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.test.ts @@ -18,7 +18,7 @@ */ import { Layout, LayoutItem, Charts } from 'src/dashboard/types'; import { VizType } from '@superset-ui/core'; -import { buildTree } from './utils'; +import { buildTree, getTreeCheckedItems } from './utils'; import type { TreeItem } from './types'; // The types defined for Layout and sub elements is not compatible with the data we get back fro a real dashboard layout @@ -18048,6 +18048,80 @@ describe('Ensure buildTree does not throw runtime errors when encountering an in }).not.toThrow(); }); + test('Does not infinitely recurse when layout contains a cycle (nested tabs circular reference)', () => { + // Simulate a corrupted layout where TAB-A → TAB-B → TAB-A (cycle) + const circularLayout = { + ROOT_ID: { + id: 'ROOT_ID', + type: 'ROOT', + children: ['TAB-A'], + parents: [], + }, + 'TAB-A': { + id: 'TAB-A', + type: 'TAB', + children: ['TAB-B'], + parents: ['ROOT_ID'], + meta: { text: 'Tab A' }, + }, + 'TAB-B': { + id: 'TAB-B', + type: 'TAB', + // Points back to TAB-A, creating a cycle + children: ['TAB-A'], + parents: ['ROOT_ID', 'TAB-A'], + meta: { text: 'Tab B' }, + }, + }; + + const rootNode = circularLayout.ROOT_ID; + const rootTreeItem: TreeItem = { key: 'ROOT_ID', title: 'Root', children: [], nodeType: 'TAB' }; + + expect(() => { + buildTree( + rootNode as unknown as LayoutItem, + rootTreeItem, + circularLayout as unknown as Layout, + {} as Charts, + ['ROOT_ID', 'TAB-A', 'TAB-B'], + [], + () => 'title', + ); + }).not.toThrow(); + }); + + test('getTreeCheckedItems does not infinitely recurse when scope rootPath creates a cycle', () => { + // Simulate a corrupted layout where ROW-A → ROW-B → ROW-A (cycle via children) + const circularLayout = { + ROOT_ID: { + id: 'ROOT_ID', + type: 'ROOT', + children: ['ROW-A'], + parents: [], + }, + 'ROW-A': { + id: 'ROW-A', + type: 'ROW', + children: ['ROW-B'], + parents: ['ROOT_ID'], + }, + 'ROW-B': { + id: 'ROW-B', + type: 'ROW', + // Points back to ROW-A, creating a cycle + children: ['ROW-A'], + parents: ['ROOT_ID', 'ROW-A'], + }, + }; + + expect(() => { + getTreeCheckedItems( + { rootPath: ['ROOT_ID'], excluded: [] }, + circularLayout as unknown as Layout, + ); + }).not.toThrow(); + }); + test('Avoids runtime error with invalid inputs', () => { expect(() => { buildTree( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts index af62f19034b..8d61b956929 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/utils.ts @@ -100,6 +100,7 @@ export const buildTree = ( initiallyExcludedCharts: number[], buildTreeLeafTitle: BuildTreeLeafTitle, sliceEntities?: Record, + visited: Set = new Set(), ) => { if (!node) { return; @@ -154,6 +155,10 @@ export const buildTree = ( if (node.type !== CHART_TYPE) { node?.children?.forEach?.(child => { + if (visited.has(child)) { + return; + } + visited.add(child); const childNode = layout?.[child]; if (childNode) { buildTree( @@ -165,6 +170,7 @@ export const buildTree = ( initiallyExcludedCharts, buildTreeLeafTitle, sliceEntities, + visited, ); } else { logging.warn( @@ -192,13 +198,19 @@ const checkTreeItem = ( layout: Layout, items: string[], excluded: number[], + visited: Set = new Set(), ) => { items.forEach(item => { + if (visited.has(item)) { + return; + } + visited.add(item); checkTreeItem( checkedItems, layout, addInvisibleParents(layout, item), excluded, + visited, ); if ( layout[item]?.type === CHART_TYPE && From ffcc6e8b635fb66437a7ce855d98a2fe5ec3ce80 Mon Sep 17 00:00:00 2001 From: Alexandru Soare <37236580+alexandrusoare@users.noreply.github.com> Date: Wed, 15 Apr 2026 15:57:04 +0300 Subject: [PATCH 002/261] fix(MCP): fix MCP logs (#39159) --- superset/mcp_service/middleware.py | 17 +- superset/mcp_service/server.py | 46 ++--- superset/utils/log.py | 7 + tests/integration_tests/event_logger_tests.py | 61 +++++++ .../mcp_service/test_middleware_logging.py | 157 ++++++++++++++++++ 5 files changed, 266 insertions(+), 22 deletions(-) diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py index 2254e8cc597..797bb70076c 100644 --- a/superset/mcp_service/middleware.py +++ b/superset/mcp_service/middleware.py @@ -130,6 +130,18 @@ class LoggingMiddleware(Middleware): in on_message(). """ + def _is_error_response(self, result: ToolResult) -> bool: + """Check if a tool result contains an error schema response. + + MCP tools return error schemas (ChartError, DashboardError, etc.) + instead of raising exceptions. These serialize to JSON containing + an "error_type" field. + """ + try: + return '"error_type"' in result.content[0].text + except (AttributeError, IndexError): + return False + def _extract_context_info( self, context: MiddlewareContext ) -> tuple[ @@ -171,8 +183,11 @@ class LoggingMiddleware(Middleware): success = False try: result = await call_next(context) - success = True + success = not self._is_error_response(result) return result + except Exception: + success = False + raise finally: duration_ms = int((time.time() - start_time) * 1000) if has_app_context(): diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py index 8f0d6feccaf..074914de4e7 100644 --- a/superset/mcp_service/server.py +++ b/superset/mcp_service/server.py @@ -28,6 +28,7 @@ from collections.abc import Sequence from typing import Annotated, Any import uvicorn +from fastmcp.server.middleware import Middleware from superset.mcp_service.app import create_mcp_app, init_fastmcp_server from superset.mcp_service.mcp_config import ( @@ -492,6 +493,24 @@ def _create_auth_provider(flask_app: Any) -> Any | None: return auth_provider +def build_middleware_list() -> list[Middleware]: + """Build the core MCP middleware list in the correct order. + + FastMCP wraps handlers so that the FIRST-added middleware is + outermost. Order here is outermost → innermost: + + 1. StructuredContentStripper — safety net, converts exceptions + to safe ToolResult text for transports that can't encode errors + 2. LoggingMiddleware — logs tool calls with success/failure status + 3. GlobalErrorHandler — catches tool exceptions, raises ToolError + """ + return [ + StructuredContentStripperMiddleware(), + LoggingMiddleware(), + GlobalErrorHandlerMiddleware(), + ] + + def run_server( host: str = "127.0.0.1", port: int = 5008, @@ -539,30 +558,15 @@ def run_server( flask_app = get_flask_app() auth_provider = _create_auth_provider(flask_app) - # Build middleware list - # FastMCP wraps handlers so that the LAST-added middleware is - # outermost. Order here is innermost → outermost. - middleware_list = [] + middleware_list = build_middleware_list() - # Add caching middleware (innermost – runs closest to the tool) - if caching_middleware := create_response_caching_middleware(): - middleware_list.append(caching_middleware) - - # Add response size guard (protects LLM clients from huge responses) - if size_guard_middleware := create_response_size_guard_middleware(): + # Add optional middleware (innermost, closest to tool) + size_guard_middleware = create_response_size_guard_middleware() + if size_guard_middleware: middleware_list.append(size_guard_middleware) - # Add logging middleware (logs all tool calls with duration tracking) - middleware_list.append(LoggingMiddleware()) - - # Add global error handler (catches all exceptions, raises ToolError) - middleware_list.append(GlobalErrorHandlerMiddleware()) - - # Strip outputSchema from tool definitions and structuredContent from - # tool responses to prevent encoding errors on Claude.ai's MCP bridge. - # MUST be outermost so it catches ToolError from GlobalErrorHandler - # and converts to plain text before the MCP SDK tries to encode it. - middleware_list.append(StructuredContentStripperMiddleware()) + if caching_middleware := create_response_caching_middleware(): + middleware_list.append(caching_middleware) mcp_instance = init_fastmcp_server( auth=auth_provider, diff --git a/superset/utils/log.py b/superset/utils/log.py index 7387cafbfcc..b0667bae68f 100644 --- a/superset/utils/log.py +++ b/superset/utils/log.py @@ -384,6 +384,13 @@ class DBEventLogger(AbstractEventLogger): from superset.models.core import Log records = kwargs.get("records", []) + curated_payload = kwargs.get("curated_payload") + + # If no records but curated_payload exists, use it as a single record + # This enables MCP middleware logging which passes curated_payload + if not records and curated_payload: + records = [curated_payload] + logs = [] for record in records: json_string: str | None diff --git a/tests/integration_tests/event_logger_tests.py b/tests/integration_tests/event_logger_tests.py index f87b5a782e3..6bd8b96a945 100644 --- a/tests/integration_tests/event_logger_tests.py +++ b/tests/integration_tests/event_logger_tests.py @@ -254,3 +254,64 @@ class TestEventLogger(unittest.TestCase): } ] assert payload["duration_ms"] >= 100 + + @patch("superset.db") + def test_curated_payload_used_when_records_empty(self, mock_db): + """Test that curated_payload is used when records is empty (MCP pattern). + + MCP middleware passes curated_payload instead of records. This test verifies + that DBEventLogger.log() creates a Log entry from curated_payload when + records is empty. + """ + logger = DBEventLogger() + + with app.test_request_context(): + logger.log( + user_id=1, + action="mcp_tool_call", + dashboard_id=None, + duration_ms=100, + slice_id=None, + referrer=None, + curated_payload={"tool": "list_charts", "success": True}, + ) + + # Verify bulk_save_objects was called with one Log object + mock_db.session.bulk_save_objects.assert_called_once() + logs = mock_db.session.bulk_save_objects.call_args[0][0] + assert len(logs) == 1 + assert logs[0].action == "mcp_tool_call" + assert logs[0].duration_ms == 100 + # Verify JSON contains the curated_payload data + from superset.utils import json as json_utils + + payload = json_utils.loads(logs[0].json) + assert payload["tool"] == "list_charts" + assert payload["success"] is True + + @patch("superset.db") + def test_records_takes_precedence_over_curated_payload(self, mock_db): + """Test that records takes precedence over curated_payload.""" + logger = DBEventLogger() + + with app.test_request_context(): + logger.log( + user_id=1, + action="test_action", + dashboard_id=None, + duration_ms=50, + slice_id=None, + referrer=None, + records=[{"from_records": True}], + curated_payload={"from_curated": True}, + ) + + # Verify only records data is used, not curated_payload + mock_db.session.bulk_save_objects.assert_called_once() + logs = mock_db.session.bulk_save_objects.call_args[0][0] + assert len(logs) == 1 + from superset.utils import json as json_utils + + payload = json_utils.loads(logs[0].json) + assert payload.get("from_records") is True + assert "from_curated" not in payload diff --git a/tests/unit_tests/mcp_service/test_middleware_logging.py b/tests/unit_tests/mcp_service/test_middleware_logging.py index 1f48d531ccc..50d23449707 100644 --- a/tests/unit_tests/mcp_service/test_middleware_logging.py +++ b/tests/unit_tests/mcp_service/test_middleware_logging.py @@ -102,6 +102,34 @@ class TestLoggingMiddlewareOnCallTool: assert call_kwargs["curated_payload"]["success"] is False assert call_kwargs["duration_ms"] >= 0 + @patch("superset.mcp_service.middleware.event_logger") + @patch("superset.mcp_service.middleware.get_user_id", return_value=42) + @pytest.mark.asyncio + async def test_on_call_tool_logs_failure_on_tool_error( + self, mock_get_user_id, mock_event_logger + ): + """on_call_tool records success=False when GlobalErrorHandler raises ToolError. + + This simulates the real middleware chain: GlobalErrorHandler catches + tool exceptions and re-raises them as ToolError. Since LoggingMiddleware + sits between GlobalErrorHandler and StructuredContentStripper, it + catches the ToolError directly. + """ + from fastmcp.exceptions import ToolError + + middleware = LoggingMiddleware() + ctx = _make_context(name="get_chart_info") + call_next = AsyncMock(side_effect=ToolError("Chart 999999 not found")) + + with pytest.raises(ToolError, match="Chart 999999 not found"): + await middleware.on_call_tool(ctx, call_next) + + mock_event_logger.log.assert_called_once() + call_kwargs = mock_event_logger.log.call_args[1] + assert call_kwargs["curated_payload"]["success"] is False + assert call_kwargs["curated_payload"]["tool"] == "get_chart_info" + assert call_kwargs["duration_ms"] >= 0 + @patch("superset.mcp_service.middleware.event_logger") @patch("superset.mcp_service.middleware.get_user_id", return_value=42) @pytest.mark.asyncio @@ -205,3 +233,132 @@ class TestExtractContextInfo: _, _, _, slice_id, _, _ = middleware._extract_context_info(ctx) assert slice_id == 66 + + +class TestIsErrorResponse: + """Tests for LoggingMiddleware._is_error_response().""" + + def test_detects_error_schema_response(self): + """Detects ToolResult containing a serialized error schema + (ChartError, DashboardError, etc.) via "error_type" field.""" + from fastmcp.tools.tool import ToolResult + from mcp import types as mt + + middleware = LoggingMiddleware() + error_json = ( + '{"error": "Chart 999 not found",' + ' "error_type": "not_found",' + ' "timestamp": "2026-04-09T00:00:00Z"}' + ) + result = ToolResult(content=[mt.TextContent(type="text", text=error_json)]) + assert middleware._is_error_response(result) is True + + def test_success_response_not_detected_as_error(self): + """Normal ToolResult is not detected as error.""" + from fastmcp.tools.tool import ToolResult + from mcp import types as mt + + middleware = LoggingMiddleware() + result = ToolResult( + content=[mt.TextContent(type="text", text="Successfully retrieved data")] + ) + assert middleware._is_error_response(result) is False + + def test_empty_content_not_detected_as_error(self): + """ToolResult with empty content is not detected as error.""" + from fastmcp.tools.tool import ToolResult + + middleware = LoggingMiddleware() + assert middleware._is_error_response(ToolResult(content=[])) is False + + @patch("superset.mcp_service.middleware.event_logger") + @patch("superset.mcp_service.middleware.get_user_id", return_value=42) + @pytest.mark.asyncio + async def test_on_call_tool_logs_failure_for_error_schema( + self, mock_get_user_id, mock_event_logger + ): + """on_call_tool logs success=False when tool returns an + error schema (e.g. ChartError).""" + from fastmcp.tools.tool import ToolResult + from mcp import types as mt + + middleware = LoggingMiddleware() + ctx = _make_context(name="get_chart_info") + + error_json = ( + '{"error": "Chart 999999 not found",' + ' "error_type": "not_found",' + ' "timestamp": "2026-04-09T00:00:00Z"}' + ) + error_result = ToolResult( + content=[mt.TextContent(type="text", text=error_json)] + ) + call_next = AsyncMock(return_value=error_result) + + result = await middleware.on_call_tool(ctx, call_next) + + assert result == error_result + mock_event_logger.log.assert_called_once() + call_kwargs = mock_event_logger.log.call_args[1] + assert call_kwargs["curated_payload"]["success"] is False + assert call_kwargs["curated_payload"]["tool"] == "get_chart_info" + + +class TestMiddlewareChainOrder: + """Test that the middleware order from server.py logs failures correctly. + + If the order is wrong (StructuredContentStripper innermost), + it swallows exceptions before LoggingMiddleware can see them, + causing success=True for failures. + """ + + @patch("superset.mcp_service.middleware.event_logger") + @patch("superset.mcp_service.middleware.get_user_id", return_value=42) + @pytest.mark.asyncio + async def test_real_middleware_chain_logs_exception_as_failure( + self, mock_get_user_id, mock_event_logger + ): + """Tool exception is logged as success=False through the + real middleware chain from build_middleware_list().""" + from functools import partial + + from fastmcp.tools.tool import ToolResult + + from superset.mcp_service.server import build_middleware_list + + middleware_list = build_middleware_list() + + async def failing_tool(context: Any) -> Any: + raise ValueError("chart not found") + + # Build chain same way FastMCP does + chain = failing_tool + for mw in reversed(middleware_list): + chain = partial(mw, call_next=chain) + + ctx = _make_context(name="get_chart_info") + result = await chain(ctx) + + # StructuredContentStripper (outermost) must catch the re-raised + # exception and convert it to a safe ToolResult with "Error:" text. + # If it's not outermost, the exception would leak to the MCP SDK. + assert isinstance(result, ToolResult) + assert result.content[0].text.startswith("Error:") + + # LoggingMiddleware must log + # success=False. If the middleware order is wrong + # (StructuredContentStripper innermost), this would be + # success=True because the exception gets swallowed + # before LoggingMiddleware sees it. + log_calls = [ + c + for c in mock_event_logger.log.call_args_list + if c[1].get("action") == "mcp_tool_call" + ] + assert len(log_calls) == 1 + assert log_calls[0][1]["curated_payload"]["success"] is False, ( + "Middleware order is wrong: StructuredContentStripper is " + "swallowing exceptions before LoggingMiddleware can detect " + "them. Ensure StructuredContentStripper is outermost " + "(first added) in build_middleware_list()." + ) From 411f769896473ef2f10ef5bcebfd221b10ca80dd Mon Sep 17 00:00:00 2001 From: Alexandru Soare <37236580+alexandrusoare@users.noreply.github.com> Date: Wed, 15 Apr 2026 15:59:11 +0300 Subject: [PATCH 003/261] feat(filters): Adding empty state for filter modal (#38909) --- .../ConfigModal/SharedStyles.tsx | 1 + .../FilterBar/FilterBar.test.tsx | 17 ++- .../FilterBar/FilterBarSettings/index.tsx | 2 +- .../ConfigModalContent/ConfigModalContent.tsx | 25 +++- .../ConfigModalSidebar/ConfigModalSidebar.tsx | 112 ++++++++++-------- .../FiltersConfigModal.test.tsx | 55 +++++++++ 6 files changed, 162 insertions(+), 50 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx b/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx index b51ac560722..eccae1077d9 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx @@ -75,6 +75,7 @@ export const BaseModalWrapper = styled(StyledModal)` export const BaseModalBody = styled.div` display: flex; height: 100%; + min-height: 500px; flex-direction: row; flex: 1; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx index 2ece961fd33..d00093f0c51 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBar.test.tsx @@ -17,7 +17,13 @@ * under the License. */ -import { act, render, screen, userEvent } from 'spec/helpers/testing-library'; +import { + act, + fireEvent, + render, + screen, + userEvent, +} from 'spec/helpers/testing-library'; import { stateWithoutNativeFilters } from 'spec/fixtures/mockStore'; import { testWithId } from 'src/utils/testUtils'; import { Preset, makeApi } from '@superset-ui/core'; @@ -387,6 +393,15 @@ test('FilterBar apply button is disabled after creating a filter', async () => { userEvent.click(screen.getByTestId(getTestId('collapsable'))); userEvent.click(screen.getByLabelText('setting')); userEvent.click(screen.getByText('Add or edit filters and controls')); + + // First add a filter via the dropdown (modal now shows empty state by default) + const dropdownButton = screen.getByTestId('new-item-dropdown-button'); + fireEvent.mouseEnter(dropdownButton); + const addFilterMenuItem = await screen.findByRole('menuitem', { + name: /add filter/i, + }); + fireEvent.click(addFilterMenuItem); + userEvent.click(screen.getByText('Value')); userEvent.click(screen.getByText('Time range')); userEvent.type( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx index 199287e678d..018c8dd14b3 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterBarSettings/index.tsx @@ -84,7 +84,7 @@ const FilterBarSettings = () => { const { openFilterConfigModal, FilterConfigModalComponent } = useFilterConfigModal({ - createNewOnOpen: filterValues.length === 0, + createNewOnOpen: false, dashboardId, }); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ConfigModalContent/ConfigModalContent.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ConfigModalContent/ConfigModalContent.tsx index 156a710e498..8bbd7fd8b96 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ConfigModalContent/ConfigModalContent.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ConfigModalContent/ConfigModalContent.tsx @@ -25,7 +25,8 @@ import { } from '@superset-ui/core'; import type { FormInstance } from '@superset-ui/core/components'; import { styled } from '@apache-superset/core/theme'; -import { Flex } from '@superset-ui/core/components'; +import { EmptyState, Flex } from '@superset-ui/core/components'; +import { t } from '@apache-superset/core/translation'; import FilterContentRenderer from './FilterContentRenderer'; import CustomizationContentRenderer from './CustomizationContentRenderer'; import { FiltersConfigFormHandle } from '../FiltersConfigForm/FiltersConfigForm'; @@ -105,6 +106,28 @@ function ConfigModalContent({ isChartCustomizationId(currentItemId) && chartCustomizationIds.includes(currentItemId); + const hasNoItems = + filterState.orderedIds.length === 0 && + customizationState.orderedIds.length === 0; + const showEmptyState = hasNoItems || !currentItemId; + + if (showEmptyState) { + return ( + + + + + + ); + } + return (
theme.sizeUnit * 3}px; + + & button { + width: 100%; + } `; // min-height: 0 lets the flex item shrink below its content size so that @@ -264,6 +268,9 @@ const ConfigModalSidebar: FC = ({
); + const hasNoItems = + filterOrderedIds.length === 0 && customizationOrderedIds.length === 0; + return ( = ({ onAddCustomization={onAddCustomization} /> - onCollapseChange(keys as string[])} - ghost - isDragging={isDragging} - > - - + - - - + ) : ( + onCollapseChange(keys as string[])} + ghost + isDragging={isDragging} > - - - + + + + + + + + + )} ); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx index 36f9977c2ce..3583671ad59 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.test.tsx @@ -772,3 +772,58 @@ test('renders a filter with a chart containing BigInt values', async () => { expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument(); }); + +test('displays empty state when modal opens with no filters and createNewOnOpen is false', () => { + defaultRender(defaultState(), { ...props, createNewOnOpen: false }); + + // Check left panel empty state + expect( + screen.getByText('No filters or customizations created yet'), + ).toBeInTheDocument(); + + // Check right panel empty state + expect( + screen.getByText( + /Manage filters and customizations to set scoping, descriptions, and limitations/, + ), + ).toBeInTheDocument(); + + // Verify no filter form is rendered (no "Untitled" filter created) + expect(screen.queryByText(FILTER_TYPE_REGEX)).not.toBeInTheDocument(); +}); + +test('does not auto-create a filter when createNewOnOpen is false', () => { + defaultRender(defaultState(), { ...props, createNewOnOpen: false }); + + // The filter configuration form should not be visible + expect(screen.queryByText(FILTER_NAME_REGEX)).not.toBeInTheDocument(); + expect(screen.queryByText(DATASET_REGEX)).not.toBeInTheDocument(); +}); + +test('empty state disappears when a filter is added via dropdown', async () => { + defaultRender(defaultState(), { + ...props, + createNewOnOpen: false, + }); + + // Verify empty state is shown initially + expect( + screen.getByText('No filters or customizations created yet'), + ).toBeInTheDocument(); + + // Add a filter via the dropdown + const dropdownButton = screen.getByTestId('new-item-dropdown-button'); + fireEvent.mouseEnter(dropdownButton); + const addFilterMenuItem = await screen.findByRole('menuitem', { + name: /add filter/i, + }); + fireEvent.click(addFilterMenuItem); + + // Verify empty state is gone and filter form is shown + await waitFor(() => { + expect( + screen.queryByText('No filters or customizations created yet'), + ).not.toBeInTheDocument(); + }); + expect(screen.getByText(FILTER_TYPE_REGEX)).toBeInTheDocument(); +}); From b76080e291778de91e32aefd50db5110e30fdfd8 Mon Sep 17 00:00:00 2001 From: "Hugh A. Miles II" Date: Wed, 15 Apr 2026 10:24:59 -0400 Subject: [PATCH 004/261] feat(security): add granular export controls - Phase 2 + 3 (#38581) Co-authored-by: Claude Haiku 4.5 Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com> Co-authored-by: Beto Dealmeida Co-authored-by: Daniel Vaz Gaspar --- UPDATING.md | 14 + .../security/granular-export-controls.mdx | 78 +++++ .../components/ResultSet/ResultSet.test.tsx | 223 +++++++++++++- .../src/SqlLab/components/ResultSet/index.tsx | 86 +++--- superset-frontend/src/SqlLab/fixtures.ts | 2 +- .../CopyToClipboard/CopyToClipboard.test.tsx | 24 ++ .../src/components/CopyToClipboard/index.tsx | 20 +- .../src/components/CopyToClipboard/types.ts | 1 + .../Header/useHeaderActionsDropdownMenu.tsx | 3 + .../DownloadMenuItems.test.tsx | 112 ++++++- .../menu/DownloadMenuItems/index.tsx | 29 +- .../src/explore/actions/hydrateExplore.ts | 4 +- .../CopyToClipboardButton.test.tsx | 28 ++ .../components/DataTableControl/index.tsx | 11 +- .../components/DataTableControls.tsx | 17 +- .../test/DataTablesPane.test.tsx | 43 ++- .../ExploreChartHeader.test.tsx | 6 +- .../useExploreAdditionalActionsMenu/index.tsx | 57 +++- superset-frontend/src/hooks/usePermissions.ts | 26 +- superset-frontend/src/utils/copy.ts | 63 +++- superset/charts/api.py | 29 +- superset/dashboards/api.py | 12 +- superset/sqllab/api.py | 21 +- .../test_granular_export_api.py | 275 ++++++++++++++++++ 24 files changed, 1092 insertions(+), 92 deletions(-) create mode 100644 docs/docs/security/granular-export-controls.mdx create mode 100644 tests/integration_tests/test_granular_export_api.py diff --git a/UPDATING.md b/UPDATING.md index c8b78b1779e..27fc3428b9e 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,20 @@ assists people when migrating to a new version. ## Next +### Granular Export Controls + +A new feature flag `GRANULAR_EXPORT_CONTROLS` introduces three fine-grained permissions that replace the legacy `can_csv` permission: + +| Permission | Controls | +|---|---| +| `can_export_data` | CSV, Excel, JSON exports | +| `can_export_image` | Screenshot/PDF exports | +| `can_copy_clipboard` | Copy-to-clipboard operations | + +When the feature flag is enabled, these permissions are enforced on both the frontend (disabled buttons with tooltips) and backend (403 responses from API endpoints). When disabled, legacy `can_csv` behavior is preserved. + +**Migration behavior:** All three new permissions are granted to every role that currently has `can_csv`, preserving existing access. Admins can then selectively revoke individual export permissions from specific roles as needed. + ### Deck.gl MapBox viewport and opacity controls are functional The Deck.gl MapBox chart's **Opacity**, **Default longitude**, **Default latitude**, and **Zoom** controls were previously non-functional — changing them had no effect on the rendered map. These controls are now wired up correctly. diff --git a/docs/docs/security/granular-export-controls.mdx b/docs/docs/security/granular-export-controls.mdx new file mode 100644 index 00000000000..806e21e02cf --- /dev/null +++ b/docs/docs/security/granular-export-controls.mdx @@ -0,0 +1,78 @@ +--- +title: Granular Export Controls +sidebar_position: 4 +--- + +# Granular Export Controls + +Superset provides granular, permission-based controls for data export, image export, and clipboard operations. These replace the legacy `can_csv` permission with three fine-grained permissions that can be assigned independently to roles. + +## Feature Flag + +Granular export controls are gated behind the `GRANULAR_EXPORT_CONTROLS` feature flag. When the flag is disabled, the legacy `can_csv` permission behavior is preserved. + +```python +FEATURE_FLAGS = { + "GRANULAR_EXPORT_CONTROLS": True, +} +``` + +## Permissions + +| Permission | Resource | Controls | +| -------------------- | ---------- | ---------------------------------------------------------------------- | +| `can_export_data` | `Superset` | CSV, Excel, and JSON data exports from charts, dashboards, and SQL Lab | +| `can_export_image` | `Superset` | Screenshot (JPEG/PNG) and PDF exports from charts and dashboards | +| `can_copy_clipboard` | `Superset` | Copy-to-clipboard operations in SQL Lab and the Explore data pane | + +## Default Role Assignments + +The migration grants all three new permissions (`can_export_data`, `can_export_image`, `can_copy_clipboard`) to every role that currently has `can_csv`. This preserves existing behavior — no role loses access during the upgrade. + +After the migration, admins can selectively revoke individual export permissions from any role to restrict access. For example, to prevent Gamma users from exporting data or images while still allowing clipboard operations, revoke `can_export_data` and `can_export_image` from the Gamma role. + +## Configuration Steps + +1. **Enable the feature flag** in `superset_config.py`: + + ```python + FEATURE_FLAGS = { + "GRANULAR_EXPORT_CONTROLS": True, + } + ``` + +2. **Run the database migration** to register the new permissions: + + ```bash + superset db upgrade + ``` + +3. **Initialize permissions** so roles are populated: + + ```bash + superset init + ``` + +4. **Verify role assignments** in **Settings > List Roles**. Confirm that each role has the expected permissions from the table above. + +5. **Customize as needed**: Grant or revoke individual export permissions on any role through the role editor. + +## User Experience + +When a user lacks a required export permission: + +- **Menu items** (CSV, Excel, JSON, screenshot) appear **disabled** with an info tooltip icon explaining the restriction +- **Buttons** (SQL Lab download, clipboard copy) appear **disabled** with a tooltip on hover +- **API endpoints** return **403 Forbidden** when the corresponding permission is missing + +## API Enforcement + +The following API endpoints enforce granular export permissions when the feature flag is enabled: + +| Endpoint | Required Permission | +| --------------------------------------------------------- | ------------------- | +| `GET /api/v1/chart/{id}/data/` (CSV/Excel format) | `can_export_data` | +| `GET /api/v1/chart/{id}/cache_screenshot/` | `can_export_image` | +| `POST /api/v1/dashboard/{id}/cache_dashboard_screenshot/` | `can_export_image` | +| `GET /api/v1/sqllab/export/{client_id}/` | `can_export_data` | +| `POST /api/v1/sqllab/export_streaming/` | `can_export_data` | diff --git a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx index 64daa73cf5b..3885f195532 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx @@ -41,12 +41,20 @@ import { queryWithNoQueryLimit, failedQueryWithFrontendTimeoutErrors, } from 'src/SqlLab/fixtures'; +import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; import { ViewLocations } from 'src/SqlLab/contributions'; import { registerToolbarAction, cleanupExtensions, } from 'spec/helpers/extensionTestHelpers'; +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: jest.fn().mockReturnValue(false), +})); + +const mockIsFeatureEnabled = isFeatureEnabled as jest.Mock; + jest.mock('src/components/ErrorMessage', () => ({ ErrorMessageWithStackTrace: () =>
Error
, })); @@ -551,7 +559,9 @@ describe('ResultSet', () => { }, }), ); - expect(queryByTestId('export-csv-button')).not.toBeInTheDocument(); + const csvButton = queryByTestId('export-csv-button'); + expect(csvButton).toBeInTheDocument(); + expect(csvButton).toBeDisabled(); }); test('should allow copy to clipboard when user has permission to export data', async () => { @@ -590,7 +600,9 @@ describe('ResultSet', () => { }, }), ); - expect(queryByTestId('copy-to-clipboard-button')).not.toBeInTheDocument(); + const clipboardButton = queryByTestId('copy-to-clipboard-button'); + expect(clipboardButton).toBeInTheDocument(); + expect(clipboardButton).toBeDisabled(); }); test('should include sqlEditorImmutableId in query object when fetching results', async () => { @@ -760,6 +772,213 @@ describe('ResultSet', () => { }, ); + test('should show CSV button with granular can_export_data permission when flag is ON', async () => { + mockIsFeatureEnabled.mockImplementation( + (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls, + ); + const { queryByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_export_data', 'Superset']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const csvButton = queryByTestId('export-csv-button'); + expect(csvButton).toBeInTheDocument(); + expect(csvButton).toBeEnabled(); + mockIsFeatureEnabled.mockReset(); + }); + + test('should disable CSV button when granular flag is ON and user lacks can_export_data', async () => { + mockIsFeatureEnabled.mockImplementation( + (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls, + ); + const { queryByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const csvButton = queryByTestId('export-csv-button'); + expect(csvButton).toBeInTheDocument(); + expect(csvButton).toBeDisabled(); + mockIsFeatureEnabled.mockReset(); + }); + + test('should show clipboard button with granular can_copy_clipboard permission when flag is ON', async () => { + mockIsFeatureEnabled.mockImplementation( + (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls, + ); + const { queryByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_copy_clipboard', 'Superset']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const clipboardButton = queryByTestId('copy-to-clipboard-button'); + expect(clipboardButton).toBeInTheDocument(); + expect(clipboardButton).toBeEnabled(); + mockIsFeatureEnabled.mockReset(); + }); + + test('should disable clipboard button when granular flag is ON and user lacks can_copy_clipboard', async () => { + mockIsFeatureEnabled.mockImplementation( + (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls, + ); + const { queryByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_export_data', 'Superset']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const clipboardButton = queryByTestId('copy-to-clipboard-button'); + expect(clipboardButton).toBeInTheDocument(); + expect(clipboardButton).toBeDisabled(); + mockIsFeatureEnabled.mockReset(); + }); + + test('should use legacy can_export_csv for both CSV and clipboard when granular flag is OFF', async () => { + mockIsFeatureEnabled.mockReturnValue(false); + const { queryByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_export_csv', 'SQLLab']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const csvButton = queryByTestId('export-csv-button'); + const clipboardButton = queryByTestId('copy-to-clipboard-button'); + expect(csvButton).toBeInTheDocument(); + expect(csvButton).toBeEnabled(); + expect(clipboardButton).toBeInTheDocument(); + expect(clipboardButton).toBeEnabled(); + mockIsFeatureEnabled.mockReset(); + }); + + test('disabled CSV button should show permission tooltip when granular flag is ON', async () => { + mockIsFeatureEnabled.mockImplementation( + (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls, + ); + const { getByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_copy_clipboard', 'Superset']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const csvButton = getByTestId('export-csv-button'); + expect(csvButton).toBeDisabled(); + fireEvent.mouseOver(csvButton.parentElement ?? csvButton); + await waitFor(() => { + expect( + screen.getByText("You don't have permission to export data"), + ).toBeInTheDocument(); + }); + mockIsFeatureEnabled.mockReset(); + }); + + test('disabled clipboard button should show permission tooltip when granular flag is ON', async () => { + mockIsFeatureEnabled.mockImplementation( + (flag: FeatureFlag) => flag === FeatureFlag.GranularExportControls, + ); + const { getByTestId } = setup( + mockedProps, + mockStore({ + ...initialState, + user: { + ...user, + roles: { + sql_lab: [['can_export_data', 'Superset']], + }, + }, + sqlLab: { + ...initialState.sqlLab, + queries: { + [queries[0].id]: queries[0], + }, + }, + }), + ); + const clipboardButton = getByTestId('copy-to-clipboard-button'); + expect(clipboardButton).toBeDisabled(); + fireEvent.mouseOver(clipboardButton.parentElement ?? clipboardButton); + await waitFor(() => { + expect( + screen.getByText("You don't have permission to copy to clipboard"), + ).toBeInTheDocument(); + }); + mockIsFeatureEnabled.mockReset(); + }); + test('renders contributed toolbar action in results slot', async () => { registerToolbarAction( ViewLocations.sqllab.results, diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx index d1858e730a8..d7c42fc2b2c 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx @@ -82,7 +82,7 @@ import { LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, } from 'src/logger/LogUtils'; import { Icons } from '@superset-ui/core/components/Icons'; -import { findPermission } from 'src/utils/findPermission'; +import { usePermissions } from 'src/hooks/usePermissions'; import { StreamingExportModal } from 'src/components/StreamingExportModal'; import { useStreamingExport } from 'src/components/StreamingExportModal/useStreamingExport'; import { useConfirmModal } from 'src/hooks/useConfirmModal'; @@ -172,7 +172,6 @@ const ResultSet = ({ defaultQueryLimit, useFixedHeight = false, }: ResultSetProps) => { - const user = useSelector(({ user }: SqlLabRootState) => user, shallowEqual); const streamingThreshold = useSelector( (state: SqlLabRootState) => state.common?.conf?.CSV_STREAMING_ROW_THRESHOLD || 1000, @@ -227,6 +226,10 @@ const ResultSet = ({ [query.results?.expanded_columns], ); + const { + canExportDataSqlLab: canExportData, + canCopyClipboardSqlLab: canCopyClipboard, + } = usePermissions(); const history = useHistory(); const dispatch = useDispatch(); const logAction = useLogAction({ queryId, sqlEditorId: query.sqlEditorId }); @@ -361,12 +364,6 @@ const ResultSet = ({ schema: query?.schema, }; - const canExportData = findPermission( - 'can_export_csv', - 'SQLLab', - user?.roles, - ); - const handleDownloadCsv = (event: React.MouseEvent) => { logAction(LOG_ACTIONS_SQLLAB_DOWNLOAD_CSV, {}); @@ -396,19 +393,26 @@ const ResultSet = ({ onClick={createExploreResultsOnClick} /> )} - {csv && canExportData && ( + {csv && (