diff --git a/UPDATING.md b/UPDATING.md index 1bbe2197d55..70a899fd40c 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -28,6 +28,10 @@ assists people when migrating to a new version. The `rls` rules passed to `POST /api/v1/security/guest_token/` are now validated strictly: a rule may only contain `dataset` and `clause`. Previously unknown fields were silently dropped, so a mistyped or legacy scope key (most commonly `datasource` instead of `dataset`) produced a rule with no `dataset`, which is treated as a *global* rule applied to every dataset the embedded resource can reach. Such a request now returns HTTP 400 identifying the offending field instead of issuing a token with an unintended global rule. Integrators that were sending extra fields in RLS rules must remove them; valid dataset-scoped (`{"dataset": 41, "clause": "..."}`) and global (`{"clause": "..."}`) rules are unaffected. +### MCP service requires `MCP_JWT_AUDIENCE` when JWT auth is enabled + +When the MCP service has JWT auth enabled (`MCP_AUTH_ENABLED = True`), an audience must be configured via `MCP_JWT_AUDIENCE` so issued tokens are bound to this service. The service now fails to start with a clear configuration error when the audience is unset, instead of starting with audience validation skipped. Deployments that enable MCP JWT auth must set `MCP_JWT_AUDIENCE` to the audience value their identity provider issues for the MCP service. API-key-only MCP deployments (JWT auth disabled) are unaffected. + ### Pivot table First/Last aggregations follow data order The pivot table chart's `First` and `Last` aggregations now return the first and last value in data (query result) order, instead of effectively returning the minimum and maximum. Existing pivot tables that use these aggregations for totals/subtotals may show different values after upgrading. For deterministic results, ensure the underlying query has a stable sort order. diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index 150cd48a395..35e15715004 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -32,6 +32,17 @@ from superset.mcp_service.jwt_verifier import DetailedJWTVerifier, MCPJWTVerifie logger = logging.getLogger(__name__) + +class MCPAuthConfigError(ValueError): + """Raised when MCP auth is enabled but configured in an unusable state. + + Distinct from the generic build errors (e.g. malformed key material) that + the auth bootstrap intentionally swallows: a configuration error of this + kind must propagate so the MCP service fails to start rather than silently + coming up without the protection the operator asked for. + """ + + # MCP Service Configuration # Note: MCP_DEV_USERNAME MUST be configured in superset_config.py # There is no default value - the service will fail if not set @@ -359,6 +370,20 @@ def create_default_mcp_auth_factory(app: Flask) -> Optional[Any]: if not (auth_enabled or api_key_enabled): return None + # When JWT auth is enabled, an audience must be configured so issued tokens + # are bound to this service. Without it the verifier accepts any otherwise + # valid same-issuer token, regardless of which service it was minted for. + # Treat a missing audience as a fatal configuration error so the service + # fails to start instead of coming up in a permissive state — the + # surrounding bootstrap would otherwise turn a None/raised provider into an + # unauthenticated server. + if auth_enabled and not app.config.get("MCP_JWT_AUDIENCE"): + raise MCPAuthConfigError( + "MCP_JWT_AUDIENCE must be set when MCP_AUTH_ENABLED is True so that " + "tokens are bound to this service. Set MCP_JWT_AUDIENCE to the " + "audience value your identity provider issues for the MCP service." + ) + jwt_verifier: Any | None = None if auth_enabled: diff --git a/superset/mcp_service/server.py b/superset/mcp_service/server.py index 03e09697ebe..967d3fb5105 100644 --- a/superset/mcp_service/server.py +++ b/superset/mcp_service/server.py @@ -750,6 +750,7 @@ def _create_auth_provider(flask_app: Any) -> Any | None: ): from superset.mcp_service.mcp_config import ( create_default_mcp_auth_factory, + MCPAuthConfigError, ) try: @@ -758,6 +759,12 @@ def _create_auth_provider(flask_app: Any) -> Any | None: "Auth provider created from default factory: %s", type(auth_provider).__name__ if auth_provider else "None", ) + except MCPAuthConfigError: + # A misconfiguration that must fail closed: re-raise so the service + # refuses to start rather than falling through to an unauthenticated + # server. The message is operator-facing config guidance and carries + # no secret material. + raise except Exception: # Do not log the exception — it may contain secrets logger.error("Failed to create auth provider from default factory") diff --git a/tests/unit_tests/mcp_service/test_mcp_config.py b/tests/unit_tests/mcp_service/test_mcp_config.py index 6ee81a3e5cd..10ced20866d 100644 --- a/tests/unit_tests/mcp_service/test_mcp_config.py +++ b/tests/unit_tests/mcp_service/test_mcp_config.py @@ -19,6 +19,8 @@ from unittest.mock import MagicMock, patch +import pytest + from superset.mcp_service.app import get_default_instructions, init_fastmcp_server @@ -383,6 +385,7 @@ def test_create_default_mcp_auth_factory_jwt_with_keys(): "MCP_AUTH_ENABLED": True, "MCP_API_KEY_ENABLED": False, "FAB_API_KEY_ENABLED": False, + "MCP_JWT_AUDIENCE": "superset-mcp", "MCP_JWT_SECRET": "shhh", }.get(key, default) @@ -405,6 +408,7 @@ def test_create_default_mcp_auth_factory_jwt_enabled_without_keys_returns_none() "MCP_AUTH_ENABLED": True, "MCP_API_KEY_ENABLED": False, "FAB_API_KEY_ENABLED": False, + "MCP_JWT_AUDIENCE": "superset-mcp", }.get(key, default) with patch("superset.mcp_service.mcp_config.logger") as mock_logger: @@ -423,6 +427,7 @@ def test_create_default_mcp_auth_factory_jwt_build_failure_returns_none(): "MCP_AUTH_ENABLED": True, "MCP_API_KEY_ENABLED": False, "FAB_API_KEY_ENABLED": False, + "MCP_JWT_AUDIENCE": "superset-mcp", "MCP_JWT_SECRET": "shhh", }.get(key, default) @@ -437,3 +442,45 @@ def test_create_default_mcp_auth_factory_jwt_build_failure_returns_none(): assert result is None mock_logger.error.assert_called_once() + + +def test_create_default_mcp_auth_factory_requires_audience_when_jwt_enabled(): + """MCP_AUTH_ENABLED=True without MCP_JWT_AUDIENCE fails closed. + + A missing audience must raise MCPAuthConfigError (rather than returning a + permissive verifier) so the bootstrap refuses to start the service instead + of accepting same-issuer tokens minted for other services. + """ + from superset.mcp_service.mcp_config import ( + create_default_mcp_auth_factory, + MCPAuthConfigError, + ) + + mock_app = MagicMock() + mock_app.config.get.side_effect = lambda key, default=None: { + "MCP_AUTH_ENABLED": True, + "MCP_API_KEY_ENABLED": False, + "FAB_API_KEY_ENABLED": False, + "MCP_JWT_SECRET": "shhh", + }.get(key, default) + + with pytest.raises(MCPAuthConfigError): + create_default_mcp_auth_factory(mock_app) + + +def test_create_default_mcp_auth_factory_audience_not_required_for_api_key_only(): + """API-key-only auth (JWT disabled) does not require MCP_JWT_AUDIENCE.""" + from superset.mcp_service.composite_token_verifier import CompositeTokenVerifier + from superset.mcp_service.mcp_config import create_default_mcp_auth_factory + + mock_app = MagicMock() + mock_app.config.get.side_effect = lambda key, default=None: { + "MCP_AUTH_ENABLED": False, + "MCP_API_KEY_ENABLED": True, + "FAB_API_KEY_PREFIXES": ["sst_"], + "MCP_REQUIRED_SCOPES": [], + }.get(key, default) + + result = create_default_mcp_auth_factory(mock_app) + + assert isinstance(result, CompositeTokenVerifier) diff --git a/tests/unit_tests/mcp_service/test_mcp_server.py b/tests/unit_tests/mcp_service/test_mcp_server.py index 1a356df62f9..adaecbc32cf 100644 --- a/tests/unit_tests/mcp_service/test_mcp_server.py +++ b/tests/unit_tests/mcp_service/test_mcp_server.py @@ -19,6 +19,8 @@ from unittest.mock import MagicMock, patch +import pytest + def test_create_event_store_returns_none_when_no_redis_url(): """EventStore returns None when no Redis URL configured (single-pod mode).""" @@ -181,3 +183,29 @@ def test_create_auth_provider_uses_default_factory_for_mcp_api_key_only() -> Non assert result is auth_provider create_default_mcp_auth_factory.assert_called_once_with(flask_app) + + +def test_create_auth_provider_propagates_auth_config_error() -> None: + """A fatal auth config error must propagate, not fall through to no auth. + + The default factory raises MCPAuthConfigError for an unusable auth + configuration. _create_auth_provider must re-raise it so the service fails + to start instead of silently returning None (which would run unauthenticated). + """ + from superset.mcp_service.mcp_config import MCPAuthConfigError + from superset.mcp_service.server import _create_auth_provider + + flask_app = MagicMock() + flask_app.config.get.side_effect = lambda key, default=None: { + "MCP_AUTH_FACTORY": None, + "MCP_AUTH_ENABLED": True, + "MCP_API_KEY_ENABLED": False, + "FAB_API_KEY_ENABLED": False, + }.get(key, default) + + with patch( + "superset.mcp_service.mcp_config.create_default_mcp_auth_factory", + side_effect=MCPAuthConfigError("MCP_JWT_AUDIENCE must be set"), + ): + with pytest.raises(MCPAuthConfigError): + _create_auth_provider(flask_app)