sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4214616067
##########
superset/daos/report.py:
##########
@@ -392,3 +404,187 @@ def bulk_delete_logs(model: ReportSchedule, from_date:
datetime) -> int | None:
)
.delete(synchronize_session="fetch")
)
+
+ @staticmethod
+ def find_with_email_recipients() -> list[ReportSchedule]:
+ """
+ Find every schedule (active or not) with at least one e-mail recipient.
+ """
+ return (
+ db.session.query(ReportSchedule)
+ .join(
+ ReportRecipients,
+ ReportRecipients.report_schedule_id == ReportSchedule.id,
+ )
+ .filter(ReportRecipients.type == ReportRecipientType.EMAIL)
+ .options(selectinload(ReportSchedule.recipients))
+ .distinct()
+ .all()
+ )
+
+ @staticmethod
+ def find_by_type(report_type: ReportScheduleType) -> list[ReportSchedule]:
+ """
+ Find every schedule (active or not) of the given type.
+ """
+ return (
+ db.session.query(ReportSchedule)
+ .filter(ReportSchedule.type == report_type)
+ .all()
+ )
+
+
+class ReportConfigDAO:
+ """
+ Access to the global Alerts & Reports configuration in ``key_value``.
+
+ One versioned document stores only settings explicitly saved by an admin.
+ Missing settings resolve to legacy application config or feature flag
values.
+ """
+
+ VERSION: Literal[1] = 1
+
+ @staticmethod
+ def get_stored_values() -> dict[str, Any]:
+ """
+ Return explicitly saved settings from the versioned document.
+
+ A stored null is distinct from a missing setting key.
+ """
+ document: ReportConfigDocument | None = KeyValueDAO.get_value(
+ KeyValueResource.ALERT_REPORT_CONFIG,
+ FIXED_RESOURCE_KEYS[KeyValueResource.ALERT_REPORT_CONFIG],
+ JsonKeyValueCodec(),
+ )
+ if document is None:
+ return {}
+ return dict(document["settings"])
+
+ @staticmethod
+ def get_fallback_value(key: ReportConfigKey) -> Any:
+ """Return the legacy application config / feature flag value for
``key``."""
+ if key == ReportConfigKey.ALERTS_ATTACH_REPORTS:
+ return
feature_flag_manager.is_feature_enabled("ALERTS_ATTACH_REPORTS")
+ if key == ReportConfigKey.DATE_FORMAT_IN_EMAIL_SUBJECT:
+ return feature_flag_manager.is_feature_enabled(
+ "DATE_FORMAT_IN_EMAIL_SUBJECT"
+ )
+ if key in (
+ ReportConfigKey.ALERT_MINIMUM_INTERVAL,
+ ReportConfigKey.REPORT_MINIMUM_INTERVAL,
+ ):
+ value = current_app.config.get(key.upper(), 0)
+ return value() if callable(value) else value
+ # These are new configs, no legacy fallback
+ if key == ReportConfigKey.LIMIT_RECIPIENTS_TO_USERS:
+ return False
+ if key == ReportConfigKey.ALLOWED_EMAIL_DOMAINS:
+ return []
+ return None
+
+ @staticmethod
+ def get_effective_value(key: ReportConfigKey) -> Any:
+ """
+ Return the value in effect for ``key``: the stored value when present,
+ otherwise the legacy fallback.
+ """
+ if key in (stored := ReportConfigDAO.get_stored_values()):
+ return stored[key]
+ return ReportConfigDAO.get_fallback_value(key)
+
+ @staticmethod
+ def get_effective_config() -> dict[str, Any]:
+ """Return the value in effect for every ``ReportConfigKey``."""
+ stored = ReportConfigDAO.get_stored_values()
+ return {
+ key.value: (
+ stored[key]
+ if key in stored
+ else ReportConfigDAO.get_fallback_value(key)
+ )
+ for key in ReportConfigKey
+ }
+
+ @staticmethod
+ def upsert(values: dict[str, Any]) -> None:
+ """
+ Merge submitted settings into the shared document without committing.
+ Only absent keys inherit application configuration; null stays
explicit.
+ """
+ stored = ReportConfigDAO.get_stored_values()
Review Comment:
`upsert` reads the stored document, merges the submitted keys in Python,
then writes the whole value back with no lock or version check. Two admins
saving different settings at the same time can both read the same snapshot, so
the later write drops the earlier one while both requests return 200. For
example, one saves `allowed_email_domains=["example.com"]` and the other saves
only `date_format_in_email_subject`; if the second commits last, the allowlist
silently disappears and delivery goes back to unrestricted. This is separate
from the first-insert collision, since the row already exists here. Could the
read-merge-write be serialized (e.g. `SELECT ... FOR UPDATE` on the row) or
guarded by a compare-and-swap on the stored value?
##########
tests/unit_tests/commands/report/executor_test.py:
##########
@@ -0,0 +1,554 @@
+# 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, 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")
+
+ 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),
+ )
+ mocker.patch(
+
"superset.commands.report.execute.ReportConfigDAO.find_disallowed_addresses",
+ return_value=["[email protected]"],
+ )
+ schedule = Mock(spec=ReportSchedule)
+ schedule.recipients = []
Review Comment:
This test sets `schedule.recipients = []` and forces
`find_disallowed_addresses` to return `[email protected]`, so it never exercises
how `_validate_recipients_policy` extracts addresses from the schedule. If that
call were changed to pass `[]` instead of the schedule's recipients, this test
would still pass, and a `bccTarget` outside the allowlist would be delivered.
Could this use a real `ReportRecipients` config (e.g.
`target="[email protected]"`, `bccTarget="[email protected]"`) with
extraction and domain checking unstubbed, and assert that
`ReportScheduleRecipientsNotAllowedError` names the external address before any
content is generated?
--
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]
