gabotorresruiz commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768910024
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py:
##########
@@ -1742,3 +1743,214 @@ 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."""
Review Comment:
Fixed the docstring. It now says a falsy int `0` hits the `or ROW_LIMIT`
fallback and resolves to the configured default before coercion runs, which is
what the assertion checks.
##########
superset/dashboards/filters.py:
##########
@@ -192,24 +193,13 @@ def _apply_viewers(self, query: Query) -> Query:
if user_id:
filters.append(Dashboard.id.in_(extra_dashboards_filter(user_id)))
- # (D) Embedded: preserved as-is
+ # Reuse the shared guest scoping so the dashboard list and chart list
+ # agree on visibility.
if is_feature_enabled("EMBEDDED_SUPERSET") and
security_manager.is_guest_user(
g.user
):
- guest_user: GuestUser = g.user
- embedded_dashboard_ids = [
- r["id"]
- for r in guest_user.resources
- if r["type"] == GuestTokenResourceType.DASHBOARD.value
- ]
- condition = (
- Dashboard.embedded.any(
- EmbeddedDashboard.uuid.in_(embedded_dashboard_ids)
- )
- if any(is_uuid(id_) for id_ in embedded_dashboard_ids)
- else Dashboard.id.in_(embedded_dashboard_ids)
- )
- filters.append(condition)
+ if (guest_condition := guest_embedded_dashboard_filter()) is not
None:
+ filters.append(guest_condition)
Review Comment:
Agreed. Reworked it so a guest returns the token scoping as the sole
predicate via an early return in `apply()`, mirroring `ChartFilter`, instead of
OR-ing it with the role paths. Also dropped the dead `_apply_legacy`, which
carried the same pattern.
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -994,8 +1004,11 @@ async def _query_from_form_data(
error_type="InvalidFormData",
)
- row_limit = (
- request.limit or form_data.get("row_limit") or
current_app.config["ROW_LIMIT"]
+ # row_limit may arrive as a str; coerce it. Keep the trailing fallback so a
+ # falsy 0 still resolves to ROW_LIMIT (as before).
+ row_limit = _coerce_row_limit(
+ request.limit or form_data.get("row_limit") or
current_app.config["ROW_LIMIT"],
+ current_app.config["ROW_LIMIT"],
)
Review Comment:
Good catch. `_coerce_row_limit` now falls back to the default for any
non-positive value, and I guarded the saved-context path that sets `row_limit`
directly, so a negative can't reach `LIMIT -1` downstream. Added `-1` and
`"-1"` cases to the coercion test.
##########
superset/dashboards/filters.py:
##########
@@ -270,7 +236,6 @@ def _apply_legacy(self, query: Query) -> Query:
or_(
Dashboard.id.in_(editor_ids_query),
Dashboard.id.in_(datasource_perm_query),
- *feature_flagged_filters,
*extra_access_filters,
)
)
Review Comment:
Right, it was dead. Nothing has called `_apply_legacy` since `apply()`
started routing everything through `_apply_viewers`. Removed the method and
trimmed the now-stale docstring section.
##########
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:
Good catch, that was a half-state I introduced. `has_guest_access` only
authorizes by dashboard id and embedded uuid, never slug, so a slug dashboard
would list but its data would be denied. Removed the slug branch so the list
filter matches the data path (slugs stay fail-closed).
--
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]
