sha174n commented on code in PR #44494:
URL: https://github.com/apache/superset/pull/44494#discussion_r4085511065
##########
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:
`dashboard_was_embedded` is bound on the line *before* `upsert`, not
re-evaluated in the `finally`, so it is already the pre-`upsert` snapshot you
are asking for. Confirmed by simulating the gate regressing: the delete does
run and the row count ends at 0.
Tightened it anyway in 590bffbdcf, the delete now targets the uuid captured
at flush rather than the dashboard id, and a comment records why the snapshot
has to precede `upsert` (it returns the existing row when the dashboard already
has one).
##########
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:
The `finally` rolls back before it commits, so an early failure has nothing
uncommitted left to persist. I simulated a raise at the precondition on line
642: no embedded row and no `query_context` change survives, the commit only
persists the restore.
The commit is needed for the other direction, where a regressed gate makes
`run()` commit and a rollback can no longer undo it. Comment reworded in
590bffbdcf to state both halves.
--
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]
