aminghadersohi commented on code in PR #41858:
URL: https://github.com/apache/superset/pull/41858#discussion_r3540089343
##########
tests/unit_tests/mcp_service/test_auth_rbac.py:
##########
@@ -478,3 +478,69 @@ def
test_scope_execute_sql_query_requires_write_scope(app_context) -> None:
assert check_tool_permission(func) is False
with _patch_token_scopes(["superset:write"]):
assert check_tool_permission(func) is True
+
+
+# ---------------------------------------------------------------------------
+# User/Role tools must request a permission FAB actually registers.
+#
+# FAB's security API views (UserApi, RoleApi) register only
+# can_get/can_info/can_post/can_put/can_delete — never can_read. Superset's
+# own ModelRestApi subclasses remap methods onto read/write, which is why
+# "can_read on Chart" exists while "can_read on User" cannot. A tool that
+# demands can_read on User/Role is therefore dead for every principal,
+# including Admin (fails closed).
+# ---------------------------------------------------------------------------
+
+# The full set of permissions FAB registers on its security API views.
+_FAB_SECURITY_VIEW_PERMISSIONS = {
+ "can_get",
+ "can_info",
+ "can_post",
+ "can_put",
+ "can_delete",
+}
+
+
+def _fab_security_view_can_access(permission_str: str, view_name: str) -> bool:
+ """Simulate can_access for a user holding every permission FAB actually
+ registers on the User/Role security views (i.e. a stock Admin)."""
+ return permission_str in _FAB_SECURITY_VIEW_PERMISSIONS
+
+
[email protected](
+ "module_path,func_name",
+ [
+ ("superset.mcp_service.user.tool.list_users", "list_users"),
+ ("superset.mcp_service.user.tool.get_user_info", "get_user_info"),
+ ("superset.mcp_service.role.tool.list_roles", "list_roles"),
+ ("superset.mcp_service.role.tool.get_role_info", "get_role_info"),
+ ],
+)
+def test_user_role_tools_usable_by_stock_admin(
Review Comment:
MEDIUM (non-blocking): this parametrized test proves the allow-path for a
stock Admin holding exactly `_FAB_SECURITY_VIEW_PERMISSIONS`, but there's no
equivalent denied-path test for these 4 tools specifically (e.g. a stock
Gamma/Viewer holding none of that set → `check_tool_permission` is False). The
generic deny mechanism is already covered by the pre-existing
`test_check_tool_permission_denied`, so this isn't blocking — but a
parametrized "stock non-admin is denied" companion test would make the negative
case explicit for `can_get on User/Role` specifically, matching the rigor of
the allow-path test above.
--
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]
