rebenitez1802 commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761530724
##########
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:
π΄ **Blocker β same root cause as `get_chart_data`**
`validate_chart_dataset(check_access=False)` still resolves the dataset via
`DatasetDAO.find_by_id` β `DatasourceFilter`, which denies a guest β
`DatasetNotAccessible` on every guest preview. Preferred fix is the
`skip_base_filter=True` existence lookup in `chart_utils.py` (see the
`get_chart_data` comment); the minimal in-diff fix restores the guest skip:
```suggestion
if getattr(chart, "id", None) is not None and not
guest_scope.is_guest_read():
validation_result = validate_chart_dataset(
chart.datasource_id, check_access=True
```
##########
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:
π‘ **Medium β this mock hides the blocker above**
Patching `validate_chart_dataset` to return `is_valid=True` means the real
`DatasetDAO.find_by_id` β `DatasourceFilter` path is never exercised, so the
guest-denial regression sails through CI. The rest of this test is sound
(`check_access is False`, `authorize_query` pinned to dashboard `6`). Add a
test that drives the *real* `validate_chart_dataset` for a guest against a live
dataset and asserts the data query succeeds β that would catch the blocker.
##########
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:
π‘ **Medium β this test doesn't exercise the coercion it's named for**
`row_limit=0` is a falsy int, so it's resolved by the pre-existing `... or
ROW_LIMIT` chain *before* `_coerce_row_limit` coerces anything β deleting the
`_coerce_row_limit` call would still pass this test. It also hides a
divergence: a cached **string** `"0"` is truthy, bypasses the `or ROW_LIMIT`
fallback, and yields `row_limit=0` ("no limit"), while int `0` falls back to
`ROW_LIMIT`. So *"a falsy 0 still resolves to ROW_LIMIT (as before)"* holds
only for int `0`. The `using_unsaved_state` coercion site (line 526) also has
no test.
Add a test that actually exercises strβint coercion:
```python
@pytest.mark.asyncio
async def test_query_from_form_data_string_row_limit_is_coerced(
monkeypatch: pytest.MonkeyPatch,
) -> None:
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()),
)
monkeypatch.setattr(
importlib.import_module("superset.commands.chart.data.get_data_command"),
"ChartDataCommand", _Command,
)
await _query_from_form_data(
{"datasource_id": 1, "datasource_type": "table", "row_limit": "250"},
GetChartDataRequest(form_data_key="k"),
_AsyncContext(),
)
assert captured["row_limit"] == 250
assert isinstance(captured["row_limit"], int)
```
##########
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:
π΄ **Blocker β this pre-check denies every guest, defeating the change**
For a guest this is not existence-only. `validate_chart_dataset`
(`chart_utils.py`) calls `DatasetDAO.find_by_id(datasource_id)` with the
default `skip_base_filter=False`; the `check_access` flag only gates the
*separate* `has_dataset_access` call, not the lookup. `DatasetDAO.base_filter =
DatasourceFilter`, which returns rows only under `can_access_all_datasources()`
(False for a guest) or a matching DB/datasource/schema grant β an embedded
guest has none. So `find_by_id` returns `None` β `is_valid=False` β
`DatasetNotAccessible` for every in-scope, live chart. The sibling
`get_chart_info` still skips this for guests on purpose
(`get_chart_info.py:113-115`: *"Guests read via the dashboard context, not
dataset RBAC; skip the perm-check"*). CI stays green only because the new test
mocks `validate_chart_dataset` (see the test comment).
**Preferred fix (keeps the existence-check intent):** thread the flag
through `validate_chart_dataset` so `check_access=False` resolves the dataset
with `DatasetDAO.find_by_id(datasource_id, skip_base_filter=True)` β existence
without RBAC. That file is not in this diff, so it can't be a one-click
suggestion.
**Minimal in-diff fix** (matches `get_chart_info`; drops the guest existence
check):
```suggestion
# Guests read via the dashboard context (authorize_query +
# raise_for_access below), not dataset RBAC, and
validate_chart_dataset
# resolves the dataset through the DatasourceFilter base filter,
which
# denies a guest outright -- so running it here returns
# DatasetNotAccessible for every guest. Skip it, as get_chart_info
does.
if not guest_scope.is_guest_read():
validation_result = validate_chart_dataset(
chart.datasource_id, check_access=True
)
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,))
```
##########
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:
π‘ **Medium β the `slug_ids` branch ships with no test**
`tests/unit_tests/utils/filters_test.py` covers disabled / non-guest /
no-resources / uuid / int / mixed, but has no slug case, so this new branch
(and any wrong-column or cast regression in it) is uncovered. Add a
`_guest_with_dashboards("my-slug")` case asserting the compiled SQL contains
`dashboards.slug IN (...)` plus the `embedded` `EXISTS`, and a uuid+int+slug
mixed case. Separately, the `DashboardAccessFilter` guest wiring in
`dashboards/filters.py` is only covered via the extracted helper β
`subjects/test_filters.py` mocks `_apply_viewers` out and sets
`is_guest_user=False` β so a dropped `append` there wouldn't be caught either.
--
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]
