bito-code-review[bot] commented on code in PR #44494:
URL: https://github.com/apache/superset/pull/44494#discussion_r4084797745
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Unconditional commit in finally</b></div>
<div id="fix">
The `finally` block ends with an unconditional `db.session.commit()` (line
657). If the test body fails before `pytest.raises` (e.g. the precondition
`raise_for_access` at line 642 raises, or `run()` raises a different error),
this commit persists the embedded row and any mutated `query_context` — the
exact leak the comment says the cleanup prevents.
</div>
</div>
<small><i>Code Review Run #b22603</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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:
<div>
<div id="suggestion">
<div id="issue"><b>Ineffective embedded-row guard</b></div>
<div id="fix">
The `dashboard_was_embedded` guard is ineffective:
`EmbeddedDashboardDAO.upsert` (`superset/daos/dashboard.py:752-756`) sets
`dashboard.embedded = [embedded]` in-session, so `bool(dashboard.embedded)` at
line 609 is already `True` before the `finally` runs and the delete never
executes. Query the DB (or snapshot before `upsert`) instead of the
relationship.
</div>
</div>
<small><i>Code Review Run #b22603</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
