fix(mcp): hide write tools from users without write permissions by aminghadersohi · Pull Request #40098 · apache/superset

aminghadersohi · 2026-05-13T20:41:31Z

SUMMARY

Add RBAC-aware tool visibility to the MCP service so that write tools are hidden from users who lack write permissions, and permission-denied errors surface cleanly to the caller.

Phase 1 — clean permission-denied errors:
MCPPermissionDeniedError did not subclass PermissionError, so GlobalErrorHandlerMiddleware._handle_error() fell through to the generic "Internal error" branch (500-style response). Fixed by:

Adding MCPPermissionDeniedError to _USER_ERROR_TYPES so it is logged at WARNING, not ERROR
Adding an explicit elif isinstance(error, MCPPermissionDeniedError) branch that converts the error to a structured ToolError with the denial message

Phase 2 — per-request tools/list filtering:

Add is_tool_visible_to_current_user(tool) to auth.py as the single source of truth for tool visibility, covering both RBAC permissions and data-model metadata privacy
Add RBACToolVisibilityMiddleware that intercepts tools/list responses and removes tools the requesting user lacks permission to execute. Fail-open: if user resolution fails, all tools are returned (call-time RBAC still enforces)
Refactor _tool_allowed_for_current_user() in server.py to delegate to is_tool_visible_to_current_user() so tool-search and tools/list share identical visibility logic
Register RBACToolVisibilityMiddleware inside StructuredContentStripperMiddleware so it filters full tool objects before outputSchema stripping
Update server instructions to note write tools require write permissions

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — MCP protocol change, no UI.

TESTING INSTRUCTIONS

Unit tests cover all new logic:

pytest tests/unit_tests/mcp_service/test_auth_rbac.py -v
pytest tests/unit_tests/mcp_service/test_middleware.py -v

Connect an MCP client as a Viewer-role user and confirm write tools (generate_chart, generate_dashboard, etc.) do not appear in tools/list.
Connect as an Admin and confirm write tools are present.
Call a write tool directly as a Viewer; confirm the response is a structured ToolError with a clear denial message (not an "Internal error").

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: MCP_RBAC_ENABLED (default True)
Changes UI
Includes DB Migration
Introduces new feature or API
Removes existing feature or API

netlify · 2026-05-13T20:43:58Z

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	`6054446`
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6a0eda1b33a18300082764f8
😎 Deploy Preview	https://deploy-preview-40098--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

codecov · 2026-05-13T21:11:54Z

Codecov Report

❌ Patch coverage is 6.81818% with 82 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.12%. Comparing base (f187a8e) to head (97eb375).
⚠️ Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/auth.py	15.38%	33 Missing ⚠️
superset/mcp_service/middleware.py	0.00%	29 Missing ⚠️
superset/mcp_service/server.py	0.00%	15 Missing ⚠️
superset/mcp_service/__main__.py	0.00%	4 Missing ⚠️
superset/db_engine_specs/base.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #40098      +/-   ##
==========================================
- Coverage   64.14%   64.12%   -0.02%     
==========================================
  Files        2592     2592              
  Lines      138841   138892      +51     
  Branches    32201    32210       +9     
==========================================
+ Hits        89064    89069       +5     
- Misses      48245    48291      +46     
  Partials     1532     1532

