codeant-ai-for-open-source[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2892095233
##########
superset/mcp_service/auth.py:
##########
@@ -264,6 +350,17 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any:
)
return await tool_func(*args, **kwargs)
+ # RBAC permission check
+ if not check_tool_permission(tool_func):
+ raise MCPPermissionDeniedError(
+ permission_name=getattr(
+ tool_func, METHOD_PERMISSION_ATTR, "read"
+ ),
+ view_name=getattr(tool_func, CLASS_PERMISSION_ATTR,
"unknown"),
+ user=user.username,
+ tool_name=tool_func.__name__,
+ )
+
Review Comment:
**Suggestion:** When RBAC denies access for an async MCP tool, the raised
MCPPermissionDeniedError currently uses the bare action name (e.g. "read" or
"execute_sql_query") instead of the full permission string ("can_read",
"can_execute_sql_query") that was actually checked, which can confuse debugging
and any code/tests that expect the real permission identifier. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Async MCP RBAC errors expose incorrect permission identifier.
- ⚠️ Debugging permission denials for async tools becomes harder.
```
</details>
```suggestion
# RBAC permission check
method_name = getattr(tool_func, METHOD_PERMISSION_ATTR,
"read")
permission_str = f"{PERMISSION_PREFIX}{method_name}"
if not check_tool_permission(tool_func):
raise MCPPermissionDeniedError(
permission_name=permission_str,
view_name=getattr(tool_func,
CLASS_PERMISSION_ATTR, "unknown"),
user=user.username,
tool_name=tool_func.__name__,
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the MCP server via `superset mcp`, which exposes MCP tools wrapped
by
`mcp_auth_hook` in `superset/mcp_service/auth.py` (async wrapper around
lines 337–375,
RBAC block at 355–363).
2. Authenticate as a Superset user who lacks the `can_read` permission on a
view (e.g.,
`Chart`), so that `security_manager.can_access("can_read", "Chart")` will
return False
inside `check_tool_permission()` at `superset/mcp_service/auth.py:74-128`.
3. Invoke any async MCP tool that is decorated with `@tool` and
`mcp_auth_hook` (metadata
set in `core_mcp_injection.py` as referenced in the `mcp_auth_hook`
docstring) and has
`CLASS_PERMISSION_ATTR="Chart"` and `METHOD_PERMISSION_ATTR` left at its
default `"read"`.
4. When the call reaches the async wrapper's RBAC block at
`superset/mcp_service/auth.py:355-363`, `check_tool_permission(tool_func)`
returns False,
and `MCPPermissionDeniedError` is raised with `permission_name="read"` even
though the
actual checked permission string was `"can_read"`, leading to a misleading
`Permission
denied: read on Chart` message and incorrect `error.permission_name`
metadata.
```
</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:** 355:363
**Comment:**
*Logic Error: When RBAC denies access for an async MCP tool, the raised
MCPPermissionDeniedError currently uses the bare action name (e.g. "read" or
"execute_sql_query") instead of the full permission string ("can_read",
"can_execute_sql_query") that was actually checked, which can confuse debugging
and any code/tests that expect the real permission identifier.
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%2F38407&comment_hash=fb2c1a6f9d3044ad00a2a5dd70c8fa11fb0f6024e50d0884bc066f762514c9ee&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38407&comment_hash=fb2c1a6f9d3044ad00a2a5dd70c8fa11fb0f6024e50d0884bc066f762514c9ee&reaction=dislike'>👎</a>
##########
superset/mcp_service/auth.py:
##########
@@ -294,6 +391,17 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
)
return tool_func(*args, **kwargs)
+ # RBAC permission check
+ if not check_tool_permission(tool_func):
+ raise MCPPermissionDeniedError(
+ permission_name=getattr(
+ tool_func, METHOD_PERMISSION_ATTR, "read"
+ ),
+ view_name=getattr(tool_func, CLASS_PERMISSION_ATTR,
"unknown"),
+ user=user.username,
+ tool_name=tool_func.__name__,
+ )
Review Comment:
**Suggestion:** For sync MCP tools, MCPPermissionDeniedError is also
constructed with the bare action name instead of the full permission string
("can_*") that security_manager.can_access actually uses, which can mislead
operators and any programmatic handlers relying on accurate permission
identifiers. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Sync MCP RBAC errors expose incorrect permission identifier.
- ⚠️ Operators debugging RBAC issues receive misleading error metadata.
```
</details>
```suggestion
# RBAC permission check
method_name = getattr(tool_func, METHOD_PERMISSION_ATTR,
"read")
permission_str = f"{PERMISSION_PREFIX}{method_name}"
if not check_tool_permission(tool_func):
raise MCPPermissionDeniedError(
permission_name=permission_str,
view_name=getattr(tool_func, CLASS_PERMISSION_ATTR,
"unknown"),
user=user.username,
tool_name=tool_func.__name__,
)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the MCP server via `superset mcp`, which exposes sync MCP tools
wrapped by
`mcp_auth_hook` in `superset/mcp_service/auth.py` (sync wrapper around lines
379–415, RBAC
block at 394–403).
2. Authenticate as a Superset user who lacks a specific permission, for
example
`can_execute_sql` on `"SQLLab"`, so that
`security_manager.can_access("can_execute_sql",
"SQLLab")` returns False inside `check_tool_permission()` at
`superset/mcp_service/auth.py:74-128`.
3. Invoke a sync MCP tool (e.g., SQL or chart-generation tools defined in
the MCP
integration and wrapped with `mcp_auth_hook`) whose `CLASS_PERMISSION_ATTR`
is `"SQLLab"`
and whose `METHOD_PERMISSION_ATTR` is `"execute_sql"`.
4. When the call reaches the sync wrapper's RBAC block at
`superset/mcp_service/auth.py:394-403`, `check_tool_permission(tool_func)`
returns False
and `MCPPermissionDeniedError` is raised with
`permission_name="execute_sql"` instead of
`"can_execute_sql"`, so `error.permission_name` and the rendered message do
not match the
permission string used in `security_manager.can_access`, potentially
confusing operators
and any code inspecting this field.
```
</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:** 394:403
**Comment:**
*Logic Error: For sync MCP tools, MCPPermissionDeniedError is also
constructed with the bare action name instead of the full permission string
("can_*") that security_manager.can_access actually uses, which can mislead
operators and any programmatic handlers relying on accurate permission
identifiers.
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%2F38407&comment_hash=3cdaca0edb242f8ae63e36ce79c45c41cbc93950b7521af46aab0a6c12ce42f0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38407&comment_hash=3cdaca0edb242f8ae63e36ce79c45c41cbc93950b7521af46aab0a6c12ce42f0&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]
