gabotorresruiz commented on code in PR #45639:
URL: https://github.com/apache/superset/pull/45639#discussion_r4364250554
##########
superset/mcp_service/auth.py:
##########
@@ -595,6 +596,40 @@ def load_user_with_relationships(
return security_manager.find_user_with_relationships(username=username,
email=email)
+def _reverify_guest_bearer(access_token: Any) -> dict[str, Any]:
+ """Re-verify a guest-marked token's raw bearer as a Superset guest token.
+
+ The guest marker and ``client_id == "guest"`` identify a token admitted by
+ ``GuestTokenVerifier``, but neither is bound to the bearer: a token source
+ that does not strip the marker (e.g. a verifier wrapped next to the
+ ``CompositeTokenVerifier`` in a FastMCP ``MultiAuth``) can pass both
through
+ from an external IdP JWT. Decoding the bearer with
``GUEST_TOKEN_JWT_SECRET``
+ binds the guest identity to a token only Superset can mint, and the decoded
+ claims (not the ``AccessToken`` copy) build the ``GuestUser``.
+
+ Raises:
+ ValueError: If the bearer is not a valid guest token (fail closed).
+ """
+ raw_token = getattr(access_token, "token", None)
+ parsed: Any = None
+ if isinstance(raw_token, str) and raw_token:
+ try:
+ parsed = security_manager.parse_jwt_guest_token(raw_token)
+ except Exception: # noqa: BLE001 — bad signature / audience / expiry
+ parsed = None
+ if (
+ not isinstance(parsed, dict)
+ or parsed.get("type") != "guest"
+ or parsed.get("user") is None
+ or parsed.get("resources") is None
+ or parsed.get("rls_rules") is None
+ or security_manager._is_guest_token_revoked(parsed) # noqa: SLF001
Review Comment:
Two notes on this condition list, neither blocking.
`exp` is the one thing `GuestTokenVerifier.verify_token` requires that this
does not: it defers at `guest_token_verifier.py:168` when the token has no
`exp`, while `parse_jwt_guest_token` only validates `exp` when it is present. I
minted a guest token with no `exp` claim on this branch and it resolved to a
`GuestUser`. Not an escalation, since signing it needs `GUEST_TOKEN_JWT_SECRET`
in the first place, but this function exists to be the authority exactly where
the guest verifier was bypassed, so it reads odd for it to be one check looser
than the verifier. `or not parsed.get("exp")` plus an `"exp"` case on
`test_guest_reverification_requires_core_structure` would close it.
The `type != "guest"` line is the only condition here with no test behind
it: I deleted it and `tests/unit_tests/mcp_service/` plus
`tests/unit_tests/security/` stayed green at 12069 passed. In core Superset it
is close to redundant, but a `CUSTOM_SECURITY_MANAGER` that overrides
`parse_jwt_guest_token` to verify against a key that also signs non-guest JWTs
makes this the only thing keeping a non-guest token out of the guest branch.
One more parametrize case pins it.
##########
superset/mcp_service/auth.py:
##########
@@ -665,17 +704,20 @@ def _resolve_user_from_jwt_context(app: Any) -> MCPUser |
None: # noqa: C901
raise ValueError(
"Guest-marked token presented but embedded guest auth is not
enabled"
)
+ guest_claims = _reverify_guest_bearer(access_token)
Review Comment:
Question rather than a finding: this now runs `parse_jwt_guest_token` and
`_is_guest_token_revoked` on every `_resolve_user_from_jwt_context`, so once
per tool call and once more for `tools/list`, where previously the revocation
lookup ran once per transport verification.
`_is_guest_token_revoked_by_embedded` does an `EmbeddedDashboardDAO.find_by_id`
with a `Dashboard.get` fallback per dashboard resource in the token, so a guest
token carrying several dashboards adds that many lookups per call. Is that a
deliberate trade for having the check be authoritative at resolution time, or
something you would rather scope to once per request?
##########
tests/unit_tests/mcp_service/test_multi_auth_client_id.py:
##########
@@ -0,0 +1,589 @@
+# 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.
+
+"""Identity resolution for tokens wrapped by FastMCP's ``MultiAuth``.
+
+FastMCP 4's ``MultiAuth`` rewrites ``AccessToken.client_id`` into a
+source-qualified JSON string and keeps the verifier's own value in
+``original_client_id``. These tests drive real verifier chains
+(``GuestTokenVerifier`` / ``CompositeTokenVerifier`` inside ``MultiAuth``) and
+check that guest and API-key tokens still resolve to the right principal, and
+that a forged guest-marked token still cannot pose as a guest.
+"""
+
+import time
+from collections.abc import Iterator
+from types import SimpleNamespace
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+import jwt
+import pytest
+from fastmcp.server.auth import AccessToken, MultiAuth
+from fastmcp.server.auth.providers.jwt import TokenVerifier
+
+from superset import security_manager
+from superset.app import SupersetApp
+from superset.mcp_service.auth import (
+ _resolve_user_from_api_key,
+ _resolve_user_from_jwt_context,
+)
+from superset.mcp_service.composite_token_verifier import (
+ API_KEY_PASSTHROUGH_CLAIM,
+ API_KEY_VALIDATED_USERNAME_CLAIM,
+ CompositeTokenVerifier,
+)
+from superset.mcp_service.guest_token_verifier import (
+ GUEST_TOKEN_CLAIM,
+ GuestTokenVerifier,
+)
+from superset.mcp_service.mcp_config import default_user_resolver
+
+GUEST_SECRET = "a-strong-guest-token-secret-for-tests" # noqa: S105
+
+# FastMCP 4's MultiAuth qualifies client_id; FastMCP 3's passes it through.
+MULTI_AUTH_QUALIFIES_CLIENT_ID = "original_client_id" in
AccessToken.model_fields
+
+
[email protected](autouse=True)
+def isolate_identity_config(app: SupersetApp) -> Iterator[None]:
+ """Restore the module-scoped app's auth configuration after each test."""
+ with patch.dict(app.config):
+ yield
+
+
+class _StubVerifier(TokenVerifier):
+ """A token source that returns a fixed AccessToken for any bearer."""
+
+ def __init__(self, result: AccessToken | None) -> None:
+ super().__init__(base_url=None, required_scopes=[])
+ self._result = result
+
+ async def verify_token(self, token: str) -> AccessToken | None:
+ return self._result
+
+
+def _guest_claims() -> dict[str, Any]:
+ return {
+ "user": {"username": "embed-guest"},
+ "resources": [{"type": "dashboard", "id": "abc-uuid"}],
+ "rls_rules": [],
+ "iat": int(time.time()),
+ "exp": int(time.time()) + 300,
+ "aud": "superset-test",
+ "type": "guest",
+ }
+
+
+def _mint_guest_token(claims: dict[str, Any], secret: str = GUEST_SECRET) ->
str:
+ return jwt.encode(claims, secret, algorithm="HS256")
+
+
[email protected]
+def guest_app(app: SupersetApp) -> SupersetApp:
+ """Enable embedded guest auth over MCP with a known guest-token secret."""
+ app.config.update(
+ {
+ "MCP_EMBEDDED_GUEST_AUTH_ENABLED": True,
+ "GUEST_TOKEN_JWT_SECRET": GUEST_SECRET,
+ "GUEST_TOKEN_JWT_ALGO": "HS256",
+ "GUEST_TOKEN_JWT_AUDIENCE": "superset-test",
+ }
+ )
+ return app
+
+
+async def _verify_through_chain(
+ app: SupersetApp, raw_token: str, *, multi_auth: bool
+) -> AccessToken | None:
+ """Run ``raw_token`` through the default guest chain, optionally in
MultiAuth."""
+ composite = CompositeTokenVerifier(
+ jwt_verifier=None,
+ api_key_prefixes=[],
+ app=app,
+ guest_verifier=GuestTokenVerifier(app=app),
+ )
+ verifier: Any = MultiAuth(verifiers=[composite]) if multi_auth else
composite
+ sm = app.appbuilder.sm
+ with (
+ patch("superset.is_feature_enabled", return_value=True),
+ patch.object(sm, "_is_guest_token_revoked", return_value=False),
+ patch.object(sm, "find_role", return_value=object()),
+ ):
+ return await verifier.verify_token(raw_token)
+
+
+def _resolve_guest(app: SupersetApp, access_token: Any) -> tuple[Any,
MagicMock]:
+ """Resolve ``access_token`` via the JWT-context path, guest flags on."""
+ fake_guest = MagicMock(name="GuestUser")
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=access_token
+ ),
+ patch("superset.mcp_service.auth.is_feature_enabled",
return_value=True),
+ patch.object(security_manager, "_is_guest_token_revoked",
return_value=False),
+ patch.object(
+ security_manager, "get_guest_user_from_token",
return_value=fake_guest
+ ) as get_guest_user,
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=None,
+ ),
+ ):
+ return _resolve_user_from_jwt_context(app), get_guest_user
+
+
+# -- guest tokens --
+
+
[email protected]("multi_auth", [False, True], ids=["plain",
"multi_auth"])
[email protected]
+async def test_guest_token_resolves_as_guest(
+ guest_app: SupersetApp, multi_auth: bool
+) -> None:
+ """A real guest token resolves to a GuestUser whether or not the verifier
+ chain is wrapped in ``MultiAuth`` (which qualifies ``client_id``)."""
+ claims = _guest_claims()
+ access_token = await _verify_through_chain(
+ guest_app, _mint_guest_token(claims), multi_auth=multi_auth
+ )
+ assert access_token is not None
+ if multi_auth and MULTI_AUTH_QUALIFIES_CLIENT_ID:
+ # The precondition for the bug: MultiAuth rewrote client_id.
+ assert access_token.client_id != "guest"
+ assert access_token.original_client_id == "guest"
+
+ result, get_guest_user = _resolve_guest(guest_app, access_token)
+
+ assert result is get_guest_user.return_value
+ passed_claims = get_guest_user.call_args.args[0]
+ assert GUEST_TOKEN_CLAIM not in passed_claims
+ assert passed_claims == claims
+
+
[email protected]("multi_auth", [False, True], ids=["plain",
"multi_auth"])
[email protected]
+async def test_forged_guest_marker_cannot_pose_as_guest(
+ guest_app: SupersetApp, multi_auth: bool
+) -> None:
+ """A token that carries the guest marker and ``client_id="guest"`` but
whose
+ bearer is not a guest token signed with ``GUEST_TOKEN_JWT_SECRET`` (e.g. an
+ external IdP JWT returned by a source that does not strip the marker) must
+ be rejected, not resolved as a guest."""
+ forged_claims = _guest_claims()
+ forged_raw = _mint_guest_token(forged_claims, secret="idp-signing-key") #
noqa: S106
+ forged = AccessToken(
+ token=forged_raw,
+ client_id="guest",
+ scopes=[],
+ claims={GUEST_TOKEN_CLAIM: True, **forged_claims},
+ )
+ access_token: Any = forged
+ if multi_auth:
+ access_token = await
MultiAuth(verifiers=[_StubVerifier(forged)]).verify_token(
+ forged_raw
+ )
+ assert access_token is not None
+
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=access_token
+ ),
+ patch("superset.mcp_service.auth.is_feature_enabled",
return_value=True),
+ patch.object(security_manager, "get_guest_user_from_token") as
get_guest_user,
+ pytest.raises(ValueError, match="not a valid guest token"),
+ ):
+ _resolve_user_from_jwt_context(guest_app)
+ get_guest_user.assert_not_called()
+
+
+def test_guest_resolution_uses_verified_claims(guest_app: SupersetApp) -> None:
+ """The GuestUser is built from the re-verified bearer, so claims edited on
+ the AccessToken after verification cannot widen the guest's resources."""
+ claims = _guest_claims()
+ tampered = AccessToken(
+ token=_mint_guest_token(claims),
+ client_id="guest",
+ scopes=[],
+ claims={
+ GUEST_TOKEN_CLAIM: True,
+ **claims,
+ "resources": [{"type": "dashboard", "id": "someone-elses"}],
+ },
+ )
+
+ _, get_guest_user = _resolve_guest(guest_app, tampered)
+
+ assert get_guest_user.call_args.args[0]["resources"] == claims["resources"]
+
+
+# -- API-key tokens --
+
+
[email protected]("multi_auth", [False, True])
[email protected]
+async def test_api_key_fast_path_through_multi_auth(
+ app: SupersetApp, multi_auth: bool
+) -> None:
+ """A transport-validated API key wrapped by ``MultiAuth`` defers from the
+ JWT path and loads the validated user on the API-key path."""
+ composite = CompositeTokenVerifier(
+ jwt_verifier=None, api_key_prefixes=["sst_"], app=app
+ )
+ with patch.object(composite, "_validate_api_key_sync",
return_value=("alice", [])):
+ verifier: Any = MultiAuth(verifiers=[composite]) if multi_auth else
composite
+ access_token = await verifier.verify_token("sst_abc123")
+ assert access_token is not None
+ # Editing the JWT-style claims must not alter the FAB-validated identity.
+ access_token.claims[API_KEY_VALIDATED_USERNAME_CLAIM] = "admin"
+ if multi_auth and MULTI_AUTH_QUALIFIES_CLIENT_ID:
+ assert access_token.client_id != "api_key"
+
+ alice = MagicMock(username="alice")
+ app.config["FAB_API_KEY_ENABLED"] = True
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=access_token
+ ),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=alice,
+ ) as load_user,
+ patch.object(app.appbuilder.sm, "validate_api_key") as validate,
+ ):
+ assert _resolve_user_from_jwt_context(app) is None
+ assert _resolve_user_from_api_key(app) is alice
+ load_user.assert_called_once_with(username="alice")
+ validate.assert_not_called()
+
+
[email protected]
+async def test_api_key_fallback_through_multi_auth(app: SupersetApp) -> None:
+ """A prefix-only API-key pass-through (no transport validation) wrapped by
+ ``MultiAuth`` is still validated against FAB on the API-key path."""
+ composite = CompositeTokenVerifier(
+ jwt_verifier=None, api_key_prefixes=["sst_"], app=None
+ )
+ access_token = await
MultiAuth(verifiers=[composite]).verify_token("sst_abc123")
+ assert access_token is not None
+
+ alice = MagicMock(username="alice")
+ app.config["FAB_API_KEY_ENABLED"] = True
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=access_token
+ ),
+ patch.object(
+ app.appbuilder.sm, "validate_api_key", return_value=alice,
create=True
+ ) as validate,
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=alice,
+ ),
+ ):
+ assert _resolve_user_from_jwt_context(app) is None
+ assert _resolve_user_from_api_key(app) is alice
+ validate.assert_called_once_with("sst_abc123")
+
+
+# -- default user resolver and helper --
+
+
[email protected]
+async def test_default_user_resolver_uses_unqualified_client_id(
+ app: SupersetApp,
+) -> None:
+ """The last-resort ``client_id`` username fallback sees the verifier's own
+ client id, not ``MultiAuth``'s qualified JSON form."""
+ token = AccessToken(token="t", client_id="svc-bot", scopes=[], claims={})
# noqa: S106
+ wrapped = await
MultiAuth(verifiers=[_StubVerifier(token)]).verify_token("t")
+ assert wrapped is not None
+
+ assert default_user_resolver(app, wrapped) == "svc-bot"
+
+
+def test_get_verifier_client_id_without_original_attribute() -> None:
+ """Tokens without ``original_client_id`` (FastMCP 3) use ``client_id``."""
+ from superset.mcp_service.composite_token_verifier import
get_verifier_client_id
+
+ assert get_verifier_client_id(SimpleNamespace(client_id="guest")) ==
"guest"
+ assert get_verifier_client_id(MagicMock(client_id="guest")) == "guest"
+ assert (
+ get_verifier_client_id(
+ SimpleNamespace(
+ client_id='["src","guest",null,null]',
original_client_id="guest"
+ )
+ )
+ == "guest"
+ )
+ assert get_verifier_client_id(SimpleNamespace()) is None
+
+
[email protected](
+ "chain", ["plain", "composite", "external_first", "composite_first"]
+)
[email protected]
+async def test_idp_api_key_claims_cannot_select_another_user(
+ app: SupersetApp, chain: str
+) -> None:
+ """Internal API-key claims in a signed IdP JWT are not validation
evidence."""
+ from fastmcp.server.auth.providers.jwt import JWTVerifier
+
+ secret = "independent-idp-signing-secret-for-tests" # noqa: S105
+ raw = jwt.encode(
+ {
+ "sub": "alice",
+ "client_id": "api_key",
+ "original_client_id": "api_key",
+ "exp": int(time.time()) + 300,
+ API_KEY_PASSTHROUGH_CLAIM: True,
+ API_KEY_VALIDATED_USERNAME_CLAIM: "admin",
+ },
+ secret,
+ algorithm="HS256",
+ )
+ external = JWTVerifier(public_key=secret, algorithm="HS256")
+ composite = CompositeTokenVerifier(external, ["sst_"], app=app)
+ verifier: Any = external
+ if chain == "composite":
+ verifier = composite
+ elif chain == "external_first":
+ verifier = MultiAuth(verifiers=[external, composite])
+ elif chain == "composite_first":
+ verifier = MultiAuth(verifiers=[composite, external])
+ access_token = await verifier.verify_token(raw)
+ assert access_token is not None
+ app.config["FAB_API_KEY_ENABLED"] = True
+ alice = MagicMock(username="alice")
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=access_token
+ ),
+ patch.object(
+ app.appbuilder.sm, "validate_api_key", return_value=None
+ ) as validate,
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
return_value=alice
+ ) as load,
+ ):
+ # A stripped token retains its JWT identity; an unwrapped external
+ # source's forged API-key assertion must fail closed at FAB validation.
+ user = _resolve_user_from_jwt_context(app)
+ if user is None:
+ with pytest.raises(PermissionError, match="Invalid or expired API
key"):
+ _resolve_user_from_api_key(app)
+ validate.assert_called_once_with(raw)
+ load.assert_not_called()
+ else:
+ assert user is alice
+ load.assert_called_once_with("alice")
+ validate.assert_not_called()
+
+
[email protected]("revocation", ["version", "dashboard"])
[email protected]("chain", ["plain", "guest_first", "external_first"])
[email protected]
+async def test_revoked_guest_cannot_bypass_guest_verifier(
+ guest_app: SupersetApp, chain: str, revocation: str
+) -> None:
+ """A different token source cannot bypass core guest-token revocation."""
+ claims = _guest_claims()
+ raw = _mint_guest_token(claims)
+ marked = AccessToken(
+ token=raw,
+ client_id="guest",
+ scopes=[],
+ claims={GUEST_TOKEN_CLAIM: True, **claims},
+ )
+ verifier: Any = _StubVerifier(marked)
+ if chain != "plain":
+ sources = [GuestTokenVerifier(guest_app), verifier]
+ verifier = MultiAuth(
+ verifiers=sources if chain == "guest_first" else sources[::-1]
+ )
+ guest_app.config["GUEST_TOKEN_REVOCATION_ENABLED"] = revocation ==
"version"
+ cutoff = SimpleNamespace(guest_token_revoked_before=int(time.time()) + 1)
+ with (
+ patch("superset.is_feature_enabled", return_value=True),
+ patch(
+
"superset.security.manager.get_current_guest_token_revocation_version",
+ return_value=1,
+ ),
+ patch(
+ "superset.daos.dashboard.EmbeddedDashboardDAO.find_by_id",
+ return_value=cutoff,
+ ),
+ patch.object(guest_app.appbuilder.sm, "find_role",
return_value=object()),
+ ):
+ access_token = await verifier.verify_token(raw)
+ assert access_token is not None
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
+ return_value=access_token,
+ ),
+ patch("superset.mcp_service.auth.is_feature_enabled",
return_value=True),
+ patch.object(security_manager, "get_guest_user_from_token") as
build,
+ pytest.raises(ValueError, match="not a valid guest token"),
+ ):
+ _resolve_user_from_jwt_context(guest_app)
+ build.assert_not_called()
+
+
[email protected]("client_id", ["guest", "api_key"])
+def test_crafted_original_client_id_is_not_validation_evidence(
+ guest_app: SupersetApp, client_id: str
+) -> None:
+ """An original_client_id field cannot authorize either internal
identity."""
+ raw = _mint_guest_token(_guest_claims(), secret="independent-idp-secret")
# noqa: S106
+ token = SimpleNamespace(
+ token=raw,
+ client_id="external",
+ original_client_id=client_id,
+ claims={
+ GUEST_TOKEN_CLAIM: True,
+ API_KEY_PASSTHROUGH_CLAIM: True,
+ API_KEY_VALIDATED_USERNAME_CLAIM: "admin",
+ },
+ )
+ guest_app.config["FAB_API_KEY_ENABLED"] = True
+ with (
+ patch("fastmcp.server.dependencies.get_access_token",
return_value=token),
+ patch("superset.mcp_service.auth.is_feature_enabled",
return_value=True),
+ patch.object(guest_app.appbuilder.sm, "validate_api_key",
return_value=None),
+ patch("superset.mcp_service.auth.load_user_with_relationships") as
load,
+ patch.object(security_manager, "get_guest_user_from_token") as build,
+ ):
+ if client_id == "guest":
+ with pytest.raises(ValueError, match="not a valid guest token"):
+ _resolve_user_from_jwt_context(guest_app)
+ else:
+ assert _resolve_user_from_jwt_context(guest_app) is None
+ with pytest.raises(PermissionError, match="Invalid or expired API
key"):
+ _resolve_user_from_api_key(guest_app)
+ load.assert_not_called()
+ build.assert_not_called()
+
+
[email protected]("missing", ["user", "resources", "rls_rules"])
+def test_guest_reverification_requires_core_structure(
+ guest_app: SupersetApp, missing: str
+) -> None:
+ """Other token sources cannot bypass the embedded flow's structural
checks."""
+ claims = _guest_claims()
+ del claims[missing]
+ token = AccessToken(
+ token=_mint_guest_token(claims),
+ client_id="guest",
+ scopes=[],
+ claims={GUEST_TOKEN_CLAIM: True, **claims},
+ )
+ with pytest.raises(ValueError, match="not a valid guest token"):
+ _resolve_guest(guest_app, token)
+
+
[email protected]("fallback", ["dev", "middleware"])
[email protected]("multi_auth", [False, True])
[email protected](
+ "disabled_setting", ["FAB_API_KEY_ENABLED", "MCP_API_KEY_ENABLED"]
+)
[email protected]
+async def test_disabled_api_key_assertion_cannot_fall_through_to_dev_user(
+ app: SupersetApp, multi_auth: bool, disabled_setting: str, fallback: str
+) -> None:
+ """An explicit API-key assertion must not degrade to a development
identity."""
+ from superset.mcp_service.auth import get_user_from_request
+
+ token = AccessToken(
+ token="external-bearer", # noqa: S106
+ client_id="api_key",
+ scopes=[],
+ claims={
+ API_KEY_PASSTHROUGH_CLAIM: True,
+ API_KEY_VALIDATED_USERNAME_CLAIM: "admin",
+ },
+ )
+ if multi_auth:
+ token = await MultiAuth(verifiers=[_StubVerifier(token)]).verify_token(
+ token.token
+ )
+ app.config.update(
+ FAB_API_KEY_ENABLED=True,
+ MCP_API_KEY_ENABLED=None,
+ MCP_DEV_USERNAME="admin" if fallback == "dev" else None,
+ )
+ app.config[disabled_setting] = False
+ with (
+ patch("superset.mcp_service.auth.g",
SimpleNamespace(user=MagicMock())),
+ patch("fastmcp.server.dependencies.get_access_token",
return_value=token),
+ patch("superset.mcp_service.auth.load_user_with_relationships") as
load,
+ pytest.raises(PermissionError, match="Authentication required"),
+ ):
+ get_user_from_request()
+ load.assert_not_called()
+
+
[email protected]
+async def test_serialized_api_key_token_revalidates_bearer(app: SupersetApp)
-> None:
+ """Losing the internal token type requires FAB validation, not claim
trust."""
+ composite = CompositeTokenVerifier(None, ["sst_"], app=app)
+ with patch.object(composite, "_validate_api_key_sync",
return_value=("alice", [])):
+ verified = await composite.verify_token("sst_abc123")
+ assert verified is not None
+ serialized = AccessToken.model_validate(verified.model_dump())
+ serialized.claims[API_KEY_VALIDATED_USERNAME_CLAIM] = "admin"
+ app.config["FAB_API_KEY_ENABLED"] = True
+ alice = MagicMock(username="alice")
+ with (
+ patch("fastmcp.server.dependencies.get_access_token",
return_value=serialized),
+ patch.object(
+ app.appbuilder.sm, "validate_api_key", return_value=alice
+ ) as validate,
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
return_value=alice
+ ) as load,
+ ):
+ assert _resolve_user_from_jwt_context(app) is None
+ assert _resolve_user_from_api_key(app) is alice
+ validate.assert_called_once_with("sst_abc123")
+ load.assert_called_once_with(username="alice")
+
+
[email protected]("entrypoint", ["tool_list", "tool_search"])
+def test_disabled_api_key_assertion_hides_tools(
+ app: SupersetApp, entrypoint: str
+) -> None:
+ """Discovery distinguishes rejected credentials from absent
authentication."""
+ from superset.mcp_service.middleware import RBACToolVisibilityMiddleware
Review Comment:
Not a blocker, but this import makes the test order-dependent.
`middleware.py:51` binds `get_user_from_request` at module import time, and
`test_auth_user_resolution.py::test_mcp_auth_hook_clears_stale_g_user` patches
`superset.mcp_service.auth.get_user_from_request` while `worker.py:366` imports
`middleware` for the first time, so `middleware.get_user_from_request` stays a
`MagicMock` once that test has run. `_visible_tools` then gets a user back
instead of the `PermissionError` and fails open:
```bash
pytest tests/unit_tests/mcp_service/test_auth_user_resolution.py \
tests/unit_tests/mcp_service/test_multi_auth_client_id.py
# 1 failed, 70 passed --
test_disabled_api_key_assertion_hides_tools[tool_list]
```
Moving `RBACToolVisibilityMiddleware` up to the module-level imports fixes
it (71 passed), because collection then binds the name before any test patches
it. Worth doing since CI runs `-n auto --dist loadfile`, so which files end up
on one worker is the scheduler's choice.
##########
docs/admin_docs/configuration/mcp-server.mdx:
##########
@@ -661,6 +661,7 @@ curl -X POST http://localhost:5008/mcp \
- A dedicated guest-token verifier validates the token against the same
`GUEST_TOKEN_JWT_SECRET` / `GUEST_TOKEN_JWT_ALGO` / `GUEST_TOKEN_JWT_AUDIENCE`
config used by embedded dashboards, replays the embedded structural checks, and
enforces revocation (global version bumps and per-dashboard
`guest_token_revoked_before` cutoffs). It runs *before* the JWT verifier
described above, since guest tokens are signed with a different key/algorithm
and would otherwise be rejected at the transport.
- A verified guest resolves to a Superset guest user as the highest-priority
identity, so it's never downgraded to API-key / `MCP_DEV_USERNAME` / dev-mode
resolution. Data access is scoped by the same checks (dataset allowlist,
dashboard access, row-level security) that apply to embedded dashboard views.
+- Guest and API-key identity resolution supports FastMCP 3 and FastMCP 4,
including verifier chains wrapped in `MultiAuth`. Guest resolution revalidates
the bearer and core revocation checks; API-key resolution trusts only FAB
validation, not identity-routing claims supplied by an external JWT.
Review Comment:
Should this get an `UPDATING.md` entry under `## Next` as well? Two things
in here change behaviour for an operator who will never read the PR description:
- A custom `MCP_USER_RESOLVER` that reads `access_token.client_id` now sees
`MultiAuth`'s qualified JSON form on FastMCP 4 and has to move to
`get_verifier_client_id`. `MCP_USER_RESOLVER` is a documented hook and the
table at line 984 still describes the old contract.
- With API keys disabled, an API-key pass-through token is now a
`PermissionError` instead of a fall-through, and `tools/list` returns an empty
list for that client rather than the public tools.
`test_api_key_disabled_skips_auth` turning into
`test_api_key_disabled_rejects_explicit_auth` is the clearest statement of it
in the diff.
Both are the right behaviour, I would just like the second one written down
somewhere an upgrader will see it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
