villebro commented on code in PR #44215:
URL: https://github.com/apache/superset/pull/44215#discussion_r4461361887
##########
superset/commands/chart/create.py:
##########
@@ -82,7 +82,11 @@ def validate(self) -> None:
validate_chart_datasource_type(datasource_type)
datasource = get_datasource_by_id(datasource_id, datasource_type)
self._properties["datasource_name"] = datasource.name
- security_manager.raise_for_access(datasource=datasource)
+ # The new chart's editor keeps querying its dataset, so access
+ # inherited through a dashboard, which lasts only as long as the
+ # dashboard grants it, doesn't count here.
+ with security_manager.dashboard_inheritance_disabled():
Review Comment:
Valid, and the pattern of these findings led us to change the design rather
than patch each path. As of 8bfcc785e1, the promiscuous grant is no longer
inheritance: it follows the legacy `DASHBOARD_RBAC` model, for listed viewers
of a published dashboard only, applied only on requests bound to that dashboard
(chart data for a member chart, native filter, multi-layer child or drill, plus
the dashboard's `/charts`/`/datasets`). Explore Save as, chart create and
repoint check the dataset with no dashboard context, so the grant can't back
them by construction. Copy (with or without `duplicate_slices`) now requires
access to each source chart in its own right. Covered in
`test_copying_a_dashboard_takes_access_to_its_charts`, against real rows.
##########
superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:
##########
@@ -261,7 +261,14 @@ def add_chart_to_existing_dashboard( # noqa: C901 —
complexity is structural
),
)
- if not security_manager.can_access_chart(new_chart):
+ from superset.mcp_service.auth import check_chart_data_access
+
+ # The dashboard keeps the chart after the caller loses any other
+ # dashboard, so access inherited through one doesn't count.
+ with security_manager.dashboard_inheritance_disabled():
Review Comment:
That block, and the context manager it relied on, are gone in 8bfcc785e1.
The MCP tools are back to master's code: `can_access_chart` no longer counts
any dashboard grant, so a chart readable only through another dashboard is
refused there without a special case. The `set_dash_metadata` layout check
covers it the same way.
##########
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:
Closed in 8bfcc785e1 by the redesign above. Explore's Save as path checks
`raise_for_access(datasource=...)` with no dashboard context, and the dashboard
grant now applies only to requests bound to that dashboard, so a viewer without
regular access to the dataset can't save a chart on 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]
