This is an automated email from the ASF dual-hosted git repository.
sha174n pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 082961ba168 fix(chart): require edit rights for query-context-only
updates (#44494)
082961ba168 is described below
commit 082961ba168f18422a356c2df8b724d735eec9ca
Author: Shaitan <[email protected]>
AuthorDate: Wed Sep 23 22:09:21 2026 +0100
fix(chart): require edit rights for query-context-only updates (#44494)
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
.../explore/components/ExploreChartPanel/index.tsx | 12 +-
superset/commands/chart/update.py | 11 +-
tests/integration_tests/charts/commands_tests.py | 133 +++++++++++++++++----
tests/integration_tests/reports/commands_tests.py | 65 +++++++++-
tests/unit_tests/commands/chart/update_test.py | 25 ++++
5 files changed, 219 insertions(+), 27 deletions(-)
diff --git
a/superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
b/superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
index f9c769b5c91..4df280bc09f 100644
--- a/superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
+++ b/superset-frontend/src/explore/components/ExploreChartPanel/index.tsx
@@ -29,6 +29,7 @@ import {
JsonObject,
getExtensionsRegistry,
} from '@superset-ui/core';
+import { logging } from '@apache-superset/core/utils';
import { URL_PARAMS } from 'src/constants';
import { getUrlParam } from 'src/utils/urlUtils';
import { css, styled, useTheme } from '@apache-superset/core/theme';
@@ -255,7 +256,16 @@ const ExploreChartPanel = ({
);
useEffect(() => {
- updateQueryContext();
+ updateQueryContext().catch(error => {
+ // Best-effort backfill; the chart renders either way. A 403 is expected
+ // (no write access, or a managed chart), so it stays at debug.
+ const message = 'Skipped background query context backfill';
+ if (error?.status === 403) {
+ logging.debug(message, error);
+ } else {
+ logging.warn(message, error);
+ }
+ });
}, [updateQueryContext]);
useEffect(() => {
diff --git a/superset/commands/chart/update.py
b/superset/commands/chart/update.py
index 57d1e407913..74be92654f5 100644
--- a/superset/commands/chart/update.py
+++ b/superset/commands/chart/update.py
@@ -169,8 +169,9 @@ class UpdateChartCommand(UpdateMixin, BaseCommand):
raise ChartNotFoundError()
# Check and update editorship; when only updating query context we
relax
- # editorship so report workers can save context. We still require chart
- # access so users cannot rewrite query context for charts they cannot
access.
+ # editorship so background workers can save context. Report and
thumbnail
+ # executors resolve against the schedule or the requesting user, not
the
+ # chart, so they are frequently not chart editors. Access is still
required.
if not is_query_context_update(self._properties):
try:
security_manager.raise_for_editorship(self._model)
@@ -181,6 +182,12 @@ class UpdateChartCommand(UpdateMixin, BaseCommand):
exceptions.append(ex)
raise_if_managed_externally(self._model, ChartForbiddenError)
else:
+ # ``raise_for_access`` admits a guest for every chart on the
dashboard
+ # its token embeds, but a guest holds no write capability (see the
+ # capability matrix in ``SECURITY.md``). The strict path inherits
this
+ # deny from ``is_editor``; this branch has to state it.
+ if security_manager.is_guest_user():
+ raise ChartForbiddenError()
try:
security_manager.raise_for_access(chart=self._model)
except SupersetSecurityException as ex:
diff --git a/tests/integration_tests/charts/commands_tests.py
b/tests/integration_tests/charts/commands_tests.py
index 40e7fbc32ea..a87215c5474 100644
--- a/tests/integration_tests/charts/commands_tests.py
+++ b/tests/integration_tests/charts/commands_tests.py
@@ -40,8 +40,11 @@ from superset.commands.exceptions import CommandInvalidError
from superset.commands.importers.exceptions import IncorrectVersionError
from superset.connectors.sqla.models import SqlaTable
from superset.daos.chart import ChartDAO
+from superset.daos.dashboard import EmbeddedDashboardDAO
from superset.models.core import Database
+from superset.models.embedded_dashboard import EmbeddedDashboard
from superset.models.slice import Slice
+from superset.security.guest_token import GuestTokenResourceType
from superset.utils import json
from superset.utils.core import override_user
from tests.integration_tests.base_tests import (
@@ -505,31 +508,47 @@ class TestChartsUpdateCommand(SupersetTestCase):
@patch("superset.utils.core.g")
@patch("superset.security.manager.g")
@pytest.mark.usefixtures("load_energy_table_with_slice")
- @pytest.mark.skip(reason="This test will be changed to use the
api/v1/data")
def test_query_context_update_command(self, mock_sm_g, mock_g):
"""
- Test that a user can generate the chart query context
- payload without affecting editors
+ A query-context-only update requires chart access, not editorship, so a
+ non-editor who has access to the chart's datasource can refresh the
+ stored query context. The editor list is left untouched.
"""
chart = db.session.query(Slice).all()[0]
pk = chart.id
admin = security_manager.find_user(username="admin")
- chart.editors = subjects_from_users([admin])
- db.session.commit()
+ # alpha is not an editor of this chart but has all-datasource access,
so
+ # ``raise_for_access(chart=...)`` admits it on the relaxed path. Bind
the
+ # user before the first commit, so the audit columns the commit stamps
+ # get a real user rather than the bare ``MagicMock``.
user = security_manager.find_user(username="alpha")
mock_g.user = mock_sm_g.user = user
- query_context = json.dumps({"foo": "bar"})
- json_obj = {
- "query_context_generation": True,
- "query_context": query_context,
- }
- command = UpdateChartCommand(pk, json_obj)
- command.run()
- chart = db.session.query(Slice).get(pk)
- assert chart.query_context == query_context
- assert len(chart.editors) == 1
- assert user_is_editor(admin, chart)
+
+ # This chart row is shared with every other test that selects one
+ # positionally, and both writes below are committed, so restore them.
+ original_query_context = chart.query_context
+ original_editors = list(chart.editors)
+
+ try:
+ chart.editors = subjects_from_users([admin])
+ db.session.commit()
+ query_context = json.dumps({"foo": "bar"})
+ json_obj = {
+ "query_context_generation": True,
+ "query_context": query_context,
+ }
+ command = UpdateChartCommand(pk, json_obj)
+ command.run()
+ chart = db.session.query(Slice).get(pk)
+ assert chart.query_context == query_context
+ assert len(chart.editors) == 1
+ assert user_is_editor(admin, chart)
+ finally:
+ chart = db.session.query(Slice).get(pk)
+ chart.query_context = original_query_context
+ chart.editors = original_editors
+ db.session.commit()
@patch("superset.commands.chart.update.ChartDAO.find_by_id")
@patch("superset.commands.chart.update.g")
@@ -540,13 +559,12 @@ class TestChartsUpdateCommand(SupersetTestCase):
self, mock_sm_g, mock_core_g, mock_update_g, mock_find_by_id
) -> None:
"""
- A query_context-only update relaxes the editor requirement but must
- still require access to the chart. We bypass the DAO ``ChartFilter``
- base filter (by patching ``find_by_id`` to return the chart directly)
- so the request reaches the new explicit ``raise_for_access`` check, and
- assert that a non-editor with no access to the chart's datasource is
- rejected with ``ChartForbiddenError``. This deterministically exercises
- the new branch and would fail on master, where the check is absent.
+ A query-context-only update relaxes the editor requirement but still
+ gates on chart access via ``raise_for_access(chart=...)``. We bypass
the
+ DAO ``ChartFilter`` base filter (by patching ``find_by_id`` to return
+ the chart directly) so the request reaches that check, and assert that
a
+ non-editor with no access to the chart's datasource is rejected with
+ ``ChartForbiddenError``.
"""
chart = db.session.query(Slice).filter_by(slice_name="Energy
Sankey").one()
pk = chart.id
@@ -573,6 +591,75 @@ class TestChartsUpdateCommand(SupersetTestCase):
with pytest.raises(ChartForbiddenError):
UpdateChartCommand(pk, json_obj).run()
+ @patch.dict(
+ "superset.extensions.feature_flag_manager._feature_flags",
+ EMBEDDED_SUPERSET=True,
+ )
+ @patch("superset.commands.chart.update.ChartDAO.find_by_id")
+ @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
+ def test_query_context_update_denies_guest(self, mock_find_by_id) -> None:
+ """
+ The relaxed path gates on chart access, which a guest token does pass
+ for the member charts of the dashboard it embeds. A guest nonetheless
+ holds no write capability, so a query-context-only update is denied.
+ """
+ dashboard = self.get_dash_by_slug("births")
+ chart = dashboard.slices[0]
+ original_query_context = chart.query_context
+ # Snapshot before ``upsert``, which returns the existing row when the
+ # dashboard is already embedded: only a row this test inserted may be
+ # deleted below.
+ dashboard_was_embedded = bool(dashboard.embedded)
+ embedded = EmbeddedDashboardDAO.upsert(dashboard, [])
+ db.session.flush() # the uuid is only populated on flush
+ embedded_uuid = embedded.uuid
+
+ # A real guest principal for a dashboard that actually contains the
+ # chart, so ``is_guest_user`` and ``raise_for_access`` both run for
+ # real rather than a mock standing in for either.
+ guest = security_manager.get_guest_user_from_token(
+ {
+ "user": {},
+ "resources": [
+ {
+ "type": GuestTokenResourceType.DASHBOARD,
+ "id": str(embedded.uuid),
+ }
+ ],
+ "rls_rules": [],
+ "iat": 10,
+ "exp": 20,
+ }
+ )
+
+ # Bypass ChartFilter so the command's own gates decide the outcome.
+ mock_find_by_id.return_value = chart
+
+ json_obj = {
+ "query_context_generation": True,
+ "query_context": json.dumps({"foo": "bar"}),
+ }
+ try:
+ with override_user(guest):
+ # Precondition: this guest clears the access gate, so the deny
+ # below can only come from the guest check itself.
+ security_manager.raise_for_access(chart=chart)
+
+ with pytest.raises(ChartForbiddenError):
+ UpdateChartCommand(chart.id, json_obj).run()
+ finally:
+ # Should the guest gate regress, ``run()`` commits before
+ # ``pytest.raises`` fails, and a rollback cannot undo a commit.
Drop
+ # the row this test created rather than leak it into later tests;
+ # on the passing path the rollback already discarded it.
+ db.session.rollback()
+ if not dashboard_was_embedded:
+ db.session.query(EmbeddedDashboard).filter_by(
+ uuid=embedded_uuid
+ ).delete()
+ chart.query_context = original_query_context
+ db.session.commit()
+
@patch("superset.commands.chart.update.g")
@patch("superset.utils.core.g")
@patch("superset.security.manager.g")
diff --git a/tests/integration_tests/reports/commands_tests.py
b/tests/integration_tests/reports/commands_tests.py
index c3555de37c1..264e55c54e3 100644
--- a/tests/integration_tests/reports/commands_tests.py
+++ b/tests/integration_tests/reports/commands_tests.py
@@ -47,7 +47,8 @@ except ImportError: # pragma: no cover
# Flask-SQLAlchemy 2.x
from flask_sqlalchemy import BaseQuery
-from superset import db
+from superset import db, security_manager
+from superset.commands.chart.update import UpdateChartCommand
from superset.commands.report.exceptions import (
AlertQueryError,
AlertQueryInvalidTypeError,
@@ -93,7 +94,9 @@ from superset.reports.notifications.exceptions import (
NotificationParamException,
)
from superset.tasks.types import ExecutorType
+from superset.tasks.utils import get_executor
from superset.utils import json
+from superset.utils.core import override_user
from superset.utils.database import get_example_database
from superset.utils.report_execution import ReportExecutionContext
from superset.utils.webdriver import PlaywrightTimeout
@@ -371,6 +374,33 @@ def create_report_email_chart_with_csv_no_query_context():
cleanup_report_schedule(report_schedule)
[email protected]
+def create_report_csv_no_query_context_executor_not_chart_editor(get_user):
+ """A CSV report on a chart with no stored query context, whose executor
+ edits the report but is deliberately not an editor of the chart."""
+ alpha = get_user("alpha")
+ admin = get_user("admin")
+ chart = db.session.query(Slice).first()
+ original_query_context = chart.query_context
+ original_editors = list(chart.editors)
+ chart.query_context = None
+ # Only admin may edit the chart, so the report's executor is not a chart
editor.
+ chart.editors = _subjects_for_users([admin])
+ report_schedule = create_report_notification(
+ email_target="[email protected]",
+ chart=chart,
+ report_format=ReportDataFormat.CSV,
+ name="report_csv_no_query_context_executor_not_chart_editor",
+ editors=_subjects_for_users([alpha]),
+ )
+ yield report_schedule
+
+ # Shared chart row: restore what this fixture changed (cleanup commits).
+ chart.query_context = original_query_context
+ chart.editors = original_editors
+ cleanup_report_schedule(report_schedule)
+
+
@pytest.fixture
def create_report_email_dashboard():
dashboard = db.session.query(Dashboard).first()
@@ -1219,6 +1249,39 @@ def
test_email_chart_report_schedule_with_csv_no_query_context(
screenshot_mock.assert_called_once()
[email protected]("load_birth_names_dashboard_with_slices")
+def test_csv_report_query_context_backfill_allows_non_chart_editor_executor(
+ create_report_csv_no_query_context_executor_not_chart_editor,
+):
+ """
+ A report executor that is not a chart editor can still backfill the chart's
+ stored query context, which the CSV path depends on.
+
+ Driven directly rather than through ``AsyncExecuteReportScheduleCommand``:
+ the full report path mocks out the screenshot, which is the only thing that
+ issues the query-context-only ``PUT``, so it would pass either way.
+ """
+ report_schedule =
create_report_csv_no_query_context_executor_not_chart_editor
+ chart = report_schedule.chart
+
+ # The executor ALERT_REPORTS_EXECUTORS resolves to for this report.
+ _, username = get_executor(executors=[ExecutorType.EDITOR],
model=report_schedule)
+ assert username == "alpha"
+
+ query_context = json.dumps({"mock": "query_context"})
+ with override_user(security_manager.find_user(username)):
+ # The executor is not an editor of the chart, which is what makes this
+ # the regression-prone case.
+ assert not security_manager.is_editor(chart)
+ UpdateChartCommand(
+ chart.id,
+ {"query_context_generation": True, "query_context": query_context},
+ ).run()
+
+ db.session.refresh(chart)
+ assert chart.query_context == query_context
+
+
@pytest.mark.usefixtures(
"load_birth_names_dashboard_with_slices",
"create_report_email_chart_with_text",
diff --git a/tests/unit_tests/commands/chart/update_test.py
b/tests/unit_tests/commands/chart/update_test.py
index 413be4acc98..a7ddfa941b6 100644
--- a/tests/unit_tests/commands/chart/update_test.py
+++ b/tests/unit_tests/commands/chart/update_test.py
@@ -255,6 +255,31 @@ def
test_update_chart_query_context_non_editor_with_access_allowed(
raise_for_access.assert_called_once_with(chart=find_by_id.return_value)
+def test_update_chart_query_context_denied_for_guest_user(
+ mocker: MockerFixture,
+) -> None:
+ """An embedded guest token holds no write capability on any resource, so a
+ query-context-only update is refused before the access check even though
+ ``raise_for_access`` admits a guest for the charts its dashboard embeds."""
+ # The guest deny raises before any chart attribute is read, so the default
+ # MagicMock the patch installs is enough of a model here.
+ mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+ mocker.patch(
+ "superset.commands.chart.update.security_manager.is_guest_user",
+ return_value=True,
+ )
+ raise_for_access = mocker.patch(
+ "superset.commands.chart.update.security_manager.raise_for_access",
+ )
+
+ with pytest.raises(ChartForbiddenError):
+ UpdateChartCommand(
+ 1, {"query_context": "{}", "query_context_generation": True}
+ ).validate()
+
+ raise_for_access.assert_not_called()
+
+
def test_update_chart_editor_can_perform_regular_update(
mocker: MockerFixture,
) -> None:
