[fix] Replace regex URLs with custom Path Converters #682

Replaced legacy regex 're_path' with strict Django 'path' converters and
updated tests to manually construct invalid URLs for 404 testing.

Fixes #682

Author: stktyagi

openwisp_controller/config/admin.py

@@ -20,7 +20,7 @@
 from django.shortcuts import get_object_or_404
 from django.template.loader import get_template
 from django.template.response import TemplateResponse
-from django.urls import path, register_converter, reverse
+from django.urls import path, reverse
 from django.utils.html import format_html, mark_safe
 from django.utils.translation import gettext_lazy as _
 from django.utils.translation import ngettext_lazy
@@ -42,20 +42,14 @@
 from ..admin import MultitenantAdminMixin
 from . import settings as app_settings
 from .base.vpn import AbstractVpn
-from .converters import UUIDAnyConverter, UUIDAnyOrFKConverter
 from .exportable import DeviceResource
 from .filters import DeviceGroupFilter, GroupFilter, TemplatesFilter
 from .utils import send_file
 from .widgets import DeviceGroupJsonSchemaWidget, JsonSchemaWidget
 
-register_converter(UUIDAnyConverter, "uuid_any")
-register_converter(UUIDAnyOrFKConverter, "uuid_or_fk")
-
 logger = logging.getLogger(__name__)
 prefix = "config/"
-uuid_regex = (
-    r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
-)
+
 Config = load_model("config", "Config")
 Device = load_model("config", "Device")
 DeviceGroup = load_model("config", "DeviceGroup")

openwisp_controller/config/apps.py

@@ -61,8 +61,8 @@ def register_path_converters(self):
         converters = get_converters()
         if "uuid_any" not in converters:
             register_converter(UUIDAnyConverter, "uuid_any")
-        if "uuid_any_or_fk" not in converters:
-            register_converter(UUIDAnyOrFKConverter, "uuid_any_or_fk")
+        if "uuid_or_fk" not in converters:
+            register_converter(UUIDAnyOrFKConverter, "uuid_or_fk")
 
     def __setmodels__(self):
         self.device_model = load_model("config", "Device")

openwisp_controller/config/tests/pytest.py

@@ -5,18 +5,14 @@
 from channels.security.websocket import AllowedHostsOriginValidator
 from channels.testing import WebsocketCommunicator
 from django.contrib.auth.models import Permission
-from django.urls import re_path
+from django.urls import path
 from swapper import load_model
 
 from ..base.channels_consumer import BaseDeviceConsumer
 from .utils import CreateDeviceMixin
 
 Device = load_model("config", "Device")
 
-uuid_regex = (
-    r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
-)
-
 
 @pytest.mark.asyncio
 @pytest.mark.django_db(transaction=True)
