bito-code-review[bot] commented on code in PR #43388:
URL: https://github.com/apache/superset/pull/43388#discussion_r3831867571
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate forbidden error test code</b></div>
<div id="fix">
Detected syntactic duplication in test_restore_chart.py at lines 263-279 and
294-310. Consider extracting the forbidden error handling test logic into a
shared helper function to reduce code duplication.
</div>
</div>
<small><i>Code Review Run #147078</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate dashboard forbidden test code</b></div>
<div id="fix">
Detected syntactic duplication in test_restore_dashboard.py at lines 272-288
and 303-319. Consider extracting the forbidden error handling test logic into a
shared helper function to reduce code duplication.
</div>
</div>
<small><i>Code Review Run #147078</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate mock setup in find_users tests</b></div>
<div id="fix">
Detected syntactic duplication in test_find_users.py at lines 346-357 and
268-279. Consider extracting the shared mock setup for db, security_manager,
and or_ into a reusable helper function or fixture.
</div>
</div>
<small><i>Code Review Run #147078</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
