Merge branch 'master' into issues/1049-send-generic-message

Author: stktyagi

.github/workflows/ci.yml

@@ -21,7 +21,6 @@ jobs:
       fail-fast: false
       matrix:
         python-version:
-          - "3.9"
           - "3.10"
           - "3.11"
           - "3.12"
@@ -31,11 +30,6 @@ jobs:
           - django~=5.1.0
           - django~=5.2.0
         exclude:
-          # Django 5.1+ requires Python >=3.10
-          - python-version: "3.9"
-            django-version: django~=5.1.0
-          - python-version: "3.9"
-            django-version: django~=5.2.0
           # Python 3.13 supported only in Django >=5.1.3
           - python-version: "3.13"
             django-version: django~=4.2.0

Dockerfile

@@ -1,6 +1,6 @@
 # NOTE: This Docker image is for development purposes only.
 
-FROM python:3.9-slim-buster
+FROM python:3.12-slim-bookworm
 
 RUN apt update && \
     apt install --yes zlib1g-dev libjpeg-dev gdal-bin libproj-dev \

docs/developer/installation.rst

@@ -10,7 +10,7 @@ Developer Installation Instructions
 Dependencies
 ------------
 
-- Python >= 3.9
+- Python >= 3.10
 - OpenSSL
 
 Installing for Development

openwisp_controller/config/admin.py

@@ -352,10 +352,13 @@ def get_temp_model_instance(self, **options):
         config_model = self.Meta.model
         instance = config_model(**options)
         device_model = config_model.device.field.related_model
-        org = Organization.objects.get(pk=self.data["organization"])
+        if not (org_id := self.data.get("organization")):
+            # We cannot validate the templates without an organization.
+            return
+        org = Organization.objects.get(pk=org_id)
         instance.device = device_model(
-            name=self.data["name"],
-            mac_address=self.data["mac_address"],
+            name=self.data.get("name", ""),
+            mac_address=self.data.get("mac_address", ""),
             organization=org,
         )
         return instance
@@ -369,6 +372,12 @@ def clean_templates(self):
             # when adding self.instance is empty, we need to create a
             # temporary instance that we'll use just for validation
             config = self.get_temp_model_instance(**data)
+            if not config:
+                # The request does not contain vaild data to create a temporary
+                # Device instance. Thus, we cannot validate the templates.
+                # The Device validation will be handled by DeviceAdmin.
+                # Therefore, we don't need to raise any error here.
+                return
         else:
             config = self.instance
         if config.backend and templates:

openwisp_controller/config/base/device.py

@@ -435,6 +435,25 @@ def get_temp_config_instance(self, **options):
         config.device = self
         return config
 
+    def should_skip_push_update(self):
+        """
+        Check if push update should be skipped for this device.
+
+        Clears the skip flag after reading it.
+        """
+        result = getattr(self, "_skip_push_update", False)
+        if result:
+            delattr(self, "_skip_push_update")
+        return result
+
+    def skip_push_update_on_save(self):
+        """
+        Marks the device to skip push update on the next save.
+        This is useful when updating device properties during re-registration
+        to avoid triggering unnecessary push operations.
+        """
+        self._skip_push_update = True
+
     def can_be_updated(self):
         """
         returns True if the device can and should be updated

openwisp_controller/config/controller/views.py

@@ -289,7 +289,7 @@ class DeviceRegisterView(UpdateLastIpMixin, CsrfExtemptMixin, View):
     model = Device
     org_config_settings_model = OrganizationConfigSettings
 
-    UPDATABLE_FIELDS = ["os", "model", "system"]
+    UPDATABLE_FIELDS = ["name", "os", "model", "system"]
 
     def init_object(self, **kwargs):
         """
@@ -402,10 +402,13 @@ def post(self, request, *args, **kwargs):
         new = False
         try:
             device = self.model.objects.get(key=key)
-            # update hw info
+            # update device info
             for attr in self.UPDATABLE_FIELDS:
                 if attr in request.POST:
-                    setattr(device, attr, request.POST.get(attr))
+                    if attr == "name":
+                        self._update_device_name(request, device)
+                    else:
+                        setattr(device, attr, request.POST.get(attr))
             config = device.config
         # if get queryset fails, instantiate a new Device and Config
         except self.model.DoesNotExist:
@@ -457,6 +460,27 @@ def post(self, request, *args, **kwargs):
             s.format(**attributes), content_type="text/plain", status=201
         )
 
