codeant-ai-for-open-source[bot] commented on code in PR #41404:
URL: https://github.com/apache/superset/pull/41404#discussion_r3471849529
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -152,15 +153,41 @@ def serialize_role_object(role: Any) -> RoleInfo | None:
if not role:
return None
+ try:
+ raw_permissions = getattr(role, "permissions", None)
+ except DetachedInstanceError:
+ raw_permissions = None
+
+ permissions: list[str] | None = None
+ if raw_permissions is not None:
+ permissions = []
+ try:
+ for permission in raw_permissions:
+ permission_name = _serialize_permission_name(permission)
+ if permission_name is not None:
+ permissions.append(permission_name)
+ except (DetachedInstanceError, TypeError):
+ permissions = []
Review Comment:
**Suggestion:** The exception handler wraps the entire permissions iteration
and resets `permissions` to an empty list on the first bad item, which drops
all previously serialized valid permissions. This causes incorrect output
(roles can appear to have no permissions) whenever a single detached/malformed
permission object is encountered. Catch errors per permission item and
continue, instead of clearing the whole list. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dashboard MCP role serializer can drop valid permissions.
- ⚠️ Roles may appear to have no permissions to clients.
- ⚠️ Misrepresents authorization state in dashboard-related MCP tooling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the dashboard role serializer helper at
`superset/mcp_service/dashboard/schemas.py:151-176`
(`serialize_role_object`), which
builds `permissions` from `raw_permissions` using a single try/except around
the entire
`for permission in raw_permissions` loop and resets `permissions = []` in
the `except
(DetachedInstanceError, TypeError)` block.
2. Observe the existing unit test
`test_dashboard_role_serializer_serializes_permission_view_names` in
`superset/tests/unit_tests/mcp_service/dashboard/tool/test_dashboard_tools.py`
which calls
`serialize_role_object(role)` at line 146 (per Grep) with a role whose
`permissions` is a
simple list containing one valid PermissionView-like object.
3. Modify that test (or write a new one alongside it) to construct a role
whose
`permissions` attribute is a custom iterable that yields a valid permission
object first
and then raises `TypeError` (or a stub that raises `DetachedInstanceError`)
on the second
iteration, e.g. an object whose `__iter__` yields `[permission_view]` then
raises; call
`serialize_role_object(role)` in the test as in the existing call at
`test_dashboard_tools.py:146`.
4. When the test runs, `serialize_role_object` successfully appends the
first permission
name, then the second iteration raises `TypeError` inside the loop; the
`except
(DetachedInstanceError, TypeError)` at `schemas.py:169-170` executes,
resetting
`permissions` from `["can_read on Dashboard"]` to `[]`, and the returned
`RoleInfo.permissions` is an empty list instead of containing the
successfully serialized
permission, demonstrating that a single bad permission entry drops all
previously
collected valid permissions.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=40e54a8789754f9c9d41125cafed8b0e&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=40e54a8789754f9c9d41125cafed8b0e&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/dashboard/schemas.py
**Line:** 161:170
**Comment:**
*Logic Error: The exception handler wraps the entire permissions
iteration and resets `permissions` to an empty list on the first bad item,
which drops all previously serialized valid permissions. This causes incorrect
output (roles can appear to have no permissions) whenever a single
detached/malformed permission object is encountered. Catch errors per
permission item and continue, instead of clearing the whole list.
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%2F41404&comment_hash=a4e76599c9e47758db884aef5dd9698ac5d447c7d67da917eb531b07256cf8ab&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41404&comment_hash=a4e76599c9e47758db884aef5dd9698ac5d447c7d67da917eb531b07256cf8ab&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]
