Copilot commented on code in PR #40648:
URL: https://github.com/apache/superset/pull/40648#discussion_r3337944348
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -452,6 +453,32 @@ def test_query_context_update_command(self, mock_sm_g,
mock_g):
assert len(chart.owners) == 1
assert chart.owners[0] == admin
+ @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_g):
Review Comment:
`@patch` adds a mock argument per decorator; this test patches three `g`
objects but the function signature only accepts two, so the test will error
before running. Add the missing mock parameter (and set its `user`) to keep the
Flask `g.user` consistent across modules.
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -452,6 +453,32 @@ def test_query_context_update_command(self, mock_sm_g,
mock_g):
assert len(chart.owners) == 1
assert chart.owners[0] == admin
+ @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_g):
+ """
+ A query_context-only update relaxes the ownership requirement but must
+ still require access to the chart: a non-owner without access to the
+ chart's datasource is rejected with ChartForbiddenError.
+ """
+ chart = db.session.query(Slice).all()[0]
+ pk = chart.id
+ admin = security_manager.find_user(username="admin")
+ chart.owners = [admin]
+ db.session.commit()
+
+ # gamma has no access to the energy datasource and does not own the
chart
+ gamma = security_manager.find_user(username="gamma")
+ mock_g.user = mock_sm_g.user = gamma
+ json_obj = {
Review Comment:
This test patches `superset.commands.chart.update.g` as well, but only
assigns `user` on two of the three mocked `g` objects. For consistency with the
other UpdateChartCommand tests (and to avoid future breakage if the command
starts reading `g.user` on this path), set `mock_u_g.user` too.
##########
superset/commands/chart/update.py:
##########
@@ -126,6 +131,11 @@ def validate(self) -> None: # noqa: C901
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
Review Comment:
This change alters authorization for query_context-only updates. The PR adds
a negative test (gamma forbidden), but there’s no non-skipped test asserting
that a non-owner *with* chart/datasource access (e.g. alpha) can still perform
the query_context backfill successfully (the existing success test is currently
`@skip`). Adding a positive test would help prevent regressions in Explore
lazy-backfill and report executor flows.
--
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]
