fix(mcp): honor AUTH_ROLE_ADMIN and warn on permission-less protected tools#40659
Conversation
dosubot
Bot
added
authentication
Code Review Agent Run #819485Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40659 +/- ##
==========================================
- Coverage 64.05% 64.05% -0.01%
==========================================
Files 2662 2662
Lines 143254 143261 +7
Branches 32941 32941
==========================================
+ Hits 91764 91765 +1
- Misses 49903 49909 +6
Partials 1587 1587
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens MCP authorization behavior by (1) honoring Superset’s configurable admin role name (AUTH_ROLE_ADMIN) when applying admin bypass logic in MCP field-level permission filtering, and (2) surfacing potentially dangerous RBAC misconfiguration for MCP tools by warning when a protected tool declares no class_permission_name.
Changes:
- Update MCP field-permission admin detection to use
current_app.config["AUTH_ROLE_ADMIN"]instead of hardcoded"Admin". - Add a warning log when
check_tool_permission()encounters a tool withoutclass_permission_name(while still allowing access). - Add a unit test ensuring the admin bypass follows
AUTH_ROLE_ADMINwhen the admin role is renamed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
superset/mcp_service/utils/permissions_utils.py |
Uses configured AUTH_ROLE_ADMIN for the admin bypass in MCP field-level permission checks. |
superset/mcp_service/auth.py |
Logs a warning when a protected tool has no RBAC class permission metadata, while maintaining backward-compatible allow behavior. |
tests/unit_tests/mcp_service/utils/test_permissions_utils.py |
Adds coverage for honoring AUTH_ROLE_ADMIN in the MCP permission helper. |
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Code Review — PR #40659
HEAD SHA: 68ba87ec43291b8a72640515b258058be447167e
The two fixes are directionally correct. Two issues should be addressed in this PR rather than deferred to follow-ups.
HIGH — Per-call WARNING log will flood production logs
superset/mcp_service/auth.py, lines 126–130
check_tool_permission is called on every authenticated MCP tool invocation (both async wrapper line 656 and sync wrapper line 703 in mcp_auth_hook). The new logger.warning fires on every such call for any protect=True tool that has no class_permission_name. If any deployed tool has that configuration, every user request emits a WARNING, burying real permission-denial alerts and producing significant log churn in production.
The signal is right; the frequency and level are wrong. Two options:
- Preferred: move to registration time — warn once when the tool is registered, not per-call
- Fallback: downgrade to
logger.debugso it's opt-in rather than default noise
This was flagged by the codeant reviewer and rusackas acknowledged a follow-up. Given it's an operational hazard on every tool call, it should ship fixed in this PR.
MEDIUM — New test asserts False for incidental reasons, not RBAC correctness
tests/unit_tests/mcp_service/utils/test_permissions_utils.py, lines 46–51
not_admin = Mock()
not_admin.roles = [_role("Admin")]
assert user_has_permission(not_admin, "can_read", "Chart") is FalseThis passes because security_manager.has_access is not mocked and raises/returns False in the unit test context — not because the RBAC check correctly evaluated the user's permissions. A future change to the exception handling could make this assertion pass or fail for unrelated reasons.
Fix: mock security_manager.has_access to return False and assert it was called:
with patch("superset.mcp_service.utils.permissions_utils.security_manager") as mock_sm:
mock_sm.has_access.return_value = False
assert user_has_permission(not_admin, "can_read", "Chart") is False
mock_sm.has_access.assert_called_once()This was noted by Copilot and acknowledged as a follow-up — it should be in this PR.
MEDIUM — Silent behavioral change: lowercase "admin" role loses bypass
superset/mcp_service/utils/permissions_utils.py, line 100
The old check accepted both "Admin" and "admin". The new check only matches AUTH_ROLE_ADMIN (default "Admin"). Any deployment with a FAB role literally named "admin" (lowercase) will silently lose the MCP field-level admin bypass. The fix is correct — lowercase "admin" never should have bypassed — but the behavioral change should be noted in UPDATING.md or called out explicitly in the PR description as a breaking change for that edge case.
MEDIUM — config["AUTH_ROLE_ADMIN"] subscript fails silently via broad except
superset/mcp_service/utils/permissions_utils.py, line 97
FAB always sets AUTH_ROLE_ADMIN (default "Admin"), so a KeyError is unlikely. But if it ever occurs, it's silently swallowed by except Exception as e and user_has_permission returns False with only a generic "Error checking permission" log. Using .get("AUTH_ROLE_ADMIN", "Admin") would be more explicit about the fallback and avoid the silent path for a configuration problem.
NIT — Inline import lacks a comment
superset/mcp_service/utils/permissions_utils.py, line 92
from flask import current_app is deferred inside the function body. This follows an existing pattern in the file (from flask import g, from superset import security_manager), but a brief comment explaining why it's inline (app context not guaranteed at module import time) would help future readers.
Scan coverage (all 19 run)
Scans 2–19 came back clean: no SQL injection, no missing auth decorators, no hardcoded credentials, no dangerous builtins, no deprecated APIs, no f-strings in logger calls, no module-level config access, no unused imports.
Praise
- Correctly removing the loose
"admin"alias is the right call — only the configuredAUTH_ROLE_ADMINshould bypass, not an arbitrary lowercase variant. - All new logger calls use lazy
%sformatting. try/finallyin the test correctly prevents config pollution between tests.- Keeping the allow behavior in
check_tool_permissionwhenclass_permission_nameis absent is correct — fail-closing would break legitimate tools that useprotect=Truewithout an RBAC class mapping. - PR description cleanly separates the two findings with their motivations.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
sadpandajoe
added
the
merge-if-green
Code Review Agent Run #91fdc4Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
rusackas
moved this from Needs Review
to Approved and/or Merged
in Superset Review Help Wanted
…ed tools - user_has_permission() hardcoded the admin role check to "Admin"/"admin", ignoring AUTH_ROLE_ADMIN. Deployments that rename the admin role would have admins over-filtered (denied the field-level sensitive-data bypass). Use the configured AUTH_ROLE_ADMIN. - check_tool_permission() silently allows a permission-protected tool that declares no class_permission_name. This is a supported configuration, but a protected tool with an accidentally-omitted permission would fail open with no signal. Keep the allow behavior but log a warning so the misconfiguration is visible. Add a test verifying the admin bypass follows AUTH_ROLE_ADMIN. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolves the three review threads that were previously answered with "follow-up" promises: - check_tool_permission: warn ONCE per tool (not on every protected-tool call) when a tool declares no class_permission_name — keeps the fail-open visibility without the per-call WARNING noise. - user_has_permission: the AUTH_ROLE_ADMIN bypass now also honors the admin role when it is inherited via group membership (User.groups -> Group.roles), not just directly-assigned roles. - test_user_has_permission_admin_uses_configured_role_name: explicitly mock security_manager.has_access to return False and assert it is called, instead of relying on incidental Mock behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
rusackas
force-pushed
the
fix/mcp-authz-hardening
branch
from
c960c91 to
11fcdc8
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #5e05edActionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
Two MCP authorization-hardening fixes:
user_has_permission()(superset/mcp_service/utils/permissions_utils.py) hardcoded the admin check to role name"Admin"/"admin", ignoring the configurableAUTH_ROLE_ADMIN. In deployments that rename the admin role, admins would be over-filtered (denied the field-level sensitive-data bypass) — a functionality regression, not an escalation. Now usescurrent_app.config["AUTH_ROLE_ADMIN"].check_tool_permission()(superset/mcp_service/auth.py) silently allows a permission-protected tool that declares noclass_permission_name. That's a supported configuration, so a blanket fail-closed would break legitimate tools — but an accidentally permission-less sensitive tool would fail open with no signal. Keep the allow behavior, but log a warning so the misconfiguration is visible.TESTING INSTRUCTIONS
New test: the admin bypass follows
AUTH_ROLE_ADMIN(a renamed admin role is honored; the hardcoded "Admin" no longer grants it).ADDITIONAL INFORMATION
🤖 Generated with Claude Code