villebro commented on code in PR #44215:
URL: https://github.com/apache/superset/pull/44215#discussion_r4457975873
##########
superset/security/manager.py:
##########
@@ -5715,154 +6013,34 @@ 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))
+ # 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: 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 (
- # 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_)
+ allow_dashboard_inheritance
+ and
self._promiscuous_viewer_inherits_datasource(datasource)
Review Comment:
Valid, and agreed with Ville that create and repoint should refuse it. Fixed
in 4616e0dc26: `CreateChartCommand.validate()` and the repoint check in
`UpdateChartCommand.validate()` run `raise_for_access(datasource=...)` inside
`dashboard_inheritance_disabled()`, so a promiscuous viewer can still view and
explore the dashboard's charts, but creating or repointing a chart onto the
dataset needs access that doesn't come from a dashboard. Covered in
`test_viewer_inherits_published_dashboard_only` against real rows; both checks
pass without the change and are refused now. SECURITY.md now lists turning
dashboard-inherited access into lasting access (attach, create or repoint) as
in scope.
--
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]
