sadpandajoe commented on code in PR #45451:
URL: https://github.com/apache/superset/pull/45451#discussion_r4324328137
##########
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:
Nothing exercises this call site: the new helper tests call
`apply_extra_access_exclusions` directly, and the existing
`ChartFilter`/`DashboardAccessFilter` tests configure no exclusion hook. Moving
the call below the `can_access_all_datasources()`/`is_admin()` return, or above
the guest return, would leave every test green. Could you add a test per filter
that sets a hook and asserts the excluded id is still withheld for an
all-datasource/admin user and that an embedded guest bypasses it?
##########
superset/commands/report/base.py:
##########
@@ -142,6 +146,17 @@ 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.
+ exclusion_kind = f"report_target_{kind}s"
+ if is_excluded_by_extra_access(exclusion_kind, object_id):
+ # An empty reason falls through to the class-level message.
+ raise ReportScheduleForbiddenError(
Review Comment:
The deployment's `reason` never reaches the caller on update:
`ReportScheduleRestApi.put` catches `ReportScheduleForbiddenError` and returns
a bare `response_403()` (`superset/reports/api.py`, the `except
ReportScheduleForbiddenError` branch in `put`), so a PUT onto an excluded chart
or dashboard gets the generic denial instead of the configured text. Should the
PUT handler pass `str(ex)` through, or is the reason meant to be create-only?
##########
superset/utils/filters.py:
##########
@@ -176,3 +177,118 @@ def guest_embedded_dashboard_filter() ->
Optional[ColumnElement[bool]]:
# dashboards, never a plain internal id.
conditions.append(and_(Dashboard.id.in_(int_ids),
Dashboard.embedded.any()))
return or_(*conditions) if conditions else false()
+
+
+def apply_extra_access_exclusions(
+ query: "Query",
+ asset_kind: str,
+ id_column: "ColumnElement[Any]",
+) -> "Query":
+ """Withhold the asset IDs an ``EXTRA_ACCESS_QUERY_EXCLUSIONS`` hook names.
+
+ ``EXTRA_ACCESS_QUERY_FILTERS`` can only widen a result set -- its
conditions
+ are OR'd with the role paths -- so a deployment with its own notion of a
+ withheld asset cannot express that through it. This is the matching AND
NOT.
+
+ Returns ``query`` unchanged when no hook is configured or there is no
+ authenticated user, so the default build is unaffected.
+ """
+
+ clause = extra_access_exclusion_clause(asset_kind, id_column)
+ return query if clause is None else query.filter(clause)
+
+
+def extra_access_exclusion_clause(
+ asset_kind: str,
+ id_column: "ColumnElement[Any]",
+) -> Optional["ColumnElement[bool]"]:
+ """The ``NOT EXISTS`` clause for an exclusion hook, or None when inactive.
+
+ Deliberately ``NOT EXISTS`` rather than ``NOT IN``: a hook subquery that
+ yields a single NULL id makes ``NOT IN`` evaluate to NULL for every row,
+ which would silently empty the entire list -- for admins too. ``NOT
EXISTS``
+ simply does not match a NULL, so a malformed hook withholds nothing instead
+ of everything.
+ """
+ hook, _reason = _extra_access_exclusion_entry(asset_kind)
+ if not hook:
+ return None
+
+ from superset.utils.core import get_user_id
+
+ user_id = get_user_id()
+ if not user_id:
Review Comment:
When `get_user_id()` is `None` (anonymous requests, e.g. Public-role access
to published dashboards) both the list clause and `is_excluded_by_extra_access`
become no-ops, so a configured exclusion is silently skipped for exactly those
requests. The config comment says exclusions hold for every principal except
embedded guests; should the anonymous case be documented as a second exception,
or should the hook be evaluated for it?
##########
superset/dashboards/filters.py:
##########
@@ -130,6 +131,12 @@ def apply(self, query: Query, value: Any) -> Query:
if (guest_condition := guest_embedded_dashboard_filter()) is not None:
return query.filter(guest_condition)
+ # Applied before the short-circuit below: an exclusion has to hold for
+ # admins too, since that is precisely the principal the role paths stop
+ # narrowing. Guests are excepted above -- their token already scopes
+ # them.
+ query = apply_extra_access_exclusions(query, "dashboards",
Dashboard.id)
Review Comment:
The exclusion is only applied on the filter path, but
`DashboardDAO.get_by_id_or_slug` skips `base_filter` for UUIDs
(`Dashboard.get(id_or_slug)`), so a user who gets 404 on
`/api/v1/dashboard/<id>` for an excluded dashboard still gets 200 on
`/api/v1/dashboard/<uuid>`. Is the exclusion meant to be a visibility rule for
direct reads too, or only for lists, exports and pickers? If the former, should
the UUID branch ask the same question?
--
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]
