Skip to content

feat(mcp): add dashboard owner, role, and certification management tools#41606

Draft
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:sc-111121-dashboard-owner-tools
Draft

feat(mcp): add dashboard owner, role, and certification management tools#41606
aminghadersohi wants to merge 1 commit into
apache:masterfrom
aminghadersohi:sc-111121-dashboard-owner-tools

Conversation

@aminghadersohi

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #40957 (update_dashboard MCP tool). During that review, owners, roles, certified_by, and certification_details were scoped out of update_dashboard because they're distinct governance concerns from layout/theme/metadata, and a full-replacement list has no safety guard (owners=[] could silently orphan a dashboard; roles=[]/partial lists could silently widen or narrow access). See discussion: #40957 (comment)

This PR adds three dedicated tools instead, each with explicit add/remove (or set/clear) semantics:

  • manage_dashboard_ownersadd_owner_ids/remove_owner_ids. Rejects removals that are not currently owners, and rejects any change that would leave the dashboard with zero owners. Surfaces a warning when a non-admin caller's self-removal is auto-reverted by populate_owner_list's existing self-protection.
  • manage_dashboard_rolesadd_role_ids/remove_role_ids for the DASHBOARD_RBAC access list. Reports dashboard_rbac_enabled and warns when the flag is off (roles are stored but inert).
  • manage_dashboard_certification — independent certified_by/certification_details fields; None leaves a field unchanged, "" clears it, mirroring the slug/css clear-with-empty-string convention already used by update_dashboard.

theme_id/color_scheme remain out of scope, per the original discussion — there's no list_themes/get_theme_info discovery tool yet, so a caller could only guess an ID.

All three tools are RBAC-gated the same way as update_dashboard (class_permission_name="Dashboard", method_permission_name="write", security_manager.raise_for_ownership). Since their responses disclose current owners/roles, app.py's server instructions were updated with a narrow carve-out (mirroring the existing find_users carve-out): that data may only be used to confirm the operation the caller explicitly requested on that specific dashboard, not to answer general "who owns/can access X" questions.

Test plan

  • pytest tests/unit_tests/mcp_service/dashboard/ — 288 passed (26 new + all existing dashboard MCP tests, no regressions)
  • pre-commit run on changed files (mypy, ruff, ruff-format, pylint, auto-walrus) — all passed
  • New tools registered in dashboard/tool/__init__.py and imported in app.py
  • Manual review of DEFAULT_INSTRUCTIONS tool listing and write-tool/privacy bullets for accuracy

update_dashboard intentionally dropped owners/roles as full-replacement
fields (no "keep >=1 owner" guard, and a partial list could silently
narrow/widen access). Add three dedicated tools with explicit add/remove
semantics instead: manage_dashboard_owners (rejects removing the last
owner), manage_dashboard_roles (DASHBOARD_RBAC access list), and
manage_dashboard_certification (certified_by/certification_details).
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 36.87708% with 190 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.34%. Comparing base (ce9b9b0) to head (bf5198d).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
..._service/dashboard/tool/manage_dashboard_owners.py 23.80% 64 Missing ⚠️
...p_service/dashboard/tool/manage_dashboard_roles.py 22.85% 54 Missing ⚠️
...e/dashboard/tool/manage_dashboard_certification.py 25.00% 45 Missing ⚠️
superset/mcp_service/dashboard/schemas.py 67.85% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #41606      +/-   ##
==========================================
- Coverage   64.42%   64.34%   -0.08%     
==========================================
  Files        2668     2671       +3     
  Lines      147182   147483     +301     
  Branches    33947    33976      +29     
==========================================
+ Hits        94821    94905      +84     
- Misses      50646    50861     +215     
- Partials     1715     1717       +2     
Flag Coverage Δ
hive 39.09% <36.87%> (-0.01%) ⬇️
mysql ?
postgres 57.63% <36.87%> (-0.10%) ⬇️
presto 40.64% <36.87%> (-0.02%) ⬇️
python 59.00% <36.87%> (-0.14%) ⬇️
sqlite 57.27% <36.87%> (-0.09%) ⬇️
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants