rebenitez1802 commented on PR #41606:
URL: https://github.com/apache/superset/pull/41606#issuecomment-4985005483
Request changes: strong PR — editorship authorization, input/output
sanitization, and the zero-owner guard are all sound and well-tested. One
should-fix before merge (the roles tool is missing the anti-enumeration no-op
guard its sibling deliberately implements) plus polish items.
🟡 **Medium — `manage_dashboard_roles` is missing the no-op short-circuit
that `manage_dashboard_owners` has**
`manage_dashboard_owners` deliberately short-circuits a no-op and returns an
*empty* `owners` list (owners tool ~L345–357) so it "cannot be used as a
disguised directory lookup," but `manage_dashboard_roles` has no equivalent
guard: an editor calling it with an already-assigned role commits a redundant
write and gets the **full** current `roles` list back every time (roles tool
~L263–336), contradicting its own docstring privacy promise (L221–226). This is
not a capability-matrix escalation — the caller is already an editor who can
see roles in the UI, so it's out of scope as a SECURITY.md vuln — but it breaks
the author's own stated invariant and is the one behavioral inconsistency worth
fixing. Fix: add the guard right after `assert new_role_ids is not None`
(~L261), mirroring the owners tool, and add a test asserting `payload["roles"]
== []` on a no-op:
```python
if set(new_role_ids) == set(current_role_ids):
ctx.info(f"Dashboard {dashboard.id} roles unchanged; no-op request.")
return ManageDashboardRolesResponse(
dashboard_url=_dashboard_url(dashboard),
viewers_enabled=viewers_enabled,
warnings=warnings
+ ["No effective change: requested roles already match the current
state."],
)
```
🟢 **Low — Owners tool commits a redundant write on a caller self-removal
no-op**
When a non-admin removes only themselves, `new_owner_ids` drops the caller
so the first no-op check (~L350) misses; `_apply_owner_change` then commits,
and `populate_subject_list(ensure_no_lockout=True)` re-adds the caller, so the
redundant-work is only detected at the second check (~L382) *after* the DB
write. Result is correct, but the write is wasted. Fix: hoist the detection
ahead of the commit, or leave a comment noting the rare path.
🟢 **Low — A valid role can be rejected as "does not exist"**
`get_or_create_user_subject` auto-creates a missing USER subject, but the
roles path uses `subjects_from_roles`, which silently skips any role lacking a
synced `Subject` row (`superset/subjects/utils.py:144-161`); the tool then
reports it as `"role IDs do not exist … Use list_roles"` even though
`list_roles` would show it. Normally mitigated by the `sync_role_subject` hooks
and the CLI backfill, so impact is low — but the error is misleading for
pre-sync roles and asymmetric with the owners tool.
🟢 **Low — `_find_and_authorize_dashboard` / `_dashboard_url` duplicated
across four tool files**
The ~53-line auth helper and the URL helper are copy-pasted across
`manage_dashboard_owners`, `manage_dashboard_roles`,
`manage_dashboard_certification`, and `update_dashboard`, so a future fix to
the auth flow needs four edits. Fine for this PR; Fix (follow-up ok): extract
to a shared `dashboard/tool/` utility.
🟢 **Low — New schemas use `typing.List[...]` instead of `list[...]`**
`ManageDashboard{Owners,Roles}Request/Response` and
`ManageDashboardCertificationResponse` use
`List[int]`/`List[SubjectInfo]`/`List[str]` (schemas.py ~L1086, 1160, 1230,
1280, 1425), while the file already has `from __future__ import annotations`
and uses `list[int]` internally. Fix: `List[...]` → `list[...]` to match the
Python 3.10+ convention for new code.
🟢 **Low — Test coverage gaps on the defensive branches**
Untested (all confirmed correct by reading, just uncovered): the
`refresh()`-fails-after-commit warning in all three tools, the nested
`rollback()`-also-fails path, the `dashboard.viewers` lazy-load
`SQLAlchemyError` in the roles tool, and the `SubjectsNotFoundValidationError`
handler in the owners tool. Each is a one-line `side_effect=SQLAlchemyError`
mock.
🟢 **Low — PR description has two stale names**
The description says the roles response reports `dashboard_rbac_enabled`
(code: `viewers_enabled`, gated by `ENABLE_VIEWERS`) and that the tools gate on
`raise_for_ownership` (code: `raise_for_editorship`). The code is correct and
consistent with `update_dashboard`; just update the PR body.
🟢 **Low (informational) — Reviewed & cleared: `removed_owner_ids=[]` when a
self-removal is auto-reverted is by design**
Flagging this because it looks like a bug at a glance: when
`ensure_no_lockout` re-adds a non-admin caller, `removed_owner_ids` is empty.
The delta comment (owners tool ~L371–374) intentionally reports deltas against
the *actual* post-resolution state so a reverted removal is "never disclosed as
a change," and a `warnings` entry already explains the revert. Since the field
means "actually removed," `[]` is correct — no change needed.
Verified clean (no action): editorship authorization is identical to
`update_dashboard` and enforced on every path; XSS + LLM-prompt-injection
sanitization on cert input and all disclosed labels/text; the zero-owner guard
and ownership-transfer flow; certification `None`/`""`/value semantics with the
sanitize-to-empty guard; the pre-commit value-capture pattern; ASF license
headers; and alphabetical `app.py` registration.
--
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]
