aminghadersohi commented on PR #41606:
URL: https://github.com/apache/superset/pull/41606#issuecomment-4930249938
## Bot review sweep (2026-07-09)
Addressed both bot reviews:
- **Copilot** (permissions disclosure): `manage_dashboard_roles`'s response
no longer includes role `permissions` — `serialize_role_object` now always
omits them, matching `get_role_info`'s "permissions only on explicit lookup"
contract. (This also supersedes the `model_copy(update={"permissions": None})`
autofix that landed directly on this branch — the root-cause fix means
`permissions` is never computed in the first place.)
- **Copilot + Bito (CWE-79)**, independently flagged the same gap:
`certification_details` is now sanitized the same way as `certified_by` (both
render in the `CertifiedBadge` tooltip). Added a matching XSS regression test.
All 289 dashboard MCP unit tests pass; pre-commit (mypy/ruff/pylint) clean.
Pushed as 635fbb89f8.
## ⚠️ Blocking: base has diverged (`mergeable: CONFLICTING`)
While checking rebase status I found this PR's premise has been invalidated
by #38831 ("introduce Subject model and entity editors/viewers"), merged to
master yesterday (2026-07-08, `33f0fc93ed`). That PR is a hard breaking change:
- `Dashboard.owners` and `Dashboard.roles` no longer exist on master —
replaced by `Dashboard.editors`/`Dashboard.viewers` (`Subject`-backed, unifying
User/Role/Group).
- `DASHBOARD_RBAC` is gone; superseded by the `ENABLE_VIEWERS` flag.
- `populate_owner_list`, `compute_owner_list`, `populate_roles`,
`OwnersNotFoundValidationError`, `RolesNotFoundValidationError` are all gone,
replaced by
`populate_subject_list`/`compute_subjects`/`SubjectsNotFoundValidationError`.
- Notably, upstream's `compute_subjects` already centralizes a "must have at
least one editor" lockout guard — the exact gap `manage_dashboard_owners` was
written to close for the old `owners` field.
`manage_dashboard_certification` is unaffected (plain text columns).
`manage_dashboard_owners`/`manage_dashboard_roles` are not mechanically
portable, though — rebasing them onto the new Subject model requires real
design decisions I don't want to guess at unilaterally:
1. Add/remove semantics (this PR's design) vs. upstream's full-replace-list
API shape for `compute_subjects`.
2. Whether a dashboard-access-management tool should support Group subjects
now, or stay User/Role-only for parity with the old tools.
3. There's currently no MCP discovery tool that resolves a person/role/group
to a Subject ID (`find_users` only returns plain User rows) — a prerequisite
that doesn't exist yet.
4. Whether `manage_dashboard_owners`+`manage_dashboard_roles` should
collapse into one `manage_dashboard_editors`/`manage_dashboard_viewers` pair,
since editors/viewers are now the same underlying `Subject` type.
I'll leave this PR as-is (not rebased) pending a decision on scope — happy
to redesign the owners/roles tools against the new Subject model once there's
agreement on the above, or split `manage_dashboard_certification` into its own
PR if that's preferred to unblock in the meantime.
--
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]