@@ -28,8 +24,8 @@ class TestDeviceConsumer(CreateDeviceMixin):
                 AuthMiddlewareStack(
                     URLRouter(
                         [
-                            re_path(
-                                rf"^ws/controller/device/(?P<pk>{uuid_regex})/$",
+                            path(
+                                "ws/controller/device/<uuid_any:pk>/",
                                 BaseDeviceConsumer.as_asgi(),
                             )
                         ]

openwisp_controller/config/tests/test_controller.py

@@ -272,9 +272,8 @@ def test_device_checksum_requested_signal_is_emitted(self):
 
     def test_device_checksum_bad_uuid(self):
         d = self._create_device_config()
-        pk = "{}-wrong".format(d.pk)
-        valid = reverse("controller:device_checksum", args=[pk])
-        bad = valid + "junk/"
+        valid = reverse("controller:device_checksum", args=[d.pk])
+        bad = valid.replace(str(d.pk), f"{d.pk}-wrong")
         resp = self.client.get(bad, {"key": d.key})
         self.assertEqual(resp.status_code, 404)
 
@@ -338,9 +337,9 @@ def test_deactivated_device_download_config(self):
 
     def test_device_download_config_bad_uuid(self):
         d = self._create_device_config()
-        pk = "{}-wrong".format(d.pk)
-        bad_path = f"/controller/device/download-config/{pk}/"
-        response = self.client.get(bad_path, {"key": d.key})
+        valid = reverse("controller:device_download_config", args=[d.pk])
+        bad = valid.replace(str(d.pk), f"{d.pk}-wrong")
+        response = self.client.get(bad, {"key": d.key})
         self.assertEqual(response.status_code, 404)
 
     def test_vpn_checksum_requested_signal_is_emitted(self):
@@ -397,9 +396,9 @@ def test_vpn_checksum(self):
 
     def test_vpn_checksum_bad_uuid(self):
         v = self._create_vpn()
-        pk = "{}-wrong".format(v.pk)
-        bad_path = f"/controller/vpn/checksum/{pk}/"
-        response = self.client.get(bad_path, {"key": v.key})
+        valid = reverse("controller:vpn_checksum", args=[v.pk])
+        bad = valid.replace(str(v.pk), f"{v.pk}-wrong")
+        response = self.client.get(bad, {"key": v.key})
         self.assertEqual(response.status_code, 404)
 
     @capture_any_output()
@@ -513,9 +512,9 @@ def test_vpn_download_config(self):
 
     def test_vpn_download_config_bad_uuid(self):
         v = self._create_vpn()
-        pk = "{}-wrong".format(v.pk)
-        bad_path = f"/controller/vpn/download-config/{pk}/"
-        response = self.client.get(bad_path, {"key": v.key})
+        valid_url = reverse("controller:vpn_download_config", args=[v.pk])
+        bad_url = valid_url.replace(str(v.pk), f"{v.pk}-wrong")
+        response = self.client.get(bad_url, {"key": v.key})
         self.assertEqual(response.status_code, 404)
 
     @capture_any_output()
@@ -970,9 +969,9 @@ def test_deactivated_device_report_status(self):
 
     def test_device_report_status_bad_uuid(self):
         d = self._create_device_config()
-        pk = "{}-wrong".format(d.pk)
-        bad_path = f"/controller/device/report-status/{pk}/"
-        response = self.client.post(bad_path, {"key": d.key})
+        valid_url = reverse("controller:device_report_status", args=[d.pk])
+        bad_url = valid_url.replace(str(d.pk), f"{d.pk}-wrong")
+        response = self.client.post(bad_url, {"key": d.key})
         self.assertEqual(response.status_code, 404)
 
     @capture_any_output()
@@ -1080,15 +1079,15 @@ def test_deactivated_device_update_info(self):
 
     def test_device_update_info_bad_uuid(self):
         d = self._create_device_config()
-        pk = "{}-wrong".format(d.pk)
         params = {
             "key": d.key,
             "model": "TP-Link TL-WDR4300 v2",
             "os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330",
             "system": "Atheros AR9344 rev 3",
         }
-        bad_path = f"/controller/device/update-info/{pk}/"
-        response = self.client.post(bad_path, params)
+        valid_url = reverse("controller:device_update_info", args=[d.pk])
+        bad_url = valid_url.replace(str(d.pk), f"{d.pk}-wrong")
+        response = self.client.post(bad_url, params)
         self.assertEqual(response.status_code, 404)
 
     def test_device_update_info_400(self):

openwisp_controller/config/urls.py

@@ -1,14 +1,6 @@
-from django.urls import path, register_converter
+from django.urls import path
 
-from .converters import UUIDAnyConverter, UUIDAnyOrFKConverter
 from .views import schema
 
-register_converter(UUIDAnyConverter, "uuid_any")
-register_converter(UUIDAnyOrFKConverter, "uuid_or_fk")
-
-
-register_converter(UUIDAnyConverter, "uuid_any")
-register_converter(UUIDAnyOrFKConverter, "uuid_or_fk")
-
 app_name = "openwisp_controller"
 urlpatterns = [path("config/schema.json", schema, name="schema")]

openwisp_controller/config/utils.py

@@ -6,15 +6,12 @@
 from django.db.models import Q
 from django.http import Http404, HttpResponse
 from django.shortcuts import get_object_or_404 as base_get_object_or_404
-from django.urls import path, re_path
+from django.urls import path
 from django.utils.translation import gettext_lazy as _
 from openwisp_notifications.signals import notify
 from openwisp_notifications.utils import _get_object_link
 
 logger = logging.getLogger(__name__)
-uuid_regex = (
-    r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
-)
 
 
 def get_object_or_404(model, **kwargs):
@@ -123,23 +120,23 @@ def get_controller_urls(views_module):
     used by third party apps to reduce boilerplate
     """
     urls = [
-        re_path(
-            rf"controller/device/checksum/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/device/checksum/<uuid_any:pk>/",
             views_module.device_checksum,
             name="device_checksum",
         ),
-        re_path(
-            rf"controller/device/download-config/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/device/download-config/<uuid_any:pk>/",
             views_module.device_download_config,
             name="device_download_config",
         ),
-        re_path(
-            rf"controller/device/update-info/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/device/update-info/<uuid_any:pk>/",
             views_module.device_update_info,
             name="device_update_info",
         ),
-        re_path(
-            rf"controller/device/report-status/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/device/report-status/<uuid_any:pk>/",
             views_module.device_report_status,
             name="device_report_status",
         ),
@@ -148,34 +145,34 @@ def get_controller_urls(views_module):
             views_module.device_register,
             name="device_register",
         ),
-        re_path(
-            rf"controller/vpn/checksum/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/vpn/checksum/<uuid_any:pk>/",
             views_module.vpn_checksum,
             name="vpn_checksum",
         ),
-        re_path(
-            rf"controller/vpn/download-config/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/vpn/download-config/<uuid_any:pk>/",
             views_module.vpn_download_config,
             name="vpn_download_config",
         ),
         # legacy URLs
-        re_path(
-            rf"controller/checksum/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/checksum/<uuid_any:pk>/",
             views_module.device_checksum,
             name="checksum_legacy",
         ),
-        re_path(
-            rf"controller/download-config/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/download-config/<uuid_any:pk>/",
             views_module.device_download_config,
             name="download_config_legacy",
         ),
-        re_path(
-            rf"controller/update-info/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/update-info/<uuid_any:pk>/",
             views_module.device_update_info,
             name="update_info_legacy",
         ),
-        re_path(
-            rf"controller/report-status/(?P<pk>{uuid_regex})/$",
+        path(
+            "controller/report-status/<uuid_any:pk>/",
             views_module.device_report_status,
             name="report_status_legacy",
         ),