sadpandajoe commented on code in PR #44992:
URL: https://github.com/apache/superset/pull/44992#discussion_r4215731178
##########
superset/commands/security/reset.py:
##########
@@ -62,7 +63,10 @@ def run(self) -> None:
db.session.delete(database)
db.session.query(Dashboard).delete()
db.session.query(Slice).delete()
- db.session.query(KeyValueEntry).delete()
+ config_uuid = FIXED_RESOURCE_KEYS[KeyValueResource.ALERT_REPORT_CONFIG]
+ db.session.query(KeyValueEntry).filter(
+ KeyValueEntry.uuid.is_(None) | (KeyValueEntry.uuid != config_uuid)
Review Comment:
Preserving the configuration row keeps its `changed_by_fk`/`created_by_fk`
pointing at the admin who saved it, and those `ab_user` FKs have no `ON
DELETE`. If that user was later demoted from Admin, the reset deletes them
below while the retained row still references them, so the reset can fail on an
FK-enforcing database (and no test runs this command with a config row
present). Should the preserved row's audit columns be cleared before the users
are deleted?
##########
superset/commands/report/execute.py:
##########
@@ -1542,7 +1597,8 @@ def _get_notification_content(self) ->
NotificationContent: # noqa: C901
)
if (
- self._report_schedule.chart
+ self._attachments_enabled()
Review Comment:
On master the embedded table for a `TEXT` alert with a chart was built
regardless of the attachments setting, but with this gate a deployment running
with attachments disabled (`ALERTS_ATTACH_REPORTS` off) silently stops
receiving that table after upgrade, even with `ALERT_REPORT_DYNAMIC_EXECUTOR`
off. The PR description says flag-off behavior is unchanged, so is dropping the
embedded data for those existing alerts intended, or should this branch stay
ungated?
##########
tests/unit_tests/reports/utils_test.py:
##########
@@ -0,0 +1,222 @@
+# 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.
+from unittest.mock import Mock
+
+import pytest
+from pytest_mock import MockerFixture
+
+from superset.commands.report.exceptions import
ReportScheduleExecutorNotFoundError
+from superset.reports.models import ReportRecipientType
+from superset.reports.utils import (
+ cron_meets_minimum_interval,
+ find_disallowed_addresses,
+ get_dynamic_executor,
+ get_email_addresses,
+ get_email_domain,
+)
+
+
+def test_get_email_addresses_from_dict_payload_includes_cc_and_bcc() -> None:
+ recipients = [
+ {
+ "type": ReportRecipientType.EMAIL,
+ "recipient_config_json": {
+ "target": "[email protected]",
+ "ccTarget": "[email protected]",
+ "bccTarget": "[email protected]",
+ },
+ },
+ {"type": ReportRecipientType.SLACK, "recipient_config_json":
{"target": "#c"}},
+ ]
+ assert get_email_addresses(recipients) == ["[email protected]", "[email protected]",
"[email protected]"]
+
+
+def test_get_email_addresses_from_model_with_json_string() -> None:
+ recipient = Mock()
+ recipient.type = ReportRecipientType.EMAIL
+ recipient.recipient_config_json = '{"target": "[email protected];[email protected]"}'
+ broken = Mock()
+ broken.type = ReportRecipientType.EMAIL
+ broken.recipient_config_json = "not json"
+ assert get_email_addresses([recipient, broken]) == ["[email protected]",
"[email protected]"]
+ assert get_email_addresses(None) == []
+
+
+def test_whitespace_separated_recipient_cannot_bypass_domain_policy() -> None:
+ recipient = Mock(
+ type=ReportRecipientType.EMAIL,
+ recipient_config_json=(
+ '{"target": "[email protected] [email protected]"}'
+ ),
+ )
+
+ addresses = get_email_addresses([recipient])
+
+ assert addresses == ["[email protected]", "[email protected]"]
+ assert find_disallowed_addresses(
+ addresses, allowed_domains=["example.com"], known_emails=None
+ ) == ["[email protected]"]
+
+
+def test_get_email_domain() -> None:
+ assert get_email_domain("[email protected]") == "example.com"
+ assert get_email_domain("not-an-email") is None
+
+
+def test_find_disallowed_addresses_domain_allowlist() -> None:
+ disallowed = find_disallowed_addresses(
+ ["[email protected]", "[email protected]", "[email protected]",
"[email protected]"],
+ allowed_domains=["Example.com"],
+ known_emails=None,
+ )
+ assert disallowed == ["[email protected]"]
+
+
+def test_find_disallowed_addresses_users_only() -> None:
+ disallowed = find_disallowed_addresses(
+ ["[email protected]", "[email protected]"],
+ allowed_domains=None,
+ known_emails={"[email protected]"},
+ )
+ assert disallowed == ["[email protected]"]
+
+
+def test_find_disallowed_addresses_no_policy() -> None:
+ assert (
+ find_disallowed_addresses(
+ ["[email protected]"], allowed_domains=[], known_emails=None
+ )
+ == []
+ )
+
+
[email protected](
+ ("cron", "minimum_interval", "expected"),
+ [
+ ("* * * * *", 0, True),
+ ("* * * * *", 119, True),
+ ("* * * * *", 300, False),
+ ("*/5 * * * *", 300, True),
+ ("*/5 * * * *", 600, False),
+ ("0 * * * *", 3600, True),
+ ("0 */2 * * *", 3600 * 3, False),
+ ("0 0 * * *", 3600 * 24, True),
+ ],
+)
+def test_cron_meets_minimum_interval(
+ cron: str, minimum_interval: int, expected: bool
+) -> None:
+ assert cron_meets_minimum_interval(cron, minimum_interval) is expected
+
+
+def test_get_dynamic_executor_disabled(mocker: MockerFixture) -> None:
+ mocker.patch("superset.reports.utils.is_feature_enabled",
return_value=False)
+ schedule = Mock(
+ run_as=Mock(),
+ run_alert_query_as=Mock(),
+ run_as_type=None,
Review Comment:
Both executor types are `None` here, so `get_dynamic_executor` returns
`None` even if the `ALERT_REPORT_DYNAMIC_EXECUTOR` guard in
`superset/reports/utils.py` is removed; the same gap exists in
`test_get_executor_user_ignores_run_as_when_feature_disabled`, which sets
`run_as` but never `run_as_type`. A schedule saved with `fixed_user` executors
then run with the flag turned off would pick up the stored identity without any
test failing. Could these set `run_as_type`/`run_alert_query_as_type` to
`fixed_user` so the flag check is what produces `None`?
--
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]
