bito-code-review[bot] commented on code in PR #44494:
URL: https://github.com/apache/superset/pull/44494#discussion_r4085512737
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -573,6 +591,71 @@ def test_query_context_update_requires_chart_access(
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
+ dashboard_was_embedded = bool(dashboard.embedded)
+ embedded = EmbeddedDashboardDAO.upsert(dashboard, [])
+ db.session.flush() # the uuid is only populated on flush
+
+ # 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, persisting both the embedded row and the
+ # new query context. A rollback cannot undo a commit, so clear them
+ # explicitly rather than leaking them into every later test.
+ db.session.rollback()
+ if not dashboard_was_embedded:
+ db.session.query(EmbeddedDashboard).filter_by(
+ dashboard_id=dashboard.id
+ ).delete()
Review Comment:
<!-- Bito Reply -->
The explanation provided is correct. Since `dashboard_was_embedded` is
captured before the `upsert` operation, it accurately reflects the state of the
dashboard prior to the modification. The subsequent logic in the `finally`
block correctly uses this snapshot to determine whether to clean up the
embedded dashboard record, ensuring that the test environment remains
consistent regardless of the outcome of the `UpdateChartCommand` execution.
**tests/integration_tests/charts/commands_tests.py**
```
dashboard_was_embedded = bool(dashboard.embedded)
embedded = EmbeddedDashboardDAO.upsert(dashboard, [])
db.session.flush() # the uuid is only populated on flush
# ...
finally:
db.session.rollback()
if not dashboard_was_embedded:
db.session.query(EmbeddedDashboard).filter_by(
dashboard_id=dashboard.id
).delete()
```
##########
tests/integration_tests/charts/commands_tests.py:
##########
@@ -573,6 +591,71 @@ def test_query_context_update_requires_chart_access(
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
+ dashboard_was_embedded = bool(dashboard.embedded)
+ embedded = EmbeddedDashboardDAO.upsert(dashboard, [])
+ db.session.flush() # the uuid is only populated on flush
+
+ # 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, persisting both the embedded row and the
+ # new query context. A rollback cannot undo a commit, so clear them
+ # explicitly rather than leaking them into every later test.
+ db.session.rollback()
+ if not dashboard_was_embedded:
+ db.session.query(EmbeddedDashboard).filter_by(
+ dashboard_id=dashboard.id
+ ).delete()
+ chart.query_context = original_query_context
+ db.session.commit()
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the unconditional `db.session.commit()` in
the `finally` block is valid. If an unexpected error occurs before the
`pytest.raises` block, the `finally` block will execute, committing any partial
changes (such as the embedded row or modified `query_context`) that were
intended to be rolled back. To ensure a clean state regardless of where the
test fails, the commit should be conditional or the session should be handled
differently to avoid persisting unintended side effects.
**tests/integration_tests/charts/commands_tests.py**
```
finally:
# Should the guest gate regress, ``run()`` commits before
# ``pytest.raises`` fails, persisting both the embedded row and
the
# new query context. A rollback cannot undo a commit, so clear
them
# explicitly rather than leaking them into every later test.
db.session.rollback()
if not dashboard_was_embedded:
db.session.query(EmbeddedDashboard).filter_by(
dashboard_id=dashboard.id
).delete()
chart.query_context = original_query_context
db.session.commit()
```
--
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]
