rebenitez1802 commented on PR #41003:
URL: https://github.com/apache/superset/pull/41003#issuecomment-4693691214
**Preset-side reconciliation notes** (from validating this against Preset's
downstream MCP deployment)
This aligns well with what we need — and it's cleaner than the shell-side
stand-in we'd been prototyping. The decisive compatibility win: verification
goes through `sm.parse_jwt_guest_token`, which Preset overrides to validate
Manager-minted **RS256/JWKS** tokens — so the verifier accepts our tokens
despite the core default being HS256/`GUEST_TOKEN_JWT_SECRET`. The namespaced
`_superset_mcp_guest_token` claim + `client_id == "guest"` check is also a nice
hardening over a raw `type == "guest"` check.
A few things the Preset wrapper still needs to reconcile when it adopts this
(flagging so the core API stays friendly to it):
1. **Injected JWT verifier.** Preset uses its own
`LifecycleAwareJWTVerifier`, not the stock JWT verifier. Does
`CompositeTokenVerifier` accept an **injected** `jwt_verifier` so a downstream
can keep its lifecycle verifier as the JWT path while still composing
`GuestTokenVerifier` first? If not, a small constructor hook would help.
2. **HTTP-layer gate ordering.** Preset wraps core with a Starlette
permission middleware that runs **before** FastMCP auth and currently 401s
guests on identity claims. That's downstream's problem to branch — just noting
the guest gets gated upstream of `GuestTokenVerifier`, so resolution-only
changes here don't fully unblock a wrapped deployment.
3. **Per-workspace audience.** Preset's guest `aud` is per-workspace
(multi-tenant), not a single shared `GUEST_TOKEN_JWT_AUDIENCE`. Since
validation delegates to `sm.parse_jwt_guest_token`, this works — but the
"`GUEST_TOKEN_JWT_AUDIENCE` is unset" startup warning may misfire for
multi-tenant SMs. Consider letting the SM signal it handles audience itself.
4. **Revocation.** It calls `sm._is_guest_token_revoked`; downstream SMs
that don't define it fall back to core's behavior. Worth documenting as an SM
extension point.
5. **Deny-list default.** `{find_users, get_instance_info}` matches the
confirmed enumeration leaks; we'd also add `get_schema` downstream (structural
metadata). Configurable already, so non-blocking — just flagging the choice.
None of these are blockers for this PR — they're downstream-integration
notes. Happy to help shape the `CompositeTokenVerifier` constructor if useful
for (1).
--
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]
