rusackas commented on code in PR #43388:
URL: https://github.com/apache/superset/pull/43388#discussion_r3845269580
##########
superset/mcp_service/mcp_config.py:
##########
@@ -495,6 +506,16 @@ def create_default_mcp_auth_factory(app: Flask) ->
Optional[Any]:
if not (auth_enabled or api_key_enabled or guest_enabled):
return None
+ # MCP_DEV_USERNAME makes user resolution fall back to a fixed user for
+ # requests that carry no resolvable identity, which defeats the point of
+ # having transport auth enabled. Refuse the combination outright.
+ if auth_enabled and app.config.get("MCP_DEV_USERNAME"):
Review Comment:
Fixed. The README's combined `superset_config.py` example set both
`MCP_DEV_USERNAME` and `MCP_AUTH_ENABLED = True` together, which now raises at
startup — split it into two clearly separate dev-mode and production examples,
updated the env-var table's description and default, and added an UPDATING.md
entry for the required migration.
##########
tests/unit_tests/mcp_service/chart/tool/test_restore_chart.py:
##########
@@ -240,3 +271,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 = [
Review Comment:
Fixed. The mock's return value now keys off the actual `skip_base_filter`
kwarg of each call (via a side_effect function) instead of call order, so a
regression that leaves `skip_base_filter=True` on the re-lookup makes the mock
return the chart on both calls and the test's assertions fail, instead of
silently passing. Applied the same fix to the identical pattern in
test_restore_dashboard.py.
--
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]
