codeant-ai-for-open-source[bot] commented on code in PR #41003:
URL: https://github.com/apache/superset/pull/41003#discussion_r3531310489
##########
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}
Review Comment:
**Suggestion:** Add a concrete type annotation for this newly introduced
local mapping before it is cast, so the variable itself is explicitly typed.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly introduced local variable is assigned a mapping value without a
type annotation, and the rule explicitly flags modified Python code that omits
type hints on relevant variables that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8020cd764b114c5ca2b6f883fd57d87a&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=8020cd764b114c5ca2b6f883fd57d87a&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:515
**Comment:**
*Custom Rule: Add a concrete type annotation for this newly introduced
local mapping before it is cast, so the variable itself is explicitly typed.
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=dd758140c9666e95b962fc3530f10edbc87320946eadcb28fab60312fe0748cb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41003&comment_hash=dd758140c9666e95b962fc3530f10edbc87320946eadcb28fab60312fe0748cb&reaction=dislike'>👎</a>
##########
superset/mcp_service/auth.py:
##########
@@ -228,7 +236,35 @@ def _log_scope_denial(
)
-def check_tool_permission(func: Callable[..., Any], *, log_denial: bool =
True) -> bool:
+# Guest deny-list default (when MCP_GUEST_DENIED_TOOLS is unset); blocks tools
+# with no RBAC class that would otherwise fall open. Sync with mcp_config.py.
+_DEFAULT_GUEST_DENIED_TOOLS = frozenset({"find_users", "get_instance_info"})
Review Comment:
**Suggestion:** Add an explicit type annotation to this new module-level
constant so it satisfies the type-hint requirement for relevant variables.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a new module-level variable introduced in the PR without an explicit
type annotation, and the custom rule requires type hints for relevant variables
that can be annotated.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fbda3b95b0cd4462ab01e3c74e5f85f4&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=fbda3b95b0cd4462ab01e3c74e5f85f4&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:** 241:241
**Comment:**
*Custom Rule: Add an explicit type annotation to this new module-level
constant so it satisfies the type-hint requirement for relevant variables.
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=0c2828e8df3468cd690be66eea93a7ae2ecaeab290d34c8d1625592be6ec2002&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41003&comment_hash=0c2828e8df3468cd690be66eea93a7ae2ecaeab290d34c8d1625592be6ec2002&reaction=dislike'>👎</a>
##########
superset/mcp_service/guest_token_verifier.py:
##########
@@ -0,0 +1,180 @@
+# 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.
+
+"""
+Guest-token verifier for the MCP service.
+
+Recognizes Superset embedded *guest tokens* presented as MCP Bearer tokens and
+validates them by reusing core's guest-token machinery
+(``SupersetSecurityManager``): signature/expiry/audience via
+``parse_jwt_guest_token`` (HS256 against ``GUEST_TOKEN_JWT_SECRET``), the same
+structural claim checks the web embedded flow runs, and revocation via
+``_is_guest_token_revoked``.
+
+This verifier runs BEFORE the JWT verifier in ``CompositeTokenVerifier``: the
MCP
+JWT verifier pins its own algorithm/keys (default RS256 against the MCP
+JWKS/keys) and would reject an HS256 guest token before any resolution code
runs,
+so guest tokens must be recognized at a verifier that runs first.
+
+Gated behind ``MCP_EMBEDDED_GUEST_AUTH_ENABLED`` (opt-in, default False) AND
the
+``EMBEDDED_SUPERSET`` feature flag. On any failure the verifier returns
``None``
+so the token falls through to the next verifier — mirroring the web request
+loader, which returns ``None`` for an invalid guest token rather than raising.
+"""
+
+import asyncio
+import logging
+from typing import Any
+
+from fastmcp.server.auth import AccessToken
+from fastmcp.server.auth.providers.jwt import TokenVerifier
+
+logger = logging.getLogger(__name__)
+
+# Namespaced claim flagging an AccessToken as a verified guest token. The
paired
+# ``client_id == "guest"`` check keeps an external IdP JWT from posing as a
guest.
+GUEST_TOKEN_CLAIM = "_superset_mcp_guest_token" # noqa: S105
Review Comment:
**Suggestion:** Add an explicit type annotation to the module-level constant
`GUEST_TOKEN_CLAIM` to satisfy the type-hint requirement for relevant
variables. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The rule requires type hints for relevant variables that can be annotated.
This module-level constant is a new, annotatable Python variable and is
currently untyped, so the suggestion identifies a real rule violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=80c1658b251943bbaddbf45f21c99129&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=80c1658b251943bbaddbf45f21c99129&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/guest_token_verifier.py
**Line:** 50:50
**Comment:**
*Custom Rule: Add an explicit type annotation to the module-level
constant `GUEST_TOKEN_CLAIM` to satisfy the type-hint requirement for relevant
variables.
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=25c8b25235a4c5f76fbcd12ffe196cd06628a11b45a2437676c980bca0672b1a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41003&comment_hash=25c8b25235a4c5f76fbcd12ffe196cd06628a11b45a2437676c980bca0672b1a&reaction=dislike'>👎</a>
##########
tests/unit_tests/mcp_service/test_guest_token_auth.py:
##########
@@ -0,0 +1,550 @@
+# 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 embedded guest-token authentication on the MCP transport.
+
+Covers the three layers of the feature:
+- ``GuestTokenVerifier`` (transport): validates a guest token by reusing core's
+ guest-token machinery and emits a marked ``AccessToken``.
+- ``CompositeTokenVerifier`` routing: guests are tried before the JWT verifier.
+- ``_resolve_user_from_jwt_context`` (resolution): builds the ``GuestUser``
from
+ the verified claims, and ignores look-alike tokens lacking the marker.
+- The guest deny-list for sensitive enumeration tools.
+"""
+
+from contextlib import contextmanager
+from types import SimpleNamespace
+from typing import Any
+from unittest.mock import MagicMock, patch
+
+import jwt
+import pytest
+from fastmcp.server.auth import AccessToken
+from flask import g
+
+from superset.constants import CHANGE_ME_GUEST_TOKEN_JWT_SECRET
+from superset.mcp_service.auth import (
+ _resolve_user_from_jwt_context,
+ _tool_denied_for_guest,
+ check_tool_permission,
+ CLASS_PERMISSION_ATTR,
+ is_tool_visible_to_current_user,
+ METHOD_PERMISSION_ATTR,
+)
+from superset.mcp_service.composite_token_verifier import
CompositeTokenVerifier
+from superset.mcp_service.guest_token_verifier import (
+ GUEST_TOKEN_CLAIM,
+ GuestTokenVerifier,
+)
+from superset.mcp_service.mcp_config import (
+ _is_mcp_guest_auth_enabled,
+ _validate_guest_config,
+ MCPAuthConfigError,
+)
+from superset.security.guest_token import GuestUser
+
+
+def _access_token(client_id: str, claims: dict[str, Any]) -> AccessToken:
+ """Build a fastmcp AccessToken for routing tests."""
+ fake_token = "fake-token" # noqa: S105 — test fixture, not a real
credential
+ return AccessToken(token=fake_token, client_id=client_id, scopes=[],
claims=claims)
+
+
+def _parsed_guest_claims(**overrides: Any) -> dict[str, Any]:
+ """A decoded guest-token claims dict shaped like core's mint output."""
+ claims = {
+ "user": {"username": "embed-guest"},
+ "resources": [{"type": "dashboard", "id": "abc-uuid"}],
+ "rls_rules": [],
+ "iat": 1,
+ "exp": 9_999_999_999,
+ "aud": "superset",
+ "type": "guest",
+ }
+ claims.update(overrides)
+ return claims
+
+
+class _FakeApp:
+ """Minimal Flask-app stand-in for unit-testing GuestTokenVerifier."""
+
+ def __init__(self, config: dict[str, Any], sm: Any) -> None:
+ self.config = config
+ self.appbuilder = SimpleNamespace(sm=sm)
+
+ @contextmanager
+ def app_context(self) -> Any:
+ yield
+
+
+def _make_guest_verifier(
+ *,
+ parsed: dict[str, Any] | None = None,
+ parse_error: Exception | None = None,
+ revoked: bool = False,
+ role_exists: bool = True,
+ mcp_enabled: bool = True,
+) -> tuple[GuestTokenVerifier, MagicMock]:
+ sm = MagicMock()
+ if parse_error is not None:
+ sm.parse_jwt_guest_token.side_effect = parse_error
+ else:
+ sm.parse_jwt_guest_token.return_value = (
+ _parsed_guest_claims() if parsed is None else parsed
+ )
+ sm._is_guest_token_revoked.return_value = revoked
+ sm.find_role.return_value = object() if role_exists else None
+ config = {
+ "MCP_EMBEDDED_GUEST_AUTH_ENABLED": mcp_enabled,
+ "GUEST_ROLE_NAME": "Public",
+ }
+ return GuestTokenVerifier(app=_FakeApp(config, sm)), sm
+
+
+# -- GuestTokenVerifier --
+
+
[email protected]
+async def test_guest_verifier_accepts_valid_token() -> None:
+ verifier, _ = _make_guest_verifier()
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("raw-guest-token")
+
+ assert token is not None
+ assert token.client_id == "guest"
+ assert token.claims[GUEST_TOKEN_CLAIM] is True
+ # The parsed guest claims are carried through for resolution.
+ assert token.claims["user"] == {"username": "embed-guest"}
+ assert token.claims["type"] == "guest"
+
+
[email protected]
+async def test_guest_verifier_noop_when_mcp_flag_off() -> None:
+ verifier, sm = _make_guest_verifier(mcp_enabled=False)
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("raw-guest-token")
+
+ assert token is None
+ # Short-circuits before doing any parsing work.
+ sm.parse_jwt_guest_token.assert_not_called()
+
+
[email protected]
+async def test_guest_verifier_defers_when_embedded_disabled() -> None:
+ verifier, _ = _make_guest_verifier()
+ with patch("superset.is_feature_enabled", return_value=False):
+ token = await verifier.verify_token("raw-guest-token")
+
+ assert token is None
+
+
[email protected]
+async def test_guest_verifier_defers_on_bad_signature() -> None:
+ verifier, _ = _make_guest_verifier(parse_error=ValueError("bad signature"))
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("not-a-guest-token")
+
+ assert token is None
+
+
[email protected]
+async def test_guest_verifier_rejects_non_guest_type() -> None:
+ verifier, _ =
_make_guest_verifier(parsed=_parsed_guest_claims(type="access"))
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("raw-token")
+
+ assert token is None
+
+
[email protected]
+async def test_guest_verifier_rejects_missing_structural_claims() -> None:
+ bad = _parsed_guest_claims()
+ del bad["resources"]
+ verifier, _ = _make_guest_verifier(parsed=bad)
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("raw-token")
+
+ assert token is None
+
+
[email protected]
+async def test_guest_verifier_rejects_revoked_token() -> None:
+ verifier, _ = _make_guest_verifier(revoked=True)
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("raw-guest-token")
+
+ assert token is None
+
+
[email protected]
+async def test_guest_verifier_rejects_when_guest_role_missing() -> None:
+ verifier, _ = _make_guest_verifier(role_exists=False)
+ with patch("superset.is_feature_enabled", return_value=True):
+ token = await verifier.verify_token("raw-guest-token")
+
+ assert token is None
+
+
+# -- CompositeTokenVerifier routing --
+
+
+class _StubVerifier:
+ base_url = None
+ required_scopes: list[str] = []
+
+ def __init__(self, return_value: Any) -> None:
+ self._return_value = return_value
+ self.called = False
+
+ async def verify_token(self, token: str) -> Any:
+ self.called = True
+ return self._return_value
+
+
[email protected]
+async def test_composite_tries_guest_before_jwt() -> None:
+ guest_at = _access_token("guest", {GUEST_TOKEN_CLAIM: True})
+ guest = _StubVerifier(guest_at)
+ jwt = _StubVerifier(_access_token("idp", {}))
+ composite = CompositeTokenVerifier(
+ jwt_verifier=jwt, api_key_prefixes=[], app=None, guest_verifier=guest
+ )
+
+ result = await composite.verify_token("some-token")
+
+ assert result is guest_at
+ assert guest.called is True
+ assert jwt.called is False # guest short-circuits the JWT path
+
+
[email protected]
+async def test_composite_falls_through_to_jwt_for_non_guest() -> None:
+ jwt_result = _access_token("idp", {})
+ guest = _StubVerifier(None) # not a guest token
+ jwt = _StubVerifier(jwt_result)
+ composite = CompositeTokenVerifier(
+ jwt_verifier=jwt, api_key_prefixes=[], app=None, guest_verifier=guest
+ )
+
+ result = await composite.verify_token("some-token")
+
+ assert result is jwt_result
+ assert guest.called is True
+ assert jwt.called is True
+
+
+# -- _resolve_user_from_jwt_context --
+
+
+def test_resolve_builds_guest_user_from_token(app) -> None:
Review Comment:
**Suggestion:** Add a type annotation for the `app` fixture parameter in
this test function signature. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This test function introduces a new untyped parameter `app` in Python code.
The custom rule explicitly requires type hints on functions and relevant
variables, so this is a real violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7d32076be75d4ac8895c56d84372308f&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=7d32076be75d4ac8895c56d84372308f&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:** tests/unit_tests/mcp_service/test_guest_token_auth.py
**Line:** 253:253
**Comment:**
*Custom Rule: Add a type annotation for the `app` fixture parameter in
this test function signature.
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=cdba67ea49b22fb106e6cf9e13d5321fd007ef2d7dcf5556bf7291a5db608026&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41003&comment_hash=cdba67ea49b22fb106e6cf9e13d5321fd007ef2d7dcf5556bf7291a5db608026&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]
