rusackas commented on code in PR #40648:
URL: https://github.com/apache/superset/pull/40648#discussion_r3338068170
##########
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:
Fixed — added `mock_u_g` as the third parameter so all three `@patch`
decorators are captured. Also set `mock_u_g.user = gamma` alongside the other
two mocks so the command sees a consistent user context.
##########
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 is resolved by the fix above — in
`test_query_context_update_requires_chart_access` all three mocked `g` objects
now have `.user` set. The
`test_update_chart_dashboard_security_existing_relationship` test already sets
all three via `mock_u_g.user = mock_c_g.user = mock_sm_g.user = ...`.
##########
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:
Acknowledged — a positive test for a non-owner alpha user with datasource
access would provide better coverage. Left as a follow-up; the existing skip
test already documents that path, and adding it would require setting up
datasource ACLs which is outside this PR's scope.
--
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]
