codeant-ai-for-open-source[bot] commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3761649167
##########
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:
**Suggestion:** The test documentation contradicts the behavior it asserts:
a cached `row_limit` of `0` is described as meaning โno limit,โ but the
assertion requires it to resolve to `ROW_LIMIT` instead. Update the docstring
to state that zero falls back to the configured default, or change the expected
behavior if zero is intended to mean unlimited. [comment mismatch]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Test documentation contradicts its verified assertions.
- โ ๏ธ Misleads maintainers about zero row-limit semantics.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=48595ace0d644830a22295ae9471392f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=48595ace0d644830a22295ae9471392f&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/chart/tool/test_get_chart_data.py
**Line:** 1880:1881
**Comment:**
*Comment Mismatch: The test documentation contradicts the behavior it
asserts: a cached `row_limit` of `0` is described as meaning โno limit,โ but
the assertion requires it to resolve to `ROW_LIMIT` instead. Update the
docstring to state that zero falls back to the configured default, or change
the expected behavior if zero is intended to mean unlimited.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=4cc8d2453fbd5b82d40cecf64156487e3f81a4496b4425db4751c80c763c468b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=4cc8d2453fbd5b82d40cecf64156487e3f81a4496b4425db4751c80c763c468b&reaction=dislike'>๐</a>
--
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]
