sadpandajoe commented on code in PR #45451:
URL: https://github.com/apache/superset/pull/45451#discussion_r4326909623
##########
tests/unit_tests/reports/filters_test.py:
##########
@@ -172,3 +172,51 @@ def
test_report_execution_log_filter_non_admin_scoped_to_log_fk(
(filter_expr,) = query.filter.call_args[0]
assert filter_expr.left.table is ReportExecutionLog.__table__
assert filter_expr.left.name == "report_schedule_id"
+
+
+# -- Report target related-field filters --
+#
+# A scheduled report renders as the deployment's configured executor rather
+# than as its author, so the assets selectable as a report target can be a
+# subset of the assets the author may read. These filters express that by
+# reading their own exclusion key, and inherit every other rule.
+
+
+@patch("superset.reports.filters.apply_extra_access_exclusions")
+def test_report_target_chart_filter_uses_its_own_exclusion_key(
+ mock_exclusions: MagicMock,
+) -> None:
+ from superset.charts.filters import ChartFilter
+ from superset.reports.filters import ReportTargetChartFilter
+
+ query = MagicMock()
+ mock_exclusions.return_value = query
+
+ with patch.object(ChartFilter, "apply", return_value="delegated") as
mock_parent:
+ f = ReportTargetChartFilter("id", MagicMock())
+ assert f.apply(query, None) == "delegated"
+
+ # The ordinary "charts" key must not be what narrows a report target list.
+ assert mock_exclusions.call_args.args[1] == "report_target_charts"
+ # ...and the parent's rules still run, on the narrowed query.
+ mock_parent.assert_called_once_with(query, None)
Review Comment:
Both picker tests have the exclusion helper return the same `query` object
they pass in, then assert the parent `apply` receives that object. If
`ReportTargetChartFilter.apply` stopped using the helper's result (say, the
`query = ...` assignment was dropped), both tests would still pass while
excluded targets reappear in the related-field picker. Could the mock return a
distinct sentinel and the assertion check that the parent receives it, for the
dashboard variant too?
##########
superset/daos/dashboard.py:
##########
@@ -422,6 +423,13 @@ def get_by_id_or_slug(cls, id_or_slug: int | str) ->
Dashboard:
if is_uuid(id_or_slug):
# just get dashboard if it's uuid
dashboard = Dashboard.get(id_or_slug)
+ # This branch skips ``base_filter``, so an
+ # ``EXTRA_ACCESS_QUERY_EXCLUSIONS`` entry -- which reaches the
+ # id branch below through that filter -- would not apply here, and
+ # the same dashboard would be absent by id while readable by uuid.
+ # Withheld means withheld whichever identifier names it.
+ if dashboard and is_excluded_by_extra_access("dashboards",
dashboard.id):
Review Comment:
This makes the uuid read agree with the id read, but other direct reads of
the same assets still skip the exclusion. `/superset/dashboard/<id_or_slug>/`
resolves through `Dashboard.get()` plus `raise_for_access()`, so a withheld
dashboard still returns a 200 page with its title and description. For charts,
`/api/v1/explore/?slice_id=<id>` loads the `Slice` through `get_form_data`,
gated only by `can_access_chart`, so a chart that `/api/v1/chart/<id>` 404s
still returns its saved form data. Is "withheld" meant to cover those reads, or
only lists, exports, pickers and the REST detail endpoints? If it is the
narrower contract, should the docs and this comment say so?
--
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]
