fitzee commented on code in PR #44093:
URL: https://github.com/apache/superset/pull/44093#discussion_r4001332303
##########
superset/security/manager.py:
##########
@@ -4967,7 +4980,9 @@ def member_datasource_accessible() -> bool:
return True
return False
- if not member_slices or member_datasource_accessible():
+ if dashboard.published and (
Review Comment:
**Operational: this can silently break *already-scheduled* reports on their
next run, not just create/validate.** UPDATING.md notes a datasource-entitled
non-editor "can no longer create or validate a report schedule against an
unpublished no-viewers dashboard" — but the same tightening also hits
execution. If an alert/report is already configured against an unpublished,
empty-viewers dashboard and runs as a fixed non-owner account
(`ALERT_REPORTS_EXECUTE_AS` / a service or selenium user) that holds
`datasource_access` on a member chart, this branch now raises
`SupersetSecurityException` where it previously admitted — so a
previously-working report fails on its next scheduled run with no migration or
warning. This is a legitimate consequence of a security fix, but worth calling
out the *runtime-breakage of existing schedules* in the release note (not only
create/validate) so operators can publish or re-scope affected dashboards ahead
of upgrade.
##########
UPDATING.md:
##########
@@ -42,6 +42,10 @@ Resample projections remain capped by `MAX_RESAMPLE_ROWS`
(default
year, …) that previously skipped the check because they have no fixed
`Timedelta`.
+### Dashboard read fallback requires a published dashboard
+
+The object-read gate's datasource-based fallback — including the admit for
dashboards with no charts — now applies to **published** dashboards only,
matching the list filter's fallback, which was already published-only.
Previously an *unpublished* dashboard with an empty viewers list was readable
by any authenticated user who could access one member datasource (or by every
authenticated user, when it had no charts — including markdown-only
dashboards), even though it appeared in no default list; and removing the last
viewer subject from a dashboard silently widened access, because the viewer
branch is published-gated while the fallback was not. Owners (folded into
editors by the subjects model), editors — including resolver-granted editors —
and admins are unaffected: they are admitted before the fallback regardless of
published state. Everything consuming the gate inherits the tightening; most
visibly, a datasource-entitled non-editor can no longer create or validate a re
port schedule against an unpublished no-viewers dashboard.
Review Comment:
**Doc accuracy: the "matching the list filter's fallback" claim doesn't hold
for the no-charts admit.** This sentence lumps "the admit for dashboards with
no charts" into "now applies to published dashboards only, matching the list
filter's fallback." But the list filter does **not** admit chart-less
dashboards for ordinary users — filters.py:178-182 states "for ordinary users a
dashboard with no charts is never yielded here … even though the object gate
allows opening it — a deliberate, pre-existing asymmetry." So for the no-charts
case the object gate and the list filter still diverge. The manager.py comment
gets this right (it explicitly notes "published chart-less dashboards stay
directly readable while absent from lists — that asymmetry is intentional");
it's only UPDATING.md that overstates the parity. Suggest qualifying the
sentence so an operator reasoning about list-vs-direct-URL parity isn't misled
for chart-less dashboards.
##########
superset/security/manager.py:
##########
@@ -4967,7 +4980,9 @@ def member_datasource_accessible() -> bool:
return True
return False
- if not member_slices or member_datasource_accessible():
+ if dashboard.published and (
+ not member_slices or member_datasource_accessible()
Review Comment:
**Residual fail-open the PR re-touches but leaves open: `not member_slices`
still admits any authenticated user to a published chart-less dashboard.** For
a *published* markdown-only dashboard with an empty viewers list,
`dashboard.published and (not member_slices or …)` short-circuits `True` for a
scoped user holding zero datasource grants — so they can read the markdown
content. This is pre-existing and the comment marks it intentional ("chart-less
dashboards can still carry markdown content"), and "published" does imply broad
intra-instance visibility, so it may well be by design. Flagging only because
the PR edits this exact line: the list filter deliberately does the opposite
(see filters.py:178-182 — a chart-less dashboard is "never yielded here … even
though the object gate allows opening it"), so a published markdown dashboard
with sensitive text is directly readable but invisible in lists. If that's the
intended contract, no change needed; if not, this was the mom
ent to close it.
--
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]
