aminghadersohi commented on code in PR #40098:
URL: https://github.com/apache/superset/pull/40098#discussion_r3277397509


##########
superset/mcp_service/server.py:
##########
@@ -403,38 +400,23 @@ def _summary_serializer(tools: Sequence[Any]) -> 
list[dict[str, Any]]:
 def _tool_allowed_for_current_user(tool: Any) -> bool:
     """Return whether the current Flask user can see this tool in search 
results."""
     try:
-        from flask import current_app, g
-
-        if not current_app.config.get("MCP_RBAC_ENABLED", True):
-            return True
+        from flask import g
 
-        from superset import security_manager
         from superset.mcp_service.auth import (
-            CLASS_PERMISSION_ATTR,
             get_user_from_request,
-            METHOD_PERMISSION_ATTR,
-            PERMISSION_PREFIX,
+            is_tool_visible_to_current_user,
         )
 
-        tool_func = getattr(tool, "fn", None)
-        if tool_requires_data_model_metadata_access(tool_func) and not (
-            user_can_view_data_model_metadata()
-        ):
-            return False
-
-        class_permission_name = getattr(tool_func, CLASS_PERMISSION_ATTR, None)
-        if not class_permission_name:
-            return True
-
         if not getattr(g, "user", None):
             try:
                 g.user = get_user_from_request()
-            except ValueError:
-                return False
+            except (ValueError, PermissionError):
+                # Can't resolve user; only hide protected tools. Public tools
+                # (no _class_permission_name) pass through regardless.
+                func = getattr(tool, "fn", tool)
+                return not getattr(func, "_class_permission_name", None)

Review Comment:
   Fixed in 9ca3f8ec01. Split the combined `except (ValueError, 
PermissionError)` into two handlers: `PermissionError` now returns `False` 
(deny-all, matching `RBACToolVisibilityMiddleware`'s fail-closed path), while 
`ValueError` retains the public-tool fallback for the no-auth-configured case. 
Added a regression test 
`test_tool_search_permission_filter_denies_all_on_invalid_credentials` to cover 
this path.



-- 
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]

Reply via email to