test(chart): address review — deterministic access test + nits

- Patch ChartDAO.find_by_id in the integration test to bypass the
  ChartFilter base filter so the new raise_for_access gate is what
  denies; assert ChartForbiddenError only (fails on master).
- Add -> None, fix mock param names, filter by slice_name.
- Pin raise_for_access(chart=...) in unit tests; add _access_exc()
  helper so the access-denied case uses an access error, not an
  ownership error.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

superset/commands/chart/update.py

@@ -120,8 +120,13 @@ class UpdateChartCommand(UpdateMixin, BaseCommand):
         if not self._model:
             raise ChartNotFoundError()
 
-        # Check and update ownership; when only updating query context we ignore
-        # ownership so the update can be performed by report workers
+        # Check and update ownership; when only updating query context we relax
+        # the ownership requirement so that non-owners (report workers and any
+        # viewer whose UI lazily backfills a missing query_context) can perform
+        # the update. We still require access to the chart in that case, so a
+        # user cannot rewrite the query_context of a chart they cannot access
+        # (raise_for_access permits admins, owners, and users with access to the
+        # chart's datasource).
         if not is_query_context_update(self._properties):
             try:
                 security_manager.raise_for_ownership(self._model)
@@ -134,6 +139,11 @@ class UpdateChartCommand(UpdateMixin, BaseCommand):
                 raise ChartForbiddenError() from ex
             except ValidationError as ex:
                 exceptions.append(ex)
+        else:
+            try:
+                security_manager.raise_for_access(chart=self._model)
+            except SupersetSecurityException as ex:
+                raise ChartForbiddenError() from ex
 
         # validate tags
         try:

tests/integration_tests/charts/commands_tests.py

@@ -25,6 +25,7 @@ from flask import g  # noqa: F401
 from superset import db, security_manager
 from superset.commands.chart.create import CreateChartCommand
 from superset.commands.chart.exceptions import (
+    ChartForbiddenError,
     ChartNotFoundError,
     WarmUpCacheChartNotFoundError,
 )
@@ -452,6 +453,43 @@ class TestChartsUpdateCommand(SupersetTestCase):
         assert len(chart.owners) == 1
         assert chart.owners[0] == admin
 
+    @patch("superset.commands.chart.update.ChartDAO.find_by_id")
+    @patch("superset.commands.chart.update.g")
+    @patch("superset.utils.core.g")
+    @patch("superset.security.manager.g")
+    @pytest.mark.usefixtures("load_energy_table_with_slice")
+    def test_query_context_update_requires_chart_access(
+        self, mock_sm_g, mock_core_g, mock_update_g, mock_find_by_id
+    ) -> None:
+        """
+        A query_context-only update relaxes the ownership 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-owner 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.
+        """
+        chart = db.session.query(Slice).filter_by(slice_name="Energy Sankey").one()
+        pk = chart.id
+        admin = security_manager.find_user(username="admin")
+        chart.owners = [admin]
+        db.session.commit()
+
+        # Return the chart directly, bypassing ChartFilter, so the command's
+        # own raise_for_access gate is what denies the request.
+        mock_find_by_id.return_value = chart
+
+        # gamma has no access to the energy datasource and does not own the chart
+        gamma = security_manager.find_user(username="gamma")
+        mock_core_g.user = mock_sm_g.user = mock_update_g.user = gamma
+        json_obj = {
+            "query_context_generation": True,
+            "query_context": json.dumps({"foo": "bar"}),
+        }
+        with pytest.raises(ChartForbiddenError):
+            UpdateChartCommand(pk, json_obj).run()
+
     @patch("superset.commands.chart.update.g")
     @patch("superset.utils.core.g")
     @patch("superset.security.manager.g")

tests/unit_tests/commands/chart/update_test.py

@@ -33,6 +33,16 @@ def _ownership_exc() -> SupersetSecurityException:
     )
 
 
+def _access_exc() -> SupersetSecurityException:
+    return SupersetSecurityException(
+        SupersetError(
+            error_type=SupersetErrorType.CHART_SECURITY_ACCESS_ERROR,
+            message="User does not have access to this chart",
+            level=ErrorLevel.ERROR,
+        )
+    )
+
+
 def test_update_chart_ownership_enforced_for_regular_update(
     mocker: MockerFixture,
 ) -> None:
@@ -54,20 +64,70 @@ def test_update_chart_ownership_enforced_for_regular_update(
 def test_update_chart_query_context_skips_ownership_check(
     mocker: MockerFixture,
 ) -> None:
-    """Query-context-only updates skip ownership so report workers can save context."""
+    """Query-context-only updates skip the ownership check (so report workers can
+    save context) but still require access to the chart."""
     find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
     find_by_id.return_value = mocker.MagicMock(id=1, tags=[], dashboards=[])
     raise_for_ownership = mocker.patch(
         "superset.commands.chart.update.security_manager.raise_for_ownership",
         side_effect=_ownership_exc(),
     )
+    raise_for_access = mocker.patch(
+        "superset.commands.chart.update.security_manager.raise_for_access",
+    )
 
     UpdateChartCommand(
         1, {"query_context": "{}", "query_context_generation": True}
     ).validate()
 
     find_by_id.assert_called_once_with(1)
+    # ownership is relaxed, but chart access is still enforced
     raise_for_ownership.assert_not_called()
+    raise_for_access.assert_called_once_with(chart=find_by_id.return_value)
+
+
+def test_update_chart_query_context_requires_chart_access(
+    mocker: MockerFixture,
+) -> None:
+    """A query-context-only update by someone without access to the chart is
+    rejected, even though the ownership check is relaxed for this path."""
+    find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+    find_by_id.return_value = mocker.MagicMock(id=1, tags=[], dashboards=[])
+    mocker.patch(
+        "superset.commands.chart.update.security_manager.raise_for_access",
+        side_effect=_access_exc(),
+    )
+
+    with pytest.raises(ChartForbiddenError):
+        UpdateChartCommand(
+            1, {"query_context": "{}", "query_context_generation": True}
+        ).validate()
+
+
+def test_update_chart_query_context_non_owner_with_access_allowed(
+    mocker: MockerFixture,
+) -> None:
+    """A non-owner who *does* have access to the chart (e.g. an alpha user with
+    datasource access, or a report worker) can perform a query-context-only
+    backfill: ownership is relaxed and ``raise_for_access`` does not deny."""
+    find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id")
+    find_by_id.return_value = mocker.MagicMock(id=1, tags=[], dashboards=[])
+    raise_for_ownership = mocker.patch(
+        "superset.commands.chart.update.security_manager.raise_for_ownership",
+        side_effect=_ownership_exc(),
+    )
+    # access check passes (no exception) -> the non-owner is permitted
+    raise_for_access = mocker.patch(
+        "superset.commands.chart.update.security_manager.raise_for_access",
+    )
+
+    # Should not raise: the update is allowed despite the user not being an owner
+    UpdateChartCommand(
+        1, {"query_context": "{}", "query_context_generation": True}
+    ).validate()
+
+    raise_for_ownership.assert_not_called()
+    raise_for_access.assert_called_once_with(chart=find_by_id.return_value)
 
 
 def test_update_chart_owner_can_perform_regular_update(