rusackas commented on code in PR #43388:
URL: https://github.com/apache/superset/pull/43388#discussion_r3832237279
##########
tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py:
##########
@@ -240,3 +244,70 @@ async def
test_restore_chart_rejects_boolean_identifier(mcp_server: object) -> N
async with Client(mcp_server) as client:
with pytest.raises(ToolError):
await client.call_tool("restore_chart", {"request": {"identifier":
True}})
+
+
+@patch(_FIND)
[email protected]
+async def test_restore_chart_inaccessible_chart_reads_as_not_found(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A chart outside the caller's RBAC scope must not leak its existence or
+ title: the unfiltered restore lookup finds it, the base-filtered re-lookup
+ does not, so the tool must answer exactly as if it does not exist."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ mock_find.side_effect = [
+ _mock_chart(chart_id=10, slice_name="Secret KPI"), # unfiltered lookup
+ None, # base-filtered re-lookup
+ ]
Review Comment:
Leaving this. Each test covers a distinct scenario (RBAC-scope-invisible vs
visible-non-editor); the shared setup is a handful of lines and keeping them
self-contained beats coupling two tests through a helper.
##########
tests/unit_tests/mcp_service/dashboard/tool/test_restore_dashboard.py:
##########
@@ -248,3 +252,71 @@ async def
test_restore_dashboard_rejects_boolean_identifier(
await client.call_tool(
"restore_dashboard", {"request": {"identifier": True}}
)
+
+
+@patch(_FIND)
[email protected]
+async def test_restore_dashboard_inaccessible_dashboard_reads_as_not_found(
+ mock_find: Mock, mcp_server: object
+) -> None:
+ """A dashboard outside the caller's RBAC scope must not leak its
+ existence or title: the unfiltered restore lookup finds it, the
+ base-filtered re-lookup does not, so the tool must answer exactly as if it
+ does not exist."""
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ mock_find.side_effect = [
+ _mock_dashboard(dashboard_id=10, title="Secret Board"),
+ None, # base-filtered re-lookup
+ ]
Review Comment:
Leaving this, same reasoning as the restore_chart one: two distinct
scenarios, small shared setup, not worth coupling through a helper.
##########
tests/unit_tests/mcp_service/system/tool/test_find_users.py:
##########
@@ -336,3 +336,31 @@ async def
test_find_users_escapes_literal_backslash(mcp_server):
assert ilike_call is not None
assert ilike_call.args[0] == "%\\\\%"
assert ilike_call.kwargs.get("escape") == "\\"
+
+
[email protected]
+async def test_find_users_does_not_match_on_email(mcp_server):
+ """Email must not be a searchable column: substring or exact email
+ matching would let any MCP credential confirm which addresses have
+ accounts (an email-disclosure oracle the web API reserves for admins)."""
+ session, _ = _patch_user_query([])
Review Comment:
Leaving this. Each test exercises a different wildcard/escaping case and
stays independently readable; that's the intentional style here over a shared
fixture.
--
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]
