codeant-ai-for-open-source[bot] commented on code in PR #40659:
URL: https://github.com/apache/superset/pull/40659#discussion_r3338025811
##########
superset/mcp_service/auth.py:
##########
@@ -119,7 +119,15 @@ def check_tool_permission(func: Callable[..., Any]) ->
bool:
class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
if not class_permission_name:
- # No RBAC configured for this tool; allow by default.
+ # No RBAC configured for this tool; allow by default. This is a
+ # supported configuration (a protected tool may intentionally
+ # declare no permission class), but surface it so an accidental
+ # omission on a sensitive tool doesn't silently fail open.
+ logger.warning(
+ "Tool %s is permission-protected but declares no "
+ "class_permission_name; allowing access without an RBAC check",
+ func.__name__,
+ )
Review Comment:
**Suggestion:** This warning is emitted on every invocation of any protected
tool that intentionally has no `class_permission_name`, which can produce
high-volume warning logs in normal operation (for example core/system tools
without RBAC class mapping). Emit this once at registration time or downgrade
to debug to avoid noisy false-positive operational alerts and log churn.
[incorrect log level]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP tools without class_permission_name log warnings each call.
- ⚠️ High-volume warnings can obscure real permission-denial alerts.
- ⚠️ Operations dashboards may show persistent noisy warning spikes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Define an MCP tool using the concrete decorator in
`superset/core/mcp/core_mcp_injection.py:create_tool_decorator` (lines
61–174) with
`protect=True` (the default) and **no** `class_permission_name` argument;
this is a
supported configuration per the decorator docstring (authentication without
RBAC mapping).
2. The decorator wraps the tool with `mcp_auth_hook` from
`superset/mcp_service/auth.py`
(lines ~80–167) when `protect` is true, so every invocation of this tool
goes through the
auth wrapper.
3. When a client calls this tool (e.g., via the MCP server initialized in
`superset/mcp_service/app.py` around lines 584+, where tools are imported
and registered),
the async or sync wrapper in `mcp_auth_hook` calls
`check_tool_permission(tool_func)`
before executing the tool body (async path at `auth.py` lines 82–105, sync
path at lines
122–151).
4. Inside `check_tool_permission` (defined at
`superset/mcp_service/auth.py:91`),
`class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)` and `if
not
class_permission_name:` branch to the warning at lines 126–130: the
`logger.warning("Tool
%s is permission-protected but declares no class_permission_name; allowing
access without
an RBAC check", func.__name__)` executes **on every invocation** of such a
tool, producing
repeated warning-level log entries during normal operation.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=edc6844070b74e898ff18346e707616a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=edc6844070b74e898ff18346e707616a&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:** 126:130
**Comment:**
*Incorrect Log Level: This warning is emitted on every invocation of
any protected tool that intentionally has no `class_permission_name`, which can
produce high-volume warning logs in normal operation (for example core/system
tools without RBAC class mapping). Emit this once at registration time or
downgrade to debug to avoid noisy false-positive operational alerts and log
churn.
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%2F40659&comment_hash=9a4b71385af7d690eff4a7d412ece3d9a56bfdae31c3e02ab823e873f1c168a5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40659&comment_hash=9a4b71385af7d690eff4a7d412ece3d9a56bfdae31c3e02ab823e873f1c168a5&reaction=dislike'>👎</a>
##########
superset/mcp_service/utils/permissions_utils.py:
##########
@@ -89,10 +89,15 @@ def user_has_permission(
return False
try:
- # Check if user is admin (has all permissions)
+ from flask import current_app
+
+ # Check if user is admin (has all permissions). Use the configured
+ # admin role name rather than hardcoding "Admin", so deployments that
+ # rename the admin role (AUTH_ROLE_ADMIN) still grant admins the
bypass.
+ admin_role_name = current_app.config["AUTH_ROLE_ADMIN"]
Review Comment:
**Suggestion:** This introduces a hard dependency on Flask app context for
all permission checks. When `user_has_permission` is called with an explicit
`user` outside an active Flask context, `current_app.config` raises
`RuntimeError`, the broad `except` returns `False`, and the function denies
access without ever evaluating `security_manager.has_access` /
`user.get_permissions`. Use a safe fallback (`config.get`) and avoid failing
the whole check when app context is unavailable. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Standalone scripts using user_has_permission deny all access.
- ⚠️ Future background jobs may mis-evaluate field permissions.
- ⚠️ FieldPermissionsMiddleware reuse without app context misbehaves.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In any environment where Superset models are available but no Flask
application context
is active (no `app.app_context()` has been pushed), import
`user_has_permission` from
`superset.mcp_service.utils.permissions_utils` (function defined starting at
line 74).
2. Construct a `User`-like object with a `roles` attribute (e.g., using
`flask_appbuilder.security.sqla.models.User` or a `Mock` with `.roles`) and
call
`user_has_permission(user, "can_sql_json", "Superset")` directly, without
wrapping the
call in a Flask app context.
3. When `user_has_permission` executes, it enters the `try` block at line 91
and runs
`from flask import current_app` followed by `admin_role_name =
current_app.config["AUTH_ROLE_ADMIN"]` at lines 92–97; because there is no
active Flask
app context, accessing `current_app.config` raises `RuntimeError("Working
outside of
application context")`.
4. This `RuntimeError` is caught by the broad `except Exception as e` at
lines 56–60,
which logs `"Error checking permission %s for user %s: %s"` and returns
`False` without
ever reaching the normal permission evaluation
(`security_manager.has_access` or iterating
`user.get_permissions()`), causing any caller (including future reuse from
`get_allowed_fields` / `filter_sensitive_data` and the
`FieldPermissionsMiddleware` in
`superset/mcp_service/middleware.py:906-119`) to see a blanket denial when
using this
helper outside an app context.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1e87b407c8d8414cb40e7e2af33bc706&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1e87b407c8d8414cb40e7e2af33bc706&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/utils/permissions_utils.py
**Line:** 92:97
**Comment:**
*Logic Error: This introduces a hard dependency on Flask app context
for all permission checks. When `user_has_permission` is called with an
explicit `user` outside an active Flask context, `current_app.config` raises
`RuntimeError`, the broad `except` returns `False`, and the function denies
access without ever evaluating `security_manager.has_access` /
`user.get_permissions`. Use a safe fallback (`config.get`) and avoid failing
the whole check when app context is unavailable.
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%2F40659&comment_hash=401019eb651337ed3b97ef2a87f1b000279441698bf0a888dc518ecd55601d42&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40659&comment_hash=401019eb651337ed3b97ef2a87f1b000279441698bf0a888dc518ecd55601d42&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]