Flag	Coverage Δ
hive	`39.30% <6.81%> (-0.03%)`	⬇️
mysql	`58.81% <6.81%> (-0.04%)`	⬇️
postgres	`58.89% <6.81%> (-0.04%)`	⬇️
presto	`40.97% <6.81%> (-0.03%)`	⬇️
python	`60.45% <6.81%> (-0.05%)`	⬇️
sqlite	`58.53% <6.81%> (-0.04%)`	⬇️
unit	`100.00% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 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.

Copilot

Pull request overview

This PR adds RBAC-aware MCP tool visibility so users without permissions see fewer tools in discovery, while improving permission-denied error handling.

Changes:

Adds shared MCP tool visibility checks and a tools/list filtering middleware.
Converts MCPPermissionDeniedError into user-facing ToolError responses and warning-level logs.
Updates MCP instructions and unit tests for RBAC visibility behavior.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`superset/mcp_service/auth.py`	Adds shared tool visibility helper and extracts app-context selection logic.
`superset/mcp_service/middleware.py`	Adds RBAC tool-list filtering and permission-denied error handling.
`superset/mcp_service/server.py`	Wires RBAC visibility middleware and delegates tool-search filtering.
`superset/mcp_service/app.py`	Updates MCP instructions for permission-aware tool availability.
`superset/mcp_service/__main__.py`	Reuses the shared middleware stack for stdio entrypoint setup.
`tests/unit_tests/mcp_service/test_auth_rbac.py`	Adds tests for shared tool visibility logic.
`tests/unit_tests/mcp_service/test_middleware.py`	Adds tests for permission-denied handling and RBAC tool-list middleware.
`tests/unit_tests/mcp_service/test_tool_search_transform.py`	Updates privacy-function patch targets after visibility refactor.

Phase 1: MCPPermissionDeniedError falls through to GlobalErrorHandlerMiddleware's generic "Internal error" branch (500-style response) because it doesn't subclass PermissionError. Fixed by adding it to _USER_ERROR_TYPES and an explicit elif branch in _handle_error() that converts it to a clean ToolError. Phase 2: Add RBACToolVisibilityMiddleware that intercepts tools/list and removes tools the calling user lacks permission to execute. Add is_tool_visible_to_current_user() to auth.py as the single source of truth for tool visibility, shared by both the new middleware and the existing tool-search transform. Register the middleware inside StructuredContentStripperMiddleware so it filters full tool objects before outputSchema stripping. Fail open: if user resolution fails, all tools are returned (call-time RBAC still enforces). Also update server instructions to note write tools require write permissions.

- Fail closed (return only public tools) when credentials are invalid (PermissionError from bad API key, ValueError from unknown dev username); fail open only when no auth source is configured at all - Extract _get_app_context_manager() to module level in auth.py so RBACToolVisibilityMiddleware reuses the same context-selection logic as mcp_auth_hook, preventing external g.user from being shadowed - Add RBACToolVisibilityMiddleware to __main__.py stdio entry point via build_middleware_list() to keep all transports in sync - Fix stale patch targets in test_tool_search_transform.py: update superset.mcp_service.server.user_can_view_data_model_metadata → superset.mcp_service.privacy.user_can_view_data_model_metadata - Qualify write tool listings in instructions with "(requires write access)" and add a permissions preamble so read-only users are not confused by tools they cannot call Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…lity - app.py: clarify execute_sql requires SQL Lab access (not write access) in both the instructions preamble and Permission Awareness section - auth.py: add log_denial param to check_tool_permission() to suppress noisy WARNING logs during tools/list scanning; downgrade "No authenticated user found" from ERROR to DEBUG in _setup_user_context - middleware.py: fail completely closed (return []) on credential failures instead of returning tools with no class_permission_name, which could include protect=True tools requiring auth; remove _public_tools_only helper - server.py: catch PermissionError (invalid API key) in addition to ValueError in _tool_allowed_for_current_user - tests: add tests for fail-closed branches (PermissionError, bad ValueError, and no-auth-configured ValueError in RBACToolVisibilityMiddleware)

…bility Thread 1 (app.py): Restructure the permission preamble to unambiguously separate write-access operations from SQL Lab access. Previously the preamble listed "saving SQL queries" inside the write-operations clause which could be read as including execute_sql. Now each permission type is its own bullet with explicit tool names. Thread 2 (server.py): Make _tool_allowed_for_current_user consistent with RBACToolVisibilityMiddleware: "No authenticated user found" ValueError now returns True (fail-open, show the tool) instead of False. Other ValueErrors and PermissionError remain fail-closed. Previously tool-search mode would hide all tools when no auth was configured, while tools/list showed all. Thread 3 (middleware.py): Replace _setup_user_context() with a direct call to get_user_from_request() in on_list_tools. _setup_user_context carries per-call execution overhead (retry loop, session management, error logging) that is inappropriate and noisy at list time. The middleware now controls all logging for list-time auth failures directly. Also updates all RBACToolVisibilityMiddleware tests to patch get_user_from_request instead of _setup_user_context, matching the refactored implementation.

- auth.py: collapse check_tool_permission signature to one line (ruff-format) - auth.py: extract _log_user_resolution_failure() helper to reduce _setup_user_context cyclomatic complexity from 11 to 10 (ruff C901) - test_middleware.py: shorten docstring to stay within 88-char limit (ruff E501)

- Restore "Available tools:" section header in app.py instructions so test_get_default_instructions_declares_data_boundary can find it - Revert fail-open change in _tool_allowed_for_current_user: tool-search should stay fail-closed (hide protected tools) when no user is resolved; only RBACToolVisibilityMiddleware.on_list_tools is fail-open for the no-auth-configured case Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…tructions Remove 'or running SQL' from the write-operations bullet so that SQL execution is not grouped under can_write. execute_sql is controlled by the separate execute_sql_query permission on SQLLab, which is already called out in its own bullet below. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Move contextlib, flask, and auth imports from function bodies to module level in auth.py and middleware.py. The only remaining local import is get_flask_app in _get_app_context_manager, which is deferred because importing it at module level would trigger create_app() before Superset is fully initialised (e.g. during unit-test collection). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… visibility - Fix ruff error: consolidate contextlib imports into single from-import - Fix test patch targets: middleware tests must patch middleware module after imports were promoted to module level (not auth module) - Fix _tool_allowed_for_current_user: pass public tools through when user resolution fails (only hide tools with _class_permission_name) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Move MCPPermissionDeniedError after underscore-prefixed import to satisfy ruff I001 (isort ordering: underscore names sort before M). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Apply ruff PT023: remove unnecessary parentheses from @pytest.mark.asyncio() decorators (34 occurrences). Required by ruff 0.9.7+ used in pre-commit (next). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…nd test_middleware.py Move create_response_size_guard_middleware and build_middleware_list from function body to module level in __main__.py (no circular import issue). Move MCPPermissionDeniedError and RBACToolVisibilityMiddleware from repeated local imports to module-level imports in test_middleware.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Split the combined except clause in _tool_allowed_for_current_user so that PermissionError (bad API key) returns False for every tool, matching RBACToolVisibilityMiddleware's fail-closed behaviour. ValueError (no auth source configured) retains the existing public-tool fallback. Adds a test to cover the new deny-all path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

1. MiddlewareContext is a frozen dataclass so assigning context.mcp_call_id raises FrozenInstanceError at runtime. Replace with a module-level ContextVar that LoggingMiddleware sets and StructuredContentStripperMiddleware reads. 2. _tool_allowed_for_current_user in server.py accesses flask.g without a Flask app context when called from the synthetic search_tools tool, causing RuntimeError and silently filtering out all search results. Guard with has_app_context() and push one via _get_app_context_manager() when none exists. Also remove unknown pylintrc option and suppress pre-existing consider-using-transaction warning in db_engine_specs/base.py.

The test was patching flask_singleton.get_flask_app, but when a Flask app context is already active (as in the test environment), _get_app_context_manager() never calls get_flask_app() so the mock had no effect. Patch _get_app_context_manager directly instead, which is the function that actually triggers the fail-open path.

bito-code-review · 2026-05-22T05:19:24Z

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

…he#40098) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

pull-request-size Bot added the size/L label May 13, 2026

pull-request-size Bot added size/XL and removed size/L labels May 13, 2026

aminghadersohi force-pushed the mcp-rbac-tool-visibility branch from a4a7f87 to 3993a04 Compare May 14, 2026 23:29

aminghadersohi requested a review from Copilot May 14, 2026 23:30

Copilot started reviewing on behalf of aminghadersohi May 14, 2026 23:30 View session

Copilot AI reviewed May 14, 2026

View reviewed changes

sadpandajoe added the review:draft label May 18, 2026

aminghadersohi force-pushed the mcp-rbac-tool-visibility branch from 4765078 to be145c3 Compare May 20, 2026 15:56

aminghadersohi marked this pull request as ready for review May 20, 2026 15:57

dosubot Bot added the authentication:RBAC Related to RBAC label May 20, 2026

aminghadersohi marked this pull request as draft May 20, 2026 15:59

aminghadersohi marked this pull request as ready for review May 20, 2026 21:05

aminghadersohi requested a review from Antonio-RiveroMartnez May 20, 2026 21:06

aminghadersohi commented May 20, 2026

View reviewed changes

Comment thread superset/mcp_service/__main__.py Outdated

aminghadersohi commented May 20, 2026

View reviewed changes

Comment thread tests/unit_tests/mcp_service/test_middleware.py Outdated

codeant-ai-for-open-source Bot reviewed May 20, 2026

View reviewed changes

Comment thread superset/mcp_service/server.py Outdated

aminghadersohi and others added 11 commits May 21, 2026 10:10

ci: trigger CI run after rebase

f0a9de4

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(mcp): fix unsorted imports in middleware.py

2ce863b

Move MCPPermissionDeniedError after underscore-prefixed import to satisfy ruff I001 (isort ordering: underscore names sort before M). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

aminghadersohi and others added 3 commits May 21, 2026 10:10

fix(mcp): fix pytest mark parentheses style in test_middleware.py

a163f94

Apply ruff PT023: remove unnecessary parentheses from @pytest.mark.asyncio() decorators (34 occurrences). Required by ruff 0.9.7+ used in pre-commit (next). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

aminghadersohi force-pushed the mcp-rbac-tool-visibility branch from 91b8442 to 6054446 Compare May 21, 2026 10:10

Antonio-RiveroMartnez approved these changes May 21, 2026

View reviewed changes

aminghadersohi added 3 commits May 21, 2026 12:47

Merge branch 'master' into mcp-rbac-tool-visibility

00a077b

aminghadersohi merged commit e25d708 into apache:master May 21, 2026
68 checks passed

sha174n pushed a commit to sha174n/superset that referenced this pull request May 22, 2026

fix(mcp): hide write tools from users without write permissions (apac…

abb7280

…he#40098) Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(mcp): hide write tools from users without write permissions#40098

fix(mcp): hide write tools from users without write permissions#40098
aminghadersohi merged 17 commits into
apache:masterfrom
aminghadersohi:mcp-rbac-tool-visibility

aminghadersohi commented May 13, 2026

Uh oh!

netlify Bot commented May 13, 2026 •

edited

Loading

Uh oh!

codecov Bot commented May 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bito-code-review Bot commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

aminghadersohi commented May 13, 2026

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

netlify Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for superset-docs-preview ready!

Uh oh!

codecov Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bito-code-review Bot commented May 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify Bot commented May 13, 2026 •

edited

Loading

codecov Bot commented May 13, 2026 •

edited

Loading