+    def _update_device_name(self, request, device):
+        """
+        Updates the device name during re-registration if the name in the
+        payload is not equal to the device's MAC address.
+
+        Agents send the MAC address as hostname when the hostname is "OpenWrt"
+        or if default_hostname is set to *. This method prevents overwriting
+        user-configured hostnames with default/generic MAC address values.
+        """
+        name = request.POST.get("name")
+        mac_address = device.mac_address
+        normalized_mac = (
+            mac_address.replace(":", "").replace("-", "").lower()
+            if mac_address
+            else None
+        )
+        normalized_name = name.replace(":", "").replace("-", "").lower()
+        if normalized_name != normalized_mac:
+            device.name = name
+            device.skip_push_update_on_save()
+
 
 class GetVpnView(SingleObjectMixin, View):
     """

openwisp_controller/config/tests/test_admin.py

@@ -2279,6 +2279,36 @@ def test_templates_fetch_queries_10(self):
         config = self._create_config(organization=self._get_org())
         self._verify_template_queries(config, 10)
 
+    def test_empty_device_form_with_config_inline(self):
+        org = self._get_org()
+        template = self._create_template(organization=org)
+        path = reverse(f"admin:{self.app_label}_device_add")
+        # Submit form without required device fields but with config inline
+        # This reproduces the scenario where user clicks "Add another Configuration"
+        # and submits without filling device details
+        params = {
+            "config-0-backend": "netjsonconfig.OpenWrt",
+            "config-0-templates": str(template.pk),
+            "config-0-config": json.dumps({}),
+            "config-0-context": "",
+            "config-TOTAL_FORMS": 1,
+            "config-INITIAL_FORMS": 0,
+            "config-MIN_NUM_FORMS": 0,
+            "config-MAX_NUM_FORMS": 1,
+            "deviceconnection_set-TOTAL_FORMS": 0,
+            "deviceconnection_set-INITIAL_FORMS": 0,
+            "deviceconnection_set-MIN_NUM_FORMS": 0,
+            "deviceconnection_set-MAX_NUM_FORMS": 1000,
+            "command_set-TOTAL_FORMS": 0,
+            "command_set-INITIAL_FORMS": 0,
+            "command_set-MIN_NUM_FORMS": 0,
+            "command_set-MAX_NUM_FORMS": 1000,
+        }
+        response = self.client.post(path, params)
+        self.assertEqual(response.status_code, 200)
+        self.assertContains(response, "errorlist")
+        self.assertEqual(Device.objects.count(), 0)
+
 
 class TestTransactionAdmin(
     CreateConfigTemplateMixin,

openwisp_controller/config/tests/test_controller.py

@@ -5,7 +5,7 @@
 from django.core.exceptions import ValidationError
 from django.http.response import Http404
 from django.test import TestCase
-from django.urls import reverse
+from django.urls import reverse, reverse_lazy
 from swapper import load_model
 
 from openwisp_utils.tests import capture_any_output, catch_signal
@@ -38,13 +38,8 @@
 Organization = load_model("openwisp_users", "Organization")
 
 
-class TestController(CreateConfigTemplateMixin, TestVpnX509Mixin, TestCase):
-    """
-    tests for config.controller
-    """
-
-    def setUp(self):
-        self.register_url = reverse("controller:device_register")
+class TestRegistrationMixin:
+    register_url = reverse_lazy("controller:device_register")
 
     def _create_org(self, shared_secret=TEST_ORG_SHARED_SECRET, **kwargs):
         org = super()._create_org(**kwargs)
@@ -53,6 +48,27 @@ def _create_org(self, shared_secret=TEST_ORG_SHARED_SECRET, **kwargs):
         )
         return org
 
+    def _get_reregistration_payload(self, device, **kwargs):
+        data = {
+            "secret": str(device.organization.config_settings.shared_secret),
+            "key": TEST_CONSISTENT_KEY,
+            "mac_address": device.mac_address,
+            "backend": "netjsonconfig.OpenWrt",
+            "model": "TP-Link TL-WDR4300 v2",
+            "os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330",
+            "system": "Atheros AR9344 rev 3",
+        }
+        data.update(**kwargs)
+        return data
+
+
+class TestController(
+    TestRegistrationMixin, CreateConfigTemplateMixin, TestVpnX509Mixin, TestCase
+):
+    """
+    tests for config.controller
+    """
+
     def _check_header(self, response):
         self.assertEqual(response["X-Openwisp-Controller"], "true")
 
@@ -820,6 +836,83 @@ def test_device_registration_update_hw_info_no_config(self):
         self.assertEqual(d.system, params["system"])
         self.assertEqual(d.model, params["model"])
 
+    @patch.object(Device, "skip_push_update_on_save")
+    def test_device_registration_update_hostname(self, mocked_method):
+        """
+        Test that hostname is updated when the name in payload
+        is not the MAC address stored in OpenWISP
+        """
+        device = self._create_device_config(
+            device_opts={
+                "name": "old-hostname",
+                "mac_address": TEST_MACADDR,
+                "key": TEST_CONSISTENT_KEY,
+            }
+        )
+        params = self._get_reregistration_payload(
+            device=device,
+            name="new-custom-hostname",
+        )
+        self.assertNotEqual(device.name, params["name"])
+        response = self.client.post(self.register_url, params)
+        self.assertEqual(response.status_code, 201)
+        device.refresh_from_db()
+        self.assertEqual(device.name, "new-custom-hostname")
+        mocked_method.assert_called_once()
+
+    @patch.object(Device, "skip_push_update_on_save")
+    def test_device_registration_hostname_not_updated_when_mac_address(
+        self, mocked_method
+    ):
+        """
+        Test that hostname is not updated when the name in payload
+        equals the MAC address (agents send MAC address as hostname
+        when hostname is OpenWrt or if default_hostname is set to *)
+        """
+        device = self._create_device_config(
+            device_opts={
+                "name": "meaningful-hostname",
+                "key": TEST_CONSISTENT_KEY,
+            }
+        )
+        params = self._get_reregistration_payload(
+            device=device,
+            name=TEST_MACADDR,
+        )
+        response = self.client.post(self.register_url, params)
+        self.assertEqual(response.status_code, 201)
+        device.refresh_from_db()
+        self.assertEqual(device.name, "meaningful-hostname")
+        mocked_method.assert_not_called()
+
+    @patch.object(Device, "skip_push_update_on_save")
+    def test_device_registration_hostname_comparison_case_insensitive(
+        self, mocked_method
+    ):
+        """
+        Test that MAC address comparison is case-insensitive and works
+        with different formats (colons, dashes, no separators)
+        """
+        mac_address = "00:11:22:33:aa:BB"
+        name = mac_address.replace(":", "-")
+        device = self._create_device_config(
+            device_opts={
+                "mac_address": mac_address,
+                "name": name,
+                "key": TEST_CONSISTENT_KEY,
+            }
+        )
+        params = self._get_reregistration_payload(
+            device=device,
+            name="00-11-22-33-aa-bb",
+        )
+        response = self.client.post(self.register_url, params)
+        self.assertEqual(response.status_code, 201)
+        device.refresh_from_db()
+        # Hostname should not be changed
+        self.assertEqual(device.name, name)
+        mocked_method.assert_not_called()
+
     def test_device_report_status_running(self):
         """
         maintained for backward compatibility

