diff --git a/superset/daos/report.py b/superset/daos/report.py index 83e22e3e215..ed8527cfa38 100644 --- a/superset/daos/report.py +++ b/superset/daos/report.py @@ -200,7 +200,8 @@ class ReportScheduleDAO(BaseDAO[ReportSchedule]): item = ReportSchedule() if attributes: - if recipients := attributes.pop("recipients", None): + if "recipients" in attributes: + recipients = attributes.pop("recipients") attributes["recipients"] = [ ReportRecipients( type=recipient["type"], diff --git a/superset/reports/schemas.py b/superset/reports/schemas.py index 75d11b9a758..81d58e537c2 100644 --- a/superset/reports/schemas.py +++ b/superset/reports/schemas.py @@ -14,6 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +import re from typing import Any, Optional, Union from croniter import croniter @@ -120,8 +121,10 @@ class ValidatorConfigJSONSchema(Schema): threshold = fields.Float() +EMAIL_REGEX = re.compile(r"^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$") + + class ReportRecipientConfigJSONSchema(Schema): - # TODO if email check validity target = fields.String() ccTarget = fields.String() # noqa: N815 bccTarget = fields.String() # noqa: N815 @@ -138,6 +141,34 @@ class ReportRecipientSchema(Schema): ) recipient_config_json = fields.Nested(ReportRecipientConfigJSONSchema) + @validates_schema + def validate_email_recipients(self, data: dict[str, Any], **kwargs: Any) -> None: + if data.get("type") != ReportRecipientType.EMAIL.value: + return + + config = data.get("recipient_config_json") or {} + + def validate_addresses(field: str, value: str | None, required: bool) -> None: + if not value or not value.strip(): + if required: + raise ValidationError( + {field: ["Email target is required for Email recipients"]} + ) + return + invalid = [ + addr.strip() + for addr in re.split(r"[,;]", value) + if addr.strip() and not EMAIL_REGEX.match(addr.strip()) + ] + if invalid: + raise ValidationError( + {field: [f"Invalid email address(es): {', '.join(invalid)}"]} + ) + + validate_addresses("target", config.get("target"), required=True) + validate_addresses("ccTarget", config.get("ccTarget"), required=False) + validate_addresses("bccTarget", config.get("bccTarget"), required=False) + class ReportSchedulePostSchema(Schema): type = fields.String( diff --git a/tests/integration_tests/reports/api_tests.py b/tests/integration_tests/reports/api_tests.py index 6ff6547a58b..ad49736fa9b 100644 --- a/tests/integration_tests/reports/api_tests.py +++ b/tests/integration_tests/reports/api_tests.py @@ -1527,6 +1527,177 @@ class TestReportSchedulesApi(SupersetTestCase): assert updated_model.chart_id == report_schedule_data["chart"] assert updated_model.database_id == report_schedule_data["database"] + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_clear_recipients(self): + """ + ReportSchedule API: clear recipients on empty list + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + assert len(report_schedule.recipients) == 2 + + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [], + } + + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 200 + db.session.expire(report_schedule) + assert report_schedule.recipients == [] + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_empty_email_target(self): + """ + ReportSchedule API: Test update with empty email target returns 400 + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [ + { + "type": ReportRecipientType.EMAIL, + "recipient_config_json": {"target": ""}, + } + ], + } + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 400 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_invalid_email(self): + """ + ReportSchedule API: Test update with invalid email returns 400 + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [ + { + "type": ReportRecipientType.EMAIL, + "recipient_config_json": {"target": "notanemail"}, + } + ], + } + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 400 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_invalid_cc_email(self): + """ + ReportSchedule API: Test update with invalid ccTarget + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [ + { + "type": ReportRecipientType.EMAIL, + "recipient_config_json": { + "target": "valid@example.com", + "ccTarget": "bademail", + }, + } + ], + } + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 400 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_invalid_bcc_email(self): + """ + ReportSchedule API: Test update with invalid bccTarget + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [ + { + "type": ReportRecipientType.EMAIL, + "recipient_config_json": { + "target": "valid@example.com", + "bccTarget": "bademail", + }, + } + ], + } + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 400 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_slack_empty_target_allowed(self): + """ + ReportSchedule API: Test that Slack recipients skip email validation + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [ + { + "type": ReportRecipientType.SLACK, + "recipient_config_json": {"target": ""}, + } + ], + } + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 200 + + @pytest.mark.usefixtures("create_report_schedules") + def test_update_report_schedule_valid_email_with_cc_bcc(self): + """ + ReportSchedule API: Test update with valid email fields + """ + report_schedule = ( + db.session.query(ReportSchedule) + .filter(ReportSchedule.name == "name2") + .one_or_none() + ) + self.login(ADMIN_USERNAME) + report_schedule_data = { + "recipients": [ + { + "type": ReportRecipientType.EMAIL, + "recipient_config_json": { + "target": "valid@example.com", + "ccTarget": "cc@example.com", + "bccTarget": "bcc@example.com", + }, + } + ], + } + uri = f"api/v1/report/{report_schedule.id}" + rv = self.put_assert_metric(uri, report_schedule_data, "put") + assert rv.status_code == 200 + @pytest.mark.usefixtures("create_working_shared_report_schedule") def test_update_report_schedule_state_working(self): """