fix: add fallback and validation for report and cron timezones (#17338)

* add fallback and validation for report and cron timezones

* add logging to exception catch

* Run black

Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>

CONTRIBUTING.md

@@ -420,7 +420,7 @@ For example, the image referenced above actually lives in `superset-frontend/src
 
 #### OS Dependencies
 
-Make sure your machine meets the [OS dependencies](https://superset.apache.org/docs/installation/installing-superset-from-scratch#os-dependencies) before following these steps.  
+Make sure your machine meets the [OS dependencies](https://superset.apache.org/docs/installation/installing-superset-from-scratch#os-dependencies) before following these steps.
 You also need to install MySQL or [MariaDB](https://mariadb.com/downloads).
 
 Ensure that you are using Python version 3.7 or 3.8, then proceed with:
@@ -523,6 +523,7 @@ Frontend assets (TypeScript, JavaScript, CSS, and images) must be compiled in or
 ##### nvm and node
 
 First, be sure you are using the following versions of Node.js and npm:
+
 - `Node.js`: Version 16
 - `npm`: Version 7
 
@@ -765,15 +766,21 @@ Note that the test environment uses a temporary directory for defining the
 SQLite databases which will be cleared each time before the group of test
 commands are invoked.
 
-There is also a utility script included in the Superset codebase to run python tests. The [readme can be
+There is also a utility script included in the Superset codebase to run python integration tests. The [readme can be
 found here](https://github.com/apache/superset/tree/master/scripts/tests)
 
-To run all tests for example, run this script from the root directory:
+To run all integration tests for example, run this script from the root directory:
 
 ```bash
 scripts/tests/run.sh
 ```
 
+You can run unit tests found in './tests/unit_tests' for example with pytest. It is a simple way to run an isolated test that doesn't need any database setup
+
+```bash
+pytest ./link_to_test.py
+```
+
 ### Frontend Testing
 
 We use [Jest](https://jestjs.io/) and [Enzyme](https://airbnb.io/enzyme/) to test TypeScript/JavaScript. Tests can be run with:

superset/reports/schemas.py

@@ -21,6 +21,7 @@
 from marshmallow import fields, Schema, validate, validates_schema
 from marshmallow.validate import Length, Range, ValidationError
 from marshmallow_enum import EnumField
+from pytz import all_timezones
 
 from superset.models.reports import (
     ReportCreationMethodType,
@@ -153,7 +154,11 @@ class ReportSchedulePostSchema(Schema):
         allow_none=False,
         required=True,
     )
-    timezone = fields.String(description=timezone_description, default="UTC")
+    timezone = fields.String(
+        description=timezone_description,
+        default="UTC",
+        validate=validate.OneOf(choices=tuple(all_timezones)),
+    )
     sql = fields.String(
         description=sql_description, example="SELECT value FROM time_series_table"
     )
@@ -233,7 +238,11 @@ class ReportSchedulePutSchema(Schema):
         validate=[validate_crontab, Length(1, 1000)],
         required=False,
     )
-    timezone = fields.String(description=timezone_description, default="UTC")
+    timezone = fields.String(
+        description=timezone_description,
+        default="UTC",
+        validate=validate.OneOf(choices=tuple(all_timezones)),
+    )
     sql = fields.String(
         description=sql_description,
         example="SELECT value FROM time_series_table",

superset/tasks/cron_util.py

@@ -15,21 +15,29 @@
 # specific language governing permissions and limitations
 # under the License.
 
+import logging
 from datetime import datetime, timedelta, timezone as dt_timezone
 from typing import Iterator
 
-import pytz
 from croniter import croniter
+from pytz import timezone as pytz_timezone, UnknownTimeZoneError
 
 from superset import app
 
+logger = logging.getLogger(__name__)
+
 
 def cron_schedule_window(cron: str, timezone: str) -> Iterator[datetime]:
     window_size = app.config["ALERT_REPORTS_CRON_WINDOW_SIZE"]
     # create a time-aware datetime in utc
     time_now = datetime.now(tz=dt_timezone.utc)
-    tz = pytz.timezone(timezone)
-    utc = pytz.timezone("UTC")
+    try:
+        tz = pytz_timezone(timezone)
+    except UnknownTimeZoneError:
+        # fallback to default timezone
+        tz = pytz_timezone("UTC")
+        logger.warning("Timezone %s was invalid. Falling back to 'UTC'", timezone)
+    utc = pytz_timezone("UTC")
     # convert the current time to the user's local time for comparison
     time_now = time_now.astimezone(tz)
     start_at = time_now - timedelta(seconds=1)

tests/integration_tests/reports/api_tests.py

@@ -19,6 +19,8 @@
 from datetime import datetime
 import json
 
+import pytz
+
 import pytest
 import prison
 from sqlalchemy.sql import func
@@ -706,6 +708,37 @@ def test_create_report_schedule_schema(self):
         data = json.loads(rv.data.decode("utf-8"))
         assert data == {"message": {"timezone": ["Field may not be null."]}}
 
+        # Test that report cannot be created with an invalid timezone
+        report_schedule_data = {
+            "type": ReportScheduleType.ALERT,
+            "name": "new5",
+            "description": "description",
+            "creation_method": ReportCreationMethodType.ALERTS_REPORTS,
+            "crontab": "0 9 * * *",
+            "recipients": [
+                {
+                    "type": ReportRecipientType.EMAIL,
+                    "recipient_config_json": {"target": "target@superset.org"},
+                },
+                {
+                    "type": ReportRecipientType.SLACK,
+                    "recipient_config_json": {"target": "channel"},
+                },
+            ],
+            "working_timeout": 3600,
+            "timezone": "this is not a timezone",
+            "dashboard": dashboard.id,
+            "database": example_db.id,
+        }
+        rv = self.client.post(uri, json=report_schedule_data)
+        assert rv.status_code == 400
+        data = json.loads(rv.data.decode("utf-8"))
+        assert data == {
+            "message": {
+                "timezone": [f"Must be one of: {', '.join(pytz.all_timezones)}."]
+            }
+        }
+
         # Test that report should reflect the timezone value passed in
         report_schedule_data = {
             "type": ReportScheduleType.ALERT,

tests/unit_tests/tasks/test_cron_util.py

@@ -64,6 +64,44 @@ def test_cron_schedule_window_los_angeles(
         )
 
 
+@pytest.mark.parametrize(
+    "current_dttm, cron, expected",
+    [
+        ("2020-01-01T00:59:01Z", "0 1 * * *", []),
+        (
+            "2020-01-01T00:59:02Z",
+            "0 1 * * *",
+            [FakeDatetime(2020, 1, 1, 1, 0).strftime("%A, %d %B %Y, %H:%M:%S")],
+        ),
+        (
+            "2020-01-01T00:59:59Z",
+            "0 1 * * *",
+            [FakeDatetime(2020, 1, 1, 1, 0).strftime("%A, %d %B %Y, %H:%M:%S")],
+        ),
+        (
+            "2020-01-01T01:00:00Z",
+            "0 1 * * *",
+            [FakeDatetime(2020, 1, 1, 1, 0).strftime("%A, %d %B %Y, %H:%M:%S")],
+        ),
+        ("2020-01-01T01:00:01Z", "0 1 * * *", []),
+    ],
+)
+def test_cron_schedule_window_invalid_timezone(
+    app_context: AppContext, current_dttm: str, cron: str, expected: List[FakeDatetime]
+) -> None:
+    """
+    Reports scheduler: Test cron schedule window for "invalid timezone"
+    """
+
+    with freeze_time(current_dttm):
+        datetimes = cron_schedule_window(cron, "invalid timezone")
+        # it should default to UTC
+        assert (
+            list(cron.strftime("%A, %d %B %Y, %H:%M:%S") for cron in datetimes)
+            == expected
+        )
+
+
 @pytest.mark.parametrize(
     "current_dttm, cron, expected",
     [