Fix redirect weekdays and monthdays handling

Treat these parameters as an array, and drop support for negating the values with a "!" in JSON as it is unneeded.
openwisp · Aug 1, 2020 · 0c70308 · 0c70308
1 parent a9cb284
commit 0c70308
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 12 deletions.
diff --git a/netjsonconfig/backends/openwrt/converters/firewall.py b/netjsonconfig/backends/openwrt/converters/firewall.py
@@ -131,8 +131,10 @@ def __intermediate_redirects(self, redirects):
                     redirect["proto"] = proto[0]
                 elif set(proto) == {"tcp", "udp"}:
                     redirect["proto"] = "tcpudp"
+
             resultdict.update(redirect)
             result.append(resultdict)
+
         return result
 
     def __get_auto_name_redirect(self, redirect):
@@ -203,5 +205,7 @@ def __netjson_redirect(self, redirect):
                     redirect["proto"] = ["tcp", "udp"]
                 else:
                     redirect["proto"] = [proto]
+        if "monthdays" in redirect:
+            redirect["monthdays"] = [int(x) for x in redirect["monthdays"]]
 
         return self.type_cast(redirect)
diff --git a/netjsonconfig/backends/openwrt/schema.py b/netjsonconfig/backends/openwrt/schema.py
@@ -873,27 +873,44 @@
                                     "pattern": time_regex,
                                     "propertyOrder": 17,
                                 },
-                                # FIXME: regex needed. Also, should this be an array?
+                                # Note: here we don't support negation of values like
+                                # the UCI syntax does, as it's not necessary.
                                 "weekdays": {
-                                    "type": "string",
+                                    "type": "array",
                                     "title": "weekdays",
                                     "description": "Only match traffic during the given week days, "
-                                    'e.g. "sun mon thu fri" to only match on Sundays, Mondays, Thursdays and '
-                                    "Fridays. The list can be inverted by prefixing it with an exclamation "
-                                    'mark, e.g. "! sat sun" to always match but not on Saturdays and '
-                                    "Sundays.",
+                                    'e.g. ["sun", "mon", "thu", "fri"] to only match on Sundays, '
+                                    "Mondays, Thursdays and Fridays.",
                                     "propertyOrder": 18,
+                                    "items": {
+                                        "type": "string",
+                                        "title": "weekday",
+                                        "enum": [
+                                            "mon",
+                                            "tue",
+                                            "wed",
+                                            "thur",
+                                            "fri",
+                                            "sat",
+                                            "sun",
+                                        ],
+                                    },
                                 },
-                                # FIXME: regex needed. Also, should this be an array?
+                                # Note: here we don't support negation of values like
+                                # the UCI syntax does, as it's not necessary.
                                 "monthdays": {
-                                    "type": "string",
+                                    "type": "array",
                                     "title": "monthdays",
                                     "description": "Only match traffic during the given days of the "
-                                    'month, e.g. "2 5 30" to only match on every 2nd, 5th and 30th '
-                                    "day of the month. The list can be inverted by prefixing it with "
-                                    'an exclamation mark, e.g. "! 31" to always match but on the '
-                                    "31st of the month.",
+                                    "month, e.g. [2, 5, 30] to only match on every 2nd, 5th and 30th "
+                                    "day of the month.",
                                     "propertyOrder": 19,
+                                    "items": {
+                                        "type": "integer",
+                                        "title": "day of month",
+                                        "minimum": 1,
+                                        "maximum": 31,
+                                    },
                                 },
                                 "utc_time": {
                                     "type": "boolean",

diff --git a/tests/openwrt/test_firewall.py b/tests/openwrt/test_firewall.py
@@ -413,3 +413,74 @@ def test_render_redirect_1(self):
     def test_parse_redirect_1(self):
         o = OpenWrt(native=self._redirect_1_uci)
         self.assertEqual(o.config, self._redirect_1_netjson)
+
+    _redirect_2_netjson = {
+        "firewall": {
+            "redirects": [
+                {
+                    "name": "Adblock DNS, port 53",
+                    "src": "lan",
+                    "proto": ["tcp", "udp"],
+                    "src_dport": "53",
+                    "dest_port": "53",
+                    "target": "DNAT",
+                    # Contrived, unrealistic example for testing
+                    "weekdays": ["mon", "tue", "wed"],
+                    "monthdays": [1, 2, 3, 29, 30],
+                }
+            ]
+        }
+    }
+
+    _redirect_2_uci = textwrap.dedent(
+        """\
+        package firewall
+
+        config defaults 'defaults'
+
+        config redirect 'redirect_Adblock DNS, port 53'
+            option name 'Adblock DNS, port 53'
+            option src 'lan'
+            option proto 'tcpudp'
+            option src_dport '53'
+            option dest_port '53'
+            option target 'DNAT'
+            list weekdays 'mon'
+            list weekdays 'tue'
+            list weekdays 'wed'
+            list monthdays '1'
+            list monthdays '2'
+            list monthdays '3'
+            list monthdays '29'
+            list monthdays '30'
+        """
+    )
+
+    def test_render_redirect_2(self):
+        o = OpenWrt(self._redirect_2_netjson)
+        expected = self._tabs(self._redirect_2_uci)
+        self.assertEqual(o.render(), expected)
+
+    def test_parse_redirect_2(self):
+        o = OpenWrt(native=self._redirect_2_uci)
+        self.assertEqual(o.config, self._redirect_2_netjson)
+
+    def test_redirect_weekdays_validation_error_1(self):
+        o = OpenWrt({"firewall": {"redirects": [{"weekdays": ["mon", "xxx"]}]}})
+        with self.assertRaises(ValidationError):
+            o.validate()
+
+    def test_redirect_weekdays_validation_error_2(self):
+        o = OpenWrt({"firewall": {"redirects": [{"weekdays": ["mon", 1]}]}})
+        with self.assertRaises(ValidationError):
+            o.validate()
+
+    def test_redirect_monthdays_validation_error_1(self):
+        o = OpenWrt({"firewall": {"redirects": [{"monthdays": [2, 8, 32]}]}})
+        with self.assertRaises(ValidationError):
+            o.validate()
+
+    def test_redirect_monthdays_validation_error_2(self):
+        o = OpenWrt({"firewall": {"redirects": [{"monthdays": [0, 2, 8]}]}})
+        with self.assertRaises(ValidationError):
+            o.validate()