sadpandajoe commented on code in PR #45451:
URL: https://github.com/apache/superset/pull/45451#discussion_r4325009718
##########
superset/charts/filters.py:
##########
@@ -123,6 +124,13 @@ def apply(self, query: Query, value: Any) -> Query:
if (guest_dashboards := guest_embedded_dashboard_filter()) is not None:
return query.filter(self.model.dashboards.any(guest_dashboards))
+ # Applied before the short-circuit below: an exclusion has to hold for
+ # principals who can reach every dataset, since those are exactly the
+ # ones the role paths stop narrowing. Guests are excepted above --
+ # their token already scopes them, and withholding here would strip
+ # charts from a dashboard they were legitimately granted.
+ query = apply_extra_access_exclusions(query, "charts", Slice.id)
Review Comment:
Applying the exclusion here also reaches `find_by_ids` for the chart export
that `DashboardExport` runs on `model.slices`. If chart 7 is withheld, an Admin
can still list dashboard 3, but `GET /api/v1/dashboard/export/?q=!(3)` raises
`ChartNotFoundError` on the id-count mismatch and the API returns a bare 404
for the whole bundle, where admins could always export before. Is failing the
whole dashboard export the intended behavior for a dashboard that contains a
withheld chart, or should the export skip the withheld chart?
##########
superset/commands/report/base.py:
##########
@@ -142,6 +143,13 @@ def _check_object_access(
if not obj:
exceptions.append(not_found_exc())
else:
+ # The related-field filter narrows the picker, but a list filter is
+ # not an authorisation boundary: ``find_by_id`` above applies the
+ # ordinary access filter, so an excluded id posted directly would
+ # otherwise be accepted. Ask the report-target question here, where
+ # the id is actually resolved.
+ if is_excluded_by_extra_access(f"report_target_{kind}s",
object_id):
Review Comment:
This only runs when the payload names a `chart` or `dashboard`. An editor of
an existing inactive report whose stored target has since been withheld can
send `PUT /api/v1/report/<id>` with just `{"active": true}`:
`validate_chart_dashboard(update=True)` takes the final `else` branch, which
only checks that a target exists, so scheduling resumes. Posting that same
target explicitly is refused with a 403. Should the effective stored target
also go through this check when a report is activated or rescheduled without
resubmitting 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]
