diff --git a/superset/commands/report/exceptions.py b/superset/commands/report/exceptions.py index b51c2d1cd1f..318e3df1175 100644 --- a/superset/commands/report/exceptions.py +++ b/superset/commands/report/exceptions.py @@ -16,6 +16,7 @@ # under the License. import math +from typing import Optional from flask_babel import lazy_gettext as _ @@ -200,6 +201,19 @@ class ReportScheduleDataFrameFailedError(CommandException): message = _("Report Schedule execution failed when generating a dataframe.") +class ReportScheduleExecutorNotFoundError(CommandException): + status = 422 + + def __init__(self, username: str = "", exception: Optional[Exception] = None): + super().__init__( + _( + "Report Schedule executor user %(username)s was not found.", + username=f'"{username}"' if username else "", + ), + exception, + ) + + class ReportScheduleExecuteUnexpectedError(CommandException): message = _("Report Schedule execution got an unexpected error.") diff --git a/superset/commands/report/execute.py b/superset/commands/report/execute.py index 676f5776ad9..e45f5fd2948 100644 --- a/superset/commands/report/execute.py +++ b/superset/commands/report/execute.py @@ -17,7 +17,7 @@ import logging from collections.abc import Sequence from datetime import datetime, timedelta -from typing import Any, Optional, Union +from typing import Any, Optional, TYPE_CHECKING, Union from uuid import UUID import pandas as pd @@ -37,6 +37,7 @@ from superset.commands.report.exceptions import ( ReportScheduleDataFrameFailedError, ReportScheduleDataFrameTimeout, ReportScheduleExecuteUnexpectedError, + ReportScheduleExecutorNotFoundError, ReportScheduleNotFoundError, ReportSchedulePreviousWorkingError, ReportScheduleScreenshotFailedError, @@ -83,9 +84,36 @@ from superset.utils.screenshots import ChartScreenshot, DashboardScreenshot from superset.utils.slack import get_channels_with_search, SlackChannelTypes from superset.utils.urls import get_url_path +if TYPE_CHECKING: + from flask_appbuilder.security.sqla.models import User + logger = logging.getLogger(__name__) +def resolve_executor_user(model: ReportSchedule) -> tuple["User", str]: + """ + Resolve the executor user for a report schedule. + + Determines the configured executor username via ``get_executor`` and looks up + the corresponding user. A deleted/disabled user or a misconfigured + ``ALERT_REPORTS_EXECUTORS`` makes ``security_manager.find_user`` return + ``None``; rather than passing ``None`` into the webdriver/auth flow (which + fails with an opaque NoneType error), raise a dedicated, actionable error. + + :returns: the ``(user, username)`` pair — the username is returned alongside + the user because several call sites log it after resolution. + :raises ReportScheduleExecutorNotFoundError: if the executor user is missing. + """ + _, username = get_executor( + executors=app.config["ALERT_REPORTS_EXECUTORS"], + model=model, + ) + user = security_manager.find_user(username) + if user is None: + raise ReportScheduleExecutorNotFoundError(username) + return user, username + + class BaseReportState: current_states: list[ReportState] = [] initial: bool = False @@ -136,9 +164,17 @@ class BaseReportState: Update the report schedule type and channels for all slack recipients to v2. V2 uses ids instead of names for channels. """ + # Track each recipient mutated in this pass with its original (type, + # config) so a partial failure can revert ALL of them — not just the + # loop variable. Restoring the values to their loaded state keeps the + # persisted rows unchanged regardless of any intervening commit. + mutated: list[tuple[ReportRecipients, ReportRecipientType, str]] = [] try: for recipient in self._report_schedule.recipients: if recipient.type == ReportRecipientType.SLACK: + mutated.append( + (recipient, recipient.type, recipient.recipient_config_json) + ) recipient.type = ReportRecipientType.SLACKV2 slack_recipients = json.loads(recipient.recipient_config_json) # V1 method allowed to use leading `#` in the channel name @@ -170,8 +206,15 @@ class BaseReportState: } ) except Exception as ex: - # Revert to v1 to preserve configuration (requires manual fix) - recipient.type = ReportRecipientType.SLACK + # Revert every mutated recipient to v1 (both type AND config) to + # preserve configuration (requires manual fix). Reverting the full + # set — not just the loop variable — keeps earlier recipients + # consistent; iterating the snapshot also avoids the UnboundLocalError + # that a bare loop-variable reference raises on a pre-iteration + # failure (which would mask the real error). + for reverted_recipient, original_type, original_config in mutated: + reverted_recipient.type = original_type + reverted_recipient.recipient_config_json = original_config msg = f"Failed to update slack recipients to v2: {str(ex)}" logger.exception(msg) raise UpdateFailedError(msg) from ex @@ -423,11 +466,7 @@ class BaseReportState: """ start_time = datetime.utcnow() - _, username = get_executor( - executors=app.config["ALERT_REPORTS_EXECUTORS"], - model=self._report_schedule, - ) - user = security_manager.find_user(username) + user, username = resolve_executor_user(self._report_schedule) max_width = app.config["ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH"] @@ -513,11 +552,7 @@ class BaseReportState: def _get_csv_data(self) -> bytes: start_time = datetime.utcnow() url = self._get_url(result_format=ChartDataResultFormat.CSV) - _, username = get_executor( - executors=app.config["ALERT_REPORTS_EXECUTORS"], - model=self._report_schedule, - ) - user = security_manager.find_user(username) + user, username = resolve_executor_user(self._report_schedule) auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user) if self._report_schedule.chart.query_context is None: @@ -567,11 +602,7 @@ class BaseReportState: start_time = datetime.utcnow() url = self._get_url(result_format=ChartDataResultFormat.JSON) - _, username = get_executor( - executors=app.config["ALERT_REPORTS_EXECUTORS"], - model=self._report_schedule, - ) - user = security_manager.find_user(username) + user, username = resolve_executor_user(self._report_schedule) auth_cookies = machine_auth_provider_factory.instance.get_auth_cookies(user) if self._report_schedule.chart.query_context is None: @@ -1176,6 +1207,15 @@ class AsyncExecuteReportScheduleCommand(BaseCommand): if not self._model: raise ReportScheduleExecuteUnexpectedError() + # Resolve the executor at the run() boundary the same way master + # does: tolerate a missing user (find_user -> None) so the state + # machine still runs and its error envelope writes the ERROR + # execution-log row and sends the owner notification. The dedicated + # ReportScheduleExecutorNotFoundError guard lives at the content + # sites (_get_screenshots / _get_csv_data / _get_embedded_data), + # which raise inside that envelope. Guarding here instead would + # surface a 422 above the state machine, suppressing both the log + # row and the owner notification. _, username = get_executor( executors=app.config["ALERT_REPORTS_EXECUTORS"], model=self._model, diff --git a/tests/unit_tests/commands/report/execute_test.py b/tests/unit_tests/commands/report/execute_test.py index 5661dc0559a..68a19ef9921 100644 --- a/tests/unit_tests/commands/report/execute_test.py +++ b/tests/unit_tests/commands/report/execute_test.py @@ -28,6 +28,7 @@ from superset.commands.exceptions import UpdateFailedError from superset.commands.report.exceptions import ( ReportScheduleAlertGracePeriodError, ReportScheduleCsvFailedError, + ReportScheduleExecutorNotFoundError, ReportSchedulePreviousWorkingError, ReportScheduleScreenshotFailedError, ReportScheduleScreenshotTimeout, @@ -1096,6 +1097,81 @@ def test_screenshot_width_calculation( ) +def _executor_report_state(mocker: MockerFixture) -> BaseReportState: + report_schedule = create_report_schedule(mocker) + # _get_csv_data/_get_embedded_data build a chart-data URL from chart_id + # before resolving the executor; give it a concrete value so URL building + # succeeds and the executor resolution is actually reached. + report_schedule.chart_id = 1 + report_schedule.force_screenshot = False + return BaseReportState( + report_schedule=report_schedule, + scheduled_dttm=datetime.now(), + execution_id=UUID("084e7ee6-5557-4ecd-9632-b7f39c9ec524"), + ) + + +@pytest.mark.parametrize( + "method_name", + ["_get_screenshots", "_get_csv_data", "_get_embedded_data"], +) +def test_get_content_raises_when_executor_user_missing( + app: SupersetApp, mocker: MockerFixture, method_name: str +) -> None: + """ + When the configured executor user cannot be resolved + (``security_manager.find_user`` returns ``None``), each content path raises a + dedicated ``ReportScheduleExecutorNotFoundError`` naming the username, rather + than passing ``None`` downstream and failing with an opaque NoneType error. + """ + app.config.update( + { + "ALERT_REPORTS_MAX_CUSTOM_SCREENSHOT_WIDTH": 1600, + "WEBDRIVER_WINDOW": {"slice": (800, 600), "dashboard": (800, 600)}, + "ALERT_REPORTS_EXECUTORS": {}, + } + ) + report_state = _executor_report_state(mocker) + + with ( + patch("superset.commands.report.execute.security_manager") as mock_sm, + patch("superset.commands.report.execute.get_executor") as mock_get_executor, + patch("superset.commands.report.execute.machine_auth_provider_factory"), + ): + mock_get_executor.return_value = ("executor", "ghost_user") + mock_sm.find_user = mocker.MagicMock(return_value=None) + + with pytest.raises(ReportScheduleExecutorNotFoundError, match="ghost_user"): + getattr(report_state, method_name)() + + +def test_resolve_executor_user_returns_user_and_username( + app: SupersetApp, mocker: MockerFixture +) -> None: + """ + Happy path: when the executor user exists, the helper returns the + ``(user, username)`` tuple unchanged — locking the no-behavior-change exit + criterion for the four call sites. + """ + from superset.commands.report.execute import resolve_executor_user + + app.config.update({"ALERT_REPORTS_EXECUTORS": {}}) + report_schedule = create_report_schedule(mocker) + mock_user = mocker.MagicMock() + + with ( + patch("superset.commands.report.execute.security_manager") as mock_sm, + patch("superset.commands.report.execute.get_executor") as mock_get_executor, + ): + mock_get_executor.return_value = ("executor", "real_user") + mock_sm.find_user = mocker.MagicMock(return_value=mock_user) + + user, username = resolve_executor_user(report_schedule) + + assert user is mock_user + assert username == "real_user" + + def test_update_recipient_to_slack_v2(mocker: MockerFixture): """ Test converting a Slack recipient to Slack v2 format. @@ -1171,6 +1247,122 @@ def test_update_recipient_to_slack_v2_missing_channels(mocker: MockerFixture): mock_cmmd.update_report_schedule_slack_v2() +def test_update_recipient_to_slack_v2_reverts_all_on_partial_failure( + mocker: MockerFixture, +) -> None: + """ + When the second of two Slack recipients fails channel resolution, BOTH + recipients are fully reverted — type AND exact original + ``recipient_config_json`` string — not just the loop variable's type. This + prevents the intervening ``create_log`` commit from flushing a half-migrated, + inconsistent state. + """ + + def channels_side_effect(search_string, types, exact_match): + if search_string == "Channel-1": + return [ + { + "id": "id_channel_1", + "name": "Channel-1", + "is_member": True, + "is_private": False, + } + ] + # Second recipient: no channel found → length mismatch → UpdateFailedError + return [] + + mocker.patch( + "superset.commands.report.execute.get_channels_with_search", + side_effect=channels_side_effect, + ) + original_config_1 = json.dumps({"target": "Channel-1"}) + original_config_2 = json.dumps({"target": "Channel-2"}) + mock_report_schedule = ReportSchedule( + name="Test Report", + recipients=[ + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=original_config_1, + ), + ReportRecipients( + type=ReportRecipientType.SLACK, + recipient_config_json=original_config_2, + ), + ], + ) + + mock_cmmd = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + + with pytest.raises(UpdateFailedError): + mock_cmmd.update_report_schedule_slack_v2() + + first, second = mock_report_schedule.recipients + # The first recipient was mutated to v2 before the second failed; it must be + # reverted to its exact original type AND config string. + assert first.type == ReportRecipientType.SLACK + assert first.recipient_config_json == original_config_1 + assert second.type == ReportRecipientType.SLACK + assert second.recipient_config_json == original_config_2 + + +def test_update_recipient_to_slack_v2_pre_iteration_failure( + mocker: MockerFixture, +) -> None: + """ + A failure raised while accessing/iterating the recipients (before the loop + variable is bound) surfaces as ``UpdateFailedError``, not a ``NameError`` + that would mask the real error. + """ + + class _ExplodingRecipients: + def __iter__(self): + raise RuntimeError("recipients exploded") + + mock_report_schedule = mocker.MagicMock() + mock_report_schedule.recipients = _ExplodingRecipients() + + mock_cmmd = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + + with pytest.raises(UpdateFailedError): + mock_cmmd.update_report_schedule_slack_v2() + + +def test_update_recipient_to_slack_v2_no_slack_recipients_is_noop( + mocker: MockerFixture, +) -> None: + """ + With no SLACK recipients there is nothing to migrate: the method returns + without raising and leaves the non-Slack recipients untouched. + """ + mock_search = mocker.patch( + "superset.commands.report.execute.get_channels_with_search", + ) + mock_report_schedule = ReportSchedule( + recipients=[ + ReportRecipients( + type=ReportRecipientType.EMAIL, + recipient_config_json=json.dumps({"target": "user@example.com"}), + ), + ], + ) + + mock_cmmd: BaseReportState = BaseReportState( + mock_report_schedule, "January 1, 2021", "execution_id_example" + ) + mock_cmmd.update_report_schedule_slack_v2() + + assert mock_cmmd._report_schedule.recipients[0].type == ReportRecipientType.EMAIL + assert ( + mock_cmmd._report_schedule.recipients[0].recipient_config_json + == '{"target": "user@example.com"}' + ) + mock_search.assert_not_called() + + # --------------------------------------------------------------------------- # Tier 1: _update_query_context + create_log # ---------------------------------------------------------------------------