sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4222828924
##########
superset-frontend/src/features/alerts/AlertReportModal.tsx:
##########
@@ -985,10 +1072,63 @@ const AlertReportModal:
FunctionComponent<AlertReportModalProps> = ({
extra: contentType === ContentType.Dashboard ? currentAlert?.extra : {},
};
+ if (!isReport && !attachmentControlsVisible && !isEditMode) {
+ data.report_format = 'NONE';
+ }
+ if (!attachmentControlsVisible && isEditMode) {
+ // Hidden settings stay stored; only the format controls attachment
delivery.
+ delete data.chart;
+ delete data.dashboard;
+ delete data.extra;
+ delete data.custom_width;
+ delete data.force_screenshot;
+ }
+
if (data.recipients && !data.recipients.length) {
delete data.recipients;
}
+ // Preserve existing identities unless an admin chooses or the user takes
over.
+ delete data.run_as;
+ delete data.run_alert_query_as;
+ delete data.run_as_type;
+ delete data.run_alert_query_as_type;
+ if (dynamicExecutorEnabled && isAdmin) {
+ data.run_as_type = currentAlert?.run_as_type ?? null;
Review Comment:
With the flag on, an admin editing an existing report always re-sends the
stored `run_as`/`run_as_type` (and the query executor just below) even if
nothing about the executor changed. `validate_run_as` then re-validates that
user, so if the stored executor has since been deactivated, an admin who only
renames the report or changes its schedule gets "User does not exist or is
inactive" and can't save, while the same PUT without the executor fields would
succeed. Could these fields be sent only when the admin actually changed the
selection, so unrelated edits keep the stored value?
##########
tests/unit_tests/commands/report/executor_test.py:
##########
@@ -0,0 +1,613 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Execution-time behavior introduced by SIP-209: the per-schedule "Run As"
+executor, the runtime "attachments for alerts" setting and the recipient
+policy enforcement.
+"""
+
+from datetime import datetime
+from unittest.mock import Mock
+from uuid import uuid4
+
+import pytest
+from pytest_mock import MockerFixture
+
+from superset.commands.report.alert import AlertCommand
+from superset.commands.report.exceptions import (
+ ReportScheduleExecutorNotFoundError,
+ ReportScheduleRecipientsNotAllowedError,
+)
+from superset.commands.report.execute import (
+ alerts_attach_reports_enabled,
+ BaseReportState,
+ get_executor_user,
+ resolve_executor_user,
+)
+from superset.reports.models import (
+ ReportConfigKey,
+ ReportRecipients,
+ ReportRecipientType,
+ ReportSchedule,
+)
+from superset.tasks.exceptions import ExecutorNotFoundError
+
+
+def _user(username: str, active: bool = True) -> Mock:
+ user = Mock()
+ user.username = username
+ user.is_active = active
+ return user
+
+
+def test_get_executor_user_prefers_run_as_when_enabled(mocker: MockerFixture)
-> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+ get_executor =
mocker.patch("superset.commands.report.execute.get_executor")
+ run_as = _user("explicit")
+ model = ReportSchedule()
+ model.run_as = run_as
+ model.run_as_type = "fixed_user"
+ model.run_alert_query_as = None
+
+ assert get_executor_user(model) == (run_as, "explicit")
+ assert resolve_executor_user(model) == (run_as, "explicit")
+ get_executor.assert_not_called()
+
+
+def test_get_executor_user_inactive_run_as_is_reported(mocker: MockerFixture)
-> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+ model = ReportSchedule()
+ model.run_as = _user("gone", active=False)
+ model.run_as_type = "fixed_user"
+ model.run_alert_query_as = None
+
+ assert get_executor_user(model) == (None, "gone")
+ with pytest.raises(ReportScheduleExecutorNotFoundError, match="gone"):
+ resolve_executor_user(model)
+
+
+def test_get_executor_user_falls_back_to_legacy_resolution(
+ mocker: MockerFixture,
+) -> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+ mocker.patch(
+ "superset.commands.report.execute.get_executor",
+ return_value=("editor", "legacy"),
+ )
+ legacy = _user("legacy")
+ mocker.patch(
+ "superset.commands.report.execute.security_manager.find_user",
+ return_value=legacy,
+ )
+ model = ReportSchedule()
+ model.run_as = _user("stale")
+ model.run_as_type = None
+ model.run_alert_query_as = None
+
+ assert get_executor_user(model) == (legacy, "legacy")
+
+
+def test_missing_legacy_executor_has_no_username(mocker: MockerFixture) ->
None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=False)
+ mocker.patch(
+ "superset.commands.report.execute.get_executor",
+ side_effect=ExecutorNotFoundError(),
+ )
+ model = ReportSchedule()
+
+ assert get_executor_user(model) == (None, None)
+ with pytest.raises(
+ ReportScheduleExecutorNotFoundError,
+ match="Scheduled task executor not found",
+ ):
+ resolve_executor_user(model)
+
+
+def test_get_executor_user_ignores_run_as_when_feature_disabled(
+ mocker: MockerFixture,
+) -> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=False)
+ mocker.patch(
+ "superset.commands.report.execute.get_executor",
+ return_value=("editor", "legacy"),
+ )
+ legacy = _user("legacy")
+ mocker.patch(
+ "superset.commands.report.execute.security_manager.find_user",
+ return_value=legacy,
+ )
+ model = ReportSchedule()
+ model.run_as = _user("explicit")
+ model.run_as_type = "fixed_user"
+
+ assert get_executor_user(model) == (legacy, "legacy")
+
+
+def test_alert_query_uses_alert_query_executor(mocker: MockerFixture) -> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+ get_executor = mocker.patch("superset.commands.report.alert.get_executor")
+
mocker.patch("superset.commands.report.alert.security_manager.raise_for_access")
+ override_user =
mocker.patch("superset.commands.report.alert.override_user")
+ template_processor = Mock()
+ template_processor.process_template.return_value = "SELECT 1"
+ mocker.patch(
+ "superset.commands.report.alert.jinja_context.get_template_processor",
+ return_value=template_processor,
+ )
+ mocker.patch.object(AlertCommand, "_validate_rendered_sql")
+ mocker.patch.dict(
+ "flask.current_app.config", {"MUTATE_ALERT_QUERY": False}, clear=False
+ )
+
+ query_user = _user("query_user")
+ schedule = Mock(run_as_type="fixed_user",
run_alert_query_as_type="fixed_user")
+ schedule.id = 1
+ schedule.sql = "SELECT 1"
+ schedule.run_as = _user("content_user")
+ schedule.run_alert_query_as = query_user
+ schedule.database.apply_limit_to_sql.return_value = "SELECT 1 LIMIT 2"
+
+ AlertCommand(schedule, uuid4())._execute_query()
+
+ override_user.assert_called_once_with(query_user)
+ get_executor.assert_not_called()
+
+
+def test_alert_query_inactive_executor_raises(mocker: MockerFixture) -> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+ template_processor = Mock()
+ template_processor.process_template.return_value = "SELECT 1"
+ mocker.patch(
+ "superset.commands.report.alert.jinja_context.get_template_processor",
+ return_value=template_processor,
+ )
+ mocker.patch.object(AlertCommand, "_validate_rendered_sql")
+ mocker.patch.dict(
+ "flask.current_app.config", {"MUTATE_ALERT_QUERY": False}, clear=False
+ )
+ schedule = Mock(run_as_type="fixed_user", run_alert_query_as_type=None)
+ schedule.id = 1
+ schedule.sql = "SELECT 1"
+ schedule.run_as = _user("gone", active=False)
+ schedule.run_alert_query_as = None
+
+ with pytest.raises(ReportScheduleExecutorNotFoundError, match="gone"):
+ AlertCommand(schedule, uuid4())._execute_query()
+
+
[email protected](
+ ("stored", "flag", "expected"),
+ [
+ (None, True, True),
+ (None, False, False),
+ (True, False, True),
+ (False, True, False),
+ ],
+)
+def test_alerts_attach_reports_enabled(
+ mocker: MockerFixture, stored: bool | None, flag: bool, expected: bool
+) -> None:
+ mocker.patch(
+ "superset.commands.report.execute.ReportConfigDAO.get_effective_value",
+ return_value=stored if stored is not None else flag,
+ )
+ feature_flag_manager = mocker.patch(
+ "superset.commands.report.execute.feature_flag_manager"
+ )
+ feature_flag_manager.is_feature_enabled.return_value = flag
+
+ assert alerts_attach_reports_enabled() is expected
+
+
+def _policy(allowed_domains: list[str], limit_to_users: bool):
+ def effective(key: ReportConfigKey):
+ if key == ReportConfigKey.ALLOWED_EMAIL_DOMAINS:
+ return allowed_domains
+ if key == ReportConfigKey.LIMIT_RECIPIENTS_TO_USERS:
+ return limit_to_users
+ return None
+
+ return effective
+
+
+def test_send_refuses_disallowed_recipients(mocker: MockerFixture) -> None:
+ mocker.patch(
+ "superset.commands.report.execute.ReportConfigDAO.get_effective_value",
+ side_effect=_policy(["example.com"], False),
+ )
+ schedule = Mock(spec=ReportSchedule)
+ schedule.recipients = [
+ ReportRecipients(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=(
+ '{"target": "[email protected]", '
+ '"bccTarget": "[email protected]"}'
+ ),
+ )
+ ]
+ schedule.working_timeout = None
+ state = BaseReportState(schedule, datetime.utcnow(), uuid4())
+ get_content = mocker.patch.object(state, "_get_notification_content")
+ send = mocker.patch.object(state, "_send")
+
+ with pytest.raises(
+ ReportScheduleRecipientsNotAllowedError, match="[email protected]"
+ ):
+ state.send()
+
+ get_content.assert_not_called()
+ send.assert_not_called()
+
+
+def test_send_skips_policy_when_unrestricted(mocker: MockerFixture) -> None:
+ mocker.patch(
+ "superset.commands.report.execute.ReportConfigDAO.get_effective_value",
+ side_effect=_policy([], False),
+ )
+ find_disallowed = mocker.patch(
+
"superset.commands.report.execute.ReportConfigDAO.find_disallowed_addresses"
+ )
+ schedule = Mock(spec=ReportSchedule)
+ schedule.recipients = ["recipient"]
+ schedule.working_timeout = None
+ state = BaseReportState(schedule, datetime.utcnow(), uuid4())
+ content = Mock()
+ mocker.patch.object(state, "_get_notification_content",
return_value=content)
+ send = mocker.patch.object(state, "_send")
+
+ state.send()
+
+ find_disallowed.assert_not_called()
+ send.assert_called_once_with(content, ["recipient"])
+
+
[email protected]("alert_query", [False, True])
+def test_deleted_specific_user_never_falls_back(
+ mocker: MockerFixture, alert_query: bool
+) -> None:
+ from superset.reports.utils import get_dynamic_executor
+
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+ legacy = mocker.patch("superset.commands.report.execute.get_executor")
+ schedule = ReportSchedule(run_as_type="fixed_user")
+ with pytest.raises(ReportScheduleExecutorNotFoundError):
+ get_dynamic_executor(schedule, alert_query=alert_query)
+ assert get_executor_user(schedule)[0] is None
+ legacy.assert_not_called()
+
+
+def test_alert_template_and_database_share_query_identity(
+ mocker: MockerFixture,
+) -> None:
+ from flask import g
+
+ from superset.utils.core import get_user
+
+ content_user = _user("content")
+ query_user = _user("query")
+ mocker.patch.object(g, "user", content_user, create=True)
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=True)
+
mocker.patch("superset.commands.report.alert.security_manager.raise_for_access")
+ mocker.patch.object(AlertCommand, "_validate_rendered_sql")
+ mocker.patch.dict("flask.current_app.config", {"MUTATE_ALERT_QUERY": True})
+ schedule = Mock(
+ run_as=content_user,
+ run_alert_query_as=query_user,
+ run_as_type="fixed_user",
+ run_alert_query_as_type="fixed_user",
+ )
+ schedule.sql = "SELECT '{{ current_username() }}'"
+
+ def render(sql: str) -> str:
+ assert get_user() is query_user
+ return "SELECT 'query'"
+
+ def processor(**kwargs: object) -> Mock:
+ assert get_user() is query_user
+ return Mock(process_template=render)
+
+ def mutate(sql: str) -> str:
+ assert get_user() is query_user
+ return sql
+
+ def query(**kwargs: object) -> Mock:
+ assert get_user() is query_user
+ return Mock()
+
+ mocker.patch(
+ "superset.commands.report.alert.jinja_context.get_template_processor",
+ side_effect=processor,
+ )
+ schedule.database.apply_limit_to_sql.side_effect = lambda sql, limit: sql
+ schedule.database.mutate_sql_based_on_config.side_effect = mutate
+ schedule.database.get_df.side_effect = query
+ AlertCommand(schedule, uuid4())._execute_query()
+ schedule.database.get_df.assert_called_once_with(sql="SELECT 'query'")
+ assert get_user() is content_user
+
+
+def test_retry_notices_skip_disallowed_recipients_but_notify_editors(
+ mocker: MockerFixture,
+) -> None:
+ from superset.subjects.types import SubjectType
+
+ schedule = Mock(spec=ReportSchedule)
+ schedule.working_timeout = None
+ schedule.retry_notify_owners = True
+ schedule.retry_notify_recipients = True
+ schedule.editors = [
+ Mock(type=SubjectType.USER, user=Mock(email="[email protected]"))
+ ]
+ schedule.recipients = [Mock()]
+ schedule.name = "Daily"
+ state = BaseReportState(schedule, datetime.utcnow(), uuid4())
+ mocker.patch.object(
+ state,
+ "_validate_recipients_policy",
+
side_effect=ReportScheduleRecipientsNotAllowedError(["[email protected]"]),
+ )
+ mocker.patch.object(state, "_get_log_data", return_value={})
+ mocker.patch.object(state, "_get_url", return_value="https://example.com";)
+ send = mocker.patch.object(state, "_send")
+ state.send_retry_notification(1, 3, "failed")
+ recipients = send.call_args.args[1]
+ assert len(recipients) == 1
+ assert "[email protected]" in recipients[0].recipient_config_json
+ send.reset_mock()
+ with pytest.raises(ReportScheduleRecipientsNotAllowedError):
+ state.send_final_failure_report("failed")
+ send.assert_not_called()
+
+
+def test_cleared_attachment_setting_does_not_restore_feature_flag(
+ mocker: MockerFixture,
+) -> None:
+ mocker.patch(
+ "superset.commands.report.execute.ReportConfigDAO.get_effective_value",
+ return_value=None,
+ )
+ assert alerts_attach_reports_enabled() is False
+
+
[email protected](
+ "exception",
+ [
+ ReportScheduleExecutorNotFoundError("deleted"),
+ ReportScheduleRecipientsNotAllowedError(["[email protected]"]),
+ ],
+)
+def test_configuration_errors_never_retry(
+ exception: Exception, mocker: MockerFixture
+) -> None:
+ schedule = ReportSchedule(retry_on_failure=True, retry_attempt=2)
+ state = BaseReportState(schedule, datetime.now(), str(uuid4()))
+ reset = mocker.patch.object(state, "_reset_retry_counter")
+ assert state._handle_retry_or_error(str(exception), exception) is False
Review Comment:
This test never enables `ALERT_REPORTS_RETRY`, whose default is off, so
`_handle_retry_or_error` would also return False at the disabled-feature check.
If the configuration-error early return were removed, this would still pass
while a deleted executor or blocked recipient could start scheduling retries
with the flag on. Could the test turn the flag on with remaining retry budget
and also assert `_schedule_retry` is not called?
--
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]
