Copilot commented on code in PR #40659:
URL: https://github.com/apache/superset/pull/40659#discussion_r3338100472
##########
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 uses `patch`, but it isn't imported in this module
yet.
##########
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:
This test currently relies on `user_has_permission()` raising inside the
real `security_manager.has_access()` when passed a bare `Mock` user, which
produces a warning log and doesn't explicitly validate the intended “falls
through to permission check and denies” behavior. Mock
`superset.security_manager.has_access` to return False and assert it was called.
##########
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:
The admin bypass only checks `user.roles`, but Superset users can also
inherit roles via groups (FAB groups -> roles). Since MCP explicitly loads
`User.groups`/`Group.roles` for permission checks, admins granted through a
group would miss this bypass and be over-filtered. Consider checking group
roles as well when matching `AUTH_ROLE_ADMIN`.
--
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]
