sadpandajoe commented on code in PR #44215:
URL: https://github.com/apache/superset/pull/44215#discussion_r4451517751
##########
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:
This new dataset-inheritance leg also satisfies
`CreateChartCommand.validate()` (`commands/chart/create.py:85`) and the
datasource check on chart repoint (`commands/chart/update.py:262`), which both
call `raise_for_access(datasource=...)` with inheritance allowed. A Gamma
viewer of published dashboard A, with no grant on its dataset X, can POST a
chart on X (or Save as from Explore) and becomes that chart's editor. Once they
are removed from A or A is unpublished, `_has_promiscuous_chart_access` still
admits `form_data.slice_id = <their chart>` through `is_editor(chart)`, so they
keep querying X with no accessible dashboard. The attach gate's own comment
says inherited access should last only as long as the dashboard grants it;
should the create and repoint checks opt out with
`allow_dashboard_inheritance=False` the same way?
##########
superset/daos/dashboard.py:
##########
@@ -343,8 +343,14 @@ def _raise_for_unreadable_new_charts(
selectinload(Slice.editors),
selectinload(Slice.viewers),
).filter(Slice.id.in_([chart.id for chart in candidates])).all()
+ # Access inherited through another dashboard doesn't count: it lasts only
+ # as long as that dashboard grants it, while membership here would keep it.
denied: list[int] = sorted(
- chart.id for chart in candidates if not
security_manager.can_access_chart(chart)
+ chart.id
+ for chart in candidates
+ if not security_manager.can_access_chart(
+ chart, allow_dashboard_inheritance=False
Review Comment:
This gate only runs when slices are attached through the DAO.
`generate_dashboard` (`mcp_service/dashboard/tool/generate_dashboard.py:274`)
checks `can_access_chart(chart)` with inheritance allowed and then assigns
`dashboard.slices` directly (around line 412), so it never reaches
`_raise_for_unreadable_new_charts`. A viewer of published dashboard D1 can call
it with a chart from D1 and get a new dashboard D2 on which they are an editor;
after they lose D1, the editors leg (no `published` requirement) keeps the
chart and its dataset readable. Should that tool use
`allow_dashboard_inheritance=False` or go through the DAO gate so the invariant
described in the comment above holds there too?
##########
superset/security/manager.py:
##########
@@ -2670,46 +2670,324 @@ def _semantic_layer_grant_allows(
return False
return self.can_access("datasource_access", layer_perm)
- def can_access_datasource(self, datasource: "BaseDatasource") -> bool:
+ def can_access_datasource(
+ self,
+ datasource: "BaseDatasource",
+ *,
+ allow_dashboard_inheritance: bool = True,
+ ) -> bool:
"""
Return True if the user can access the specified datasource, False
otherwise.
:param datasource: The datasource
+ :param allow_dashboard_inheritance: Whether access inherited through a
+ dashboard under ``VIEWER_PROMISCUOUS_MODE`` counts; see
+ :meth:`raise_for_access`
:returns: Whether the user can access the datasource
"""
try:
- self.raise_for_access(datasource=datasource)
+ if allow_dashboard_inheritance:
+ self.raise_for_access(datasource=datasource)
+ else:
+ # Passed only when set, so an operator's in-place replacement
+ # of ``raise_for_access`` with the older signature keeps
+ # working on the default path.
+ self.raise_for_access(
Review Comment:
The compatibility comment says an older-signature `raise_for_access`
override keeps working on the default path, but this branch is also reached on
the default path internally: the chart fallback (around line 6143) and the
dashboard fallback (around line 6116) call `can_access_datasource(...,
allow_dashboard_inheritance=False)`. With an operator override that has the
pre-PR signature and delegates to `super()`, an ordinary `can_access_chart(C)`
for a user with an explicit grant on C's dataset (chart has no viewers, user
not an editor) then raises an uncaught `TypeError` here instead of returning
True. This happens even with `ENABLE_VIEWERS` off. Is the
override-compatibility guarantee meant to cover these nested calls, and if so
how should they avoid passing the keyword?
--
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]
