rusackas commented on code in PR #40659:
URL: https://github.com/apache/superset/pull/40659#discussion_r3343244332
##########
superset/mcp_service/auth.py:
##########
@@ -119,7 +119,15 @@ def check_tool_permission(func: Callable[..., Any]) ->
bool:
class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
if not class_permission_name:
- # No RBAC configured for this tool; allow by default.
+ # No RBAC configured for this tool; allow by default. This is a
+ # supported configuration (a protected tool may intentionally
+ # declare no permission class), but surface it so an accidental
+ # omission on a sensitive tool doesn't silently fail open.
+ logger.warning(
+ "Tool %s is permission-protected but declares no "
+ "class_permission_name; allowing access without an RBAC check",
+ func.__name__,
+ )
Review Comment:
Done in 0bfab4f2d8 — this now warns once per tool (tracked in a module-level
set) instead of on every protected-tool call, so the fail-open case still
surfaces without the per-call noise.
##########
superset/mcp_service/utils/permissions_utils.py:
##########
@@ -89,10 +89,15 @@ def user_has_permission(
return False
try:
- # Check if user is admin (has all permissions)
+ from flask import current_app
+
+ # Check if user is admin (has all permissions). Use the configured
+ # admin role name rather than hardcoding "Admin", so deployments that
+ # rename the admin role (AUTH_ROLE_ADMIN) still grant admins the
bypass.
+ admin_role_name = current_app.config["AUTH_ROLE_ADMIN"]
if hasattr(user, "roles"):
for role in user.roles:
- if role.name in ("Admin", "admin"):
+ if role.name == admin_role_name:
return True
Review Comment:
Done in 0bfab4f2d8 — the AUTH_ROLE_ADMIN bypass now also honors the admin
role when it's inherited via group membership (User.groups → Group.roles), not
just directly-assigned roles.
##########
tests/unit_tests/mcp_service/utils/test_permissions_utils.py:
##########
@@ -19,13 +19,40 @@
from unittest.mock import Mock
+from flask import current_app
+
from superset.mcp_service.utils.permissions_utils import (
apply_field_permissions_to_columns,
filter_sensitive_data,
get_allowed_fields,
+ user_has_permission,
)
+def test_user_has_permission_admin_uses_configured_role_name():
+ """The admin bypass honors AUTH_ROLE_ADMIN, not a hardcoded 'Admin'."""
+
+ def _role(name: str) -> Mock:
+ role = Mock()
+ role.name = name # set after construction (Mock intercepts name=)
+ return role
+
+ original = current_app.config["AUTH_ROLE_ADMIN"]
+ current_app.config["AUTH_ROLE_ADMIN"] = "Superuser"
+ try:
+ admin = Mock()
+ admin.roles = [_role("Superuser")]
+ assert user_has_permission(admin, "can_read", "Chart") is True
+
+ # The previously-hardcoded "Admin" name no longer grants the bypass;
+ # it falls through to the real permission check, which denies here.
+ not_admin = Mock()
+ not_admin.roles = [_role("Admin")]
+ assert user_has_permission(not_admin, "can_read", "Chart") is False
Review Comment:
Done in 0bfab4f2d8 — the test now explicitly patches
security_manager.has_access to return False and asserts it was called, rather
than relying on incidental Mock behavior.
--
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]
