fix(mcp): user/role tools demanded a permission FAB never registers

[gkneighb]

SUMMARY

Fixes a bug that makes four shipped MCP tools — list_users, get_user_info, list_roles, get_role_info — unusable for every principal, including Admin, on a stock instance.

Root cause. The tools declare class_permission_name="User"/"Role" and rely on the @tool decorator's default method permission, read, so check_tool_permission demands can_read on User / can_read on Role. But those permission-view pairs can never exist: FAB's security API views (UserApi, RoleApi) register only can_get / can_info / can_post / can_put / can_delete. Superset's own ModelRestApi subclasses remap their methods onto read/write — which is why can_read on Chart exists — but the FAB security views get no such remapping. Since Admin only holds permissions that are registered, even Admin is denied (probe evidence from a stock instance: "Permission denied: can_read on Role for user admin"; the metadata DB holds 14 Admin grants on those views, none of them can_read).

The failure is closed (deny), so this is not a security issue — just four dead tools.

Fix.

• Declare method_permission_name="get" on the four tools, matching the permission FAB actually registers for their views (can_get covers both the list and single-item GET endpoints).
• Add "get": "superset:read" to _METHOD_TO_REQUIRED_SCOPE in superset/mcp_service/auth.py — that map fails closed for unmapped method permissions (by design), so without this entry scoped-JWT deployments would still see the tools denied. Its own doc comment mandates the addition: "When introducing a new method permission, add it here."

find_users was also suspected but is not affected — it declares no class_permission_name, so it takes the documented allow-by-default path.

Found via an automated REST↔MCP capability probe run against master 1c3b070d34.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A (MCP service; no UI changes)

TESTING INSTRUCTIONS

pytest tests/unit_tests/mcp_service/test_auth_rbac.py

New regression tests: a simulated stock Admin holding exactly the FAB-registered permission set (can_get/can_info/can_post/can_put/can_delete) must pass check_tool_permission for each of the four tools (fails before this change with can_read), plus an assertion that the get method permission is scope-mapped.

Manual: on any instance with MCP_RBAC_ENABLED = True (the default), call list_users as Admin — before this change it returns "Permission denied: can_read on User"; after, it succeeds.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

🤖 Generated with Claude Code

https://claude.ai/code/session_014z1fQxTadTEvpCJLx9WnB8

[bito-code-review]

Code Review Agent Run #bcd21a

Actionable Suggestions - 0

Review Details

• Files reviewed - 6 · Commit Range: 9b91108..9b91108

  • superset/mcp_service/auth.py
  • superset/mcp_service/role/tool/get_role_info.py
  • superset/mcp_service/role/tool/list_roles.py
  • superset/mcp_service/user/tool/get_user_info.py
  • superset/mcp_service/user/tool/list_users.py
  • tests/unit_tests/mcp_service/test_auth_rbac.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.74%. Comparing base (1c3b070) to head (9b91108).
⚠️ Report is 31 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #41858      +/-   ##
==========================================
- Coverage   64.77%   64.74%   -0.04%     
==========================================
  Files        2689     2691       +2     
  Lines      148945   149230     +285     
  Branches    34369    34416      +47     
==========================================
+ Hits        96486    96615     +129     
- Misses      50693    50837     +144     
- Partials     1766     1778      +12     

Flag	Coverage Δ
hive	39.14% <ø> (-0.06%)	⬇️
mysql	57.80% <ø> (-0.06%)	⬇️
postgres	57.86% <ø> (-0.07%)	⬇️
presto	40.67% <ø> (-0.07%)	⬇️
python	59.27% <ø> (-0.06%)	⬇️
sqlite	57.45% <ø> (-0.06%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aminghadersohi]

TIGHTEN vs LOOSEN verdict: NEITHER — fixes a fail-closed-for-everyone bug without expanding the effective access boundary.

OLD: can_read on User/can_read on Role — never registered by FAB (UserApi/RoleApi only register can_get/can_info/can_post/can_put/can_delete via @permission_name("get") on ModelRestApi.get/get_list), so these tools were dead for every principal including Admin (fail-closed, not a leak).

NEW: can_get on User/can_get on Role — the exact permission FAB registers for the GET endpoints these tools proxy. Critically, SupersetSecurityManager.ADMIN_ONLY_VIEW_MENUS lists "User"/"Role" literally, and _is_admin_only() gates on view_menu name, not permission string — so any permission on these views (can_get included) is Admin-only when Gamma/Alpha/Public role PVM sets sync. The can_read→can_get swap cannot cross that boundary.

Guest/embedded tokens: still denied. Guest roles resolve to GUEST_ROLE_NAME (default "Public"), and Public/Gamma exclude admin-only PVMs — a guest token can't carry can_get on User/Role out of the box.

Consistency: all 4 tools get the identical method_permission_name="get"; auth.py's scope map adds one key (get→superset:read, same tier as the removed "read" mapping) without touching read/write/delete/execute_sql_query for other tools.

Data exposure (serialize_user_object / sensitive-field gating) is pre-existing code, untouched by this diff.

Scans 1–20: no violations — diff is comments + one dict entry + method_permission_name kwargs + tests, no new runtime branches.

One inline suggestion below (non-blocking).

[aminghadersohi]

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.

[bito-code-review]

The reviewer's suggestion to add a negative test case for non-admin users is appropriate and would improve the test suite's rigor. Adding a parametrized test that verifies check_tool_permission returns False for a user lacking the required permissions (e.g., a viewer) would explicitly confirm the negative path for these specific tools, complementing the existing positive test for the Admin role.

tests/unit_tests/mcp_service/test_auth_rbac.py

@pytest.mark.parametrize(
    "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_denied_for_viewer(app_context, module_path, func_name) -> None:
    """A viewer (holding no permissions) must be denied access to user/role tools."""
    import importlib
    func = getattr(importlib.import_module(module_path), func_name)
    g.user = MagicMock(username="viewer")
    mock_sm = MagicMock()
    mock_sm.can_access = MagicMock(return_value=False)
    with patch("superset.mcp_service.auth.security_manager", mock_sm):
        assert check_tool_permission(func) is False

[rusackas]

Thanks @gkneighb, this is a clean fix and the test actually proves the stock-admin failure mode instead of just asserting it. @aminghadersohi's suggestion for a negative-path companion test is a nice add but not a blocker, as noted. LGTM, approving now that CI's clean.