codeant-ai-for-open-source[bot] commented on code in PR #38747:
URL: https://github.com/apache/superset/pull/38747#discussion_r2961313357
##########
superset/mcp_service/auth.py:
##########
@@ -172,62 +172,121 @@ def load_user_with_relationships(
return query.first()
-def get_user_from_request() -> User:
+def _resolve_user_from_jwt_context(app: Any) -> User | None:
"""
- Get the current user for the MCP tool request.
+ Resolve the current user from the MCP SDK's per-request JWT context.
- Priority order:
- 1. g.user if already set (by Preset workspace middleware)
- 2. MCP_DEV_USERNAME from configuration (for development/testing)
+ Uses FastMCP's ``get_access_token()`` which returns the JWT AccessToken
+ for the current async task via a ContextVar — safe across concurrent
+ requests, unlike ``g.user`` which can be stale.
+
+ The username is extracted from token claims using a configurable resolver
+ (``MCP_USER_RESOLVER`` config) or the default ``default_user_resolver()``.
Returns:
- User object with roles and groups eagerly loaded
+ User object with relationships loaded, or None if no JWT context
+ (i.e. no token present — caller should fall through to next source).
Raises:
- ValueError: If user cannot be authenticated or found
+ ValueError: If JWT resolves a username that doesn't exist in the DB
+ (fail closed — do NOT fall through to weaker auth sources).
"""
- from flask import current_app
+ try:
+ from fastmcp.server.dependencies import get_access_token
+ except ImportError:
+ logger.debug("fastmcp.server.dependencies not available, skipping JWT
context")
+ return None
- # First check if user is already set by Preset workspace middleware
- if hasattr(g, "user") and g.user:
- return g.user
+ access_token = get_access_token()
+ if access_token is None:
+ return None
- # Fall back to configured username for development/single-user deployments
- username = current_app.config.get("MCP_DEV_USERNAME")
+ # Use configurable resolver or default
+ from superset.mcp_service.mcp_config import default_user_resolver
+
+ resolver = app.config.get("MCP_USER_RESOLVER", default_user_resolver)
+ username = resolver(app, access_token)
if not username:
- auth_enabled = current_app.config.get("MCP_AUTH_ENABLED", False)
- jwt_configured = bool(
- current_app.config.get("MCP_JWKS_URI")
- or current_app.config.get("MCP_JWT_PUBLIC_KEY")
- or current_app.config.get("MCP_JWT_SECRET")
- )
- details = []
- details.append(
- f"g.user was not set by JWT middleware "
- f"(MCP_AUTH_ENABLED={auth_enabled}, "
- f"JWT keys configured={jwt_configured})"
- )
- details.append("MCP_DEV_USERNAME is not configured")
- raise ValueError(
- "No authenticated user found. Tried:\n"
- + "\n".join(f" - {d}" for d in details)
- + "\n\nEither pass a valid JWT bearer token or configure "
- "MCP_DEV_USERNAME for development."
+ logger.warning(
+ "JWT context present but no username could be extracted from
claims"
)
+ return None
Review Comment:
**Suggestion:** When a JWT is present but no username can be extracted, the
function currently returns `None` and falls through to weaker auth sources
(`MCP_DEV_USERNAME` or `g.user`), which can lead to unintended privilege
elevation. Fail closed when token identity extraction fails. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Invalid JWT identity may execute as dev user.
- ❌ Weak fallback can override intended JWT identity checks.
- ⚠️ Security boundary between auth sources becomes ambiguous.
```
</details>
```suggestion
raise ValueError(
"JWT context present but no username could be extracted from
claims"
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger any protected tool call (wrapping path:
`superset/core/mcp/core_mcp_injection.py:127-129` ->
`superset/mcp_service/auth.py:362`).
2. Provide a JWT context where token exists but resolver returns no identity
(current
behavior explicitly tested in
`tests/unit_tests/mcp_service/test_auth_user_resolution.py:106-114`).
3. `_resolve_user_from_jwt_context()` logs warning and returns `None`
(`superset/mcp_service/auth.py:210-214`) instead of failing closed.
4. `get_user_from_request()` then falls through to weaker sources
(`MCP_DEV_USERNAME` at
`255-263`, then `g.user` at `265-266`), allowing authentication by fallback
despite JWT
identity extraction failure.
```
</details>
<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:** 211:214
**Comment:**
*Security: When a JWT is present but no username can be extracted, the
function currently returns `None` and falls through to weaker auth sources
(`MCP_DEV_USERNAME` or `g.user`), which can lead to unintended privilege
elevation. Fail closed when token identity extraction fails.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=6e0657630fe70fe6662590f52213657d39092ebe9203e2099991cae360e079ab&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=6e0657630fe70fe6662590f52213657d39092ebe9203e2099991cae360e079ab&reaction=dislike'>👎</a>
##########
superset/mcp_service/auth.py:
##########
@@ -172,62 +172,121 @@ def load_user_with_relationships(
return query.first()
-def get_user_from_request() -> User:
+def _resolve_user_from_jwt_context(app: Any) -> User | None:
"""
- Get the current user for the MCP tool request.
+ Resolve the current user from the MCP SDK's per-request JWT context.
- Priority order:
- 1. g.user if already set (by Preset workspace middleware)
- 2. MCP_DEV_USERNAME from configuration (for development/testing)
+ Uses FastMCP's ``get_access_token()`` which returns the JWT AccessToken
+ for the current async task via a ContextVar — safe across concurrent
+ requests, unlike ``g.user`` which can be stale.
+
+ The username is extracted from token claims using a configurable resolver
+ (``MCP_USER_RESOLVER`` config) or the default ``default_user_resolver()``.
Returns:
- User object with roles and groups eagerly loaded
+ User object with relationships loaded, or None if no JWT context
+ (i.e. no token present — caller should fall through to next source).
Raises:
- ValueError: If user cannot be authenticated or found
+ ValueError: If JWT resolves a username that doesn't exist in the DB
+ (fail closed — do NOT fall through to weaker auth sources).
"""
- from flask import current_app
+ try:
+ from fastmcp.server.dependencies import get_access_token
+ except ImportError:
+ logger.debug("fastmcp.server.dependencies not available, skipping JWT
context")
+ return None
- # First check if user is already set by Preset workspace middleware
- if hasattr(g, "user") and g.user:
- return g.user
+ access_token = get_access_token()
+ if access_token is None:
+ return None
- # Fall back to configured username for development/single-user deployments
- username = current_app.config.get("MCP_DEV_USERNAME")
+ # Use configurable resolver or default
+ from superset.mcp_service.mcp_config import default_user_resolver
+
+ resolver = app.config.get("MCP_USER_RESOLVER", default_user_resolver)
+ username = resolver(app, access_token)
if not username:
- auth_enabled = current_app.config.get("MCP_AUTH_ENABLED", False)
- jwt_configured = bool(
- current_app.config.get("MCP_JWKS_URI")
- or current_app.config.get("MCP_JWT_PUBLIC_KEY")
- or current_app.config.get("MCP_JWT_SECRET")
- )
- details = []
- details.append(
- f"g.user was not set by JWT middleware "
- f"(MCP_AUTH_ENABLED={auth_enabled}, "
- f"JWT keys configured={jwt_configured})"
- )
- details.append("MCP_DEV_USERNAME is not configured")
- raise ValueError(
- "No authenticated user found. Tried:\n"
- + "\n".join(f" - {d}" for d in details)
- + "\n\nEither pass a valid JWT bearer token or configure "
- "MCP_DEV_USERNAME for development."
+ logger.warning(
+ "JWT context present but no username could be extracted from
claims"
)
+ return None
- # Use helper function to load user with all required relationships
user = load_user_with_relationships(username)
Review Comment:
**Suggestion:** The resolver can return an email (via
`default_user_resolver`), but this code only looks up by username, so valid JWT
users identified by email are incorrectly rejected. Add an email fallback
lookup before failing closed. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Email-identified JWT users are rejected as nonexistent.
- ❌ MCP tool access denied despite valid authentication.
- ⚠️ Common OIDC email-only claims become unusable.
```
</details>
```suggestion
user = load_user_with_relationships(username=username)
if not user and "@" in username:
user = load_user_with_relationships(email=username)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Send an authenticated MCP tool request so `mcp_auth_hook` resolves user
through
`get_user_from_request()` (`superset/mcp_service/auth.py:251-252`).
2. Use a token where resolver output is email (default resolver prefers
`email` when
`sub`/`preferred_username` absent:
`superset/mcp_service/mcp_config.py:321-336`; validated
by test `tests/unit_tests/mcp_service/test_auth_user_resolution.py:297-300`).
3. `_resolve_user_from_jwt_context()` calls
`load_user_with_relationships(username)`
(`superset/mcp_service/auth.py:216`), and that query filters only
`User.username ==
username` when `username` arg is passed
(`superset/mcp_service/auth.py:167-170`).
4. If Superset user has different username but matching email, lookup misses
and raises
`ValueError` (`superset/mcp_service/auth.py:217-223`), denying valid JWT
user access to
tools.
```
</details>
<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:** 216:216
**Comment:**
*Logic Error: The resolver can return an email (via
`default_user_resolver`), but this code only looks up by username, so valid JWT
users identified by email are incorrectly rejected. Add an email fallback
lookup before failing closed.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=707d023d0cdc52558ab4da545772ad8121bfb26a5815123eba6b9e4c4036074d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=707d023d0cdc52558ab4da545772ad8121bfb26a5815123eba6b9e4c4036074d&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/test_auth_user_resolution.py:
##########
@@ -0,0 +1,330 @@
+# 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.
+
+"""Tests for MCP user resolution priority and stale g.user prevention."""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+from flask import g
+
+from superset.mcp_service.auth import (
+ _resolve_user_from_jwt_context,
+ get_user_from_request,
+ mcp_auth_hook,
+)
+from superset.mcp_service.mcp_config import default_user_resolver
+
+
+def _make_mock_user(username: str = "testuser") -> MagicMock:
+ """Create a mock User with required attributes."""
+ user = MagicMock()
+ user.username = username
+ user.roles = []
+ user.groups = []
+ return user
+
+
+def _make_access_token(
+ claims: dict[str, str] | None = None, **kwargs: str
+) -> MagicMock:
+ """Create a mock AccessToken matching FastMCP's format."""
+ token = MagicMock()
+ token.claims = claims or {}
+ token.client_id = kwargs.get("client_id", "")
+ token.scopes = kwargs.get("scopes", [])
+ # Remove auto-created attributes so getattr fallbacks work correctly
+ for attr in ("subject", "payload"):
+ if attr not in kwargs:
+ delattr(token, attr)
+ for attr in kwargs:
+ setattr(token, attr, kwargs[attr])
+ return token
+
+
+# -- _resolve_user_from_jwt_context --
+
+
+def test_jwt_context_resolves_correct_user(app) -> None:
+ """JWT context with valid claims resolves the correct DB user."""
+ mock_user = _make_mock_user("alice")
+ token = _make_access_token(claims={"sub": "alice"})
+
+ with app.app_context():
+ with (
+ patch("fastmcp.server.dependencies.get_access_token",
return_value=token),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=mock_user,
+ ),
+ ):
+ result = _resolve_user_from_jwt_context(app)
+
+ assert result is not None
+ assert result.username == "alice"
+
+
+def test_jwt_context_returns_none_when_no_token(app) -> None:
+ """No JWT token present returns None (fall through to next source)."""
+ with app.app_context():
+ with patch("fastmcp.server.dependencies.get_access_token",
return_value=None):
+ result = _resolve_user_from_jwt_context(app)
+
+ assert result is None
+
+
+def test_jwt_context_raises_for_unknown_user(app) -> None:
+ """JWT resolves a username not in DB — raises ValueError (fail closed)."""
+ token = _make_access_token(claims={"sub": "nonexistent"})
+
+ with app.app_context():
+ with (
+ patch("fastmcp.server.dependencies.get_access_token",
return_value=token),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=None,
+ ),
+ ):
+ with pytest.raises(ValueError, match="not found in Superset
database"):
+ _resolve_user_from_jwt_context(app)
+
+
+def test_jwt_context_returns_none_when_no_username_in_claims(app) -> None:
+ """JWT present but claims have no extractable username — returns None."""
+ token = _make_access_token(claims={"iss": "some-issuer"})
+
+ with app.app_context():
+ with patch("fastmcp.server.dependencies.get_access_token",
return_value=token):
+ result = _resolve_user_from_jwt_context(app)
+
+ assert result is None
+
+
+def test_jwt_context_uses_custom_resolver(app) -> None:
+ """Custom MCP_USER_RESOLVER config is used when set."""
+ mock_user = _make_mock_user("custom_user")
+ token = _make_access_token(claims={"custom_field": "custom_user"})
+ custom_resolver = MagicMock(return_value="custom_user")
+
+ with app.app_context():
+ app.config["MCP_USER_RESOLVER"] = custom_resolver
+ try:
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=token
+ ),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=mock_user,
+ ),
+ ):
+ result = _resolve_user_from_jwt_context(app)
+ finally:
+ app.config.pop("MCP_USER_RESOLVER", None)
+
+ assert result is not None
+ assert result.username == "custom_user"
+ custom_resolver.assert_called_once_with(app, token)
+
+
+# -- get_user_from_request priority order --
+
+
+def test_jwt_takes_priority_over_stale_g_user(app) -> None:
+ """Core regression test: JWT user wins over stale g.user."""
+ stale_user = _make_mock_user("stale_bob")
+ jwt_user = _make_mock_user("jwt_alice")
+ token = _make_access_token(claims={"sub": "jwt_alice"})
+
+ with app.app_context():
+ g.user = stale_user
+ with (
+ patch("fastmcp.server.dependencies.get_access_token",
return_value=token),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=jwt_user,
+ ),
+ ):
+ result = get_user_from_request()
+
+ assert result.username == "jwt_alice"
+
+
+def test_dev_username_fallback_when_no_jwt(app) -> None:
+ """MCP_DEV_USERNAME used when no JWT context available."""
+ mock_user = _make_mock_user("dev_admin")
+
+ with app.app_context():
+ app.config["MCP_DEV_USERNAME"] = "dev_admin"
+ try:
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=None
+ ),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=mock_user,
+ ),
+ ):
+ result = get_user_from_request()
+ finally:
+ app.config.pop("MCP_DEV_USERNAME", None)
+
+ assert result.username == "dev_admin"
+
+
+def test_g_user_fallback_when_no_jwt_and_no_dev_username(app) -> None:
+ """g.user used as last-resort fallback (Preset middleware
compatibility)."""
+ preset_user = _make_mock_user("preset_user")
+
+ with app.app_context():
+ app.config.pop("MCP_DEV_USERNAME", None)
+ g.user = preset_user
+ with patch("fastmcp.server.dependencies.get_access_token",
return_value=None):
+ result = get_user_from_request()
+
+ assert result.username == "preset_user"
+
+
+def test_raises_when_no_auth_source(app) -> None:
+ """ValueError raised when no auth source is available."""
+ with app.app_context():
+ app.config.pop("MCP_DEV_USERNAME", None)
+ g.pop("user", None)
+ with patch("fastmcp.server.dependencies.get_access_token",
return_value=None):
+ with pytest.raises(ValueError, match="No authenticated user
found"):
+ get_user_from_request()
+
+
+def test_dev_username_not_found_raises(app) -> None:
+ """MCP_DEV_USERNAME configured but user not in DB raises ValueError."""
+ with app.app_context():
+ app.config["MCP_DEV_USERNAME"] = "ghost"
+ try:
+ with (
+ patch(
+ "fastmcp.server.dependencies.get_access_token",
return_value=None
+ ),
+ patch(
+ "superset.mcp_service.auth.load_user_with_relationships",
+ return_value=None,
+ ),
+ ):
+ with pytest.raises(ValueError, match="not found"):
+ get_user_from_request()
+ finally:
+ app.config.pop("MCP_DEV_USERNAME", None)
+
+
+# -- g.user clearing in mcp_auth_hook --
+
+
+def test_mcp_auth_hook_clears_stale_g_user(app) -> None:
+ """mcp_auth_hook clears g.user before setting up user context."""
+ stale_user = _make_mock_user("stale")
+ fresh_user = _make_mock_user("fresh")
+
+ def dummy_tool():
+ """Dummy tool."""
+ return g.user.username
+
+ wrapped = mcp_auth_hook(dummy_tool)
+
+ with app.app_context():
+ g.user = stale_user
+ with patch(
+ "superset.mcp_service.auth.get_user_from_request",
+ return_value=fresh_user,
+ ):
+ result = wrapped()
+
+ assert result == "fresh"
+
+
+def test_mcp_auth_hook_clears_stale_g_user_async(app) -> None:
+ """mcp_auth_hook clears g.user before setting up user context (async)."""
+ import asyncio
+
+ stale_user = _make_mock_user("stale")
+ fresh_user = _make_mock_user("fresh")
+
+ async def dummy_tool():
+ """Dummy tool."""
+ return g.user.username
+
+ wrapped = mcp_auth_hook(dummy_tool)
+
+ with app.app_context():
+ g.user = stale_user
+ with patch(
+ "superset.mcp_service.auth.get_user_from_request",
+ return_value=fresh_user,
+ ):
+ result = asyncio.get_event_loop().run_until_complete(wrapped())
Review Comment:
**Suggestion:** This async test drives the coroutine with
`asyncio.get_event_loop().run_until_complete(...)`, which is brittle on Python
3.11+ because there may be no current event loop (or it may already be closed),
causing a `RuntimeError` and flaky CI failures. Use `asyncio.run(...)` to
create and manage a fresh loop for this synchronous test. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP auth regression test can fail intermittently.
- ⚠️ Python unit CI matrix may become flaky.
- ⚠️ Contributor confidence drops from non-deterministic test failures.
```
</details>
```suggestion
result = asyncio.run(wrapped())
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Superset Python unit tests via CI command in
`.github/workflows/superset-python-unittest.yml:47`, which executes `pytest
...
./tests/unit_tests`.
2. The test runner executes `test_mcp_auth_hook_clears_stale_g_user_async` in
`tests/unit_tests/mcp_service/test_auth_user_resolution.py:258`.
3. That test reaches
`asyncio.get_event_loop().run_until_complete(wrapped())` at
`tests/unit_tests/mcp_service/test_auth_user_resolution.py:277`.
4. On environments where no current loop exists (notably newer asyncio
behavior),
`get_event_loop()` can raise `RuntimeError`, failing this test path; using
`asyncio.run(...)` avoids dependence on ambient loop state.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/test_auth_user_resolution.py
**Line:** 277:277
**Comment:**
*Possible Bug: This async test drives the coroutine with
`asyncio.get_event_loop().run_until_complete(...)`, which is brittle on Python
3.11+ because there may be no current event loop (or it may already be closed),
causing a `RuntimeError` and flaky CI failures. Use `asyncio.run(...)` to
create and manage a fresh loop for this synchronous test.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=45affa336a772b00eb1936869c0e592c27d8a68aee9c4248bd732a9a77977ba4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=45affa336a772b00eb1936869c0e592c27d8a68aee9c4248bd732a9a77977ba4&reaction=dislike'>👎</a>
##########
superset/mcp_service/mcp_config.py:
##########
@@ -318,11 +318,28 @@ def create_default_mcp_auth_factory(app: Flask) ->
Optional[Any]:
return None
-def default_user_resolver(app: Any, access_token: Any) -> Optional[str]:
- """Extract username from JWT token claims."""
- if hasattr(access_token, "subject"):
+def default_user_resolver(app: Any, access_token: Any) -> str | None:
+ """Extract username from JWT token claims.
+
+ Checks the ``claims`` dict first (FastMCP's AccessToken format),
+ then falls back to legacy attribute access for backward compatibility.
+ """
+ # FastMCP AccessToken stores JWT claims in a dict
+ claims = getattr(access_token, "claims", None)
+ if isinstance(claims, dict) and claims:
+ username = (
+ claims.get("sub")
+ or claims.get("preferred_username")
+ or claims.get("email")
+ or claims.get("username")
Review Comment:
**Suggestion:** The resolver currently prioritizes `sub` before
`preferred_username`, but in OIDC tokens `sub` is often a stable opaque ID (not
the Superset username). This can make user lookup fail and raise a hard
authentication error even when a valid username claim is present. Prefer
username-oriented claims first and use `sub` as a fallback. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ JWT MCP auth fails with opaque OIDC sub.
- ⚠️ Protected tools/resources reject valid authenticated users.
- ⚠️ Requires custom MCP_USER_RESOLVER workaround in deployments.
```
</details>
```suggestion
claims.get("preferred_username")
or claims.get("username")
or claims.get("email")
or claims.get("sub")
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable JWT-based MCP auth and keep default resolver path
(`superset/mcp_service/auth.py:207` uses `MCP_USER_RESOLVER` fallback to
`default_user_resolver`).
2. Create a Superset user `alice`, then send an MCP authenticated call with
token claims
containing both `sub` (opaque ID like `00u123`) and `preferred_username`
(`alice`) to a
protected MCP function (example resource uses `@mcp_auth_hook` at
`superset/mcp_service/system/resources/instance_metadata.py:36-38`).
3. Call flow enters `mcp_auth_hook` (`superset/mcp_service/auth.py:81-86`) →
`_setup_user_context()` (`auth.py:331`) → `get_user_from_request()`
(`auth.py:251`) →
`_resolve_user_from_jwt_context()` (`auth.py:175`).
4. `default_user_resolver()` picks `claims["sub"]` first
(`superset/mcp_service/mcp_config.py:331`), then
`_resolve_user_from_jwt_context()`
performs username lookup with that opaque value (`auth.py:216`) and raises
hard failure
when user not found (`auth.py:220`), despite valid `preferred_username`
being present.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/mcp_config.py
**Line:** 331:334
**Comment:**
*Logic Error: The resolver currently prioritizes `sub` before
`preferred_username`, but in OIDC tokens `sub` is often a stable opaque ID (not
the Superset username). This can make user lookup fail and raise a hard
authentication error even when a valid username claim is present. Prefer
username-oriented claims first and use `sub` as a fallback.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=dc54ad424410ea1ad0d910a1e45391984dc20b03141a4337e91c566ce2ad3dff&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=dc54ad424410ea1ad0d910a1e45391984dc20b03141a4337e91c566ce2ad3dff&reaction=dislike'>👎</a>
##########
superset/mcp_service/auth.py:
##########
@@ -339,6 +398,9 @@ def _get_app_context_manager() ->
AbstractContextManager[None]:
@functools.wraps(tool_func)
async def async_wrapper(*args: Any, **kwargs: Any) -> Any:
with _get_app_context_manager():
+ # Clear stale g.user to prevent user impersonation across
+ # tool calls when no per-request middleware refreshes it.
+ g.pop("user", None)
Review Comment:
**Suggestion:** `mcp_auth_hook` now unconditionally clears `g.user`, which
also wipes a valid request-scoped user set by external middleware (for example
Preset), causing authenticated requests to fail. Only clear `g.user` when there
is no active request context, so stale app-context state is removed without
breaking legitimate per-request identity. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Preset middleware identity can be dropped before authorization.
- ❌ Protected MCP tools may fail authentication unexpectedly.
- ⚠️ Breaks documented `g.user` fallback compatibility path.
```
</details>
```suggestion
from flask import has_request_context
if not has_request_context():
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Register any protected MCP tool (for example `get_instance_info`) which
is decorated
with `@tool` in `superset/mcp_service/system/tool/get_instance_info.py:76`;
that decorator
applies `mcp_auth_hook` via
`superset/core/mcp/core_mcp_injection.py:127-129`.
2. Execute the wrapped tool inside a Flask request context where external
middleware
already set `g.user` (the fallback path is explicitly documented in
`superset/mcp_service/auth.py:235-236`).
3. In `mcp_auth_hook` (`superset/mcp_service/auth.py:400-404` for async,
`443-447` for
sync), current code unconditionally runs `g.pop("user", None)` before
`_setup_user_context()`.
4. `_setup_user_context()` calls `get_user_from_request()`
(`superset/mcp_service/auth.py:172`), which then checks JWT,
`MCP_DEV_USERNAME`, and
finally `g.user` (`252-266`); because `g.user` was just removed, fallback
identity is lost
and request can fail with `ValueError` (`282-287`).
```
</details>
<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:** 403:403
**Comment:**
*Logic Error: `mcp_auth_hook` now unconditionally clears `g.user`,
which also wipes a valid request-scoped user set by external middleware (for
example Preset), causing authenticated requests to fail. Only clear `g.user`
when there is no active request context, so stale app-context state is removed
without breaking legitimate per-request identity.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=73ec727c8156fe6515ee26c337ef095dc955667d1ad3c36c810b3baa9230d423&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=73ec727c8156fe6515ee26c337ef095dc955667d1ad3c36c810b3baa9230d423&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]