openwisp_controller/connection/apps.py

@@ -64,7 +64,7 @@ def ready(self):
 
     @classmethod
     def config_modified_receiver(cls, **kwargs):
-        transaction.on_commit(lambda: cls._launch_update_config(kwargs["device"].pk))
+        transaction.on_commit(lambda: cls._launch_update_config(kwargs["device"]))
 
     @classmethod
     def command_save_receiver(cls, sender, created, instance, **kwargs):
@@ -81,14 +81,17 @@ def command_save_receiver(cls, sender, created, instance, **kwargs):
         )
 
     @classmethod
-    def _launch_update_config(cls, device_id):
+    def _launch_update_config(cls, device):
         """
         Calls the background task update_config only if
         no other tasks are running for the same device
         """
         from .tasks import update_config
 
-        update_config.delay(device_id)
+        # Check if push update should be skipped
+        if device.should_skip_push_update():
+            return
+        update_config.delay(str(device.pk))
 
     @classmethod
     def is_working_changed_receiver(

openwisp_controller/connection/tests/test_tasks.py

@@ -4,13 +4,15 @@
 from unittest import mock
 
 from celery.exceptions import SoftTimeLimitExceeded
-from django.test import TestCase
+from django.test import TestCase, TransactionTestCase
 from swapper import load_model
 
+from ...config.tests.test_controller import TestRegistrationMixin
 from .. import tasks
 from .utils import CreateConnectionsMixin
 
 Command = load_model("connection", "Command")
+OrganizationConfigSettings = load_model("config", "OrganizationConfigSettings")
 
 
 class TestTasks(CreateConnectionsMixin, TestCase):
@@ -85,3 +87,22 @@ def test_launch_command_exception(self, *args):
         command.refresh_from_db()
         self.assertEqual(command.status, "failed")
         self.assertEqual(command.output, "Internal system error: test error\n")
+
+
+class TestTransactionTasks(
+    TestRegistrationMixin, CreateConnectionsMixin, TransactionTestCase
+):
+    @mock.patch.object(tasks.update_config, "delay")
+    def test_update_config_hostname_changed_on_reregister(self, mocked_update_config):
+        device = self._create_device_config()
+        self._create_device_connection(device=device)
+        # Trigger re-registration with new hostname
+        response = self.client.post(
+            self.register_url,
+            self._get_reregistration_payload(
+                device,
+                name="new-hostname",
+            ),
+        )
+        self.assertEqual(response.status_code, 201)
+        mocked_update_config.assert_not_called()