From fec56c7c1532037d6c5c0a15ac4099d98ce02696 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 23 Apr 2026 19:51:51 -0400 Subject: [PATCH 01/31] fix(mcp): create ApiKey permissions on init and support API keys with 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. --- .../mcp_service/composite_token_verifier.py | 80 +++++++++++++ .../test_composite_token_verifier.py | 105 ++++++++++++++++++ 2 files changed, 185 insertions(+) create mode 100644 superset/mcp_service/composite_token_verifier.py create mode 100644 tests/unit_tests/mcp_service/test_composite_token_verifier.py diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py new file mode 100644 index 000000000000..e4f5c6a5c430 --- /dev/null +++ b/superset/mcp_service/composite_token_verifier.py @@ -0,0 +1,80 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Composite token verifier for MCP authentication. + +Routes Bearer tokens to the appropriate verifier based on prefix: +- Tokens matching FAB_API_KEY_PREFIXES (e.g. ``sst_``) are passed through + to the Flask layer where ``_resolve_user_from_api_key()`` handles + actual validation via FAB SecurityManager. +- All other tokens are delegated to the wrapped JWT verifier. +""" + +import logging + +from fastmcp.server.auth import AccessToken +from fastmcp.server.auth.providers.jwt import TokenVerifier + +logger = logging.getLogger(__name__) + + +class CompositeTokenVerifier(TokenVerifier): + """Routes Bearer tokens between API key pass-through and JWT verification. + + API key tokens (identified by prefix) are accepted at the transport layer + with a marker claim so that ``_resolve_user_from_jwt_context()`` can + detect them and fall through to ``_resolve_user_from_api_key()`` for + actual validation. + + Args: + jwt_verifier: The wrapped JWT verifier for non-API-key tokens. + api_key_prefixes: List of prefixes that identify API key tokens + (e.g. ``["sst_"]``). + """ + + def __init__( + self, + jwt_verifier: TokenVerifier, + api_key_prefixes: list[str], + ) -> None: + super().__init__( + base_url=getattr(jwt_verifier, "base_url", None), + required_scopes=jwt_verifier.required_scopes, + ) + self._jwt_verifier = jwt_verifier + self._api_key_prefixes = tuple(api_key_prefixes) + + async def verify_token(self, token: str) -> AccessToken | None: + """Verify a Bearer token. + + If the token starts with an API key prefix, return a pass-through + AccessToken with a ``_api_key_passthrough`` claim. The Flask-layer + ``_resolve_user_from_api_key()`` performs the real validation. + + Otherwise, delegate to the wrapped JWT verifier. + """ + if any(token.startswith(prefix) for prefix in self._api_key_prefixes): + logger.debug("API key token detected (prefix match), passing through") + return AccessToken( + token=token, + client_id="api_key", + scopes=[], + claims={"_api_key_passthrough": True}, + ) + + return await self._jwt_verifier.verify_token(token) diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py new file mode 100644 index 000000000000..537fe4d8f079 --- /dev/null +++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py @@ -0,0 +1,105 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for CompositeTokenVerifier.""" + +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastmcp.server.auth import AccessToken + +from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier + + +@pytest.fixture +def mock_jwt_verifier(): + verifier = MagicMock() + verifier.required_scopes = [] + verifier.verify_token = AsyncMock() + return verifier + + +@pytest.fixture +def composite_verifier(mock_jwt_verifier): + return CompositeTokenVerifier( + jwt_verifier=mock_jwt_verifier, + api_key_prefixes=["sst_", "pat_"], + ) + + +@pytest.mark.asyncio +async def test_api_key_token_returns_passthrough(composite_verifier) -> None: + """Tokens matching an API key prefix return a pass-through AccessToken.""" + api_key = "sst_abc123secret" # noqa: S105 + result = await composite_verifier.verify_token(api_key) + + assert result is not None + assert result.token == api_key + assert result.client_id == "api_key" + assert result.claims.get("_api_key_passthrough") is True + + +@pytest.mark.asyncio +async def test_second_prefix_matches(composite_verifier) -> None: + """All configured prefixes are checked, not just the first.""" + result = await composite_verifier.verify_token("pat_mytoken") + + assert result is not None + assert result.claims.get("_api_key_passthrough") is True + + +@pytest.mark.asyncio +async def test_jwt_token_delegates_to_wrapped_verifier( + composite_verifier, mock_jwt_verifier +) -> None: + """Non-API-key tokens are delegated to the wrapped JWT verifier.""" + jwt_token = "eyJhbGciOiJSUzI1NiJ9.jwt_payload" # noqa: S105 + jwt_result = AccessToken( + token=jwt_token, + client_id="oauth_client", + scopes=["read"], + claims={"sub": "user1"}, + ) + mock_jwt_verifier.verify_token.return_value = jwt_result + + result = await composite_verifier.verify_token("eyJhbGciOiJSUzI1NiJ9.jwt_payload") + + assert result is jwt_result + mock_jwt_verifier.verify_token.assert_awaited_once_with( + "eyJhbGciOiJSUzI1NiJ9.jwt_payload" + ) + + +@pytest.mark.asyncio +async def test_invalid_jwt_returns_none(composite_verifier, mock_jwt_verifier) -> None: + """When the JWT verifier rejects a token, None is returned.""" + mock_jwt_verifier.verify_token.return_value = None + + result = await composite_verifier.verify_token("not_a_valid_token") + + assert result is None + mock_jwt_verifier.verify_token.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_api_key_does_not_call_jwt_verifier( + composite_verifier, mock_jwt_verifier +) -> None: + """API key tokens bypass the JWT verifier entirely.""" + await composite_verifier.verify_token("sst_test_key") + + mock_jwt_verifier.verify_token.assert_not_awaited() From 3c4e77b7b80115428b9fdbb22bd3f55ac43a8ad2 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 23 Apr 2026 19:52:17 -0400 Subject: [PATCH 02/31] fix(mcp): wire composite verifier and add ApiKey permission sync 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. --- superset/mcp_service/auth.py | 8 ++ superset/mcp_service/mcp_config.py | 15 ++++ superset/security/manager.py | 9 ++ .../mcp_service/test_auth_api_key.py | 82 +++++++++++++++---- 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 69d8e6255317..5320821d88a5 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -285,6 +285,14 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: if access_token is None: return None + # API key pass-through: CompositeTokenVerifier accepted this token + # at the transport layer but defers actual validation to + # _resolve_user_from_api_key() (priority 2 in get_user_from_request). + claims = getattr(access_token, "claims", None) + if isinstance(claims, dict) and claims.get("_api_key_passthrough"): + logger.debug("API key pass-through token detected, deferring to API key auth") + return None + # Use configurable resolver or default from superset.mcp_service.mcp_config import default_user_resolver diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index d12b44bbc87e..d38a5d0eca39 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -348,6 +348,21 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: auth_provider = JWTVerifier(**common_kwargs) + # Wrap with CompositeTokenVerifier when API key auth is enabled + # so that API key tokens (e.g. sst_...) pass through the transport + # layer instead of being rejected by the JWT verifier. + if app.config.get("FAB_API_KEY_ENABLED", False): + from superset.mcp_service.composite_token_verifier import ( + CompositeTokenVerifier, + ) + + api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) + auth_provider = CompositeTokenVerifier( + jwt_verifier=auth_provider, + api_key_prefixes=api_key_prefixes, + ) + logger.info("API key auth enabled for MCP (prefixes: %s)", api_key_prefixes) + return auth_provider except Exception: # Do not log the exception — it may contain the HS256 secret diff --git a/superset/security/manager.py b/superset/security/manager.py index 9a5055d43fe0..44c23f86597b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1422,6 +1422,15 @@ def create_custom_permissions(self) -> None: self.add_permission_view_menu("can_tag", "Chart") self.add_permission_view_menu("can_tag", "Dashboard") + # API Key permissions (FAB's ApiKeyApi blueprint). + # Superset uses AppBuilder(update_perms=False) so FAB skips + # permission creation during blueprint registration. Create them + # explicitly here so that ``superset init`` picks them up and + # sync_role_definitions assigns them to the Admin role. + if current_app.config.get("FAB_API_KEY_ENABLED", False): + for perm in ("can_list", "can_create", "can_get", "can_delete"): + self.add_permission_view_menu(perm, "ApiKey") + def create_missing_perms(self) -> None: """ Creates missing FAB permissions for datasources, schemas and metrics. diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 6a0bcab6719b..7fa9b2934817 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -22,7 +22,10 @@ import pytest from flask import g -from superset.mcp_service.auth import get_user_from_request +from superset.mcp_service.auth import ( + _resolve_user_from_jwt_context, + get_user_from_request, +) @pytest.fixture @@ -222,25 +225,72 @@ def test_relationship_reload_failure_returns_original_user(app, mock_user) -> No assert result is mock_user +# -- Bearer token present but not matching API key prefix -- + + +@pytest.mark.usefixtures("_enable_api_keys") +def test_non_matching_bearer_token_skips_api_key_auth(app) -> None: + """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES + (e.g., a JWT token), extract_api_key_from_request returns None and API key + auth is skipped, falling through to the next auth method.""" + mock_sm = MagicMock() + mock_sm.extract_api_key_from_request.return_value = None + + with app.test_request_context( + headers={"Authorization": "Bearer eyJhbGciOiJIUzI1NiJ9.not-an-api-key"} + ): + g.user = None + app.appbuilder = MagicMock() + app.appbuilder.sm = mock_sm + + with pytest.raises(ValueError, match="No authenticated user found"): + get_user_from_request() + + # extract was called but returned None, so validate should NOT be called + mock_sm.extract_api_key_from_request.assert_called_once() + mock_sm.validate_api_key.assert_not_called() + + +# -- API key pass-through from CompositeTokenVerifier -- + + +def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None: + """When CompositeTokenVerifier passes through an API key token, + _resolve_user_from_jwt_context should detect the _api_key_passthrough + claim and return None so get_user_from_request falls through to + _resolve_user_from_api_key.""" + mock_access_token = MagicMock() + mock_access_token.claims = {"_api_key_passthrough": True} + + with patch( + "fastmcp.server.dependencies.get_access_token", + return_value=mock_access_token, + ): + result = _resolve_user_from_jwt_context(app) + + assert result is None + + # -- SecurityManager method name regression test -- -def test_security_manager_has_expected_api_key_methods() -> None: +def test_security_manager_has_expected_api_key_methods(app) -> None: """Regression test: verify the SecurityManager method names referenced in auth._resolve_user_from_api_key() actually exist on the FAB SecurityManager class. This catches future renames before they silently break API key auth - at runtime (SC-99414: _extract_api_key_from_request vs + at runtime (see PR #39437: _extract_api_key_from_request vs extract_api_key_from_request).""" - from superset import security_manager - - sm = security_manager - assert hasattr(sm, "extract_api_key_from_request"), ( - "FAB SecurityManager is missing 'extract_api_key_from_request'. " - "auth._resolve_user_from_api_key() references this method by name — " - "update auth.py if the FAB API changed." - ) - assert hasattr(sm, "validate_api_key"), ( - "FAB SecurityManager is missing 'validate_api_key'. " - "auth._resolve_user_from_api_key() references this method by name — " - "update auth.py if the FAB API changed." - ) + with app.app_context(): + from superset import security_manager + + sm = security_manager + assert hasattr(sm, "extract_api_key_from_request"), ( + "FAB SecurityManager is missing 'extract_api_key_from_request'. " + "auth._resolve_user_from_api_key() references this method by name — " + "update auth.py if the FAB API changed." + ) + assert hasattr(sm, "validate_api_key"), ( + "FAB SecurityManager is missing 'validate_api_key'. " + "auth._resolve_user_from_api_key() references this method by name — " + "update auth.py if the FAB API changed." + ) From 6c767efe6740dabdc1ae7c13174a96e7284d7fad Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 23 Apr 2026 20:18:44 -0400 Subject: [PATCH 03/31] fix(mcp): add type annotations to test fixtures and parameters Address code review feedback: add explicit type annotations to all new test function parameters and fixture return types. --- .../mcp_service/test_auth_api_key.py | 7 ++++--- .../test_composite_token_verifier.py | 20 ++++++++++++------- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 7fa9b2934817..b53e624e83a1 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -22,6 +22,7 @@ import pytest from flask import g +from superset.app import SupersetApp from superset.mcp_service.auth import ( _resolve_user_from_jwt_context, get_user_from_request, @@ -229,7 +230,7 @@ def test_relationship_reload_failure_returns_original_user(app, mock_user) -> No @pytest.mark.usefixtures("_enable_api_keys") -def test_non_matching_bearer_token_skips_api_key_auth(app) -> None: +def test_non_matching_bearer_token_skips_api_key_auth(app: SupersetApp) -> None: """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES (e.g., a JWT token), extract_api_key_from_request returns None and API key auth is skipped, falling through to the next auth method.""" @@ -254,7 +255,7 @@ def test_non_matching_bearer_token_skips_api_key_auth(app) -> None: # -- API key pass-through from CompositeTokenVerifier -- -def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None: +def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> None: """When CompositeTokenVerifier passes through an API key token, _resolve_user_from_jwt_context should detect the _api_key_passthrough claim and return None so get_user_from_request falls through to @@ -274,7 +275,7 @@ def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None: # -- SecurityManager method name regression test -- -def test_security_manager_has_expected_api_key_methods(app) -> None: +def test_security_manager_has_expected_api_key_methods(app: SupersetApp) -> None: """Regression test: verify the SecurityManager method names referenced in auth._resolve_user_from_api_key() actually exist on the FAB SecurityManager class. This catches future renames before they silently break API key auth diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py index 537fe4d8f079..8f496305a1c5 100644 --- a/tests/unit_tests/mcp_service/test_composite_token_verifier.py +++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py @@ -26,7 +26,7 @@ @pytest.fixture -def mock_jwt_verifier(): +def mock_jwt_verifier() -> MagicMock: verifier = MagicMock() verifier.required_scopes = [] verifier.verify_token = AsyncMock() @@ -34,7 +34,7 @@ def mock_jwt_verifier(): @pytest.fixture -def composite_verifier(mock_jwt_verifier): +def composite_verifier(mock_jwt_verifier: MagicMock) -> CompositeTokenVerifier: return CompositeTokenVerifier( jwt_verifier=mock_jwt_verifier, api_key_prefixes=["sst_", "pat_"], @@ -42,7 +42,9 @@ def composite_verifier(mock_jwt_verifier): @pytest.mark.asyncio -async def test_api_key_token_returns_passthrough(composite_verifier) -> None: +async def test_api_key_token_returns_passthrough( + composite_verifier: CompositeTokenVerifier, +) -> None: """Tokens matching an API key prefix return a pass-through AccessToken.""" api_key = "sst_abc123secret" # noqa: S105 result = await composite_verifier.verify_token(api_key) @@ -54,7 +56,9 @@ async def test_api_key_token_returns_passthrough(composite_verifier) -> None: @pytest.mark.asyncio -async def test_second_prefix_matches(composite_verifier) -> None: +async def test_second_prefix_matches( + composite_verifier: CompositeTokenVerifier, +) -> None: """All configured prefixes are checked, not just the first.""" result = await composite_verifier.verify_token("pat_mytoken") @@ -64,7 +68,7 @@ async def test_second_prefix_matches(composite_verifier) -> None: @pytest.mark.asyncio async def test_jwt_token_delegates_to_wrapped_verifier( - composite_verifier, mock_jwt_verifier + composite_verifier: CompositeTokenVerifier, mock_jwt_verifier: MagicMock ) -> None: """Non-API-key tokens are delegated to the wrapped JWT verifier.""" jwt_token = "eyJhbGciOiJSUzI1NiJ9.jwt_payload" # noqa: S105 @@ -85,7 +89,9 @@ async def test_jwt_token_delegates_to_wrapped_verifier( @pytest.mark.asyncio -async def test_invalid_jwt_returns_none(composite_verifier, mock_jwt_verifier) -> None: +async def test_invalid_jwt_returns_none( + composite_verifier: CompositeTokenVerifier, mock_jwt_verifier: MagicMock +) -> None: """When the JWT verifier rejects a token, None is returned.""" mock_jwt_verifier.verify_token.return_value = None @@ -97,7 +103,7 @@ async def test_invalid_jwt_returns_none(composite_verifier, mock_jwt_verifier) - @pytest.mark.asyncio async def test_api_key_does_not_call_jwt_verifier( - composite_verifier, mock_jwt_verifier + composite_verifier: CompositeTokenVerifier, mock_jwt_verifier: MagicMock ) -> None: """API key tokens bypass the JWT verifier entirely.""" await composite_verifier.verify_token("sst_test_key") From aa069c92373caadfe1ecd7f305e824a80759d9d5 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 23 Apr 2026 20:29:08 -0400 Subject: [PATCH 04/31] fix(mcp): remove prefixes from log to satisfy CodeQL Remove API key prefixes from log message to avoid CodeQL false positive about clear-text logging of sensitive data. --- superset/mcp_service/mcp_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index d38a5d0eca39..a0d9ae89336b 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -361,7 +361,7 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: jwt_verifier=auth_provider, api_key_prefixes=api_key_prefixes, ) - logger.info("API key auth enabled for MCP (prefixes: %s)", api_key_prefixes) + logger.info("API key auth enabled for MCP") return auth_provider except Exception: From c16eaa4b0911e3c6c35b71e6fac67132701c4793 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 8 May 2026 14:26:05 -0400 Subject: [PATCH 05/31] fix(mcp): validate API keys via FastMCP AccessToken and lock down ApiKey perms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- superset/mcp_service/auth.py | 61 +++-- .../mcp_service/composite_token_verifier.py | 36 ++- superset/mcp_service/mcp_config.py | 141 ++++++---- superset/mcp_service/server.py | 8 +- superset/security/manager.py | 4 + .../mcp_service/test_auth_api_key.py | 253 +++++++++++------- .../test_composite_token_verifier.py | 53 +++- 7 files changed, 365 insertions(+), 191 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 5320821d88a5..04b2e802a669 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -288,8 +288,12 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: # API key pass-through: CompositeTokenVerifier accepted this token # at the transport layer but defers actual validation to # _resolve_user_from_api_key() (priority 2 in get_user_from_request). + from superset.mcp_service.composite_token_verifier import ( + API_KEY_PASSTHROUGH_CLAIM, + ) + claims = getattr(access_token, "claims", None) - if isinstance(claims, dict) and claims.get("_api_key_passthrough"): + if isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM): logger.debug("API key pass-through token detected, deferring to API key auth") return None @@ -323,37 +327,53 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: def _resolve_user_from_api_key(app: Any) -> User | None: """ - Resolve the current user from an API key in the Authorization header. + Resolve the current user from an API key passed via Bearer token. - Uses FAB SecurityManager's API key validation. Only attempts when - FAB_API_KEY_ENABLED is True and a request context is active. + Reads the token from FastMCP's per-request ``AccessToken`` (set by + ``CompositeTokenVerifier`` when a Bearer token matches an API key + prefix). The streamable-http transport does not push a Flask request + context, so we cannot rely on ``flask.request`` headers — the verifier + already saw the token and stashed it on the ``AccessToken``. Returns: - User object with relationships loaded, or None if no API key present - or API key auth is not enabled/available. + User object with relationships loaded, or None if no API key + pass-through token is present or API key auth is not enabled. Raises: - PermissionError: If an API key is present but invalid/expired, - or if validation is not available in this FAB version. + PermissionError: If an API key pass-through token is present but + invalid/expired (fail closed — do NOT fall through to weaker + auth sources like ``MCP_DEV_USERNAME``), or if validation is + not available in this FAB version. """ - if not app.config.get("FAB_API_KEY_ENABLED", False) or not has_request_context(): + if not app.config.get("FAB_API_KEY_ENABLED", False): return None - sm = app.appbuilder.sm - # extract_api_key_from_request is FAB's method for reading - # the Bearer token from the Authorization header and matching prefixes. - # Not all FAB versions include this method, so guard with hasattr. - if not hasattr(sm, "extract_api_key_from_request"): - logger.debug( - "FAB SecurityManager does not have extract_api_key_from_request; " - "API key authentication is not available in this FAB version" - ) + try: + from fastmcp.server.dependencies import get_access_token + except ImportError: + logger.debug("fastmcp.server.dependencies not available, skipping API key auth") + return None + + access_token = get_access_token() + if access_token is None: return None - api_key_string = sm.extract_api_key_from_request() - if api_key_string is None: + # Only validate tokens that the CompositeTokenVerifier flagged as + # API key pass-throughs. Plain JWTs were already validated by the JWT + # verifier and resolved in _resolve_user_from_jwt_context. + from superset.mcp_service.composite_token_verifier import ( + API_KEY_PASSTHROUGH_CLAIM, + ) + + claims = getattr(access_token, "claims", None) + if not (isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM)): + return None + + api_key_string = getattr(access_token, "token", None) + if not api_key_string: return None + sm = app.appbuilder.sm if not hasattr(sm, "validate_api_key"): logger.warning( "FAB SecurityManager does not have validate_api_key; " @@ -535,7 +555,6 @@ def _setup_user_context() -> User | None: # tool calls when no per-request middleware refreshes it. # Only clear in app-context-only mode; preserve g.user when # a request context is active (external middleware set it). - from flask import has_request_context if not has_request_context(): g.pop("user", None) diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py index e4f5c6a5c430..e404e364373a 100644 --- a/superset/mcp_service/composite_token_verifier.py +++ b/superset/mcp_service/composite_token_verifier.py @@ -22,7 +22,9 @@ - Tokens matching FAB_API_KEY_PREFIXES (e.g. ``sst_``) are passed through to the Flask layer where ``_resolve_user_from_api_key()`` handles actual validation via FAB SecurityManager. -- All other tokens are delegated to the wrapped JWT verifier. +- All other tokens are delegated to the wrapped JWT verifier (when one is + configured); when no JWT verifier is configured, non-API-key tokens are + rejected at the transport layer. """ import logging @@ -32,6 +34,12 @@ logger = logging.getLogger(__name__) +# Namespaced claim that flags an AccessToken as an API-key pass-through. +# Namespacing avoids collision with custom claims an external IdP might +# happen to mint on a JWT — a plain ``_api_key_passthrough`` claim could +# be silently misidentified as a Superset API-key request. +API_KEY_PASSTHROUGH_CLAIM = "_superset_mcp_api_key_passthrough" + class CompositeTokenVerifier(TokenVerifier): """Routes Bearer tokens between API key pass-through and JWT verification. @@ -43,18 +51,21 @@ class CompositeTokenVerifier(TokenVerifier): Args: jwt_verifier: The wrapped JWT verifier for non-API-key tokens. + When ``None``, only API-key tokens are accepted; all other + Bearer tokens are rejected at the transport layer (used when + ``MCP_AUTH_ENABLED=False`` but ``FAB_API_KEY_ENABLED=True``). api_key_prefixes: List of prefixes that identify API key tokens (e.g. ``["sst_"]``). """ def __init__( self, - jwt_verifier: TokenVerifier, + jwt_verifier: TokenVerifier | None, api_key_prefixes: list[str], ) -> None: super().__init__( base_url=getattr(jwt_verifier, "base_url", None), - required_scopes=jwt_verifier.required_scopes, + required_scopes=getattr(jwt_verifier, "required_scopes", None) or [], ) self._jwt_verifier = jwt_verifier self._api_key_prefixes = tuple(api_key_prefixes) @@ -66,15 +77,28 @@ async def verify_token(self, token: str) -> AccessToken | None: AccessToken with a ``_api_key_passthrough`` claim. The Flask-layer ``_resolve_user_from_api_key()`` performs the real validation. - Otherwise, delegate to the wrapped JWT verifier. + Otherwise, delegate to the wrapped JWT verifier when one is + configured; if no JWT verifier is configured, reject the token. """ if any(token.startswith(prefix) for prefix in self._api_key_prefixes): logger.debug("API key token detected (prefix match), passing through") + # Populate ``scopes`` from ``self.required_scopes`` so FastMCP's + # ``RequireAuthMiddleware`` (transport-layer scope check) is + # satisfied for API-key requests. Without this, MCP_REQUIRED_SCOPES + # being non-empty would 403 every API-key call before + # ``_resolve_user_from_api_key`` even runs. return AccessToken( token=token, client_id="api_key", - scopes=[], - claims={"_api_key_passthrough": True}, + scopes=list(self.required_scopes or []), + claims={API_KEY_PASSTHROUGH_CLAIM: True}, + ) + + if self._jwt_verifier is None: + logger.debug( + "Bearer token does not match any API key prefix and no JWT " + "verifier is configured; rejecting" ) + return None return await self._jwt_verifier.verify_token(token) diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index a0d9ae89336b..837b092ecf29 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -304,71 +304,96 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: - """Default MCP auth factory using app.config values.""" - if not app.config.get("MCP_AUTH_ENABLED", False): - return None + """Default MCP auth factory using app.config values. - jwks_uri = app.config.get("MCP_JWKS_URI") - public_key = app.config.get("MCP_JWT_PUBLIC_KEY") - secret = app.config.get("MCP_JWT_SECRET") + Returns an auth provider when ``MCP_AUTH_ENABLED=True`` (JWT verifier, + optionally wrapped with ``CompositeTokenVerifier`` for API keys) or + when only ``FAB_API_KEY_ENABLED=True`` (API-key-only verifier that + rejects all non-API-key Bearer tokens at the transport). + """ + auth_enabled = app.config.get("MCP_AUTH_ENABLED", False) + api_key_enabled = app.config.get("FAB_API_KEY_ENABLED", False) - if not (jwks_uri or public_key or secret): - logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret configured") + if not (auth_enabled or api_key_enabled): return None - try: - debug_errors = app.config.get("MCP_JWT_DEBUG_ERRORS", False) + jwt_verifier: Any | None = None - common_kwargs: dict[str, Any] = { - "issuer": app.config.get("MCP_JWT_ISSUER"), - "audience": app.config.get("MCP_JWT_AUDIENCE"), - "required_scopes": app.config.get("MCP_REQUIRED_SCOPES", []), - } + if auth_enabled: + jwks_uri = app.config.get("MCP_JWKS_URI") + public_key = app.config.get("MCP_JWT_PUBLIC_KEY") + secret = app.config.get("MCP_JWT_SECRET") - # For HS256 (symmetric), use the secret as the public_key parameter - if app.config.get("MCP_JWT_ALGORITHM") == "HS256" and secret: - common_kwargs["public_key"] = secret - common_kwargs["algorithm"] = "HS256" - else: - # For RS256 (asymmetric), use public key or JWKS - common_kwargs["jwks_uri"] = jwks_uri - common_kwargs["public_key"] = public_key - common_kwargs["algorithm"] = app.config.get("MCP_JWT_ALGORITHM", "RS256") - - if debug_errors: - # DetailedJWTVerifier: detailed server-side logging of JWT - # validation failures. HTTP responses are always generic per - # RFC 6750 Section 3.1. - from superset.mcp_service.jwt_verifier import DetailedJWTVerifier - - auth_provider = DetailedJWTVerifier(**common_kwargs) + if not (jwks_uri or public_key or secret): + logger.warning("MCP_AUTH_ENABLED is True but no JWT keys/secret configured") + if not api_key_enabled: + return None else: - # Default JWTVerifier: minimal logging, generic error responses. - from fastmcp.server.auth.providers.jwt import JWTVerifier - - auth_provider = JWTVerifier(**common_kwargs) - - # Wrap with CompositeTokenVerifier when API key auth is enabled - # so that API key tokens (e.g. sst_...) pass through the transport - # layer instead of being rejected by the JWT verifier. - if app.config.get("FAB_API_KEY_ENABLED", False): - from superset.mcp_service.composite_token_verifier import ( - CompositeTokenVerifier, - ) - - api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) - auth_provider = CompositeTokenVerifier( - jwt_verifier=auth_provider, - api_key_prefixes=api_key_prefixes, - ) - logger.info("API key auth enabled for MCP") - - return auth_provider - except Exception: - # Do not log the exception — it may contain the HS256 secret - # from common_kwargs["public_key"] - logger.error("Failed to create MCP auth provider") - return None + try: + jwt_verifier = _build_jwt_verifier( + app=app, + jwks_uri=jwks_uri, + public_key=public_key, + secret=secret, + ) + except Exception: + # Do not log the exception — it may contain secrets + logger.error("Failed to create MCP JWT verifier") + if not api_key_enabled: + return None + + if api_key_enabled: + from superset.mcp_service.composite_token_verifier import ( + CompositeTokenVerifier, + ) + + api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) + logger.info("API key auth enabled for MCP") + return CompositeTokenVerifier( + jwt_verifier=jwt_verifier, + api_key_prefixes=api_key_prefixes, + ) + + return jwt_verifier + + +def _build_jwt_verifier( + app: Flask, + jwks_uri: Optional[str], + public_key: Optional[str], + secret: Optional[str], +) -> Any: + """Construct the JWT verifier from configured keys/secret.""" + debug_errors = app.config.get("MCP_JWT_DEBUG_ERRORS", False) + + common_kwargs: Dict[str, Any] = { + "issuer": app.config.get("MCP_JWT_ISSUER"), + "audience": app.config.get("MCP_JWT_AUDIENCE"), + "required_scopes": app.config.get("MCP_REQUIRED_SCOPES", []), + } + + # For HS256 (symmetric), use the secret as the public_key parameter + if app.config.get("MCP_JWT_ALGORITHM") == "HS256" and secret: + common_kwargs["public_key"] = secret + common_kwargs["algorithm"] = "HS256" + else: + # For RS256 (asymmetric), use public key or JWKS + common_kwargs["jwks_uri"] = jwks_uri + common_kwargs["public_key"] = public_key + common_kwargs["algorithm"] = app.config.get("MCP_JWT_ALGORITHM", "RS256") + + if debug_errors: + # DetailedJWTVerifier: detailed server-side logging of JWT + # validation failures. HTTP responses are always generic per + # RFC 6750 Section 3.1. + from superset.mcp_service.jwt_verifier import DetailedJWTVerifier + + return DetailedJWTVerifier(**common_kwargs) + + # MCPJWTVerifier: minimal logging + browser-friendly error page. + from superset.mcp_service.jwt_verifier import MCPJWTVerifier + + return MCPJWTVerifier(**common_kwargs) def default_user_resolver(app: Any, access_token: Any) -> str | None: diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py index dc4bffbb5989..ab6fb04a2ca9 100644 --- a/superset/mcp_service/server.py +++ b/superset/mcp_service/server.py @@ -665,7 +665,9 @@ def _create_auth_provider(flask_app: Any) -> Any | None: """Create an auth provider from Flask app config. Tries MCP_AUTH_FACTORY first, then falls back to the default factory - when MCP_AUTH_ENABLED is True. + when either ``MCP_AUTH_ENABLED`` (JWT auth) or ``FAB_API_KEY_ENABLED`` + (API key auth) is True. The default factory builds a + ``CompositeTokenVerifier`` that handles either or both auth modes. """ auth_provider = None if auth_factory := flask_app.config.get("MCP_AUTH_FACTORY"): @@ -678,7 +680,9 @@ def _create_auth_provider(flask_app: Any) -> Any | None: except Exception: # Do not log the exception — it may contain secrets logger.error("Failed to create auth provider from MCP_AUTH_FACTORY") - elif flask_app.config.get("MCP_AUTH_ENABLED", False): + elif flask_app.config.get("MCP_AUTH_ENABLED", False) or flask_app.config.get( + "FAB_API_KEY_ENABLED", False + ): from superset.mcp_service.mcp_config import ( create_default_mcp_auth_factory, ) diff --git a/superset/security/manager.py b/superset/security/manager.py index 44c23f86597b..19a8cf8c5225 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -462,6 +462,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods "PermissionViewMenu", "ViewMenu", "User", + # FAB ApiKeyApi blueprint (active when FAB_API_KEY_ENABLED=True). + # Listed unconditionally — harmless when the feature is off because + # no PVMs exist under this view menu. + "ApiKey", } | USER_MODEL_VIEWS ALPHA_ONLY_VIEW_MENUS = { diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index b53e624e83a1..11717ae0edc2 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -15,8 +15,15 @@ # specific language governing permissions and limitations # under the License. -"""Tests for API key authentication in get_user_from_request().""" +"""Tests for API key authentication in get_user_from_request(). +The streamable-http transport does not push a Flask request context, so +``_resolve_user_from_api_key`` reads the token from FastMCP's per-request +``AccessToken`` (populated by ``CompositeTokenVerifier``) rather than from +``flask.request``. These tests mock ``get_access_token`` accordingly. +""" + +from collections.abc import Generator from unittest.mock import MagicMock, patch import pytest @@ -27,17 +34,34 @@ _resolve_user_from_jwt_context, get_user_from_request, ) +from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM @pytest.fixture -def mock_user(): +def mock_user() -> MagicMock: user = MagicMock() user.username = "api_key_user" return user +def _passthrough_access_token(token: str) -> MagicMock: + """Build an AccessToken matching what CompositeTokenVerifier emits.""" + access_token = MagicMock() + access_token.token = token + access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True} + return access_token + + +def _patch_access_token(access_token: MagicMock | None): + """Patch get_access_token where _resolve_user_from_api_key imports it.""" + return patch( + "fastmcp.server.dependencies.get_access_token", + return_value=access_token, + ) + + @pytest.fixture -def _enable_api_keys(app): +def _enable_api_keys(app: SupersetApp) -> Generator[None, None, None]: """Enable FAB API key auth and clear MCP_DEV_USERNAME so the API key path is exercised instead of falling through to the dev-user fallback.""" app.config["FAB_API_KEY_ENABLED"] = True @@ -49,7 +73,7 @@ def _enable_api_keys(app): @pytest.fixture -def _disable_api_keys(app): +def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]: app.config["FAB_API_KEY_ENABLED"] = False old_dev = app.config.pop("MCP_DEV_USERNAME", None) yield @@ -62,20 +86,22 @@ def _disable_api_keys(app): @pytest.mark.usefixtures("_enable_api_keys") -def test_valid_api_key_returns_user(app, mock_user) -> None: - """A valid API key should authenticate and return the user.""" +def test_valid_api_key_returns_user(app: SupersetApp, mock_user: MagicMock) -> None: + """A valid API key pass-through token should authenticate and return the user.""" mock_sm = MagicMock() - mock_sm.extract_api_key_from_request.return_value = "sst_abc123" mock_sm.validate_api_key.return_value = mock_user - with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}): + with app.app_context(): g.user = None app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - with patch( - "superset.mcp_service.auth.load_user_with_relationships", - return_value=mock_user, + with ( + _patch_access_token(_passthrough_access_token("sst_abc123")), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), ): result = get_user_from_request() @@ -83,54 +109,61 @@ def test_valid_api_key_returns_user(app, mock_user) -> None: mock_sm.validate_api_key.assert_called_once_with("sst_abc123") -# -- Invalid API key -> PermissionError -- +# -- Invalid API key -> PermissionError (does not silently fall back) -- @pytest.mark.usefixtures("_enable_api_keys") -def test_invalid_api_key_raises(app) -> None: - """An invalid API key should raise PermissionError.""" +def test_invalid_api_key_raises(app: SupersetApp) -> None: + """An invalid API key pass-through token should raise PermissionError + (fail closed — do NOT fall through to MCP_DEV_USERNAME).""" mock_sm = MagicMock() - mock_sm.extract_api_key_from_request.return_value = "sst_bad_key" mock_sm.validate_api_key.return_value = None - with app.test_request_context(headers={"Authorization": "Bearer sst_bad_key"}): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm + # The dangerous fallthrough scenario: dev username IS set, but the + # request presented an invalid API key. The dev fallback must not + # mask the rejection. + app.config["MCP_DEV_USERNAME"] = "admin" + try: + with app.app_context(): + g.user = None + app.appbuilder = MagicMock() + app.appbuilder.sm = mock_sm - with pytest.raises(PermissionError, match="Invalid or expired API key"): - get_user_from_request() + with _patch_access_token(_passthrough_access_token("sst_bad_key")): + with pytest.raises(PermissionError, match="Invalid or expired API key"): + get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) # -- API key disabled -> falls through to next auth method -- @pytest.mark.usefixtures("_disable_api_keys") -def test_api_key_disabled_skips_auth(app) -> None: - """When FAB_API_KEY_ENABLED is False, API key auth is skipped entirely.""" +def test_api_key_disabled_skips_auth(app: SupersetApp) -> None: + """When FAB_API_KEY_ENABLED is False, API key auth is skipped entirely + even if an AccessToken is present.""" mock_sm = MagicMock() - with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}): + with app.app_context(): g.user = None app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - # Without API key auth or MCP_DEV_USERNAME, should raise ValueError - # about no authenticated user (not about invalid API key) - with pytest.raises(ValueError, match="No authenticated user found"): - get_user_from_request() + with _patch_access_token(_passthrough_access_token("sst_abc123")): + with pytest.raises(ValueError, match="No authenticated user found"): + get_user_from_request() - # SecurityManager API key methods should never be called - mock_sm.extract_api_key_from_request.assert_not_called() + mock_sm.validate_api_key.assert_not_called() -# -- No request context -> API key auth skipped -- +# -- No AccessToken -> API key auth skipped -- @pytest.mark.usefixtures("_enable_api_keys") -def test_no_request_context_skips_api_key_auth(app) -> None: - """Without a request context, API key auth should be skipped - (e.g., during MCP tool discovery with only an app context).""" +def test_no_access_token_skips_api_key_auth(app: SupersetApp) -> None: + """Without a FastMCP AccessToken (e.g., MCP_AUTH_ENABLED=False and no + auth provider installed), API key auth is skipped.""" mock_sm = MagicMock() with app.app_context(): @@ -138,20 +171,20 @@ def test_no_request_context_skips_api_key_auth(app) -> None: app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - # Explicitly mock has_request_context to False because the test - # framework's app fixture may implicitly provide a request context. - with patch("superset.mcp_service.auth.has_request_context", return_value=False): + with _patch_access_token(None): with pytest.raises(ValueError, match="No authenticated user found"): get_user_from_request() - mock_sm.extract_api_key_from_request.assert_not_called() + mock_sm.validate_api_key.assert_not_called() # -- g.user fallback when no higher-priority auth succeeds -- @pytest.mark.usefixtures("_disable_api_keys") -def test_g_user_fallback_when_no_jwt_or_api_key(app, mock_user) -> None: +def test_g_user_fallback_when_no_jwt_or_api_key( + app: SupersetApp, mock_user: MagicMock +) -> None: """When no JWT or API key auth succeeds and MCP_DEV_USERNAME is not set, g.user (set by external middleware) is used as fallback.""" with app.test_request_context(): @@ -162,106 +195,97 @@ def test_g_user_fallback_when_no_jwt_or_api_key(app, mock_user) -> None: assert result.username == "api_key_user" -# -- FAB version without extract_api_key_from_request -- - - -@pytest.mark.usefixtures("_enable_api_keys") -def test_fab_without_extract_method_skips_gracefully(app) -> None: - """If FAB SecurityManager lacks extract_api_key_from_request, - API key auth should be skipped with a debug log, not crash.""" - mock_sm = MagicMock(spec=[]) # empty spec = no attributes - - with app.test_request_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - - with pytest.raises(ValueError, match="No authenticated user found"): - get_user_from_request() - - # -- FAB version without validate_api_key -- @pytest.mark.usefixtures("_enable_api_keys") -def test_fab_without_validate_method_raises(app) -> None: - """If FAB has extract_api_key_from_request but not validate_api_key, - should raise PermissionError about unavailable validation.""" - mock_sm = MagicMock(spec=["extract_api_key_from_request"]) - mock_sm.extract_api_key_from_request.return_value = "sst_abc123" +def test_fab_without_validate_method_raises(app: SupersetApp) -> None: + """If FAB SecurityManager lacks validate_api_key, should raise + PermissionError about unavailable validation.""" + mock_sm = MagicMock(spec=[]) # empty spec = no attributes - with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}): + with app.app_context(): g.user = None app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - with pytest.raises( - PermissionError, match="API key validation is not available" - ): - get_user_from_request() + with _patch_access_token(_passthrough_access_token("sst_abc123")): + with pytest.raises( + PermissionError, match="API key validation is not available" + ): + get_user_from_request() # -- Relationship reload fallback -- @pytest.mark.usefixtures("_enable_api_keys") -def test_relationship_reload_failure_returns_original_user(app, mock_user) -> None: +def test_relationship_reload_failure_returns_original_user( + app: SupersetApp, mock_user: MagicMock +) -> None: """If load_user_with_relationships fails, the original user from validate_api_key should be returned as fallback.""" mock_sm = MagicMock() - mock_sm.extract_api_key_from_request.return_value = "sst_abc123" mock_sm.validate_api_key.return_value = mock_user - with app.test_request_context(headers={"Authorization": "Bearer sst_abc123"}): + with app.app_context(): g.user = None app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - with patch( - "superset.mcp_service.auth.load_user_with_relationships", - return_value=None, + with ( + _patch_access_token(_passthrough_access_token("sst_abc123")), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), ): result = get_user_from_request() assert result is mock_user -# -- Bearer token present but not matching API key prefix -- +# -- AccessToken without passthrough claim (plain JWT) -> skip API key auth -- @pytest.mark.usefixtures("_enable_api_keys") -def test_non_matching_bearer_token_skips_api_key_auth(app: SupersetApp) -> None: - """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES - (e.g., a JWT token), extract_api_key_from_request returns None and API key - auth is skipped, falling through to the next auth method.""" +def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None: + """When the AccessToken is a plain JWT (no ``_api_key_passthrough`` claim), + API key auth is skipped — the JWT was already validated by the JWT + verifier and resolved in _resolve_user_from_jwt_context.""" mock_sm = MagicMock() - mock_sm.extract_api_key_from_request.return_value = None - with app.test_request_context( - headers={"Authorization": "Bearer eyJhbGciOiJIUzI1NiJ9.not-an-api-key"} - ): + jwt_access_token = MagicMock() + jwt_access_token.token = "eyJhbGciOiJIUzI1NiJ9.not-an-api-key" # noqa: S105 + jwt_access_token.claims = {"sub": "alice"} + + with app.app_context(): g.user = None app.appbuilder = MagicMock() app.appbuilder.sm = mock_sm - with pytest.raises(ValueError, match="No authenticated user found"): - get_user_from_request() + with _patch_access_token(jwt_access_token): + # _resolve_user_from_jwt_context will try to resolve the user + # from the JWT claims and (in this isolated unit-test setup) + # raise ValueError because the username is not a real user. + # We assert that _resolve_user_from_api_key did NOT short-circuit + # to the API key path. + with pytest.raises(ValueError, match="not found"): + get_user_from_request() - # extract was called but returned None, so validate should NOT be called - mock_sm.extract_api_key_from_request.assert_called_once() mock_sm.validate_api_key.assert_not_called() -# -- API key pass-through from CompositeTokenVerifier -- +# -- API key pass-through detection in JWT context resolver -- def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> None: """When CompositeTokenVerifier passes through an API key token, - _resolve_user_from_jwt_context should detect the _api_key_passthrough - claim and return None so get_user_from_request falls through to - _resolve_user_from_api_key.""" + _resolve_user_from_jwt_context should detect the namespaced + pass-through claim and return None so get_user_from_request falls + through to _resolve_user_from_api_key.""" mock_access_token = MagicMock() - mock_access_token.claims = {"_api_key_passthrough": True} + mock_access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True} with patch( "fastmcp.server.dependencies.get_access_token", @@ -272,24 +296,51 @@ def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> assert result is None +# -- Plain JWT with a colliding non-namespaced claim is NOT mistaken for API key -- + + +@pytest.mark.usefixtures("_enable_api_keys") +def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path( + app: SupersetApp, +) -> None: + """A JWT minted by an external IdP that happens to include a custom + ``_api_key_passthrough`` claim (legacy unnamespaced name) must NOT be + treated as an API-key pass-through. Only the namespaced + ``API_KEY_PASSTHROUGH_CLAIM`` triggers the API-key path.""" + mock_sm = MagicMock() + + rogue_token = MagicMock() + rogue_token.token = "eyJhbGciOiJSUzI1NiJ9.rogue_jwt" # noqa: S105 + rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"} + + with app.app_context(): + g.user = None + app.appbuilder = MagicMock() + app.appbuilder.sm = mock_sm + + with _patch_access_token(rogue_token): + # JWT path tries to resolve user "alice" from DB and (in this + # isolated unit-test setup) raises ValueError. The assertion + # below confirms validate_api_key was never called — i.e., the + # rogue claim did NOT divert into _resolve_user_from_api_key. + with pytest.raises(ValueError, match="not found"): + get_user_from_request() + + mock_sm.validate_api_key.assert_not_called() + + # -- SecurityManager method name regression test -- def test_security_manager_has_expected_api_key_methods(app: SupersetApp) -> None: - """Regression test: verify the SecurityManager method names referenced in - auth._resolve_user_from_api_key() actually exist on the FAB SecurityManager - class. This catches future renames before they silently break API key auth - at runtime (see PR #39437: _extract_api_key_from_request vs - extract_api_key_from_request).""" + """Regression test: verify the SecurityManager method name referenced in + auth._resolve_user_from_api_key() actually exists on the FAB + SecurityManager class. Catches future renames before they silently break + API key auth at runtime (see PR #39437).""" with app.app_context(): from superset import security_manager sm = security_manager - assert hasattr(sm, "extract_api_key_from_request"), ( - "FAB SecurityManager is missing 'extract_api_key_from_request'. " - "auth._resolve_user_from_api_key() references this method by name — " - "update auth.py if the FAB API changed." - ) assert hasattr(sm, "validate_api_key"), ( "FAB SecurityManager is missing 'validate_api_key'. " "auth._resolve_user_from_api_key() references this method by name — " diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py index 8f496305a1c5..d493c07ea03c 100644 --- a/tests/unit_tests/mcp_service/test_composite_token_verifier.py +++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py @@ -22,7 +22,10 @@ import pytest from fastmcp.server.auth import AccessToken -from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier +from superset.mcp_service.composite_token_verifier import ( + API_KEY_PASSTHROUGH_CLAIM, + CompositeTokenVerifier, +) @pytest.fixture @@ -52,7 +55,7 @@ async def test_api_key_token_returns_passthrough( assert result is not None assert result.token == api_key assert result.client_id == "api_key" - assert result.claims.get("_api_key_passthrough") is True + assert result.claims.get(API_KEY_PASSTHROUGH_CLAIM) is True @pytest.mark.asyncio @@ -63,7 +66,7 @@ async def test_second_prefix_matches( result = await composite_verifier.verify_token("pat_mytoken") assert result is not None - assert result.claims.get("_api_key_passthrough") is True + assert result.claims.get(API_KEY_PASSTHROUGH_CLAIM) is True @pytest.mark.asyncio @@ -109,3 +112,47 @@ async def test_api_key_does_not_call_jwt_verifier( await composite_verifier.verify_token("sst_test_key") mock_jwt_verifier.verify_token.assert_not_awaited() + + +# -- API-key-only mode (no JWT verifier configured) -- + + +@pytest.mark.asyncio +async def test_api_key_only_mode_accepts_api_keys() -> None: + """When jwt_verifier is None, API key tokens are still passed through.""" + verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["sst_"]) + + result = await verifier.verify_token("sst_abc123") + + assert result is not None + assert result.claims.get(API_KEY_PASSTHROUGH_CLAIM) is True + + +@pytest.mark.asyncio +async def test_api_key_only_mode_rejects_non_api_key_tokens() -> None: + """When jwt_verifier is None, non-API-key Bearer tokens are rejected at + the transport instead of being silently accepted.""" + verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["sst_"]) + + result = await verifier.verify_token("eyJhbGciOiJSUzI1NiJ9.jwt_payload") + + assert result is None + + +@pytest.mark.asyncio +async def test_api_key_passthrough_propagates_required_scopes() -> None: + """The pass-through AccessToken must carry the verifier's required_scopes + so FastMCP's transport-level ``RequireAuthMiddleware`` does not 403 the + request before ``_resolve_user_from_api_key`` runs.""" + jwt_verifier = MagicMock() + jwt_verifier.required_scopes = ["read", "write"] + jwt_verifier.verify_token = AsyncMock() + + verifier = CompositeTokenVerifier( + jwt_verifier=jwt_verifier, api_key_prefixes=["sst_"] + ) + + result = await verifier.verify_token("sst_abc123") + + assert result is not None + assert result.scopes == ["read", "write"] From bd73f9e0ccbed1bcca609f3cf2704438da4c6ede Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 8 May 2026 14:36:16 -0400 Subject: [PATCH 06/31] refactor(mcp): hoist API key auth imports to module top 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/mcp_service/auth.py | 10 ++-------- superset/mcp_service/mcp_config.py | 5 +---- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 04b2e802a669..94b223aec08f 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -51,6 +51,8 @@ from flask import current_app, g, has_app_context, has_request_context from flask_appbuilder.security.sqla.models import Group, User +from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM + if TYPE_CHECKING: from superset.connectors.sqla.models import SqlaTable from superset.mcp_service.chart.chart_utils import DatasetValidationResult @@ -288,10 +290,6 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: # API key pass-through: CompositeTokenVerifier accepted this token # at the transport layer but defers actual validation to # _resolve_user_from_api_key() (priority 2 in get_user_from_request). - from superset.mcp_service.composite_token_verifier import ( - API_KEY_PASSTHROUGH_CLAIM, - ) - claims = getattr(access_token, "claims", None) if isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM): logger.debug("API key pass-through token detected, deferring to API key auth") @@ -361,10 +359,6 @@ def _resolve_user_from_api_key(app: Any) -> User | None: # Only validate tokens that the CompositeTokenVerifier flagged as # API key pass-throughs. Plain JWTs were already validated by the JWT # verifier and resolved in _resolve_user_from_jwt_context. - from superset.mcp_service.composite_token_verifier import ( - API_KEY_PASSTHROUGH_CLAIM, - ) - claims = getattr(access_token, "claims", None) if not (isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM)): return None diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 837b092ecf29..0694fc909753 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -22,6 +22,7 @@ from flask import Flask +from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier from superset.mcp_service.constants import ( DEFAULT_TOKEN_LIMIT, DEFAULT_WARN_THRESHOLD_PCT, @@ -343,10 +344,6 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: return None if api_key_enabled: - from superset.mcp_service.composite_token_verifier import ( - CompositeTokenVerifier, - ) - api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) logger.info("API key auth enabled for MCP") return CompositeTokenVerifier( From 9c011d5c77343d1146af6aaf095f234fe618202b Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 8 May 2026 14:47:58 -0400 Subject: [PATCH 07/31] fix(security): drop redundant explicit ApiKey perm creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ``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". --- superset/security/manager.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 19a8cf8c5225..5da6e9ced971 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1426,15 +1426,6 @@ def create_custom_permissions(self) -> None: self.add_permission_view_menu("can_tag", "Chart") self.add_permission_view_menu("can_tag", "Dashboard") - # API Key permissions (FAB's ApiKeyApi blueprint). - # Superset uses AppBuilder(update_perms=False) so FAB skips - # permission creation during blueprint registration. Create them - # explicitly here so that ``superset init`` picks them up and - # sync_role_definitions assigns them to the Admin role. - if current_app.config.get("FAB_API_KEY_ENABLED", False): - for perm in ("can_list", "can_create", "can_get", "can_delete"): - self.add_permission_view_menu(perm, "ApiKey") - def create_missing_perms(self) -> None: """ Creates missing FAB permissions for datasources, schemas and metrics. From fc426dd4f1ade470d4f44b46dd7b27568858f809 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 8 May 2026 14:57:06 -0400 Subject: [PATCH 08/31] refactor(mcp): hoist JWT verifier imports to module top MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- superset/mcp_service/mcp_config.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 0694fc909753..307045b3a8c8 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -20,6 +20,7 @@ import secrets from typing import Any, Dict, Optional +from fastmcp.server.auth.providers.jwt import JWTVerifier from flask import Flask from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier @@ -27,6 +28,7 @@ DEFAULT_TOKEN_LIMIT, DEFAULT_WARN_THRESHOLD_PCT, ) +from superset.mcp_service.jwt_verifier import DetailedJWTVerifier, MCPJWTVerifier logger = logging.getLogger(__name__) @@ -383,13 +385,9 @@ def _build_jwt_verifier( # DetailedJWTVerifier: detailed server-side logging of JWT # validation failures. HTTP responses are always generic per # RFC 6750 Section 3.1. - from superset.mcp_service.jwt_verifier import DetailedJWTVerifier - return DetailedJWTVerifier(**common_kwargs) # MCPJWTVerifier: minimal logging + browser-friendly error page. - from superset.mcp_service.jwt_verifier import MCPJWTVerifier - return MCPJWTVerifier(**common_kwargs) From 37b9468f7fa702b477b6780b3529d2db706c7435 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 8 May 2026 16:04:09 -0400 Subject: [PATCH 09/31] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- superset/mcp_service/composite_token_verifier.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py index e404e364373a..ffba111bf481 100644 --- a/superset/mcp_service/composite_token_verifier.py +++ b/superset/mcp_service/composite_token_verifier.py @@ -74,7 +74,8 @@ async def verify_token(self, token: str) -> AccessToken | None: """Verify a Bearer token. If the token starts with an API key prefix, return a pass-through - AccessToken with a ``_api_key_passthrough`` claim. The Flask-layer + AccessToken with the namespaced ``API_KEY_PASSTHROUGH_CLAIM`` + (``_superset_mcp_api_key_passthrough``). The Flask-layer ``_resolve_user_from_api_key()`` performs the real validation. Otherwise, delegate to the wrapped JWT verifier when one is From 07129f4b926552dc84144d048c4f68e3ebc19f27 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 13 May 2026 06:34:46 +0000 Subject: [PATCH 10/31] fix(mcp): fix stale patch target in auth tests and update stale docstring - 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) --- superset/mcp_service/mcp_config.py | 4 ++-- tests/unit_tests/mcp_service/test_auth_api_key.py | 2 +- tests/unit_tests/mcp_service/test_auth_user_resolution.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 307045b3a8c8..dc16b248c54b 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -339,8 +339,8 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: public_key=public_key, secret=secret, ) - except Exception: - # Do not log the exception — it may contain secrets + except Exception: # noqa: BLE001 — JWT lib raises many types; broad catch intentional + # Do not log the exception — it may contain secrets (e.g., key material) logger.error("Failed to create MCP JWT verifier") if not api_key_enabled: return None diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 11717ae0edc2..546f789d4986 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -250,7 +250,7 @@ def test_relationship_reload_failure_returns_original_user( @pytest.mark.usefixtures("_enable_api_keys") def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None: - """When the AccessToken is a plain JWT (no ``_api_key_passthrough`` claim), + """When the AccessToken is a plain JWT (no API_KEY_PASSTHROUGH_CLAIM), API key auth is skipped — the JWT was already validated by the JWT verifier and resolved in _resolve_user_from_jwt_context.""" mock_sm = MagicMock() diff --git a/tests/unit_tests/mcp_service/test_auth_user_resolution.py b/tests/unit_tests/mcp_service/test_auth_user_resolution.py index 34669e51d1ed..9142cee8253a 100644 --- a/tests/unit_tests/mcp_service/test_auth_user_resolution.py +++ b/tests/unit_tests/mcp_service/test_auth_user_resolution.py @@ -285,7 +285,7 @@ def _assert_cleared_then_return(): # framework's autouse app_context fixture may implicitly provide # a request context in some CI environments. with ( - patch("flask.has_request_context", return_value=False), + patch("superset.mcp_service.auth.has_request_context", return_value=False), patch( "superset.mcp_service.auth.get_user_from_request", side_effect=lambda: _assert_cleared_then_return(), @@ -324,7 +324,7 @@ def _assert_cleared_then_return(): with app.app_context(): g.user = stale_user with ( - patch("flask.has_request_context", return_value=False), + patch("superset.mcp_service.auth.has_request_context", return_value=False), patch( "superset.mcp_service.auth.get_user_from_request", side_effect=lambda: _assert_cleared_then_return(), From c1afc07b6740ab3c2413dbe8f300d9ef47759593 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 13 May 2026 19:57:41 +0000 Subject: [PATCH 11/31] refactor(mcp): extract duplicated app context + sm setup into helper 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. --- .../mcp_service/test_auth_api_key.py | 59 ++++++------------- 1 file changed, 19 insertions(+), 40 deletions(-) diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 546f789d4986..ab74cb2236b6 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -24,6 +24,7 @@ """ from collections.abc import Generator +from contextlib import contextmanager from unittest.mock import MagicMock, patch import pytest @@ -82,6 +83,16 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]: app.config["MCP_DEV_USERNAME"] = old_dev +@contextmanager +def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock): + """Push an app context with g.user cleared and appbuilder.sm mocked.""" + with app.app_context(): + g.user = None + app.appbuilder = MagicMock() + app.appbuilder.sm = mock_sm + yield + + # -- Valid API key -> user loaded -- @@ -91,11 +102,7 @@ def test_valid_api_key_returns_user(app: SupersetApp, mock_user: MagicMock) -> N mock_sm = MagicMock() mock_sm.validate_api_key.return_value = mock_user - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with ( _patch_access_token(_passthrough_access_token("sst_abc123")), patch( @@ -124,11 +131,7 @@ def test_invalid_api_key_raises(app: SupersetApp) -> None: # mask the rejection. app.config["MCP_DEV_USERNAME"] = "admin" try: - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with _patch_access_token(_passthrough_access_token("sst_bad_key")): with pytest.raises(PermissionError, match="Invalid or expired API key"): get_user_from_request() @@ -145,11 +148,7 @@ def test_api_key_disabled_skips_auth(app: SupersetApp) -> None: even if an AccessToken is present.""" mock_sm = MagicMock() - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with _patch_access_token(_passthrough_access_token("sst_abc123")): with pytest.raises(ValueError, match="No authenticated user found"): get_user_from_request() @@ -166,11 +165,7 @@ def test_no_access_token_skips_api_key_auth(app: SupersetApp) -> None: auth provider installed), API key auth is skipped.""" mock_sm = MagicMock() - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with _patch_access_token(None): with pytest.raises(ValueError, match="No authenticated user found"): get_user_from_request() @@ -204,11 +199,7 @@ def test_fab_without_validate_method_raises(app: SupersetApp) -> None: PermissionError about unavailable validation.""" mock_sm = MagicMock(spec=[]) # empty spec = no attributes - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with _patch_access_token(_passthrough_access_token("sst_abc123")): with pytest.raises( PermissionError, match="API key validation is not available" @@ -228,11 +219,7 @@ def test_relationship_reload_failure_returns_original_user( mock_sm = MagicMock() mock_sm.validate_api_key.return_value = mock_user - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with ( _patch_access_token(_passthrough_access_token("sst_abc123")), patch( @@ -259,11 +246,7 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None: jwt_access_token.token = "eyJhbGciOiJIUzI1NiJ9.not-an-api-key" # noqa: S105 jwt_access_token.claims = {"sub": "alice"} - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with _patch_access_token(jwt_access_token): # _resolve_user_from_jwt_context will try to resolve the user # from the JWT claims and (in this isolated unit-test setup) @@ -313,11 +296,7 @@ def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path( rogue_token.token = "eyJhbGciOiJSUzI1NiJ9.rogue_jwt" # noqa: S105 rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"} - with app.app_context(): - g.user = None - app.appbuilder = MagicMock() - app.appbuilder.sm = mock_sm - + with _mock_sm_ctx(app, mock_sm): with _patch_access_token(rogue_token): # JWT path tries to resolve user "alice" from DB and (in this # isolated unit-test setup) raises ValueError. The assertion From 690b71416fb50142751a8db3478798528c79d4cb Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 13 May 2026 21:26:42 +0000 Subject: [PATCH 12/31] =?UTF-8?q?fix(mcp):=20harden=20auth=20=E2=80=94=20P?= =?UTF-8?q?ermissionError=20propagation,=20passthrough=20client=5Fid=20gua?= =?UTF-8?q?rd,=20fail-closed=20on=20missing=20token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _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 --- superset/mcp_service/auth.py | 27 ++++++++++++++--- .../mcp_service/test_auth_api_key.py | 30 +++++++++++++++++-- 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 94b223aec08f..620763e52857 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -290,10 +290,21 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: # API key pass-through: CompositeTokenVerifier accepted this token # at the transport layer but defers actual validation to # _resolve_user_from_api_key() (priority 2 in get_user_from_request). + # Require client_id=="api_key" (set by CompositeTokenVerifier) in addition + # to the claim so that an external IdP JWT that happens to include the + # claim name is not misclassified as an API-key pass-through. claims = getattr(access_token, "claims", None) if isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM): - logger.debug("API key pass-through token detected, deferring to API key auth") - return None + if getattr(access_token, "client_id", None) == "api_key": + logger.debug( + "API key pass-through token detected, deferring to API key auth" + ) + return None + logger.debug( + "Ignoring %s claim on non-API-key token (client_id=%r); processing as JWT", + API_KEY_PASSTHROUGH_CLAIM, + getattr(access_token, "client_id", None), + ) # Use configurable resolver or default from superset.mcp_service.mcp_config import default_user_resolver @@ -362,10 +373,18 @@ def _resolve_user_from_api_key(app: Any) -> User | None: claims = getattr(access_token, "claims", None) if not (isinstance(claims, dict) and claims.get(API_KEY_PASSTHROUGH_CLAIM)): return None + # Defense-in-depth: require client_id=="api_key" (set by CompositeTokenVerifier) + # to guard against rogue external IdP JWTs that include the passthrough claim. + if getattr(access_token, "client_id", None) != "api_key": + return None api_key_string = getattr(access_token, "token", None) if not api_key_string: - return None + # Passthrough claim is set but the raw token is absent — fail closed + # rather than silently falling through to weaker auth sources. + raise PermissionError( + "API key pass-through token is missing the raw token value." + ) sm = app.appbuilder.sm if not hasattr(sm, "validate_api_key"): @@ -592,7 +611,7 @@ def _setup_user_context() -> User | None: logger.error("DB connection failed on retry during user setup: %s", e) _cleanup_session_on_error() raise - except ValueError as e: + except (ValueError, PermissionError) as e: # User resolution failed — fail closed. Do not fall back to # g.user from middleware, as that could allow a request to # proceed as a different user in multi-tenant deployments. diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index ab74cb2236b6..21d060441ff8 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -49,6 +49,7 @@ def _passthrough_access_token(token: str) -> MagicMock: """Build an AccessToken matching what CompositeTokenVerifier emits.""" access_token = MagicMock() access_token.token = token + access_token.client_id = "api_key" access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True} return access_token @@ -265,9 +266,10 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None: def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> None: """When CompositeTokenVerifier passes through an API key token, _resolve_user_from_jwt_context should detect the namespaced - pass-through claim and return None so get_user_from_request falls - through to _resolve_user_from_api_key.""" + pass-through claim AND client_id=="api_key" and return None so + get_user_from_request falls through to _resolve_user_from_api_key.""" mock_access_token = MagicMock() + mock_access_token.client_id = "api_key" mock_access_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True} with patch( @@ -279,6 +281,30 @@ def test_jwt_context_with_api_key_passthrough_returns_none(app: SupersetApp) -> assert result is None +def test_namespaced_claim_without_api_key_client_id_is_ignored( + app: SupersetApp, +) -> None: + """An external IdP JWT that includes the namespaced API_KEY_PASSTHROUGH_CLAIM + but does NOT have client_id=='api_key' must NOT divert into the API-key path. + The client_id guard prevents misclassification / DoS for affected JWT users.""" + mock_sm = MagicMock() + + rogue_token = MagicMock() + rogue_token.token = "eyJhbGciOiJSUzI1NiJ9.idp_jwt_with_rogue_claim" # noqa: S105 + rogue_token.client_id = "some-idp-client" + rogue_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True, "sub": "alice"} + + with _mock_sm_ctx(app, mock_sm): + with _patch_access_token(rogue_token): + # JWT path tries to resolve user "alice" from DB and raises + # ValueError in this isolated unit-test setup. + # validate_api_key must NOT be called — the rogue claim was ignored. + with pytest.raises(ValueError, match="not found"): + get_user_from_request() + + mock_sm.validate_api_key.assert_not_called() + + # -- Plain JWT with a colliding non-namespaced claim is NOT mistaken for API key -- From 3c80c2ffbc1c4c341b7250e687f78094882b9d4b Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 13 May 2026 22:10:36 +0000 Subject: [PATCH 13/31] refactor(mcp): delegate load_user_with_relationships to SecurityManager.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 --- superset/mcp_service/auth.py | 41 ++++--------- superset/security/manager.py | 57 ++++++++++++++++++- .../mcp_service/test_auth_api_key.py | 14 +++++ 3 files changed, 80 insertions(+), 32 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 620763e52857..22e82e038fe5 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -49,7 +49,7 @@ from typing import Any, Callable, TYPE_CHECKING, TypeVar from flask import current_app, g, has_app_context, has_request_context -from flask_appbuilder.security.sqla.models import Group, User +from flask_appbuilder.security.sqla.models import User from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM @@ -217,23 +217,14 @@ def is_tool_visible_to_current_user(tool: Any) -> bool: def load_user_with_relationships( username: str | None = None, email: str | None = None ) -> User | None: - """ - Load a user with all relationships needed for permission checks. - - This function eagerly loads User.roles, User.groups, and Group.roles - to prevent detached instance errors when the session is closed/rolled back. - - IMPORTANT: Always use this function instead of security_manager.find_user() - when loading users for MCP tool execution. The find_user() method doesn't - eagerly load Group.roles, causing "detached instance" errors when permission - checks access group.roles after the session is rolled back. + """Load a user with roles and group roles eagerly loaded. - Args: - username: The username to look up (optional if email provided) - email: The email to look up (optional if username provided) - - Returns: - User object with relationships loaded, or None if not found + Delegates to :meth:`SupersetSecurityManager.find_user_with_relationships`, + which mirrors FAB's ``find_user`` (including ``auth_username_ci`` and + ``MultipleResultsFound`` handling) while adding eager loading of + ``User.roles`` and ``User.groups.roles`` to prevent detached-instance + errors when the SQLAlchemy session is closed or rolled back after the + lookup — as happens in MCP tool-execution contexts. Raises: ValueError: If neither username nor email is provided @@ -241,21 +232,9 @@ def load_user_with_relationships( if not username and not email: raise ValueError("Either username or email must be provided") - from sqlalchemy.orm import joinedload - - from superset.extensions import db - - query = db.session.query(User).options( - joinedload(User.roles), - joinedload(User.groups).joinedload(Group.roles), - ) - - if username: - query = query.filter(User.username == username) - else: - query = query.filter(User.email == email) + from superset import security_manager - return query.first() + return security_manager.find_user_with_relationships(username=username, email=email) def _resolve_user_from_jwt_context(app: Any) -> User | None: diff --git a/superset/security/manager.py b/superset/security/manager.py index 5da6e9ced971..982c2c46e8a5 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -52,7 +52,8 @@ from jwt.api_jwt import _jwt_global_obj from sqlalchemy import and_, inspect, or_ from sqlalchemy.engine.base import Connection -from sqlalchemy.orm import eagerload +from sqlalchemy.orm import eagerload, joinedload +from sqlalchemy.orm.exc import MultipleResultsFound from sqlalchemy.orm.mapper import Mapper from sqlalchemy.orm.query import Query as SqlaQuery from sqlalchemy.sql import exists @@ -3168,6 +3169,60 @@ def get_user_by_username(self, username: str) -> Optional[User]: .one_or_none() ) + def find_user_with_relationships( + self, + username: Optional[str] = None, + email: Optional[str] = None, + ) -> Optional[User]: + """Find a user with roles and group roles eagerly loaded. + + Mirrors FAB's ``SecurityManager.find_user`` + (including ``auth_username_ci`` case-insensitive handling and + ``MultipleResultsFound`` guard) and additionally eager-loads + ``User.roles`` and ``User.groups.roles`` to prevent detached-instance + errors when the SQLAlchemy session is closed or rolled back after the + lookup — as happens in MCP tool-execution contexts. + """ + eager = [ + joinedload(self.user_model.roles), + joinedload(self.user_model.groups).joinedload("roles"), + ] + if username: + try: + if self.auth_username_ci: + from sqlalchemy import func as sa_func + + return ( + self.session.query(self.user_model) + .options(*eager) + .filter( + sa_func.lower(self.user_model.username) + == sa_func.lower(username) + ) + .one_or_none() + ) + return ( + self.session.query(self.user_model) + .options(*eager) + .filter(self.user_model.username == username) + .one_or_none() + ) + except MultipleResultsFound: + logger.error("Multiple results found for user %s", username) + return None + if email: + try: + return ( + self.session.query(self.user_model) + .options(*eager) + .filter_by(email=email) + .one_or_none() + ) + except MultipleResultsFound: + logger.error("Multiple results found for user with email %s", email) + return None + return None + def get_anonymous_user(self) -> User: return AnonymousUserMixin() diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 21d060441ff8..961d41fbbc68 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -351,3 +351,17 @@ def test_security_manager_has_expected_api_key_methods(app: SupersetApp) -> None "auth._resolve_user_from_api_key() references this method by name — " "update auth.py if the FAB API changed." ) + + +def test_security_manager_has_find_user_with_relationships(app: SupersetApp) -> None: + """Regression test: verify SupersetSecurityManager.find_user_with_relationships + exists. load_user_with_relationships() in auth.py delegates to it — a rename + or removal would silently break MCP user resolution at runtime.""" + with app.app_context(): + from superset import security_manager + + assert hasattr(security_manager, "find_user_with_relationships"), ( + "SupersetSecurityManager is missing 'find_user_with_relationships'. " + "auth.load_user_with_relationships() delegates to this method — " + "update auth.py if the method was renamed or removed." + ) From 30c4c206be56bc7b2a93600b222dccf4959d7fac Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 13 May 2026 23:04:02 +0000 Subject: [PATCH 14/31] fix(mcp): fix stale patch target in auth tests and update stale docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _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 --- tests/unit_tests/mcp_service/test_auth_api_key.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index 961d41fbbc68..e4279ffcd9d9 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -86,7 +86,14 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]: @contextmanager def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock): - """Push an app context with g.user cleared and appbuilder.sm mocked.""" + """Push an app context with g.user cleared and appbuilder.sm mocked. + + Defaults find_user_with_relationships to None so JWT/dev-user lookups + that hit the SM (via load_user_with_relationships) behave as "user not + found" without a real DB, matching the pre-refactor db.session behavior. + Tests that need a specific return value should set it on mock_sm directly. + """ + mock_sm.find_user_with_relationships.return_value = None with app.app_context(): g.user = None app.appbuilder = MagicMock() From cda5a055923fa02b2efb6fc19e575d0ce8107ddc Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 14 May 2026 16:34:36 +0000 Subject: [PATCH 15/31] =?UTF-8?q?fix(mcp):=20validate=20api=5Fkey=5Fprefix?= =?UTF-8?q?es=20in=20CompositeTokenVerifier=20=E2=80=94=20filter=20empty/n?= =?UTF-8?q?on-string=20entries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../mcp_service/composite_token_verifier.py | 10 +++- .../test_composite_token_verifier.py | 60 +++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py index ffba111bf481..8889d66af428 100644 --- a/superset/mcp_service/composite_token_verifier.py +++ b/superset/mcp_service/composite_token_verifier.py @@ -68,7 +68,15 @@ def __init__( required_scopes=getattr(jwt_verifier, "required_scopes", None) or [], ) self._jwt_verifier = jwt_verifier - self._api_key_prefixes = tuple(api_key_prefixes) + valid: list[str] = [ + p for p in api_key_prefixes if isinstance(p, str) and p.strip() + ] + invalid = [p for p in api_key_prefixes if p not in valid] + if invalid: + logger.warning( + "FAB_API_KEY_PREFIXES contains invalid entries (ignored): %r", invalid + ) + self._api_key_prefixes = tuple(valid) async def verify_token(self, token: str) -> AccessToken | None: """Verify a Bearer token. diff --git a/tests/unit_tests/mcp_service/test_composite_token_verifier.py b/tests/unit_tests/mcp_service/test_composite_token_verifier.py index d493c07ea03c..d519b39a8a9e 100644 --- a/tests/unit_tests/mcp_service/test_composite_token_verifier.py +++ b/tests/unit_tests/mcp_service/test_composite_token_verifier.py @@ -139,6 +139,66 @@ async def test_api_key_only_mode_rejects_non_api_key_tokens() -> None: assert result is None +@pytest.mark.asyncio +async def test_empty_string_prefix_is_filtered_out() -> None: + """An empty-string prefix would match every Bearer token (DoS vector). + It must be silently dropped and never stored in _api_key_prefixes.""" + verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=[""]) + + assert "" not in verifier._api_key_prefixes + # A plain JWT must NOT be misidentified as an API key. + result = await verifier.verify_token("eyJhbGciOiJSUzI1NiJ9.jwt_payload") + assert result is None + + +@pytest.mark.asyncio +async def test_whitespace_only_prefix_is_filtered_out() -> None: + """A whitespace-only prefix is also invalid and must be dropped.""" + verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=[" "]) + + assert " " not in verifier._api_key_prefixes + result = await verifier.verify_token(" starts_with_spaces") + assert result is None + + +@pytest.mark.asyncio +async def test_non_string_prefix_is_filtered_out() -> None: + """Non-string entries (e.g. None, int) must not be stored and must not + cause a TypeError during verify_token.""" + verifier = CompositeTokenVerifier( + jwt_verifier=None, + api_key_prefixes=[None, 42, "sst_"], # type: ignore[list-item] + ) + + assert None not in verifier._api_key_prefixes + assert 42 not in verifier._api_key_prefixes + assert verifier._api_key_prefixes == ("sst_",) + + +@pytest.mark.asyncio +async def test_invalid_prefixes_emit_warning(caplog: pytest.LogCaptureFixture) -> None: + """Invalid prefix entries must trigger a logger.warning so operators can + detect misconfiguration in FAB_API_KEY_PREFIXES.""" + import logging + + logger_name = "superset.mcp_service.composite_token_verifier" + with caplog.at_level(logging.WARNING, logger=logger_name): + CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["", "sst_"]) + + assert any("invalid" in record.message.lower() for record in caplog.records) + + +@pytest.mark.asyncio +async def test_all_invalid_prefixes_accepts_no_api_keys() -> None: + """When all prefixes are invalid and filtered out, no token should match + the API key path.""" + verifier = CompositeTokenVerifier(jwt_verifier=None, api_key_prefixes=["", " "]) + + assert verifier._api_key_prefixes == () + result = await verifier.verify_token("sst_abc123") + assert result is None + + @pytest.mark.asyncio async def test_api_key_passthrough_propagates_required_scopes() -> None: """The pass-through AccessToken must carry the verifier's required_scopes From bd44c242b912ca51ac98a66a03c38c8df2339525 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 14 May 2026 19:05:42 +0000 Subject: [PATCH 16/31] fix(mcp): fix stale patch target in auth tests and update stale docstring - 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 --- .../mcp_service/test_auth_api_key.py | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py index e4279ffcd9d9..afe5a53ac5b4 100644 --- a/tests/unit_tests/mcp_service/test_auth_api_key.py +++ b/tests/unit_tests/mcp_service/test_auth_api_key.py @@ -86,14 +86,7 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]: @contextmanager def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock): - """Push an app context with g.user cleared and appbuilder.sm mocked. - - Defaults find_user_with_relationships to None so JWT/dev-user lookups - that hit the SM (via load_user_with_relationships) behave as "user not - found" without a real DB, matching the pre-refactor db.session behavior. - Tests that need a specific return value should set it on mock_sm directly. - """ - mock_sm.find_user_with_relationships.return_value = None + """Push an app context with g.user cleared and appbuilder.sm mocked.""" with app.app_context(): g.user = None app.appbuilder = MagicMock() @@ -101,6 +94,19 @@ def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock): yield +def _patch_load_user_not_found(): + """Patch load_user_with_relationships to return None (user not found). + + load_user_with_relationships delegates to the global security_manager + (not app.appbuilder.sm), so tests that need the JWT path to raise + ValueError("not found") must patch it directly at the module level. + """ + return patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ) + + # -- Valid API key -> user loaded -- @@ -255,10 +261,9 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None: jwt_access_token.claims = {"sub": "alice"} with _mock_sm_ctx(app, mock_sm): - with _patch_access_token(jwt_access_token): - # _resolve_user_from_jwt_context will try to resolve the user - # from the JWT claims and (in this isolated unit-test setup) - # raise ValueError because the username is not a real user. + with _patch_access_token(jwt_access_token), _patch_load_user_not_found(): + # _resolve_user_from_jwt_context resolves "alice" from JWT claims + # and raises ValueError because the username is not a real user. # We assert that _resolve_user_from_api_key did NOT short-circuit # to the API key path. with pytest.raises(ValueError, match="not found"): @@ -302,9 +307,9 @@ def test_namespaced_claim_without_api_key_client_id_is_ignored( rogue_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True, "sub": "alice"} with _mock_sm_ctx(app, mock_sm): - with _patch_access_token(rogue_token): - # JWT path tries to resolve user "alice" from DB and raises - # ValueError in this isolated unit-test setup. + with _patch_access_token(rogue_token), _patch_load_user_not_found(): + # JWT path resolves "alice" from claims and raises ValueError + # because no such user exists. # validate_api_key must NOT be called — the rogue claim was ignored. with pytest.raises(ValueError, match="not found"): get_user_from_request() @@ -330,11 +335,9 @@ def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path( rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"} with _mock_sm_ctx(app, mock_sm): - with _patch_access_token(rogue_token): - # JWT path tries to resolve user "alice" from DB and (in this - # isolated unit-test setup) raises ValueError. The assertion - # below confirms validate_api_key was never called — i.e., the - # rogue claim did NOT divert into _resolve_user_from_api_key. + with _patch_access_token(rogue_token), _patch_load_user_not_found(): + # JWT path resolves "alice" from claims and raises ValueError. + # validate_api_key must NOT be called — the rogue claim was ignored. with pytest.raises(ValueError, match="not found"): get_user_from_request() From e64b1ac8b507c39e5e9ecd6c0aaafcadfc72994e Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 14 May 2026 19:08:34 +0000 Subject: [PATCH 17/31] fix(mcp): normalize FAB_API_KEY_PREFIXES from config before passing to 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 --- superset/mcp_service/mcp_config.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index dc16b248c54b..24f20504d836 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -346,7 +346,13 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: return None if api_key_enabled: - api_key_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) + raw_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) + # Normalize: a plain string (e.g. "sst_") would iterate as characters; + # wrap it in a list so CompositeTokenVerifier receives a proper sequence. + if isinstance(raw_prefixes, str): + api_key_prefixes = [raw_prefixes] + else: + api_key_prefixes = list(raw_prefixes) logger.info("API key auth enabled for MCP") return CompositeTokenVerifier( jwt_verifier=jwt_verifier, From 4a998483952d7008667544784bcfb443e0cc22d7 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 14 May 2026 23:37:10 +0000 Subject: [PATCH 18/31] fix(mcp): address CodeQL security warnings and add ApiKey RBAC regression 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 --- superset/mcp_service/auth.py | 7 +++++-- superset/mcp_service/composite_token_verifier.py | 6 +++++- .../security/test_granular_export_permissions.py | 11 +++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 22e82e038fe5..61361657000f 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -305,9 +305,12 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: if not user: # Fail closed: JWT says this user should exist but they don't. # Do NOT fall through to MCP_DEV_USERNAME or stale g.user. + # Avoid echoing the JWT-extracted username in the exception message + # (CodeQL py/clear-text-logging-sensitive-data). + logger.debug("JWT-authenticated user not found in database (identity from JWT)") raise ValueError( - f"JWT authenticated user '{username}' not found in Superset database. " - f"Ensure the user exists before granting MCP access." + "JWT authenticated user not found in Superset database. " + "Ensure the user exists before granting MCP access." ) return user diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py index 8889d66af428..72b22df60aaf 100644 --- a/superset/mcp_service/composite_token_verifier.py +++ b/superset/mcp_service/composite_token_verifier.py @@ -73,8 +73,12 @@ def __init__( ] invalid = [p for p in api_key_prefixes if p not in valid] if invalid: + # Log count only — actual values may be config secrets + # (CodeQL py/clear-text-logging-sensitive-data). logger.warning( - "FAB_API_KEY_PREFIXES contains invalid entries (ignored): %r", invalid + "FAB_API_KEY_PREFIXES has %d invalid entries (empty/non-string)" + " — ignored", + len(invalid), ) self._api_key_prefixes = tuple(valid) diff --git a/tests/unit_tests/security/test_granular_export_permissions.py b/tests/unit_tests/security/test_granular_export_permissions.py index 6db5e9712b2a..af703debc64b 100644 --- a/tests/unit_tests/security/test_granular_export_permissions.py +++ b/tests/unit_tests/security/test_granular_export_permissions.py @@ -91,6 +91,17 @@ def test_is_gamma_pvm_excludes_export_image(app_context: None) -> None: assert sm._is_gamma_pvm(pvm) is False +def test_api_key_view_menu_is_admin_only() -> None: + """Regression test: 'ApiKey' must be in ADMIN_ONLY_VIEW_MENUS. + + FAB registers an ApiKeyApi blueprint when FAB_API_KEY_ENABLED=True. + Without this guard any Gamma user could reach the API key management + endpoints. A rename or removal of the entry would silently re-open + that access hole. + """ + assert "ApiKey" in SupersetSecurityManager.ADMIN_ONLY_VIEW_MENUS + + def test_is_gamma_pvm_allows_copy_clipboard(app_context: None) -> None: """Verify _is_gamma_pvm returns True for can_copy_clipboard.""" from superset.extensions import appbuilder From 2a45cd9969b44ce3862cac2cba2da90e60f63b4a Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 15 May 2026 00:02:31 +0000 Subject: [PATCH 19/31] fix(mcp): remove sensitive values from log calls to satisfy CodeQL - 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 --- superset/mcp_service/auth.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 61361657000f..68b3fd6db68d 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -142,16 +142,14 @@ def check_tool_permission(func: Callable[..., Any], *, log_denial: bool = True) if not has_permission: if log_denial: logger.warning( - "Permission denied for user %s: %s on %s (tool: %s)", - g.user.username, + "Permission denied: %s on %s (tool: %s)", permission_str, class_permission_name, func.__name__, ) else: logger.debug( - "Tool hidden for user %s: %s on %s (tool: %s)", - g.user.username, + "Tool hidden: %s on %s (tool: %s)", permission_str, class_permission_name, func.__name__, @@ -280,9 +278,8 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: ) return None logger.debug( - "Ignoring %s claim on non-API-key token (client_id=%r); processing as JWT", - API_KEY_PASSTHROUGH_CLAIM, - getattr(access_token, "client_id", None), + "API key passthrough claim present but client_id is not 'api_key';" + " processing as JWT" ) # Use configurable resolver or default From 9a1d2a4b1aeaff623033aaa05693fa55eb2dd31d Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 15 May 2026 00:05:49 +0000 Subject: [PATCH 20/31] fix(mcp): use class-bound attribute in joinedload for group roles 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. --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 982c2c46e8a5..adb452a9d3bd 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3185,7 +3185,7 @@ def find_user_with_relationships( """ eager = [ joinedload(self.user_model.roles), - joinedload(self.user_model.groups).joinedload("roles"), + joinedload(self.user_model.groups).joinedload(self.group_model.roles), ] if username: try: From 1ee3ead5c6e7c952cf65e202a91fbd034087d65c Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 16 May 2026 21:15:58 +0000 Subject: [PATCH 21/31] =?UTF-8?q?fix(mcp):=20address=20dpgaspar=20review?= =?UTF-8?q?=20=E2=80=94=20imports,=20types,=20exception=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- superset/mcp_service/auth.py | 20 ++++++-------------- superset/mcp_service/mcp_config.py | 13 ++++++++----- superset/security/manager.py | 4 +--- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 68b3fd6db68d..8fa6795acfc6 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -51,7 +51,9 @@ from flask import current_app, g, has_app_context, has_request_context from flask_appbuilder.security.sqla.models import User +from superset import security_manager from superset.mcp_service.composite_token_verifier import API_KEY_PASSTHROUGH_CLAIM +from superset.mcp_service.mcp_config import default_user_resolver if TYPE_CHECKING: from superset.connectors.sqla.models import SqlaTable @@ -109,13 +111,9 @@ def check_tool_permission(func: Callable[..., Any], *, log_denial: bool = True) True if user has permission or no permission is required. """ try: - from flask import current_app - if not current_app.config.get("MCP_RBAC_ENABLED", True): return True - from superset import security_manager - if not hasattr(g, "user") or not g.user: if log_denial: logger.warning( @@ -142,14 +140,16 @@ def check_tool_permission(func: Callable[..., Any], *, log_denial: bool = True) if not has_permission: if log_denial: logger.warning( - "Permission denied: %s on %s (tool: %s)", + "Permission denied for user id=%s: %s on %s (tool: %s)", + getattr(g.user, "id", "?"), permission_str, class_permission_name, func.__name__, ) else: logger.debug( - "Tool hidden: %s on %s (tool: %s)", + "Tool hidden for user id=%s: %s on %s (tool: %s)", + getattr(g.user, "id", "?"), permission_str, class_permission_name, func.__name__, @@ -230,8 +230,6 @@ def load_user_with_relationships( if not username and not email: raise ValueError("Either username or email must be provided") - from superset import security_manager - return security_manager.find_user_with_relationships(username=username, email=email) @@ -283,8 +281,6 @@ def _resolve_user_from_jwt_context(app: Any) -> User | None: ) # Use configurable resolver or default - from superset.mcp_service.mcp_config import default_user_resolver - resolver = app.config.get("MCP_USER_RESOLVER", default_user_resolver) username = resolver(app, access_token) @@ -417,8 +413,6 @@ def get_user_from_request() -> User: Raises: ValueError: If user cannot be authenticated or found """ - from flask import current_app - # Priority 1: JWT context (per-request safe via ContextVar) if (jwt_user := _resolve_user_from_jwt_context(current_app)) is not None: return jwt_user @@ -484,8 +478,6 @@ def has_dataset_access(dataset: "SqlaTable") -> bool: Returns False on any error to fail securely. """ try: - from superset import security_manager - # Check if user has read access to the dataset if hasattr(g, "user") and g.user: # Use Superset's security manager to check dataset access diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 24f20504d836..0c312cc420c0 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -18,8 +18,9 @@ import logging import secrets -from typing import Any, Dict, Optional +from typing import Any, Dict, Optional, Sequence +from authlib.jose.errors import JoseError from fastmcp.server.auth.providers.jwt import JWTVerifier from flask import Flask @@ -339,18 +340,20 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: public_key=public_key, secret=secret, ) - except Exception: # noqa: BLE001 — JWT lib raises many types; broad catch intentional + except (ValueError, JoseError): # Do not log the exception — it may contain secrets (e.g., key material) logger.error("Failed to create MCP JWT verifier") if not api_key_enabled: return None if api_key_enabled: - raw_prefixes = app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) + raw_prefixes: str | Sequence[str] = app.config.get( + "FAB_API_KEY_PREFIXES", ["sst_"] + ) # Normalize: a plain string (e.g. "sst_") would iterate as characters; # wrap it in a list so CompositeTokenVerifier receives a proper sequence. if isinstance(raw_prefixes, str): - api_key_prefixes = [raw_prefixes] + api_key_prefixes: list[str] = [raw_prefixes] else: api_key_prefixes = list(raw_prefixes) logger.info("API key auth enabled for MCP") @@ -367,7 +370,7 @@ def _build_jwt_verifier( jwks_uri: Optional[str], public_key: Optional[str], secret: Optional[str], -) -> Any: +) -> JWTVerifier: """Construct the JWT verifier from configured keys/secret.""" debug_errors = app.config.get("MCP_JWT_DEBUG_ERRORS", False) diff --git a/superset/security/manager.py b/superset/security/manager.py index adb452a9d3bd..6a7c8ce2f053 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -50,7 +50,7 @@ from flask_babel import lazy_gettext as _ from flask_login import AnonymousUserMixin, LoginManager from jwt.api_jwt import _jwt_global_obj -from sqlalchemy import and_, inspect, or_ +from sqlalchemy import and_, func as sa_func, inspect, or_ from sqlalchemy.engine.base import Connection from sqlalchemy.orm import eagerload, joinedload from sqlalchemy.orm.exc import MultipleResultsFound @@ -3190,8 +3190,6 @@ def find_user_with_relationships( if username: try: if self.auth_username_ci: - from sqlalchemy import func as sa_func - return ( self.session.query(self.user_model) .options(*eager) From 46963f390e76e3a7d8d163fc084217069e0ed540 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 16 May 2026 21:19:22 +0000 Subject: [PATCH 22/31] fix(mcp): remove sensitive values from log calls to satisfy CodeQL 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. --- superset/mcp_service/auth.py | 4 ++-- superset/security/manager.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 8fa6795acfc6..de6227a1e5b0 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -383,10 +383,10 @@ def _resolve_user_from_api_key(app: Any) -> User | None: user_with_rels = load_user_with_relationships(username=user.username) if user_with_rels is None: logger.warning( - "Failed to reload API key user %s with relationships; " + "Failed to reload API key user id=%s with relationships; " "using original user object which may have lazy-loaded " "relationships", - user.username, + getattr(user, "id", "?"), ) return user return user_with_rels diff --git a/superset/security/manager.py b/superset/security/manager.py index 6a7c8ce2f053..67d1bcfd53c0 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3206,7 +3206,7 @@ def find_user_with_relationships( .one_or_none() ) except MultipleResultsFound: - logger.error("Multiple results found for user %s", username) + logger.error("Multiple results found for username lookup") return None if email: try: @@ -3217,7 +3217,7 @@ def find_user_with_relationships( .one_or_none() ) except MultipleResultsFound: - logger.error("Multiple results found for user with email %s", email) + logger.error("Multiple results found for email lookup") return None return None From 7d3a3e7c650459336d2ca60aa8768699df8f302a Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 16 May 2026 22:01:28 +0000 Subject: [PATCH 23/31] fix(mcp): update security_manager patch target in RBAC tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/unit_tests/mcp_service/test_auth_rbac.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit_tests/mcp_service/test_auth_rbac.py b/tests/unit_tests/mcp_service/test_auth_rbac.py index 3949203282a7..64a9684eaec0 100644 --- a/tests/unit_tests/mcp_service/test_auth_rbac.py +++ b/tests/unit_tests/mcp_service/test_auth_rbac.py @@ -122,7 +122,7 @@ def test_check_tool_permission_granted(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=True) - with patch("superset.security_manager", mock_sm): + with patch("superset.mcp_service.auth.security_manager", mock_sm): result = check_tool_permission(func) assert result is True @@ -136,7 +136,7 @@ def test_check_tool_permission_denied(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=False) - with patch("superset.security_manager", mock_sm): + with patch("superset.mcp_service.auth.security_manager", mock_sm): result = check_tool_permission(func) assert result is False @@ -151,7 +151,7 @@ def test_check_tool_permission_default_method_is_read(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=True) - with patch("superset.security_manager", mock_sm): + with patch("superset.mcp_service.auth.security_manager", mock_sm): result = check_tool_permission(func) assert result is True From 08cb50df44ea3df17e0c1610590194aa4fe40a13 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 16 May 2026 22:15:01 +0000 Subject: [PATCH 24/31] =?UTF-8?q?fix(mcp):=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20error=20class,=20fail-open,=20DRY=20permission=20lo?= =?UTF-8?q?gic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- superset/mcp_service/auth.py | 16 +++++++++++++--- superset/mcp_service/composite_token_verifier.py | 4 ++++ superset/mcp_service/mcp_config.py | 10 +++++++++- superset/security/manager.py | 4 ++++ 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index de6227a1e5b0..c9686c39834f 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -70,8 +70,13 @@ METHOD_PERMISSION_ATTR = "_method_permission_name" -class MCPPermissionDeniedError(Exception): - """Raised when user lacks required RBAC permission for an MCP tool.""" +class MCPPermissionDeniedError(PermissionError): + """Raised when user lacks required RBAC permission for an MCP tool. + + Inherits from ``PermissionError`` so the middleware classifies denials as + user errors (HTTP 403 / WARNING log / "Access denied" sanitized message) + rather than unexpected server errors. + """ def __init__( self, @@ -451,7 +456,12 @@ def get_user_from_request() -> User: "g.user was not set by external middleware", ] configured_prefixes = current_app.config.get("FAB_API_KEY_PREFIXES", ["sst_"]) - prefix_example = configured_prefixes[0] if configured_prefixes else "sst_" + if isinstance(configured_prefixes, str): + prefix_example = configured_prefixes + elif configured_prefixes: + prefix_example = configured_prefixes[0] + else: + prefix_example = "sst_" raise ValueError( "No authenticated user found. Tried:\n" + "\n".join(f" - {d}" for d in details) diff --git a/superset/mcp_service/composite_token_verifier.py b/superset/mcp_service/composite_token_verifier.py index 72b22df60aaf..f2a4d9bed86c 100644 --- a/superset/mcp_service/composite_token_verifier.py +++ b/superset/mcp_service/composite_token_verifier.py @@ -100,6 +100,10 @@ async def verify_token(self, token: str) -> AccessToken | None: # satisfied for API-key requests. Without this, MCP_REQUIRED_SCOPES # being non-empty would 403 every API-key call before # ``_resolve_user_from_api_key`` even runs. + # + # NOTE: ``MCP_REQUIRED_SCOPES`` is intentionally not enforced for + # API-key auth — FAB API keys do not carry scopes. Authorization is + # enforced downstream via ``check_tool_permission`` (RBAC). return AccessToken( token=token, client_id="api_key", diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 0c312cc420c0..0fe43e2be6d1 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -352,10 +352,18 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: ) # Normalize: a plain string (e.g. "sst_") would iterate as characters; # wrap it in a list so CompositeTokenVerifier receives a proper sequence. + # Guard against non-iterable config values (e.g. None, integers) that + # would raise TypeError and cause _create_auth_provider to fail open. if isinstance(raw_prefixes, str): api_key_prefixes: list[str] = [raw_prefixes] else: - api_key_prefixes = list(raw_prefixes) + try: + api_key_prefixes = list(raw_prefixes) + except TypeError: + logger.warning( + "FAB_API_KEY_PREFIXES must be a string or list; using default" + ) + api_key_prefixes = ["sst_"] logger.info("API key auth enabled for MCP") return CompositeTokenVerifier( jwt_verifier=jwt_verifier, diff --git a/superset/security/manager.py b/superset/security/manager.py index 67d1bcfd53c0..10fb388a3598 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3182,6 +3182,10 @@ def find_user_with_relationships( ``User.roles`` and ``User.groups.roles`` to prevent detached-instance errors when the SQLAlchemy session is closed or rolled back after the lookup — as happens in MCP tool-execution contexts. + + FAB does not expose an eager-loading option on ``find_user``, so the + query logic is mirrored here with joinedload options added. Review this + method when upgrading FAB to ensure it stays in sync with upstream. """ eager = [ joinedload(self.user_model.roles), From 53adbb71d46414c5f342439d215739bd8c9172cc Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Sat, 16 May 2026 23:27:54 +0000 Subject: [PATCH 25/31] fix(mcp): update security_manager patch target in tool-search tests _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 --- .../mcp_service/test_tool_search_transform.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/mcp_service/test_tool_search_transform.py b/tests/unit_tests/mcp_service/test_tool_search_transform.py index b8f301c9692b..5042af23e3ed 100644 --- a/tests/unit_tests/mcp_service/test_tool_search_transform.py +++ b/tests/unit_tests/mcp_service/test_tool_search_transform.py @@ -869,7 +869,7 @@ def denied_tool(): with app.app_context(): g.user = SimpleNamespace(username="viewer") with patch( - "superset.security_manager", new_callable=MagicMock + "superset.mcp_service.auth.security_manager", new_callable=MagicMock ) as security_manager: security_manager.can_access.side_effect = [True, False] @@ -970,7 +970,9 @@ def metadata_tool(): "superset.mcp_service.privacy.user_can_view_data_model_metadata", return_value=True, ), - patch("superset.security_manager", new_callable=Mock) as security_manager, + patch( + "superset.mcp_service.auth.security_manager", new_callable=Mock + ) as security_manager, ): security_manager.can_access.return_value = False result = _filter_tools_by_current_user_permission([metadata, public]) @@ -997,7 +999,9 @@ def protected_tool(): "superset.mcp_service.auth.get_user_from_request", return_value=SimpleNamespace(username="viewer"), ), - patch("superset.security_manager", new_callable=Mock) as security_manager, + patch( + "superset.mcp_service.auth.security_manager", new_callable=Mock + ) as security_manager, ): security_manager.can_access.return_value = True result = _filter_tools_by_current_user_permission([protected]) @@ -1023,7 +1027,9 @@ def test_tool_search_permission_filter_keeps_get_schema_visible_without_metadata "superset.mcp_service.privacy.user_can_view_data_model_metadata", return_value=False, ), - patch("superset.security_manager", new_callable=Mock) as security_manager, + patch( + "superset.mcp_service.auth.security_manager", new_callable=Mock + ) as security_manager, ): security_manager.can_access.return_value = True result = _filter_tools_by_current_user_permission([schema_tool]) From bc310d0f3af4621e3a0fe67b22455f2520121e62 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 22 May 2026 02:13:39 +0000 Subject: [PATCH 26/31] fix(mcp): broaden _log_user_resolution_failure type hint mypy correctly flags that the caller can pass either ValueError or PermissionError; widen the type hint to match. --- superset/mcp_service/auth.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index c9686c39834f..a75e9a83c909 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -519,14 +519,14 @@ def check_chart_data_access(chart: Any) -> "DatasetValidationResult": return validate_chart_dataset(chart, check_access=True) -def _log_user_resolution_failure(exc: ValueError) -> None: - """Log a user-resolution ValueError at the appropriate level. +def _log_user_resolution_failure(exc: ValueError | PermissionError) -> None: + """Log a user-resolution failure at the appropriate level. "No authenticated user found" is expected in unauthenticated/dev deployments (no JWT, no API key, no MCP_DEV_USERNAME configured) and during tools/list scanning — log at DEBUG to avoid ERROR noise. - All other ValueErrors (e.g. dev username not in DB) are genuine - credential failures and are logged at ERROR. + All other failures (e.g. dev username not in DB, permission denied) are + genuine credential failures and are logged at ERROR. """ if "No authenticated user found" in str(exc): logger.debug("MCP: no auth source configured, unauthenticated request") From a5d02055689ebc1bf94e0c65c0b67074d04e0dab Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 22 May 2026 02:21:45 +0000 Subject: [PATCH 27/31] fix(mcp): fix MCPPermissionDeniedError handler order and visibility test 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 --- superset/mcp_service/middleware.py | 8 +++++--- tests/unit_tests/mcp_service/test_auth_rbac.py | 8 ++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/superset/mcp_service/middleware.py b/superset/mcp_service/middleware.py index 685022c3a9fa..02688a761f69 100644 --- a/superset/mcp_service/middleware.py +++ b/superset/mcp_service/middleware.py @@ -632,6 +632,11 @@ async def _handle_error( # noqa: C901 elif isinstance(error, HTTPException): # HTTP errors from screenshot endpoints or API calls raise ToolError(f"Service error in {tool_name}: {error.detail}") from error + elif isinstance(error, MCPPermissionDeniedError): + # MCP RBAC permission denied — convert to structured ToolError. + # Must come before the generic PermissionError branch because + # MCPPermissionDeniedError inherits from PermissionError. + raise ToolError(str(error)) from error elif isinstance(error, PermissionError): # Permission/authorization errors raise ToolError( @@ -648,9 +653,6 @@ async def _handle_error( # noqa: C901 raise ToolError( f"Invalid request for {tool_name}: {_sanitize_error_for_logging(error)}" ) from error - elif isinstance(error, MCPPermissionDeniedError): - # MCP RBAC permission denied — convert to structured ToolError - raise ToolError(str(error)) from error elif isinstance(error, (ForbiddenError, SupersetSecurityException)): # Superset access denied — agent tried a tool it can't use raise ToolError( diff --git a/tests/unit_tests/mcp_service/test_auth_rbac.py b/tests/unit_tests/mcp_service/test_auth_rbac.py index 64a9684eaec0..55a97dacea79 100644 --- a/tests/unit_tests/mcp_service/test_auth_rbac.py +++ b/tests/unit_tests/mcp_service/test_auth_rbac.py @@ -280,7 +280,7 @@ def test_visibility_allowed_tool(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=True) - with patch("superset.security_manager", mock_sm): + with patch("superset.mcp_service.auth.security_manager", mock_sm): result = is_tool_visible_to_current_user(tool) assert result is True @@ -295,7 +295,7 @@ def test_visibility_denied_tool(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=False) - with patch("superset.security_manager", mock_sm): + with patch("superset.mcp_service.auth.security_manager", mock_sm): result = is_tool_visible_to_current_user(tool) assert result is False @@ -312,7 +312,7 @@ def test_visibility_data_model_metadata_denied(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=True) with ( - patch("superset.security_manager", mock_sm), + patch("superset.mcp_service.auth.security_manager", mock_sm), patch( "superset.mcp_service.privacy.user_can_view_data_model_metadata", return_value=False, @@ -334,7 +334,7 @@ def test_visibility_data_model_metadata_allowed(app_context) -> None: mock_sm = MagicMock() mock_sm.can_access = MagicMock(return_value=True) with ( - patch("superset.security_manager", mock_sm), + patch("superset.mcp_service.auth.security_manager", mock_sm), patch( "superset.mcp_service.privacy.user_can_view_data_model_metadata", return_value=True, From 5765111e37b569848364da0784b16f04a78ff7de Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 22 May 2026 18:14:36 +0000 Subject: [PATCH 28/31] fix(mcp): remove exc_info=True from tool-visibility debug log to prevent traceback-based credential leak CodeQL py/clear-text-logging-sensitive-data (alert #2283) flagged this path because exc_info=True includes the full exception traceback, which can expose sensitive local variables (tokens, API keys) from frames on the call stack. The message alone is sufficient for debugging. Co-Authored-By: Claude Sonnet 4.6 --- superset/mcp_service/auth.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index a75e9a83c909..ff8abf53729b 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -211,9 +211,7 @@ def is_tool_visible_to_current_user(tool: Any) -> bool: return check_tool_permission(tool_func, log_denial=False) except (AttributeError, RuntimeError, ValueError): - logger.debug( - "Could not evaluate tool visibility for current user", exc_info=True - ) + logger.debug("Could not evaluate tool visibility for current user") return False From dceb37de33f019629a1eed8a490fe84a6bb613c3 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Tue, 26 May 2026 17:27:58 +0000 Subject: [PATCH 29/31] fix(mcp): use consistent filter() style for email lookup in find_user_with_relationships Use `.filter(self.user_model.email == email)` to match the explicit column-attribute style used for username lookups in the same method. Co-Authored-By: Claude Sonnet 4.6 --- superset/security/manager.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 10fb388a3598..7e4938c2c31a 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3217,7 +3217,7 @@ def find_user_with_relationships( return ( self.session.query(self.user_model) .options(*eager) - .filter_by(email=email) + .filter(self.user_model.email == email) .one_or_none() ) except MultipleResultsFound: From 91c504276bd92ba4f5a56be350c3d34bb5f70cc3 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Wed, 27 May 2026 16:09:20 +0000 Subject: [PATCH 30/31] fix(mcp): replace MCPJWTVerifier with JWTVerifier after browser-hello revert MCPJWTVerifier was removed in the master revert of the browser-friendly hello page (#40467). Update mcp_config.py to import only DetailedJWTVerifier and use JWTVerifier directly for the non-debug code path, matching the current jwt_verifier.py interface. --- superset/mcp_service/mcp_config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 0fe43e2be6d1..6e93e9f76441 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -29,7 +29,7 @@ DEFAULT_TOKEN_LIMIT, DEFAULT_WARN_THRESHOLD_PCT, ) -from superset.mcp_service.jwt_verifier import DetailedJWTVerifier, MCPJWTVerifier +from superset.mcp_service.jwt_verifier import DetailedJWTVerifier logger = logging.getLogger(__name__) @@ -404,8 +404,8 @@ def _build_jwt_verifier( # RFC 6750 Section 3.1. return DetailedJWTVerifier(**common_kwargs) - # MCPJWTVerifier: minimal logging + browser-friendly error page. - return MCPJWTVerifier(**common_kwargs) + # Default JWTVerifier: minimal logging, generic error responses. + return JWTVerifier(**common_kwargs) def default_user_resolver(app: Any, access_token: Any) -> str | None: From 11dce414ed7325228b798f21e651fc80fb2a6643 Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Thu, 28 May 2026 14:24:25 +0000 Subject: [PATCH 31/31] fix(security): make ApiKey admin-only view menu conditional on FAB_API_KEY_ENABLED Move the ApiKey view-menu guard from the static ADMIN_ONLY_VIEW_MENUS set into _is_admin_only so it is only enforced when FAB_API_KEY_ENABLED=True, as requested in review. --- superset/security/manager.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index 7e4938c2c31a..30c3dc41c65b 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -463,10 +463,6 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods "PermissionViewMenu", "ViewMenu", "User", - # FAB ApiKeyApi blueprint (active when FAB_API_KEY_ENABLED=True). - # Listed unconditionally — harmless when the feature is off because - # no PVMs exist under this view menu. - "ApiKey", } | USER_MODEL_VIEWS ALPHA_ONLY_VIEW_MENUS = { @@ -1648,6 +1644,10 @@ def _is_admin_only(self, pvm: PermissionView) -> bool: and pvm.permission.name not in self.READ_ONLY_PERMISSION ): return True + if pvm.view_menu.name == "ApiKey" and current_app.config.get( + "FAB_API_KEY_ENABLED", False + ): + return True return ( pvm.view_menu.name in self.ADMIN_ONLY_VIEW_MENUS or pvm.permission.name in self.ADMIN_ONLY_PERMISSIONS