sadpandajoe commented on code in PR #44215:
URL: https://github.com/apache/superset/pull/44215#discussion_r4460880119
##########
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:
No test pins this `dashboard_inheritance_disabled()` block.
`test_link_requires_chart_access` only has cases where access is granted
outright or denied without inheritance, so dropping the block would leave every
test green and let a chart that is readable only through another dashboard be
attached to the target dashboard. Could you add a case where `can_access_chart`
is true only while inheritance is enabled, and assert `permission_denied is
True`, `dashboard is None`, and that `UpdateDashboardCommand` is not called?
##########
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:
This guard only covers `CreateChartCommand`, but other write paths still
turn dashboard-inherited dataset access into a chart the caller edits. `POST
/explore/?action=saveas` checks the dataset with the default
(inheritance-allowed) `raise_for_access` at `views/core.py:248` and then writes
through `ChartDAO.create` at line 424, and `copy_dashboard` with
`duplicate_slices` clones each member chart with the caller as editor after
checking only `is_editor(original_dash)`. A viewer of published dashboard A
with no grant on dataset T can save a chart on T (or an editor can copy the
dashboard with charts), then keep querying T through
`_has_promiscuous_chart_access` after losing A. Should these paths also run the
dataset check inside `dashboard_inheritance_disabled()`, so the invariant in
the comment above holds for them too?
--
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]
