fix(mcp): API key authentication for MCP — transport, validation, and RBAC#39604
fix(mcp): API key authentication for MCP — transport, validation, and RBAC#39604aminghadersohi wants to merge 27 commits into
Conversation
Code Review Agent Run #a46f3eActionable Suggestions - 0Review 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 |
|
The CodeQL alert indicates logging of sensitive data (password), but the code logs API key prefixes (e.g., ["sst_"]), which are configuration values, not actual passwords or secrets. This seems like a false positive. The info-level log aids debugging API key authentication setup. superset/mcp_service/mcp_config.py |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39604 +/- ##
==========================================
+ Coverage 64.20% 64.21% +0.01%
==========================================
Files 2592 2593 +1
Lines 139004 139077 +73
Branches 32273 32289 +16
==========================================
+ Hits 89241 89314 +73
Misses 48231 48231
Partials 1532 1532
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:
|
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Addressed all review comments in the latest commit: CodeQL (clear-text logging): False positive — the log line logs codeant-ai (type annotations): Added explicit type annotations to all new test function parameters and fixture return types across both test files. Fixtures now have return type annotations ( |
1cbd49c to
ed62ca2
Compare
Code Review Agent Run #933d30Actionable 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 |
ed62ca2 to
38828ab
Compare
Code Review Agent Run #257b47Actionable Suggestions - 0Review 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 |
Review:
|
38828ab to
76ad5e1
Compare
There was a problem hiding this comment.
Pull request overview
Fixes MCP authentication regressions around FAB API keys by (1) ensuring the ApiKey FAB permissions exist after superset init, and (2) allowing API-key Bearer tokens to coexist with JWT auth by routing tokens at the FastMCP transport layer and deferring API-key validation to the Flask/FAB layer.
Changes:
- Create FAB ApiKey permission-view-menus during
superset initwhenFAB_API_KEY_ENABLED=True. - Add a
CompositeTokenVerifierto pass through API-key tokens (by prefix) while still validating JWTs when configured. - Update MCP auth resolution to detect API-key pass-through and fall through to FAB
validate_api_key(), with new/updated unit tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
superset/security/manager.py |
Adds ApiKey view menu handling and explicitly creates ApiKey permissions during create_custom_permissions() when API keys are enabled. |
superset/mcp_service/composite_token_verifier.py |
Introduces a transport-layer verifier that routes Bearer tokens by prefix (API key vs JWT). |
superset/mcp_service/mcp_config.py |
Refactors default auth factory to support JWT-only, API-key-only, or hybrid mode (JWT + API keys). |
superset/mcp_service/server.py |
Enables default auth provider creation when either JWT auth or FAB API keys are enabled. |
superset/mcp_service/auth.py |
Detects API-key pass-through claim in JWT context resolution and reads API key from FastMCP AccessToken. |
tests/unit_tests/mcp_service/test_composite_token_verifier.py |
Adds unit tests validating token routing and required-scope propagation behavior. |
tests/unit_tests/mcp_service/test_auth_api_key.py |
Updates/expands unit tests to reflect AccessToken-based API key flow and pass-through claim behavior. |
15e8305 to
2bebfb7
Compare
… JWT auth Two fixes for MCP API key authentication: 1. superset init now creates ApiKey FAB permissions (can_list, can_create, can_get, can_delete) when FAB_API_KEY_ENABLED=True. Previously, because Superset uses AppBuilder(update_perms=False), FAB skipped permission creation during blueprint registration and superset init never picked them up, causing 403 errors on /api/v1/security/api_keys/. 2. CompositeTokenVerifier allows API key tokens (e.g. sst_...) to coexist with JWT auth on the MCP transport layer. Previously, when MCP_AUTH_ENABLED=True, the JWTVerifier rejected all non-JWT Bearer tokens at the transport layer before they could reach the Flask-level _resolve_user_from_api_key() handler. The composite verifier detects API key prefixes and passes them through with a marker claim, letting the existing auth priority chain handle validation.
Wire CompositeTokenVerifier into create_default_mcp_auth_factory, add _api_key_passthrough detection in _resolve_user_from_jwt_context, create ApiKey permissions in create_custom_permissions, and update test_auth_api_key with pass-through and non-matching prefix tests.
Address code review feedback: add explicit type annotations to all new test function parameters and fixture return types.
Remove API key prefixes from log message to avoid CodeQL false positive about clear-text logging of sensitive data.
…Key perms Three independent bugs let MCP requests presenting Bearer tokens with the sst_ prefix authenticate as MCP_DEV_USERNAME without any validation under streamable-http: 1. _resolve_user_from_api_key read the token from flask.request.headers, but the streamable-http transport never pushes a Flask request context — has_request_context() was always False, so the function returned None before validating, falling through to the dev-user fallback. Now reads the token from FastMCP's per-request AccessToken (which the CompositeTokenVerifier already populated) and fails closed when the key is invalid. 2. CompositeTokenVerifier was only installed when MCP_AUTH_ENABLED=True. With FAB_API_KEY_ENABLED=True alone, no transport-level verifier existed at all. The factory now builds an API-key-only verifier in that case (jwt_verifier=None) that rejects non-API-key Bearer tokens at the transport instead of silently accepting them. 3. The pass-through AccessToken was minted with scopes=[], which would make FastMCP's RequireAuthMiddleware 403 every API-key request when MCP_REQUIRED_SCOPES is non-empty. Pass-through now propagates self.required_scopes. Also addresses Daniel's review comment on superset/security/manager.py: adds "ApiKey" to ADMIN_ONLY_VIEW_MENUS so the FAB ApiKeyApi PVMs are gated to Admin instead of leaking to Alpha and Gamma. Renames the pass-through claim from _api_key_passthrough to the namespaced _superset_mcp_api_key_passthrough (exported as API_KEY_PASSTHROUGH_CLAIM) so a custom claim from an external IdP can't accidentally divert a JWT into the API-key validation path. Tests updated to mock get_access_token instead of app.test_request_context (the simulated Flask context was the reason the prior tests passed while production failed). New tests cover API-key-only verifier mode, scope propagation on pass-through, and the namespaced-claim isolation.
The API_KEY_PASSTHROUGH_CLAIM constant in auth.py and CompositeTokenVerifier in mcp_config.py have no circular-import or optional-dependency reason to be imported inline. Moved them to module top.
``superset init`` calls ``appbuilder.add_permissions(update_perms=True)`` before ``sync_role_definitions()`` (cli/main.py:84), which forces FAB to walk all registered baseviews — including ``ApiKeyApi`` (registered when ``FAB_API_KEY_ENABLED=True``) — and create their PVMs via ``add_permissions_view``. The explicit ``add_permission_view_menu`` calls in ``create_custom_permissions`` were redundant. With ``"ApiKey"`` already in ``ADMIN_ONLY_VIEW_MENUS``, the role predicate ``_is_admin_only`` gates the auto-created PVMs to Admin. Per Daniel Gaspar's review: "Adding ApiKey to ADMIN_ONLY_VIEW_MENUS should just work when FAB_API_KEY_ENABLED is True".
DetailedJWTVerifier and JWTVerifier have no circular-import or optional- dependency reason to be imported inline — fastmcp is already pulled in at module top via composite_token_verifier, and authlib is already a hard dependency. Moving them up for consistency with the rest of the module's imports.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ring - Use superset.mcp_service.auth.has_request_context as patch target in test_mcp_auth_hook_clears_stale_g_user tests; patching flask.has_request_context has no effect on the module-level import already bound in auth.py - Update test_jwt_access_token_skips_api_key_auth docstring to reference API_KEY_PASSTHROUGH_CLAIM instead of the legacy _api_key_passthrough name - Add noqa: BLE001 to broad exception catch in mcp_config.py to document that the wide catch is intentional (JWT libs raise many types, secrets guard)
Add _mock_sm_ctx() context manager to eliminate repeated boilerplate (g.user = None / app.appbuilder = MagicMock() / appbuilder.sm = mock_sm) across seven API key auth unit tests.
…nt_id guard, fail-closed on missing token - _tool_allowed_for_current_user (server.py): catch PermissionError alongside ValueError so invalid API keys return False instead of propagating through the tool-search permission filter - _setup_user_context (auth.py): catch PermissionError alongside ValueError so g.user is cleared and the error is logged consistently regardless of which failure type get_user_from_request() raises - _resolve_user_from_api_key (auth.py): require client_id=="api_key" (set by CompositeTokenVerifier) in addition to API_KEY_PASSTHROUGH_CLAIM to prevent an external IdP JWT that happens to include the claim name from being misclassified as an API-key pass-through (DoS vector) - _resolve_user_from_jwt_context (auth.py): same client_id guard so a rogue-claim JWT continues through JWT resolution instead of deferring to the API-key path (which would raise PermissionError for the user) - _resolve_user_from_api_key (auth.py): raise PermissionError (not return None) when the pass-through claim is present but the raw token is absent — fail closed rather than falling through to weaker auth - Tests: set client_id="api_key" on _passthrough_access_token helper; update test_jwt_context_with_api_key_passthrough_returns_none docstring; add test for namespaced claim on non-API-key client_id being ignored Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…er.find_user_with_relationships Fixes a gap identified in code review: the standalone load_user_with_relationships() in auth.py duplicated SecurityManager.find_user() logic but dropped two FAB behaviors: - auth_username_ci (case-insensitive username lookup) - MultipleResultsFound guard (username uniqueness not guaranteed at DB level in all FAB versions) It also hard-coded User/Group models instead of sm.user_model. Changes: - Add SupersetSecurityManager.find_user_with_relationships() to security/manager.py, mirroring FAB's find_user() (auth_username_ci, MultipleResultsFound handling, self.user_model) and adding eager loading of roles and group.roles via joinedload - Simplify load_user_with_relationships() in auth.py to a thin delegate to the new method, removing the duplicated query logic and raw Group/User imports - Add regression test asserting find_user_with_relationships() exists on the SM Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ring
_mock_sm_ctx now sets find_user_with_relationships.return_value = None so
JWT/dev-user lookups that delegate through the (now refactored)
load_user_with_relationships → security_manager.find_user_with_relationships
path behave as "user not found" in unit tests that don't patch the DB — matching
the behavior of the previous direct db.session.query() implementation.
Without this, tests that expected ValueError("not found") received a truthy
MagicMock() from the unspecified mock method, causing them to fail.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r empty/non-string entries Empty-string prefixes match every Bearer token (DoS/misclassification vector). Non-string entries cause TypeError in str.startswith(). Filter both in __init__, warn on invalid entries, and only store valid non-empty string prefixes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ring
- Remove mock_sm.find_user_with_relationships.return_value = None from
_mock_sm_ctx: load_user_with_relationships delegates to the global
security_manager (not app.appbuilder.sm), so setting it on mock_sm had
no effect and broke MagicMock(spec=[]) tests.
- Add _patch_load_user_not_found() helper that patches
superset.mcp_service.auth.load_user_with_relationships directly.
- Apply it to the 3 JWT-path tests that expect ValueError("not found"):
test_jwt_access_token_skips_api_key_auth,
test_namespaced_claim_without_api_key_client_id_is_ignored,
test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…o CompositeTokenVerifier A plain string value (e.g. FAB_API_KEY_PREFIXES = "sst_") would iterate as individual characters ['s','s','t','_'], matching far too many tokens. Wrap strings in a list at the config-read boundary so CompositeTokenVerifier always receives a proper sequence regardless of how the config is set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sion test - Remove JWT-extracted username from ValueError message in auth.py to avoid CodeQL py/clear-text-logging-sensitive-data; log at DEBUG instead - Log count of invalid FAB_API_KEY_PREFIXES entries rather than values to avoid the same CodeQL rule in composite_token_verifier.py - Add regression test asserting "ApiKey" in ADMIN_ONLY_VIEW_MENUS so a future rename cannot silently re-open the FAB ApiKeyApi to non-Admin roles
- Drop g.user.username from the permission-denied warning (CodeQL py/clear-text-logging-sensitive-data flags .username) - Replace the parametrized debug log that passed API_KEY_PASSTHROUGH_CLAIM (variable name contains KEY) with a static message
Replace string-based joinedload("roles") with the class-bound
self.group_model.roles attribute, consistent with how joinedload is
used elsewhere in Superset and forward-compatible with SQLAlchemy's
deprecation of string-based relationship names.
- auth.py: hoist security_manager, current_app, and default_user_resolver to module level; restore user id (not username) in permission-denied log so security audits remain useful without triggering CodeQL - mcp_config.py: narrow _build_jwt_verifier return type to JWTVerifier; replace bare except Exception with except (ValueError, JoseError) per authlib's exception hierarchy; annotate raw_prefixes as str|Sequence[str] to satisfy strict mypy; import JoseError and Sequence at module level - security/manager.py: move `func as sa_func` import to module level
Replace user.username and email values in logger calls with non-PII identifiers (user id integer) or remove the value entirely, so CodeQL py/clear-text-logging-sensitive-data does not flag them.
Moving security_manager to a module-level import in auth.py means
patch('superset.security_manager') no longer intercepts calls inside
auth.py — the name is bound at import time. Patch where it is used:
'superset.mcp_service.auth.security_manager'.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on logic
- MCPPermissionDeniedError now inherits PermissionError so the middleware
classifies RBAC denials as user errors (WARNING log, "Access denied"
sanitized message) rather than server errors with full stack traces.
- Fix fail-open: FAB_API_KEY_PREFIXES with a non-iterable value (e.g. None)
no longer raises TypeError and silently disables transport auth; falls back
to default prefix ["sst_"] with a warning instead.
- Fix diagnostic message in get_user_from_request(): when FAB_API_KEY_PREFIXES
is a plain string, prefix_example was taking index [0] and showing only the
first character ("s" instead of "sst_").
- DRY: _tool_allowed_for_current_user in server.py now delegates the RBAC
check to check_tool_permission (auth.py) instead of duplicating the config
flag, permission-attr lookup, and security_manager.can_access call.
- Document that MCP_REQUIRED_SCOPES is not enforced for API-key auth (FAB
keys have no scopes; RBAC is enforced via check_tool_permission instead).
- Add FAB upgrade note to find_user_with_relationships docstring: the method
mirrors FAB's find_user() internals and should be reviewed on FAB upgrades.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_tool_allowed_for_current_user now delegates to check_tool_permission
(auth.py) which uses the module-level security_manager binding.
patch('superset.security_manager') no longer intercepts those calls;
update the 4 affected tests to patch the correct location:
'superset.mcp_service.auth.security_manager'.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2bebfb7 to
747c886
Compare
mypy correctly flags that the caller can pass either ValueError or PermissionError; widen the type hint to match.
…est patch targets MCPPermissionDeniedError(PermissionError) was being caught by the generic PermissionError branch in _handle_error before reaching its own handler, so the ToolError message was sanitized to "You don't have access to this resource." instead of the structured permission message. Move the MCPPermissionDeniedError branch above PermissionError so the subclass is matched first. Also fix four visibility test patch targets: auth.py imports security_manager at module level via `from superset import security_manager`, so tests must patch `superset.mcp_service.auth.security_manager` (not `superset.security_manager`) to intercept calls inside auth.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review Agent Run #3eb4cf
Actionable Suggestions - 1
-
superset/security/manager.py - 1
- Query API inconsistency: email vs username · Line 3220-3220
Additional Suggestions - 1
-
tests/unit_tests/mcp_service/test_composite_token_verifier.py - 1
-
Incomplete test assertion coverage · Line 173-173Test `test_non_string_prefix_is_filtered_out` at line 173 checks `None not in verifier._api_key_prefixes`, but this is already implicitly validated by `test_empty_string_prefix_is_filtered_out` and `test_whitespace_only_prefix_is_filtered_out`. Consider adding a direct `verify_token(42)` call to verify the TypeError protection claimed in the docstring, or remove the redundant assertion to keep focus on observable behavior per BITO.md rule [6262].
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/security/manager.py - 1
- Dead import: eagerload unused · Line 55-55
Review Details
-
Files reviewed - 12 · Commit Range:
1213b85..20a2f34- superset/mcp_service/auth.py
- superset/mcp_service/composite_token_verifier.py
- superset/mcp_service/mcp_config.py
- superset/mcp_service/middleware.py
- superset/mcp_service/server.py
- superset/security/manager.py
- tests/unit_tests/mcp_service/test_auth_api_key.py
- tests/unit_tests/mcp_service/test_auth_rbac.py
- tests/unit_tests/mcp_service/test_auth_user_resolution.py
- tests/unit_tests/mcp_service/test_composite_token_verifier.py
- tests/unit_tests/mcp_service/test_tool_search_transform.py
- tests/unit_tests/security/test_granular_export_permissions.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ 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
| return ( | ||
| self.session.query(self.user_model) | ||
| .options(*eager) | ||
| .filter_by(email=email) |
There was a problem hiding this comment.
Email lookup at line 3220 uses .filter_by(email=email) while username paths (lines 3209, 3201) use .filter(self.user_model.username == ...). Mixing query styles creates maintenance risk if the model attribute name changes or if additional filters need to be added consistently.
Code Review Run #3eb4cf
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
SUMMARY
Makes Apache Superset API keys work end-to-end as an MCP authentication mechanism, fixes a transport-layer bypass that let bearer tokens authenticate as
MCP_DEV_USERNAMEwithout being validated, and locks the FAB ApiKey management endpoints to Admin only.Discovered while testing #39437.
What this PR fixes
1. ApiKey FAB permissions are gated to Admin only
When
FAB_API_KEY_ENABLED=True, FAB registers theApiKeyApiblueprint (class permission name"ApiKey").superset initalready callsappbuilder.add_permissions(update_perms=True)beforesync_role_definitions(), which forces FAB to walk every registered baseview and create its PVMs — so(can_list, ApiKey),(can_create, ApiKey), etc. are created automatically.The problem was that nothing scoped those PVMs to Admin, so Alpha and Gamma users would inherit them and be able to manage every user's API keys. This PR adds
"ApiKey"toADMIN_ONLY_VIEW_MENUS, which causes the existing role predicate_is_admin_onlyto assign the PVMs to Admin only. Per Daniel Gaspar's review: "Adding ApiKey to ADMIN_ONLY_VIEW_MENUS should just work when FAB_API_KEY_ENABLED is True".2. Bearer-token API keys are actually validated under streamable-http (security)
_resolve_user_from_api_keypreviously read the token viaflask.request.headers, but FastMCP's streamable-http transport never pushes a Flask request context —has_request_context()was alwaysFalse, so the function returnedNonebefore validating, falling through toMCP_DEV_USERNAME. Net effect: anyBearer sst_<anything>authenticated as the dev user.The fix reads the token from FastMCP's per-request
AccessToken(whichCompositeTokenVerifieralready populates) and fails closed on invalid keys instead of falling through to weaker auth sources.3.
CompositeTokenVerifieris installed when API keys are the only auth sourcePreviously the composite verifier was only built when
MCP_AUTH_ENABLED=True. WithFAB_API_KEY_ENABLED=Truealone, no transport-level verifier existed and any Bearer token reached the fallback chain. The auth factory now builds an API-key-only verifier in that case (jwt_verifier=None) that rejects non-API-key Bearer tokens at the transport.4. Required scopes propagate through pass-through tokens
The pass-through
AccessTokenwas emitted withscopes=[]. FastMCP'sRequireAuthMiddlewareindependently checksrequired_scopesagainstAccessToken.scopes, so a non-emptyMCP_REQUIRED_SCOPESwould 403 every API-key request before validation ran. Pass-through now copiesself.required_scopesonto the token.5. Pass-through claim is namespaced
Renamed
_api_key_passthrough→_superset_mcp_api_key_passthrough(exported asAPI_KEY_PASSTHROUGH_CLAIM) so a custom claim minted by an external IdP cannot accidentally divert a JWT into the API-key validation path.Files changed
superset/security/manager.py— add"ApiKey"toADMIN_ONLY_VIEW_MENUS(relies on FAB's automatic PVM creation duringsuperset init).superset/mcp_service/composite_token_verifier.py— new module;jwt_verifieris optional (API-key-only mode); pass-through propagates scopes; namespaced claim constant.superset/mcp_service/auth.py—_resolve_user_from_api_keyreads from FastMCPAccessToken; both resolvers useAPI_KEY_PASSTHROUGH_CLAIM; fail closed on invalid API key.superset/mcp_service/mcp_config.py— auth factory builds composite verifier when eitherMCP_AUTH_ENABLEDorFAB_API_KEY_ENABLEDis set; extracted_build_jwt_verifier.superset/mcp_service/server.py— invoke factory when either flag is on.BEFORE / AFTER
N/A — auth infrastructure, no UI changes.
TESTING INSTRUCTIONS
superset_config.py:superset init— verify Admin role getscan_list/can_create/can_get/can_deleteonApiKeyand Alpha/Gamma do not./profile/and create an API key.MCP_AUTH_ENABLED=False:Bearer sst_bogus→ 401/403 (does not silently succeed asMCP_DEV_USERNAME).MCP_AUTH_ENABLED=Trueand JWT keys configured:sst_*key still authenticates.pytest tests/unit_tests/mcp_service/test_auth_api_key.py \ tests/unit_tests/mcp_service/test_composite_token_verifier.py -vADDITIONAL INFORMATION