codeant-ai-for-open-source[bot] commented on code in PR #44558:
URL: https://github.com/apache/superset/pull/44558#discussion_r4087354819
##########
tests/unit_tests/mcp_service/chart/tool/test_get_chart_preview.py:
##########
@@ -1839,3 +1839,250 @@ def
test_saved_gauge_preview_skips_empty_aggregate_groups(
else:
assert "Blue" in result.ascii_content
assert "Empty" not in result.ascii_content
+
+
[email protected]
[email protected]("user_id", [11, 29])
+async def test_png_preview_renders_as_caller_in_isolated_context(app_context,
user_id):
+ import base64
+ import threading
+ from io import BytesIO
+
+ from flask import g
+ from PIL import Image
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ caller_thread = threading.get_ident()
+ g.user = SimpleNamespace(id=user_id)
+ g.request_marker = "request-only"
+ user = SimpleNamespace(id=user_id, is_active=True)
+ chart = SimpleNamespace(id=104)
+ png = BytesIO()
+ Image.new("RGB", (80, 60), "white").save(png, format="PNG")
+
+ def screenshot(url, element, *, user):
+ assert threading.get_ident() != caller_thread
+ assert g.user.id == user_id
+ assert not hasattr(g, "request_marker")
+ assert user.id == user_id
+ assert url == "http://localhost/superset/slice/104/?standalone=true";
+ assert element == "chart-container"
+ return png.getvalue()
+
+ with (
+ patch.object(module, "security_manager", new=MagicMock()) as manager,
+ patch.object(module, "find_chart_by_identifier", return_value=chart),
+ patch.object(module.guest_scope, "is_guest_read", return_value=False),
+ patch("superset.is_feature_enabled", return_value=False),
+ patch(
+ "superset.utils.urls.get_url_path",
+
return_value="http://localhost/superset/slice/104/?standalone=true";,
+ ),
+ patch("superset.utils.webdriver._PlaywrightBrowserManager") as
browser_manager,
+ patch("superset.utils.webdriver.WebDriverPlaywright") as driver,
+ ):
+ manager.find_user.return_value = user
+ driver.return_value.get_screenshot.side_effect = screenshot
+ result = await module._generate_png_preview(
+ 104, GetChartPreviewRequest(identifier=104, format="png")
+ )
+ assert base64.b64decode(result.data) == png.getvalue()
+ assert (result.width, result.height) == (80, 60)
+ manager.raise_for_access.assert_called_once_with(chart=chart)
+ browser_manager.return_value._cleanup.assert_called_once()
+ assert (
+ driver.call_args.kwargs["browser_manager"] is
browser_manager.return_value
+ )
+ assert g.request_marker == "request-only"
+ assert g.user.id == user_id
+
+
[email protected]
[email protected](
+ "guest,kwargs",
+ [
+ (True, {}),
+ (False, {"form_data_key": "unsaved"}),
+ (False, {"extra_form_data": {"filters": []}}),
+ ],
+)
+async def test_png_preview_rejects_unpropagated_context(app_context, guest,
kwargs):
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ with (
+ patch.object(module.guest_scope, "is_guest_read", return_value=guest),
+ patch("superset.utils.webdriver._PlaywrightBrowserManager") as manager,
+ ):
+ result = await module._generate_png_preview(
+ 104, GetChartPreviewRequest(identifier=104, format="png", **kwargs)
+ )
+ assert result.error_type == "UnsupportedFormat"
+ manager.assert_not_called()
+
+
[email protected]
[email protected]("failure", ["denied", "missing", "inactive",
"export"])
+async def test_png_preview_authorizes_before_browser(app_context, failure):
+ from flask import g
+
+ module = importlib.import_module(
+ "superset.mcp_service.chart.tool.get_chart_preview"
+ )
+ g.user = SimpleNamespace(id=11)
+ with (
+ patch.object(module, "security_manager", new=MagicMock()) as manager,
+ patch.object(
+ module,
+ "find_chart_by_identifier",
+ return_value=None if failure == "missing" else
SimpleNamespace(id=104),
+ ),
+ patch.object(module.guest_scope, "is_guest_read", return_value=False),
+ patch("superset.is_feature_enabled", return_value=failure == "export"),
+ patch("superset.utils.webdriver._PlaywrightBrowserManager") as browser,
+ ):
+ manager.find_user.return_value = SimpleNamespace(
+ id=11, is_active=failure != "inactive"
+ )
+ manager.can_access.return_value = False
+ if failure == "denied":
+ manager.raise_for_access.side_effect = ValueError("secret denied
URL")
+ result = await module._generate_png_preview(
+ 104, GetChartPreviewRequest(identifier=104, format="png")
+ )
+ assert isinstance(result, ChartError)
+ assert "secret" not in result.error
+ browser.assert_not_called()
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `100ee4c`.
The denied-case test now explicitly asserts that the returned ChartError has
error_type equal to "Forbidden", while still checking that the authorization
message is not exposed.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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]
