aminghadersohi commented on code in PR #40098:
URL: https://github.com/apache/superset/pull/40098#discussion_r3266861592
##########
superset/mcp_service/app.py:
##########
@@ -310,6 +316,10 @@ def get_default_instructions(branding: str = "Apache
Superset") -> str:
- get_instance_info returns current_user.roles (e.g., ["Admin"], ["Alpha"],
["Viewer"]).
- ALWAYS check the user's roles BEFORE suggesting write operations (creating
datasets,
charts, dashboards, or running SQL).
+- Write tools (generate_chart, generate_dashboard, update_chart,
create_virtual_dataset,
+ save_sql_query, add_chart_to_existing_dashboard, update_chart_preview)
require write
+ permissions. These tools are only listed for users who have the necessary
access.
+ If a write tool does not appear in the tool list, the current user lacks
write access.
Review Comment:
Fixed: removed `or running SQL` from the write-operations bullet. SQL
execution (`execute_sql`) is controlled by the separate `execute_sql_query`
permission on SQLLab, not by `can_write` — this is already called out
explicitly in the `execute_sql` bullet below that section.
##########
superset/mcp_service/middleware.py:
##########
@@ -399,6 +401,79 @@ async def on_call_tool(
return result
+class RBACToolVisibilityMiddleware(Middleware):
+ """Filter tools/list response based on current user's RBAC permissions.
+
+ Intercepts every ``tools/list`` request and removes tools the calling user
+ is not permitted to execute. Public tools (no ``class_permission_name``)
and
+ tools whose permission check passes are included; all others are hidden.
+
+ Fail-open vs fail-closed behaviour:
+ - No auth context at all (no Flask context, no auth header, no dev user
+ configured) → fail open (return all tools). Call-time RBAC enforces.
+ - Auth was attempted but credentials are invalid (bad API key, dev
+ username not in DB, etc.) → fail closed (return only public tools,
+ i.e. those with no ``class_permission_name``).
+ - Unexpected errors → fail open. Call-time RBAC still enforces.
+ """
+
+ @staticmethod
+ def _public_tools_only(tools: list[Tool]) -> list[Tool]:
+ """Return only tools that require no class-level permission."""
+ return [
+ t
+ for t in tools
+ if getattr(getattr(t, "fn", None), "_class_permission_name", None)
is None
+ ]
+
+ async def on_list_tools(
+ self,
+ context: MiddlewareContext[mt.ListToolsRequest],
+ call_next: CallNext[mt.ListToolsRequest, list[Tool]],
+ ) -> list[Tool]:
+ tools = await call_next(context)
+
+ try:
+ from flask import g
+
+ from superset.mcp_service.auth import (
+ _get_app_context_manager,
+ _setup_user_context,
+ is_tool_visible_to_current_user,
+ )
+
+ with _get_app_context_manager():
+ try:
+ user = _setup_user_context()
+ except ValueError as exc:
+ if "No authenticated user found" in str(exc):
+ # No auth source configured at all → fail open
+ return tools
Review Comment:
Addressed: `on_list_tools` now calls `get_user_from_request()` directly
instead of `_setup_user_context()`. The dedicated resolver performs only the
token/key/dev-user lookup with no retry loop, no session management, and no
error logging — so unauthenticated `tools/list` requests (the fail-open path)
are completely silent.
--
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]
