sadpandajoe commented on code in PR #44215:
URL: https://github.com/apache/superset/pull/44215#discussion_r4446137033
##########
superset/security/manager.py:
##########
@@ -5715,154 +5949,31 @@ def raise_for_access( # noqa: C901
assert datasource
self.raise_for_unsupported_guest_rls(datasource)
- def has_promiscuous_chart_access() -> bool:
- if not (
- form_data
- and is_feature_enabled("ENABLE_VIEWERS")
- and current_app.config.get("VIEWER_PROMISCUOUS_MODE")
- and (viewer_slice_id := form_data.get("slice_id"))
- and (
- viewer_slc := self.session.query(Slice)
- .filter(Slice.id == viewer_slice_id)
- .one_or_none()
- )
- ):
- return False
-
- if not _datasource_matches(
- datasource, viewer_slc.datasource_id,
viewer_slc.datasource_type
- ):
- return False
-
- return self.is_viewer(viewer_slc) or self.is_editor(viewer_slc)
-
if not (
self.can_access_schema(datasource)
or self.can_access("datasource_access", datasource.perm or "")
# A grant on a semantic view's parent layer covers the view,
# matching SemanticView.raise_for_access (sc-119501).
or self._semantic_layer_grant_allows(datasource)
- # ``allow_editorship`` is honored here too so a caller that
opts
- # out of the editorship accept (e.g. dataset define/repoint
- # validation) gets consistent behavior across both branches,
- # rather than the flag silently applying only to the
- # database+table / query branch above.
+ # ``allow_editorship`` is honored on both branches so a caller
+ # that opts out of the editorship accept (e.g. dataset
+ # define/repoint validation) gets consistent behavior.
or (allow_editorship and self.is_editor(datasource))
- or (
- # Grant access to the datasource only if dashboard RBAC is
enabled
- # or the user is an embedded guest user with access to the
dashboard
- # and said datasource is associated with the dashboard
chart in
- # question.
- form_data
- and (dashboard_id := form_data.get("dashboardId"))
- and (
- dashboard_ := self.session.query(Dashboard)
- .filter(Dashboard.id == dashboard_id)
- .one_or_none()
- )
- and (
- (
- is_feature_enabled("EMBEDDED_SUPERSET")
- and self.is_guest_user()
- )
- or (
- is_feature_enabled("ENABLE_VIEWERS")
- and
current_app.config.get("VIEWER_PROMISCUOUS_MODE")
- and self.is_viewer(dashboard_)
- )
- )
- and (
- (
- # Native filter.
- form_data.get("type") == "NATIVE_FILTER"
- and (native_filter_id :=
form_data.get("native_filter_id"))
- and dashboard_.json_metadata
- and (json_metadata :=
json.loads(dashboard_.json_metadata))
- and any(
- _datasource_matches(
- datasource,
- target.get("datasetId"),
- target.get("datasourceType")
- or DatasourceType.TABLE,
- )
- for fltr in json_metadata.get(
- "native_filter_configuration",
- [],
- )
- for target in fltr.get("targets", [])
- if native_filter_id == fltr.get("id")
- )
- )
- or (
- form_data.get("type") != "NATIVE_FILTER"
- and (
- (
- # Chart.
- (slice_id := form_data.get("slice_id"))
- and (
- # Direct chart access (no parent)
- (
- form_data.get("parent_slice_id")
is None
- and (
- slc :=
self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
- )
- and slc in dashboard_.slices
- and _datasource_matches(
- datasource,
- slc.datasource_id,
- slc.datasource_type,
- )
- )
- or
- # Multi-layer chart child access (has
parent)
- (
- (
- parent_id := form_data.get(
- "parent_slice_id"
- )
- )
- and (
- parent_slc :=
self.session.query(Slice)
- .filter(Slice.id == parent_id)
- .one_or_none()
- )
- and parent_slc in dashboard_.slices
- # Validate child is actually part
of parent's config # noqa: E501
- and
self._validate_child_in_parent_multilayer( # noqa: E501
- child_slice_id=slice_id,
- parent_slice=parent_slc,
- )
- # Bind the request to the child
- # chart's own datasource, mirroring
- # the direct-chart leg above.
- and (
- child_slc :=
self.session.query(Slice)
- .filter(Slice.id == slice_id)
- .one_or_none()
- )
- and _datasource_matches(
- datasource,
- child_slc.datasource_id,
- child_slc.datasource_type,
- )
- )
- )
- )
- # D2D or Drill By
- or self.has_drill_access(
- form_data, dashboard_, datasource
- )
- )
- )
- )
- and self.can_access_dashboard(dashboard_)
+ # Embedded guest / promiscuous viewer, bound to the specific
+ # dashboard request (native filter, member chart, multi-layer
+ # child, or drill). Keeps guests strictly scoped to the
request.
+ or self._has_request_bound_dashboard_datasource_access(
+ datasource, form_data
)
- # Chart-viewer/editor promiscuous mode: bypass datasource
- # access if the user is a viewer or editor of the chart
- # and promiscuous mode is enabled.
- or has_promiscuous_chart_access()
+ # Chart-viewer/editor promiscuous mode: a viewer or editor of
the
+ # chart the request references may query that chart's
datasource.
+ or self._has_promiscuous_chart_access(datasource, form_data)
+ # Dashboard-viewer promiscuous mode: a viewer or editor of a
+ # dashboard that uses this datasource inherits access to it,
+ # independent of the request's ``form_data``. This is what
+ # lets ``GET /dashboard/<id>/datasets`` serve full metadata to
a
+ # promiscuous viewer.
+ or self._promiscuous_viewer_inherits_datasource(datasource)
Review Comment:
With this branch, a promiscuous viewer of dashboard A who gets dataset T
through chart C1 now also passes `can_access_datasource(T)` for every other
chart on T. The chart fallback near line 6068 then admits an unrelated chart C2
that has no viewers and sits only on a private dashboard B, so `GET
/api/v1/explore/?slice_id=<C2>` returns C2's definition to someone with no
access to B. That seems to land in the SECURITY.md carve-out for reaching a
chart that is not on an accessible dashboard. Should the chart fallback use
only independently granted dataset access, with
`_promiscuous_viewer_inherits_chart` being the only inherited path for charts?
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -1149,6 +1205,87 @@ def
test_get_dashboard_charts_strips_form_data_without_chart_access(
assert "id" in chart
assert "slice_name" in chart
+ @pytest.mark.usefixtures("create_dashboards")
+ @with_feature_flags(ENABLE_VIEWERS=True)
+ @with_config({"VIEWER_PROMISCUOUS_MODE": True})
+ def test_get_dashboard_charts_inherited_by_promiscuous_viewer(self):
Review Comment:
This test would still pass if the new `_promiscuous_viewer_inherits_chart`
call were removed from `raise_for_access`. The fixture charts have no explicit
viewers, so Gamma reaches `form_data` through the earlier
`can_access_datasource` fallback once datasource inheritance works, and the
chart branch is never exercised. Could you add a case with a dashboard whose
member chart has explicit viewers that exclude Gamma, and assert `/charts`
still returns its `form_data` for a promiscuous viewer or editor?
##########
superset/subjects/utils.py:
##########
@@ -80,6 +86,55 @@ def get_user_subject_ids_subquery(user_id: int) ->
CompoundSelect:
return union_all(user_subj, role_subj, group_subj, group_role_subj)
+def get_inherited_slice_ids_subquery(user_id: int) -> CompoundSelect:
+ """Return a Select of Slice IDs a user inherits access to via a dashboard.
+
+ An editor or viewer of a dashboard inherits access to every chart on it.
+ This expresses the promiscuous ``ENABLE_VIEWERS`` inheritance as SQL so it
+ can back both per-object access checks and (composed opt-in) list filters.
+ The feature-flag gate is the caller's responsibility — this builder is pure
+ and never executed here.
+
+ The ``published`` gate applies to the viewers leg only, tracking the
+ dashboard read gate in ``raise_for_access``: editors are admitted
+ regardless of publication state, viewers only for a published dashboard.
+ Gating editors too would strip ``form_data`` from every chart of an
+ unpublished dashboard its own editor can legitimately open.
+ """
+ subject_subquery = get_user_subject_ids_subquery(user_id)
+
+ def via(assoc: Any, *, require_published: bool) -> Select:
+ stmt = (
+ select(dashboard_slices.c.slice_id)
+ .select_from(dashboard_slices)
+ .join(Dashboard, Dashboard.id == dashboard_slices.c.dashboard_id)
+ .join(assoc, assoc.c.dashboard_id == Dashboard.id)
+ .where(assoc.c.subject_id.in_(subject_subquery))
+ )
+ if require_published:
+ stmt = stmt.where(Dashboard.published.is_(True))
+ return stmt
+
+ return union_all(
+ via(dashboard_editors, require_published=False),
Review Comment:
This only matches persisted `dashboard_editors` rows, but
`is_editor(dashboard)` also honors `EXTRA_EDITORS_RESOLVER`, and
`can_inherit_access_via_dashboard` relies on that. A user who is a dashboard
editor only through the resolver can open the dashboard, yet both SQL-backed
inheritance helpers return false, so `/dashboard/<id>/charts` still strips
`form_data` and `/datasets` still strips metadata, which is the hang this PR
fixes. Is resolver-granted editorship meant to be excluded, or should the
inheritance check cover 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]
