QT/superset2
1
0
Fork 0
mirror of https://github.com/apache/superset.git synced 2026-05-25 17:55:15 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(mcp): address remaining Copilot review comments on RBAC tool visibility

Browse Source 
Thread 1 (app.py): Restructure the permission preamble to unambiguously
separate write-access operations from SQL Lab access. Previously the
preamble listed "saving SQL queries" inside the write-operations clause
which could be read as including execute_sql. Now each permission type
is its own bullet with explicit tool names.

Thread 2 (server.py): Make _tool_allowed_for_current_user consistent with
RBACToolVisibilityMiddleware: "No authenticated user found" ValueError now
returns True (fail-open, show the tool) instead of False. Other ValueErrors
and PermissionError remain fail-closed. Previously tool-search mode would
hide all tools when no auth was configured, while tools/list showed all.

Thread 3 (middleware.py): Replace _setup_user_context() with a direct call
to get_user_from_request() in on_list_tools. _setup_user_context carries
per-call execution overhead (retry loop, session management, error logging)
that is inappropriate and noisy at list time. The middleware now controls
all logging for list-time auth failures directly.

Also updates all RBACToolVisibilityMiddleware tests to patch
get_user_from_request instead of _setup_user_context, matching the
refactored implementation.
This commit is contained in:
Amin Ghadersohi
2026-05-15 00:24:26 +00:00
parent 9ed83c6a5b
commit 5e1ce67237
4 changed files with 32 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

13
tests/unit_tests/mcp_service/test_middleware.py
View File

@@ -1140,7 +1140,7 @@ class TestRBACToolVisibilityMiddleware:
@pytest.mark.asyncio
async def test_fails_open_when_user_is_none(self, app) -> None:
"""Returns all tools when _setup_user_context returns None."""
"""Returns all tools when get_user_from_request returns None."""
from superset.mcp_service.middleware import RBACToolVisibilityMiddleware
tools = [self._make_tool("list_charts"), self._make_tool("generate_chart")]
@@ -1151,7 +1151,7 @@ class TestRBACToolVisibilityMiddleware:
patch(
"superset.mcp_service.flask_singleton.get_flask_app", return_value=app
),
patch("superset.mcp_service.auth._setup_user_context", return_value=None),
patch("superset.mcp_service.auth.get_user_from_request", return_value=None),
):
result = await middleware.on_list_tools(MagicMock(), call_next)
@@ -1178,7 +1178,8 @@ class TestRBACToolVisibilityMiddleware:
"superset.mcp_service.flask_singleton.get_flask_app", return_value=app
),
patch(
"superset.mcp_service.auth._setup_user_context", return_value=mock_user
"superset.mcp_service.auth.get_user_from_request",
return_value=mock_user,
),
patch(
"superset.mcp_service.auth.is_tool_visible_to_current_user",
@@ -1204,7 +1205,7 @@ class TestRBACToolVisibilityMiddleware:
"superset.mcp_service.flask_singleton.get_flask_app", return_value=app
),
patch(
"superset.mcp_service.auth._setup_user_context",
"superset.mcp_service.auth.get_user_from_request",
side_effect=PermissionError("Invalid API key"),
),
):
@@ -1226,7 +1227,7 @@ class TestRBACToolVisibilityMiddleware:
"superset.mcp_service.flask_singleton.get_flask_app", return_value=app
),
patch(
"superset.mcp_service.auth._setup_user_context",
"superset.mcp_service.auth.get_user_from_request",
side_effect=ValueError("User 'ghost' not found in database"),
),
):
@@ -1248,7 +1249,7 @@ class TestRBACToolVisibilityMiddleware:
"superset.mcp_service.flask_singleton.get_flask_app", return_value=app
),
patch(
"superset.mcp_service.auth._setup_user_context",
"superset.mcp_service.auth.get_user_from_request",
side_effect=ValueError("No authenticated user found"),
),
):