codeant-ai-for-open-source[bot] commented on code in PR #40746:
URL: https://github.com/apache/superset/pull/40746#discussion_r3364286507
##########
superset/mcp_service/user/schemas.py:
##########
@@ -104,6 +104,20 @@ class UserInfo(BaseModel):
"access via get_user_info; not available in list_users because roles "
"is a relationship, not a selectable column)",
)
+
+ @field_validator("roles", mode="before")
+ @classmethod
+ def _extract_role_names(cls, v: Any) -> list[str] | None:
+ """Coerce Role ORM objects to their .name strings."""
+ if v is None:
+ return None
+ result: list[str] = []
+ for item in v:
+ if isinstance(item, str):
+ result.append(item)
+ elif hasattr(item, "name"):
+ result.append(str(item.name))
+ return result if result else None
Review Comment:
**Suggestion:** The validator collapses an explicitly empty role list to
`None` via `return result if result else None`. This breaks the existing
contract in `serialize_user_object`, which intentionally distinguishes "roles
included but empty" (`[]`) from "roles omitted/redacted" (`None`). Preserve an
empty list when the input is an empty iterable so callers can still tell those
states apart. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ get_user_info conflates empty roles with redacted roles.
- ⚠️ Clients cannot distinguish unprivileged users from redacted metadata.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/mcp_service/user/schemas.py:58-67`, note that
`serialize_user_object(user,
include_sensitive: bool, include_roles: bool)` is the centralized serializer
used by both
`list_users` and `get_user_info` tools (see imports in
`superset/mcp_service/user/tool/list_users.py:29-39` and
`superset/mcp_service/user/tool/get_user_info.py:28-33`).
2. Consider the metadata-allowed path in `get_user_info` where
`user_can_view_data_model_metadata()` returns `True` (default in tests via
fixture
`allow_data_model_metadata` at
`tests/unit_tests/mcp_service/user/tool/test_user_tools.py:84-99`);
`get_user_info` then
calls `serialize_user_object(obj, include_sensitive=can_view_sensitive)` with
`include_sensitive=True` (`get_user_info.py:68-77`).
3. Construct or obtain a FAB user object whose `roles` relationship is an
empty iterable
(e.g., via the same pattern as `create_mock_user` in
`tests/unit_tests/mcp_service/user/tool/test_user_tools.py:38-65`, but with
`roles=[]` so
`user.roles` is `[]`). When passed to `serialize_user_object` with
`include_sensitive=True` and default `include_roles=True`, the block at
`user/schemas.py:71-77` sets local `roles` to an empty list `[]`
(comprehension over an
empty iterable) instead of `None`, preserving the "roles included but empty"
meaning at
this stage.
4. The serializer then instantiates `UserInfo` with
`roles=[sanitize_for_llm_context(r,
field_path=("roles",)) for r in roles] if roles is not None else None`
(`user/schemas.py:80-95`). Because `roles` is `[]`, the argument passed into
the
`UserInfo.roles` field is an empty list `[]`. The `@field_validator("roles",
mode="before")` `_extract_role_names` (`user/schemas.py:108-120`) receives
`v=[]`, skips
the `v is None` branch (line 112), leaves `result` empty, and finally
executes `return
result if result else None` (line 120), returning `None`. As a result, the
constructed
`UserInfo` now has `roles=None`—indistinguishable from the "roles redacted /
not included"
paths where `include_sensitive=False` (email and roles intentionally
omitted), breaking
the previous semantic distinction between "no roles" (`[]`) and
"redacted/omitted roles"
(`None`).
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fb619be42ced4e368dda607099d264a3&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=fb619be42ced4e368dda607099d264a3&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/user/schemas.py
**Line:** 120:120
**Comment:**
*Logic Error: The validator collapses an explicitly empty role list to
`None` via `return result if result else None`. This breaks the existing
contract in `serialize_user_object`, which intentionally distinguishes "roles
included but empty" (`[]`) from "roles omitted/redacted" (`None`). Preserve an
empty list when the input is an empty iterable so callers can still tell those
states apart.
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%2F40746&comment_hash=e3bd0090429d19fe95253dc8db817273fa93134567264af51f49e81f6394c2f2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40746&comment_hash=e3bd0090429d19fe95253dc8db817273fa93134567264af51f49e81f6394c2f2&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]
