This is an automated email from the ASF dual-hosted git repository. sadpandajoe pushed a commit to branch fix-report-executor-dashboard-access in repository https://gitbox.apache.org/repos/asf/superset.git
commit 0ed1af38a8d2b47ce03cdb02d923f27053427f14 Author: sadpandajoe <[email protected]> AuthorDate: Mon Oct 26 16:20:47 2026 +0000 fix(reports): honor access exclusions when checking the report executor's dashboard raise_for_access does not consult EXTRA_ACCESS_QUERY_EXCLUSIONS, but the permalink command's dashboard lookup does. An executor the hook withholds the dashboard from therefore passed the save-time check and the pre-claim access check, then hit an unhandled DashboardNotFoundError while building a tab permalink, aborting the run before it was claimed or logged. Treat a withheld dashboard as an executor access denial both when the schedule is saved and when the dashboard URLs are built. Co-Authored-By: Claude Sonnet 5.5 <[email protected]> --- superset/commands/report/base.py | 9 ++ superset/commands/report/execute.py | 13 +- .../commands/report/executor_target_access_test.py | 164 ++++++++++++++++++++- 3 files changed, 184 insertions(+), 2 deletions(-) diff --git a/superset/commands/report/base.py b/superset/commands/report/base.py index 430e78507d7..790f1690739 100644 --- a/superset/commands/report/base.py +++ b/superset/commands/report/base.py @@ -719,10 +719,19 @@ class BaseReportScheduleCommand(BaseCommand): and stored_active ): return + withheld = False try: with override_user(executor): security_manager.raise_for_access(**{kind: target}) + # ``raise_for_access`` does not consult the exclusion hook, yet + # resolving the dashboard to build its permalink does, so a + # dashboard withheld from the executor can never be rendered. + withheld = kind == "dashboard" and is_excluded_by_extra_access( + "dashboards", target.id + ) except SupersetSecurityException: + withheld = True + if withheld: current_user = get_user() reveal_executor = security_manager.is_admin() or ( current_user is not None and current_user.id == executor.id diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 263f11e21be..cd76cc78828 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -128,6 +128,7 @@ from superset.utils.decorators import ( transaction, ) from superset.utils.file import sanitize_title +from superset.utils.filters import is_excluded_by_extra_access from superset.utils.pdf import build_pdf_from_screenshots from superset.utils.report_execution import ( get_report_task_timeout_options, @@ -844,12 +845,22 @@ class BaseReportState: user = get_user() if user is None or getattr(user, "is_anonymous", False): return + dashboard = self._report_schedule.dashboard try: - security_manager.raise_for_access(dashboard=self._report_schedule.dashboard) + security_manager.raise_for_access(dashboard=dashboard) except SupersetSecurityException as ex: raise ReportScheduleExecutorAccessDeniedError( "dashboard", user.username, ex.error.message ) from ex + # ``raise_for_access`` does not consult ``EXTRA_ACCESS_QUERY_EXCLUSIONS`` + # but the permalink command's dashboard lookup does, and answers with a + # not-found error that nothing before the claim handles. + if is_excluded_by_extra_access("dashboards", dashboard.id): + raise ReportScheduleExecutorAccessDeniedError( + "dashboard", + user.username, + "the dashboard is withheld from this user", + ) def get_dashboard_urls( self, user_friendly: bool = False, **kwargs: Any diff --git a/tests/unit_tests/commands/report/executor_target_access_test.py b/tests/unit_tests/commands/report/executor_target_access_test.py index 302109008d3..9803730949b 100644 --- a/tests/unit_tests/commands/report/executor_target_access_test.py +++ b/tests/unit_tests/commands/report/executor_target_access_test.py @@ -28,7 +28,10 @@ import pytest from flask import g from pytest_mock import MockerFixture -from superset.commands.dashboard.exceptions import DashboardAccessDeniedError +from superset.commands.dashboard.exceptions import ( + DashboardAccessDeniedError, + DashboardNotFoundError, +) from superset.commands.report.create import CreateReportScheduleCommand from superset.commands.report.exceptions import ( ReportScheduleExecutorAccessDeniedError, @@ -84,6 +87,15 @@ def _raise_for_access_denying(*denied: Mock) -> Any: return raise_for_access +def _excluded_for(*withheld_from: Mock) -> Any: + """An ``is_excluded_by_extra_access`` stand-in withholding "dashboards".""" + + def is_excluded(asset_kind: str, id_value: int) -> bool: + return asset_kind == "dashboards" and getattr(g, "user", None) in withheld_from + + return is_excluded + + # --------------------------------------------------------------------------- # Save time # --------------------------------------------------------------------------- @@ -257,6 +269,55 @@ def test_create_accepts_executor_with_access_or_unresolved( _create_command().validate() +def test_create_rejects_executor_withheld_by_extra_access_exclusions( + mocker: MockerFixture, app: Any +) -> None: + """ + ``raise_for_access`` does not consult ``EXTRA_ACCESS_QUERY_EXCLUSIONS``, yet + resolving the dashboard as the executor does, so an executor the hook + withholds the dashboard from can never deliver. + """ + security_manager = _stub_create(mocker, dashboard=Mock(id=7)) + security_manager.raise_for_access.side_effect = None + mocker.patch( + "superset.commands.report.base.is_excluded_by_extra_access", + side_effect=_excluded_for(MACHINE), + ) + mocker.patch.dict( + app.config, {"ALERT_REPORTS_EXECUTORS": [FixedExecutor("machine")]} + ) + + with app.test_request_context(): + g.user = SAVER + with pytest.raises(ReportScheduleInvalidError) as exc: + _create_command().validate() + + errors = exc.value._exceptions + assert [type(error) for error in errors] == [ + ReportScheduleExecutorTargetAccessError + ] + assert errors[0].field_name == "dashboard" + assert "machine" not in str(errors[0].messages) + + +def test_create_accepts_executor_not_withheld_by_extra_access_exclusions( + mocker: MockerFixture, app: Any +) -> None: + security_manager = _stub_create(mocker, dashboard=Mock(id=7)) + security_manager.raise_for_access.side_effect = None + mocker.patch( + "superset.commands.report.base.is_excluded_by_extra_access", + side_effect=_excluded_for(SAVER), + ) + mocker.patch.dict( + app.config, {"ALERT_REPORTS_EXECUTORS": [FixedExecutor("machine")]} + ) + + with app.test_request_context(): + g.user = SAVER + _create_command().validate() + + def _update_command( mocker: MockerFixture, *, @@ -335,6 +396,29 @@ def test_update_rejects_resuming_schedule_executor_cannot_access( ] +def test_update_rejects_new_target_withheld_from_executor_by_exclusions( + mocker: MockerFixture, app: Any +) -> None: + security_manager = _stub_update_security(mocker, app) + security_manager.raise_for_access.side_effect = None + mocker.patch( + "superset.commands.report.base.is_excluded_by_extra_access", + side_effect=_excluded_for(MACHINE), + ) + stored, submitted = Mock(id=7), Mock(id=8) + command = _update_command( + mocker, stored_dashboard=stored, properties={"dashboard": submitted} + ) + + exceptions: list[Any] = [] + with app.test_request_context(): + command.validate_executor_target_access(exceptions) + + assert [type(error) for error in exceptions] == [ + ReportScheduleExecutorTargetAccessError + ] + + def test_update_rechecks_when_target_kind_changes_but_id_matches( mocker: MockerFixture, app: Any ) -> None: @@ -481,6 +565,84 @@ def test_plain_dashboard_url_reports_executor_and_reason( assert SPECIFIC_REASON in str(exc.value) +def test_plain_dashboard_url_rejects_dashboard_withheld_by_exclusions( + mocker: MockerFixture, app: Any +) -> None: + """ + An executor the exclusion hook withholds the dashboard from passes + ``raise_for_access`` but renders the not-found page; name the cause. + """ + mocker.patch("superset.commands.report.execute.security_manager.raise_for_access") + mocker.patch( + "superset.commands.report.execute.is_excluded_by_extra_access", + side_effect=_excluded_for(MACHINE), + ) + state = _state(mocker, None) + + with app.test_request_context(): + g.user = MACHINE + with pytest.raises(ReportScheduleExecutorAccessDeniedError) as exc: + state.get_dashboard_urls() + + assert '"machine"' in str(exc.value) + + [email protected]( + "dashboard_state", + [ + # A single selected tab. + {"anchor": "TAB-1"}, + # Several selected tabs. + {"anchor": '["TAB-1", "TAB-2"]'}, + ], +) +def test_tab_urls_reject_dashboard_withheld_by_exclusions_before_permalink( + mocker: MockerFixture, app: Any, dashboard_state: dict[str, Any] +) -> None: + """ + The permalink command resolves the dashboard through the DAO, which raises + ``DashboardNotFoundError`` for a withheld id. That is neither handled by the + pre-claim permalink step nor recorded by the state machine, so the denial + has to be raised, as the access-denied error, before the command runs. + """ + mocker.patch("superset.commands.report.execute.security_manager.raise_for_access") + mocker.patch( + "superset.commands.report.execute.is_excluded_by_extra_access", + side_effect=_excluded_for(MACHINE), + ) + mocker.patch( + "superset.commands.report.execute.feature_flag_manager.is_feature_enabled", + return_value=True, + ) + permalink = mocker.patch( + "superset.commands.report.execute.CreateDashboardPermalinkCommand.run", + side_effect=DashboardNotFoundError(), + ) + state = _state(mocker, dashboard_state) + + with app.test_request_context(): + g.user = MACHINE + with pytest.raises(ReportScheduleExecutorAccessDeniedError): + state.get_dashboard_urls() + + permalink.assert_not_called() + + +def test_dashboard_urls_build_when_exclusions_do_not_withhold_it( + mocker: MockerFixture, app: Any +) -> None: + mocker.patch("superset.commands.report.execute.security_manager.raise_for_access") + mocker.patch( + "superset.commands.report.execute.is_excluded_by_extra_access", + side_effect=_excluded_for(SAVER), + ) + state = _state(mocker, None) + + with app.test_request_context(): + g.user = MACHINE + assert state.get_dashboard_urls() + + def test_get_dashboard_urls_without_executor_skips_access_check( mocker: MockerFixture, app: Any ) -> None:
