rusackas commented on code in PR #40659:
URL: https://github.com/apache/superset/pull/40659#discussion_r3338262542
##########
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:
Valid point. Emitting a WARNING on every protected-tool call for tools
without a class_permission_name would be noisy in practice. Downgrading to
debug or moving to registration-time is the right call — will address in a
follow-up.
##########
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"]
Review Comment:
The broad except is intentional fail-safe behavior (deny on any error). The
function is designed to be called within a Flask request context; calling it
outside one is not a supported use-case here. That said, the concern about the
except masking legitimate permission denials is noted — a narrower RuntimeError
catch would be cleaner without changing semantics.
##########
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:
Correct — the bypass only inspects user.roles directly, not group-inherited
roles. FAB group-based role inheritance is rare in practice and the MCP code
already loads User.groups/Group.roles for permission checks. The short-circuit
is intentional for the direct-role case. Group-inherited admin detection can be
added as a follow-up if operators report it.
##########
tests/unit_tests/mcp_service/utils/test_permissions_utils.py:
##########
@@ -19,13 +19,40 @@
from unittest.mock import Mock
Review Comment:
The suggested test fix in your previous comment referenced patch, but the
test file already uses current_app.config directly (see the
test_user_has_permission_admin_uses_configured_role_name test above). No patch
import is needed for the actual test added in this PR.
##########
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:
Agreed the test relies on incidental behavior from passing a bare Mock to
has_access. Explicitly mocking security_manager.has_access to return False and
asserting it was called would make the intent clearer. Will improve in a
follow-up.
--
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]
