codeant-ai-for-open-source[bot] commented on code in PR #42002:
URL: https://github.com/apache/superset/pull/42002#discussion_r3577258373
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -203,6 +227,15 @@ def _apply_field_updates(dashboard: Any, request:
UpdateDashboardRequest) -> lis
update_tags(ObjectType.dashboard, dashboard.id, dashboard.tags,
request.tags)
changed.append("tags")
+ if request.owners is not None:
+ # Full replacement of owners (empty list clears them). IDs are
+ # validated up front in _validate_update_request, so resolving here
+ # only maps the already-verified IDs to user objects.
+ owner_users: list[Any]
+ owner_users, _missing = _resolve_owners(request.owners)
+ dashboard.owners = owner_users
+ changed.append("owners")
+
Review Comment:
**Suggestion:** The owner IDs are resolved a second time during mutation,
but unresolved IDs are ignored in this phase. If a user is deleted between
validation and apply, the update will still commit with a silently truncated
owner list instead of returning `OwnersNotFound`, violating the
full-replacement contract. Reuse the validated resolved users from the
validation step (or re-check `_missing` and fail) so owner replacement is
atomic and deterministic. [race condition]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ MCP update_dashboard owner replacement can be nondeterministic.
⚠️ Concurrent user deletion silently truncates requested owner list.
⚠️ Full-replacement owners semantics violated under concurrent changes.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with the MCP service and ensure the `update_dashboard`
tool is
available; the tool entrypoint is defined in
`superset/mcp_service/dashboard/tool/update_dashboard.py:333-418` in the
`update_dashboard(request: UpdateDashboardRequest, ctx: Context)` function.
2. Trigger an update via MCP as in
`tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py:144-162`,
calling
the tool with a non-empty owners list, for example `{"identifier": 42,
"owners": [5, 9]}`
so that both user IDs initially exist and resolve in
`security_manager.get_user_by_id`.
3. During the same tool call, the handler in `update_dashboard` first runs
`_validate_update_request(dashboard, request)` at
`superset/mcp_service/dashboard/tool/update_dashboard.py:123-201`, which
calls
`_resolve_owners(request.owners)` at lines 177-185; both IDs (5 and 9)
resolve,
`missing_owner_ids` is empty, and validation succeeds, allowing the request
to proceed.
4. Before `_apply_field_updates(dashboard, request)` is executed (invoked in
the
`update_dashboard` body at
`superset/mcp_service/dashboard/tool/update_dashboard.py:382-387`), another
process
deletes user ID 9 so that `security_manager.get_user_by_id(9)` now returns
`None`; when
`_apply_field_updates` reaches the owners branch at lines 230-238, it calls
`_resolve_owners(request.owners)` a second time, receives `users=[user5]` and
`missing=[9]`, but ignores `_missing` and sets `dashboard.owners =
owner_users` followed
by `db.session.commit()` at lines 58-58, silently dropping the requested
owner 9 without
returning an `OwnersNotFound` error despite the full-replacement contract.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a18c91fd5d1740739edee98db35b6bd6&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=a18c91fd5d1740739edee98db35b6bd6&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/tool/update_dashboard.py
**Line:** 235:238
**Comment:**
*Race Condition: The owner IDs are resolved a second time during
mutation, but unresolved IDs are ignored in this phase. If a user is deleted
between validation and apply, the update will still commit with a silently
truncated owner list instead of returning `OwnersNotFound`, violating the
full-replacement contract. Reuse the validated resolved users from the
validation step (or re-check `_missing` and fail) so owner replacement is
atomic and deterministic.
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%2F42002&comment_hash=11e2b7ebaedd411a7a168aaec8e8d0b57e469a27688a697d0a67456ebb3ff578&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42002&comment_hash=11e2b7ebaedd411a7a168aaec8e8d0b57e469a27688a697d0a67456ebb3ff578&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]
