codeant-ai-for-open-source[bot] commented on code in PR #42297:
URL: https://github.com/apache/superset/pull/42297#discussion_r3717165856
##########
superset/mcp_service/auth.py:
##########
@@ -398,7 +467,9 @@ def check_tool_permission( # noqa: C901
# advertises scopes. Tokens/deployments that don't use scopes (API
keys,
# scope-less JWTs, dev-mode) fall through to RBAC-only behavior — see
# ``_token_scope_allows``.
- if has_permission and not _token_scope_allows(method_permission_name):
+ if has_permission and not _token_scope_allows(
+ method_permission_name, class_permission_name
+ ):
_log_scope_denial(
Review Comment:
**Suggestion:** The new scope check is only applied through
`check_tool_permission`, but `get_schema` performs its own dynamic RBAC check
and does not call `_token_scope_allows`. Consequently, a scoped token carrying
an unrelated resource scope can still retrieve schema metadata for any resource
for which its user has RBAC access, bypassing the intended per-resource
token/RBAC intersection. Apply the same scope check to dynamic permission paths
such as `get_schema` before allowing the operation. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Scoped tokens can bypass intended schema scope restrictions.
- ⚠️ `get_schema` exposes chart, dataset, dashboard, database, or report
metadata.
- ⚠️ Token/RBAC intersection is inconsistent across MCP tools.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=12428ba0c2d4497f8990e1ed21d55459&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=12428ba0c2d4497f8990e1ed21d55459&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:** 470:473
**Comment:**
*Incomplete Implementation: The new scope check is only applied through
`check_tool_permission`, but `get_schema` performs its own dynamic RBAC check
and does not call `_token_scope_allows`. Consequently, a scoped token carrying
an unrelated resource scope can still retrieve schema metadata for any resource
for which its user has RBAC access, bypassing the intended per-resource
token/RBAC intersection. Apply the same scope check to dynamic permission paths
such as `get_schema` before allowing the operation.
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%2F42297&comment_hash=86b952a954782b6e07599c92dea0cc784c41a73d2c9c950daceff34cdb373426&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42297&comment_hash=86b952a954782b6e07599c92dea0cc784c41a73d2c9c950daceff34cdb373426&reaction=dislike'>👎</a>
##########
superset/mcp_service/composite_token_verifier.py:
##########
@@ -168,32 +181,38 @@ async def verify_token(self, token: str) -> AccessToken |
None:
if any(token.startswith(prefix) for prefix in self._api_key_prefixes):
if self._app is not None:
loop = asyncio.get_running_loop()
- username = await loop.run_in_executor(
+ result = await loop.run_in_executor(
None, self._validate_api_key_sync, token
)
- if username is None:
+ if result is None:
logger.debug(
"API key rejected at transport layer (invalid or
expired)"
)
return None
+ username, key_scopes = result
logger.debug(
"API key validated at transport layer for user=%s",
username
)
return AccessToken(
token=token,
client_id="api_key",
- scopes=list(self.required_scopes or []),
+ # Prefer the key's own scopes over verifier-global
+ # required_scopes. Empty list = no scopes set = "no scopes
+ # advertised" back-compat per auth.py's
+ # _get_token_scopes()/_token_scope_allows().
+ scopes=key_scopes or list(self.required_scopes or []),
Review Comment:
**Suggestion:** The empty-key-scope fallback re-advertises
`self.required_scopes`, so a key with no `ApiKey.scopes` is treated as a scoped
token whenever global required scopes are configured. That contradicts the
documented no-scopes behavior in `auth.py`, where an empty scope set means
RBAC-only authorization, and can unexpectedly deny otherwise RBAC-authorized
API-key requests. Preserve the empty scope list for unscoped keys rather than
converting it into global JWT scopes. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unscoped API keys can lose RBAC-authorized MCP operations.
- ⚠️ Behavior changes when `MCP_REQUIRED_SCOPES` is configured.
- ⚠️ API-key scope semantics contradict documented back-compat behavior.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d85215dfa34b4bd5beaca7972359a18c&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=d85215dfa34b4bd5beaca7972359a18c&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/composite_token_verifier.py
**Line:** 203:203
**Comment:**
*Logic Error: The empty-key-scope fallback re-advertises
`self.required_scopes`, so a key with no `ApiKey.scopes` is treated as a scoped
token whenever global required scopes are configured. That contradicts the
documented no-scopes behavior in `auth.py`, where an empty scope set means
RBAC-only authorization, and can unexpectedly deny otherwise RBAC-authorized
API-key requests. Preserve the empty scope list for unscoped keys rather than
converting it into global JWT scopes.
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%2F42297&comment_hash=875c033e7cbc198e7006cb4daf6f1e071cde7298825798a25f85394fff0ebae6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42297&comment_hash=875c033e7cbc198e7006cb4daf6f1e071cde7298825798a25f85394fff0ebae6&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]
