codeant-ai-for-open-source[bot] commented on code in PR #41003:
URL: https://github.com/apache/superset/pull/41003#discussion_r3531352942
##########
superset/mcp_service/auth.py:
##########
@@ -430,6 +490,33 @@ def _resolve_user_from_jwt_context(app: Any) -> User |
None:
# to the claim so that an external IdP JWT that happens to include the
# claim name is not misclassified as an API-key pass-through.
claims = getattr(access_token, "claims", None)
+
+ # Embedded guest token (already admitted by the GuestTokenVerifier):
resolve
+ # as the highest-priority identity so a valid guest is never downgraded.
+ # Anti-forgery: only the GuestTokenVerifier sets the marker (the composite
+ # verifier strips it from JWT tokens) and this branch requires guest auth
+ # enabled, so a crafted IdP JWT with the marker can't pose as a guest.
+ if (
+ isinstance(claims, dict)
+ and claims.get(GUEST_TOKEN_CLAIM)
+ and getattr(access_token, "client_id", None) == "guest"
+ ):
+ if not (
+ is_feature_enabled("EMBEDDED_SUPERSET")
+ and app.config.get("MCP_EMBEDDED_GUEST_AUTH_ENABLED", False)
+ ):
+ logger.warning(
+ "Guest-marked token presented but embedded guest auth is not "
+ "enabled; rejecting"
+ )
+ return None
+ logger.debug("Resolving MCP request as embedded guest user")
+ # Drop the internal marker so it does not leak into
GuestUser.guest_token.
+ guest_claims = {k: v for k, v in claims.items() if k !=
GUEST_TOKEN_CLAIM}
+ return security_manager.get_guest_user_from_token(
+ cast("GuestToken", guest_claims)
+ )
Review Comment:
**Suggestion:** Returning a `GuestUser` here breaks existing MCP call paths
that assume a database-backed user with a numeric `id` (for example
`created_by_me`/`owned_by_me` filtering and some tool writes that read
`g.user.id`), which will raise runtime attribute errors for guest-authenticated
requests. Keep guest resolution, but add a compatibility strategy before
returning this principal (either block guest access to `id`-dependent flows or
provide a safe user-id mapping/proxy) so downstream code does not crash. [api
mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Embedded guests saving SQL queries crash on AttributeError.
- ❌ Embedded guests generating dashboards crash when resolving dashboard
owner.
- ⚠️ Created_by_me filters stay safe via get_user_id helper.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable embedded guest authentication so the new guest flow is reachable:
set the
EMBEDDED_SUPERSET feature flag and Flask config
MCP_EMBEDDED_GUEST_AUTH_ENABLED to true,
then note that _resolve_user_from_jwt_context in
superset/mcp_service/auth.py (new guest
branch around diff lines 494–519) checks GUEST_TOKEN_CLAIM and returns
security_manager.get_guest_user_from_token(...), producing a GuestUser for
guest-marked
tokens.
2. Invoke an MCP tool that is decorated with mcp_auth_hook using a valid
Superset guest
token as the Bearer token, for example the save_sql_query tool implemented in
superset/mcp_service/sql_lab/tool/save_sql_query.py:19–31, by calling the
MCP server’s
call_tool entrypoint through FastMCP (the tool is registered and wrapped by
mcp_auth_hook
in superset/mcp_service/auth.py:83–260).
3. During the tool call, mcp_auth_hook’s async_wrapper or sync_wrapper in
superset/mcp_service/auth.py:124–201 enters _get_app_context_manager and
calls
_setup_user_context (auth.py:444–515), which in turn calls
get_user_from_request
(auth.py:298–368); with the guest token, get_user_from_request delegates to
_resolve_user_from_jwt_context (auth.py:56–185), where the guest-branch
strips
GUEST_TOKEN_CLAIM, calls
security_manager.get_guest_user_from_token(cast("GuestToken",
guest_claims)) at diff lines 515–518, and returns a GuestUser which
_setup_user_context
assigns to g.user.
4. When execution reaches the body of save_sql_query in
superset/mcp_service/sql_lab/tool/save_sql_query.py:19–30, the
SavedQueryDAO.create call
builds attributes={"user_id": g.user.id, ...}, and because GuestUser in
superset/security/guest_token.py:162–196 does not define an id attribute,
the g.user.id
access raises an AttributeError, causing the MCP tool call for
guest-authenticated
requests to fail; the same pattern appears in generate_dashboard at
superset/mcp_service/dashboard/tool/generate_dashboard.py:24–27, which
re-queries the
current_user by filtering db.session.query(...).filter_by(id=g.user.id), and
will also
raise AttributeError for GuestUser, confirming that returning GuestUser here
can break
existing MCP call paths that assume a database-backed user identifier (note
that
created_by_me filters in superset/dashboards/filters.py use get_user_id and
catch
AttributeError, so those specific filters remain safe while these direct
g.user.id usages
do not).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9c7a76241d6d4e13a2c8e8e451c0bb10&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=9c7a76241d6d4e13a2c8e8e451c0bb10&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 515:518
**Comment:**
*Api Mismatch: Returning a `GuestUser` here breaks existing MCP call
paths that assume a database-backed user with a numeric `id` (for example
`created_by_me`/`owned_by_me` filtering and some tool writes that read
`g.user.id`), which will raise runtime attribute errors for guest-authenticated
requests. Keep guest resolution, but add a compatibility strategy before
returning this principal (either block guest access to `id`-dependent flows or
provide a safe user-id mapping/proxy) so downstream code does not crash.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41003&comment_hash=35df63b46df17e49a6a72961442b6cf08ce4d24ac935c3769b737f4c21ed9682&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41003&comment_hash=35df63b46df17e49a6a72961442b6cf08ce4d24ac935c3769b737f4c21ed9682&reaction=dislike'>👎</a>
--
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]
