gabotorresruiz commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761641996
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id,
chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see
guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so
a
+ # deleted dataset still returns the clean DatasetNotAccessible
contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
+ if not validation_result.is_valid:
+ await ctx.warning(
+ "Chart found but dataset is not accessible: %s"
+ % (validation_result.error,)
)
- if not validation_result.is_valid:
- await ctx.warning(
- "Chart found but dataset is not accessible: %s"
- % (validation_result.error,)
- )
- logger.warning(
- "get_chart_data: dataset not accessible for chart_id=%s:
%s",
- chart.id,
- validation_result.error,
- )
- return ChartError(
- error=validation_result.error
- or "Chart's dataset is not accessible. "
- "Dataset may have been deleted.",
- error_type="DatasetNotAccessible",
- )
- # Log any warnings (e.g., virtual dataset warnings)
- for warning in validation_result.warnings:
- await ctx.warning("Dataset warning: %s" % (warning,))
+ logger.warning(
+ "get_chart_data: dataset not accessible for chart_id=%s: %s",
+ chart.id,
+ validation_result.error,
+ )
+ return ChartError(
+ error=validation_result.error
+ or "Chart's dataset is not accessible. Dataset may have been
deleted.",
+ error_type="DatasetNotAccessible",
+ )
+ # Log any warnings (e.g., virtual dataset warnings)
+ for warning in validation_result.warnings:
+ await ctx.warning("Dataset warning: %s" % (warning,))
Review Comment:
Good catch, you're right. Traced it end to end: `check_access` only gates
`has_dataset_access`, while `find_by_id` keeps applying `DatasourceFilter`, so
every guest was getting denied. Went with your preferred fix and threaded
`skip_base_filter` through `validate_chart_dataset`, so `check_access=False` is
now a true existence-only lookup. That keeps the guest existence check instead
of dropping it the way `get_chart_info` does.
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1293,13 +1293,13 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id",
"NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id",
"NONE"))
- # Skip the dataset pre-check for transient charts (no ID) and for
guests
- # (authorized via the dashboard context, not dataset RBAC).
+ # Skip the pre-check only for transient charts (no ID). Guests keep the
+ # existence check but skip the RBAC access check
(dashboard-authorized).
from superset.mcp_service import guest_scope
- if getattr(chart, "id", None) is not None and not
guest_scope.is_guest_read():
+ if getattr(chart, "id", None) is not None:
validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ chart.datasource_id, check_access=not
guest_scope.is_guest_read()
Review Comment:
Same call underneath, so the same fix covers it. With
`validate_chart_dataset` skipping the base filter when `check_access` is False,
the guest preview keeps its existence check without the RBAC denial.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,173 @@ def
test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) ->
Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"],
"rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier",
return_value=chart),
+ patch.object(module, "validate_chart_dataset",
side_effect=fake_validate),
Review Comment:
Agreed, that mock was hiding the bug. Moved the real coverage down to
`test_chart_utils.py`: it drives `validate_chart_dataset` with
`check_access=False` and asserts `find_by_id` gets `skip_base_filter=True` and
`has_dataset_access` is never called. Verified it fails without the fix.
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,173 @@ def
test_non_integer_float_total_rows_is_truncated(self) -> None:
chart_data = _make_chart_data(total_rows=5.9)
assert chart_data.total_rows == 5
assert isinstance(chart_data.total_rows, int)
+
+
+class TestGuestScoping:
+ """Tool-level guest coverage for get_chart_data (the highest-value guest
+ tool): the data query is pinned to the token's dashboard, the dataset
+ existence check runs without the RBAC access check, and requests that a
+ guest cannot be scoped for are denied cleanly."""
+
+ @pytest.mark.asyncio
+ async def test_guest_query_pinned_to_dashboard_with_existence_only_check(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ chart = SimpleNamespace(
+ id=9,
+ slice_name="Sales",
+ viz_type="table",
+ datasource_id=1,
+ datasource_type="table",
+ query_context='{"queries": []}',
+ params=None,
+ )
+ validate_calls: dict[str, Any] = {}
+
+ def fake_validate(datasource_id: Any, check_access: bool = True) ->
Any:
+ validate_calls["check_access"] = check_access
+ return SimpleNamespace(is_valid=True, warnings=[], error=None)
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {
+ "queries": [{"data": [{"a": 1}], "colnames": ["a"],
"rowcount": 1}]
+ }
+
+ mock_authorize = MagicMock()
+ with (
+ patch.object(module, "find_chart_by_identifier",
return_value=chart),
+ patch.object(module, "validate_chart_dataset",
side_effect=fake_validate),
+ patch.object(module.guest_scope, "is_guest_read",
return_value=True),
+ patch.object(module.guest_scope, "guest_dashboard_id",
return_value=6),
+ patch.object(module.guest_scope, "authorize_query",
mock_authorize),
+ patch(
+
"superset.commands.chart.data.get_data_command.ChartDataCommand",
+ _Command,
+ ),
+ patch(
+ "superset.charts.schemas.ChartDataQueryContextSchema.load",
+ lambda self, data: object(),
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "9"}}
+ )
+
+ # F: the dataset existence check runs, but without the RBAC access
check.
+ assert validate_calls["check_access"] is False
+ # Scoping: the data query is pinned to the token's dashboard, so
+ # raise_for_access authorizes against a dashboard the guest can see.
+ mock_authorize.assert_called_once()
+ assert mock_authorize.call_args.args[1] == 6
+
+ @pytest.mark.asyncio
+ async def test_guest_out_of_scope_chart_is_not_found(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # ChartFilter scopes an out-of-scope chart out, so the lookup returns
None.
+ with (
+ patch.object(module, "find_chart_by_identifier",
return_value=None),
+ patch.object(module.guest_scope, "is_guest_read",
return_value=True),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"identifier": "123"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+
+ @pytest.mark.asyncio
+ async def test_guest_form_data_key_only_path_is_denied(
+ self, mcp_server, mock_auth
+ ) -> None:
+ from unittest.mock import patch
+
+ from fastmcp import Client
+
+ from superset.utils import json
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_data"
+ )
+ # A valid cached blob is available, so without the guest-denial branch
+ # the code would proceed to query rather than 404 on a cache miss. The
+ # NotFound here therefore pins the denial itself, not a cache miss.
+ with (
+ patch.object(module.guest_scope, "is_guest_read",
return_value=True),
+ patch.object(
+ module,
+ "get_cached_form_data",
+ return_value='{"datasource_id": 1, "datasource_type":
"table"}',
+ ),
+ ):
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "get_chart_data", {"request": {"form_data_key":
"cached-key"}}
+ )
+
+ data = json.loads(result.content[0].text)
+ assert data["error_type"] == "NotFound"
+ assert "No accessible chart found for this request." in data["error"]
+
+
[email protected]
+async def test_query_from_form_data_zero_row_limit_falls_back_to_default(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ """A falsy 0 row_limit resolves to ROW_LIMIT (0 means "no limit"), so the
+ row_limit coercion does not change behavior for a cached row_limit of 0."""
+ from flask import current_app
+
+ module =
importlib.import_module("superset.mcp_service.chart.tool.get_chart_data")
+ captured: dict[str, Any] = {}
+
+ def fake_build(form_data: Any, **kwargs: Any) -> Any:
+ captured["row_limit"] = kwargs.get("row_limit")
+ return object()
+
+ class _Command:
+ def __init__(self, query_context: Any) -> None: ...
+ def validate(self) -> None: ...
+ def run(self) -> dict[str, Any]:
+ return {"queries": [{"data": [], "colnames": [], "rowcount": 0}]}
+
+ monkeypatch.setattr(module, "build_query_context_from_form_data",
fake_build)
+ monkeypatch.setattr(
+ module,
+ "event_logger",
+ SimpleNamespace(log_context=lambda **kwargs: nullcontext()),
+ )
+ get_data_command_module = importlib.import_module(
+ "superset.commands.chart.data.get_data_command"
+ )
+ monkeypatch.setattr(get_data_command_module, "ChartDataCommand", _Command)
+
+ await _query_from_form_data(
+ {"datasource_id": 1, "datasource_type": "table", "row_limit": 0},
+ GetChartDataRequest(form_data_key="k"),
+ _AsyncContext(),
+ )
+
+ assert captured["row_limit"] == current_app.config["ROW_LIMIT"]
+ assert captured["row_limit"] != 0
Review Comment:
Right, `0` is falsy so it short circuits before the coercion ever runs.
Added a test that feeds `row_limit="250"` and checks it comes back as int
`250`, which does break if you remove `_coerce_row_limit`. Kept the `0` case as
a plain regression for the falsy fallback. Good call on the str `"0"` vs int
`0` asymmetry too, noted.
##########
superset/utils/filters.py:
##########
@@ -87,4 +92,6 @@ def guest_embedded_dashboard_filter() ->
Optional[ColumnElement[bool]]:
# branch and has_guest_access); a guest is only ever scoped to embedded
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids),
Dashboard.embedded.any()))
+ if slug_ids:
+ conditions.append(and_(Dashboard.slug.in_(slug_ids),
Dashboard.embedded.any()))
Review Comment:
Added a slug case plus a uuid+int+slug mixed case in `filters_test.py`,
asserting the compiled SQL hits `dashboards.slug` with the embedded `EXISTS`.
On the `DashboardAccessFilter` wiring: fair, it's still only exercised through
the helper. Left it there since the helper is the shared gate, but happy to add
a focused test if you'd rather.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
