fitzee commented on code in PR #44094:
URL: https://github.com/apache/superset/pull/44094#discussion_r4001404605
##########
superset/mcp_service/dashboard/tool/manage_dashboard_certification.py:
##########
@@ -92,6 +92,25 @@ def manage_dashboard_certification(
warnings=["No fields provided; dashboard unchanged."],
)
+ # Externally managed dashboards refuse certification CHANGES (the
+ # no-field inspect path above still returns current values): their
+ # source of truth lives outside Superset, so a badge set here would be
+ # overwritten (or drift from the certifying system) on the next
+ # external sync. This tool writes by direct attribute assignment +
+ # commit rather than through a command, so no command-layer check can
+ # protect it — the refusal must live in the tool itself, after the
+ # editorship check, where a caller with no edit rights keeps getting
+ # the plain editorship denial.
+ if dashboard.is_managed_externally:
Review Comment:
**Fix altitude: this guard is a per-tool special case, but the sibling
governance tools share the exact same command-bypassing direct-commit write
path and remain unguarded.** I verified none of them check
`is_managed_externally` (it appears only here + schemas.py):
- `manage_dashboard_owners.py:193` — `db.session.commit()` after mutating
owners
- `manage_dashboard_roles.py:287` — `db.session.commit()` after mutating
roles
- `update_dashboard.py:181-206` — direct assignment of `dashboard_title` /
`slug` / `json_metadata` / `css`, commit at 360
All three go through the same `find_and_authorize_dashboard` helper (which
does editorship but not this check), and the comment here explicitly cites
*"writes by direct attribute assignment + commit rather than through a command,
so no command-layer check can protect it"* — which is equally true of those
tools. So an LLM caller can still add/remove owners, change RBAC roles, or edit
title/metadata/CSS on an externally-managed dashboard, and those writes
drift/get overwritten on the next external sync — the same harm this PR guards
against for certification. Consider hoisting the check into a shared helper
(with care to preserve certification's no-field *inspect* path, which should
still work on managed dashboards), so all mutating tools inherit it.
##########
superset/mcp_service/dashboard/tool/manage_dashboard_certification.py:
##########
@@ -92,6 +92,25 @@ def manage_dashboard_certification(
warnings=["No fields provided; dashboard unchanged."],
)
+ # Externally managed dashboards refuse certification CHANGES (the
+ # no-field inspect path above still returns current values): their
+ # source of truth lives outside Superset, so a badge set here would be
+ # overwritten (or drift from the certifying system) on the next
+ # external sync. This tool writes by direct attribute assignment +
+ # commit rather than through a command, so no command-layer check can
+ # protect it — the refusal must live in the tool itself, after the
+ # editorship check, where a caller with no edit rights keeps getting
+ # the plain editorship denial.
+ if dashboard.is_managed_externally:
+ return ManageDashboardCertificationResponse(
+ permission_denied=True,
+ error=(
+ f"Dashboard '{dashboard.dashboard_title}' (ID: {dashboard.id})
"
+ "is managed externally; its certification is owned by the "
Review Comment:
**Doc nit:** the tool docstring (lines 62-73) still advertises unconditional
set/clear and never mentions that externally-managed dashboards are refused —
the new behavior is documented only in the response schema. An agent reading
the tool description will attempt the mutation and be surprised by the
`permission_denied` response. Worth a one-line note in the docstring (and
ideally the example) so the tool's own description reflects the refusal.
##########
superset/mcp_service/dashboard/tool/manage_dashboard_certification.py:
##########
@@ -92,6 +92,25 @@ def manage_dashboard_certification(
warnings=["No fields provided; dashboard unchanged."],
)
+ # Externally managed dashboards refuse certification CHANGES (the
+ # no-field inspect path above still returns current values): their
+ # source of truth lives outside Superset, so a badge set here would be
+ # overwritten (or drift from the certifying system) on the next
+ # external sync. This tool writes by direct attribute assignment +
+ # commit rather than through a command, so no command-layer check can
+ # protect it — the refusal must live in the tool itself, after the
+ # editorship check, where a caller with no edit rights keeps getting
+ # the plain editorship denial.
+ if dashboard.is_managed_externally:
+ return ManageDashboardCertificationResponse(
+ permission_denied=True,
Review Comment:
**Semantics: overloading `permission_denied` conflates a structural refusal
with an ACL denial, and they need opposite remediation.** The same flag is set
by `find_and_authorize_dashboard` for real access denials whose canonical
guidance is *"Ask the user to grant access; do not retry."* A
managed-externally refusal is structural — granting permissions can never
resolve it — yet a downstream agent that branches on the boolean (rather than
parsing the free-text `error`) will tell the user to request access and, worse,
may retry. The schema doc update helps for clients that read it, but the
boolean itself can't be disambiguated. Consider a distinct signal (e.g. a
`managed_externally: true` field or a machine-readable reason code) so agents
can route the two cases differently.
--
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]
