This is an automated email from the ASF dual-hosted git repository. aminghadersohi pushed a commit to branch work-pr-39604 in repository https://gitbox.apache.org/repos/asf/superset.git
commit 843cc9fc97ad387230aa1eeeea755c4c17598658 Author: Amin Ghadersohi <[email protected]> AuthorDate: Thu Apr 23 19:52:17 2026 -0400 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 +++ tests/unit_tests/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 69d8e625531..5320821d88a 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 d12b44bbc87..d38a5d0eca3 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 9a5055d43fe..44c23f86597 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1422,6 +1422,15 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods 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 6a0bcab6719..7fa9b293481 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 @@ from unittest.mock import MagicMock, patch 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 -- + + [email protected]("_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." + )
