mirror of
https://github.com/apache/superset.git
synced 2026-05-21 15:55:10 +00:00
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.
This commit is contained in:
@@ -218,6 +218,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
|
||||
|
||||
|
||||
@@ -335,6 +335,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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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 --
|
||||
|
||||
|
||||
@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
|
||||
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."
|
||||
)
|
||||
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."
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user