feat(mcp): support owners in update_dashboard tool#42002
Conversation
The update_dashboard MCP tool could edit title, description, slug, published, tags, css, layout and json_metadata, but there was no way to change a dashboard's owners from MCP. A dashboard created by an MCP agent is therefore owned only by that agent, and no MCP client can hand it to a human owner. Add an optional owners field (full-replacement list of user IDs, mirroring the REST dashboard owners semantics): unknown IDs are rejected up front with an OwnersNotFound error, an empty list clears owners, and omitting it leaves owners unchanged. Editorship is still enforced via raise_for_editorship, so only current owners or an Admin can change owners.
dosubot
Bot
added
the
api:dashboard
Code Review Agent Run #3e475dActionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| # 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. | ||
| users, _missing = _resolve_owners(request.owners) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for the resolved owners variables (or annotate the retained variable after unpacking) so this new assignment is fully typed. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
The new unpacking assignment introduces local variables without an explicit type annotation even though the variable shapes are known from the helper return type. This matches the rule requiring type hints for new or modified Python variables that can be annotated.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/update_dashboard.py
**Line:** 234:234
**Comment:**
*Custom Rule: Add an explicit type annotation for the resolved owners variables (or annotate the retained variable after unpacking) so this new assignment is fully typed.
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| # A non-empty owners list must reference existing users. An empty list is | ||
| # a valid "clear all owners" request and needs no lookup. | ||
| if request.owners: | ||
| _users, missing = _resolve_owners(request.owners) |
There was a problem hiding this comment.
Suggestion: Annotate the variable holding missing owner IDs in this new unpacking statement so the added validation path remains type-explicit. [custom_rule]
Severity Level: Minor 🧹
Why it matters? ⭐
This is another new unpacking assignment that omits an explicit type annotation for the local variable missing, which can be annotated. That is a real violation of the type-hint rule.
Rule source 📖
.cursor/rules/dev-standard.mdc (line 28)
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/update_dashboard.py
**Line:** 298:298
**Comment:**
*Custom Rule: Annotate the variable holding missing owner IDs in this new unpacking statement so the added validation path remains type-explicit.
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
| if request.owners: | ||
| _users, missing = _resolve_owners(request.owners) | ||
| if missing: | ||
| return DashboardError( | ||
| error=( | ||
| "Unknown owner user IDs: " | ||
| + ", ".join(str(m) for m in missing) | ||
| + ". Find valid IDs with list_users." | ||
| ), | ||
| error_type="OwnersNotFound", | ||
| ) |
There was a problem hiding this comment.
Suggestion: Owner validation does database lookups without handling database exceptions, and this block runs before the tool’s SQLAlchemy error wrapper. If the user lookup query fails (for example during transient DB issues), the exception escapes as an unhandled tool failure instead of returning the structured DatabaseError response used elsewhere in this function. Wrap owner resolution in except SQLAlchemyError and return a consistent DashboardError. [logic error]
Severity Level: Major ⚠️
- ⚠️ Owner validation can crash update_dashboard on database errors.
- ⚠️ MCP clients receive unstructured internal errors for owner lookups.
- ⚠️ Owner changes unavailable during transient database connectivity issues.Steps of Reproduction ✅
1. Invoke the MCP `update_dashboard` tool defined at
`superset/mcp_service/dashboard/tool/update_dashboard.py:63` with a request containing a
non-empty `owners` list (e.g. via FastMCP client invoking the `update_dashboard` tool).
2. The tool resolves the target dashboard via `_find_and_authorize_dashboard` at
`update_dashboard.py:53-99`, then calls `_validate_update_request(dashboard, request)` at
`update_dashboard.py:362-364` **outside** the surrounding `try/except SQLAlchemyError`
block that begins at `update_dashboard.py:112`.
3. Inside `_validate_update_request` at `update_dashboard.py:241-250`, when it reaches the
owners validation block at `update_dashboard.py:295-308`, it calls
`_resolve_owners(request.owners)` (definition at `update_dashboard.py:156-177`), which in
turn performs `security_manager.get_user_by_id(uid)` using SQLAlchemy-backed models.
4. During this lookup, simulate a transient database failure (e.g. disconnect or statement
error) so that `security_manager.get_user_by_id` raises `SQLAlchemyError`; because this
occurs in `_validate_update_request` before any local `try/except SQLAlchemyError` and
before the main `update_dashboard` `try/except SQLAlchemyError` (which only wraps
`_apply_field_updates` and `db.session.commit` at `update_dashboard.py:112-153`), the
exception bubbles up unhandled, causing the MCP tool call to fail with an internal error
instead of returning a structured `DashboardError(error_type="DatabaseError")` like the
tag-validation block at `update_dashboard.py:22-34`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/update_dashboard.py
**Line:** 297:307
**Comment:**
*Logic Error: Owner validation does database lookups without handling database exceptions, and this block runs before the tool’s SQLAlchemy error wrapper. If the user lookup query fails (for example during transient DB issues), the exception escapes as an unhandled tool failure instead of returning the structured `DatabaseError` response used elsewhere in this function. Wrap owner resolution in `except SQLAlchemyError` and return a consistent `DashboardError`.
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42002 +/- ##
==========================================
+ Coverage 64.21% 65.00% +0.78%
==========================================
Files 2744 2744
Lines 153571 153594 +23
Branches 35212 35218 +6
==========================================
+ Hits 98621 99844 +1223
+ Misses 53053 51843 -1210
- Partials 1897 1907 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
SUMMARY
The MCP
update_dashboardtool can edit a dashboard's title, description, slug,publishedflag, tags, CSS, layout andjson_metadata, but it had no way to set the dashboard's owners (and no other MCP tool sets owners either).Because of this, a dashboard created through the MCP (
generate_dashboard) is owned only by the MCP agent's user. An unpublished dashboard is visible only to its owners, so the human driving the agent cannot see or manage their own new dashboard, and no MCP client can transfer ownership to them. The only workarounds today are the Superset UI or direct metadata-DB access.This PR adds an optional
ownersfield toupdate_dashboard: a full-replacement list of user IDs, mirroring the REST dashboard owners semantics.OwnersNotFounderror.Editorship is still enforced via
raise_for_editorship, so only current owners or an Admin can change owners (same guarantee as the REST PUT).Discussion: #42001
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — MCP tool schema/behavior change, no UI.
TESTING INSTRUCTIONS
Unit tests added in
tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py:test_update_owners_replaces_list— owners fully replaced with the resolved users;ownersappears inchanged_fields.test_update_owners_unknown_id_errors— an unknown ID returnsOwnersNotFoundwith no mutation and no commit.Manual: call
update_dashboardwith{"identifier": <id>, "owners": [<user_id>, ...]}and verify owners change; a non-existent ID returnsOwnersNotFound; an empty list clears owners; omitting the field leaves owners unchanged.ADDITIONAL INFORMATION