[fix] Don't delete VpnClients when enforcing required templates

Bug:
The SortedManyToManyField clears all templates before adding new ones.
This operation emits an `m2m_changed` signal with the `post_clear` action,
which triggers the `Config.enforce_required_templates` receiver to add
required templates back.

Adding required templates emits another `m2m_changed` signal with the
`post_add` action, which executes `Config.manage_vpn_clients` receiver.
At this point, only required templates exist in the DB, hence we cannot
decide which VpnClient objects to delete.

Fix:
We don't delete any VpnClient objects when the `m2m_changed` signal
is sent for the `post_add` action for adding required templates.
The receiver will be called again with the `post_add` action when
all the templates are added back, including the required ones.
And then, it will delete any VpnClient objects that are not
associated with the current templates.

---------

Co-authored-by: Federico Capoano <[email protected]>

Author: pandafy

openwisp_controller/config/base/config.py

@@ -314,10 +314,27 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
         else:
             templates = pk_set
 
-        # Delete VPN clients that are not associated with current templates
-        instance.vpnclient_set.exclude(
-            template_id__in=instance.templates.values_list("id", flat=True)
-        ).delete()
+        # Check if all templates in pk_set are required templates. If they are,
+        # skip deletion of VpnClient objects at this point.
+        if len(pk_set) != templates.filter(required=True).count():
+            # Explanation:
+            # SortedManyToManyField clears all existing templates before adding
+            # new ones. This triggers an m2m_changed signal with the "post_clear"
+            # action, which is handled by the "enforce_required_templates" signal
+            # receiver. That receiver re-adds the required templates.
+            #
+            # Re-adding required templates triggers another m2m_changed signal
+            # with the "post_add" action. At this stage, only required templates
+            # exist in the DB, so we cannot yet determine which VpnClient objects
+            # should be deleted based on the new selection.
+            #
+            # Therefore, we defer deletion of VpnClient objects until the "post_add"
+            # signal is triggered again—after all templates, including the required
+            # ones, have been fully added. At that point, we can identify and
+            # delete VpnClient objects not linked to the final template set.
+            instance.vpnclient_set.exclude(
+                template_id__in=instance.templates.values_list("id", flat=True)
+            ).delete()
 
         if action == "post_add":
             for template in templates.filter(type="vpn"):

openwisp_controller/config/tests/test_admin.py

@@ -2012,6 +2012,99 @@ def test_device_changelist_activate_deactivate_admin_action_security(
             )
             self.assertEqual(mocked_deactivate.call_count, 1)
 
+    def test_required_template_doesnt_recreate_vpn_client(self):
+        """The required templates logic should not delete VpnClients"""
+
+        def _update_template(templates):
+            params.update(
+                {
+                    "config-0-templates": ",".join(
+                        [str(template.pk) for template in templates]
+                    )
+                }
+            )
+            response = self.client.post(path, data=params, follow=True)
+            self.assertEqual(response.status_code, 200)
+            return response
+
+        # Create required and default templates
+        required_template = self._create_template(
+            name="required-template", required=True
+        )
+        # Create VPNs and VPN templates
+        vpn1 = self._create_vpn(
+            name="test-vpn1",
+        )
+        vpn2 = self._create_vpn(
+            name="test-vpn2",
+        )
+        vpn1_template = self._create_template(
+            name="vpn1-template",
+            type="vpn",
+            vpn=vpn1,
+        )
+        vpn2_template = self._create_template(
+            name="vpn2-template",
+            type="vpn",
+            vpn=vpn2,
+        )
+        # Add a new device
+        path = reverse(f"admin:{self.app_label}_device_add")
+        params = self._get_device_params(org=self._get_org())
+        response = self.client.post(path, data=params, follow=True)
+        # Verify pre-codintions
+        self.assertEqual(response.status_code, 200)
+        config = Device.objects.first().config
+        self.assertEqual(config.vpnclient_set.count(), 0)
+        self.assertEqual(config.templates.count(), 1)
+        # Change to device change form
+        path = reverse(f"admin:{self.app_label}_device_change", args=[config.device_id])
+        params.update(
+            {
+                "config-0-id": str(config.pk),
+                "config-0-device": str(config.device_id),
+                "config-INITIAL_FORMS": 1,
+                "_continue": True,
+            }
+        )
+        # Add first VPN template via admin
+        _update_template(templates=[required_template, vpn1_template])
+        self.assertEqual(config.templates.count(), 2)
+        self.assertEqual(config.vpnclient_set.count(), 1)
+        vpn1_client = config.vpnclient_set.first()
+        # Add second VPN template via admin
+        _update_template(
+            templates=[
+                required_template,
+                vpn1_template,
+                vpn2_template,
+            ]
+        )
+        self.assertEqual(config.templates.count(), 3)
+        self.assertEqual(config.vpnclient_set.count(), 2)
+        # Verify that the first VPN client is preserved (same PK)
+        self.assertEqual(
+            vpn1_client.pk, config.vpnclient_set.filter(vpn=vpn1).first().pk
+        )
+        vpn2_client = config.vpnclient_set.filter(vpn=vpn2).first()
+        # Remove first VPN template via admin
+        _update_template(templates=[required_template, vpn2_template])
+        self.assertEqual(config.templates.count(), 2)
+        self.assertEqual(config.vpnclient_set.count(), 1)
+        # Verify only vpn2 client remains
+        self.assertEqual(config.vpnclient_set.filter(vpn=vpn1).exists(), False)
+        self.assertEqual(config.vpnclient_set.filter(vpn=vpn2).exists(), True)
+        # Verify that the vpn2 client is the same as before (same PK)
+        self.assertEqual(
+            vpn2_client.pk, config.vpnclient_set.filter(vpn=vpn2).first().pk
+        )
+        # Remove the second VPN template and ensure the required template remains.
+        # This verifies that VpnClient object should get deleted it there's only
+        # one required template applied.
+        _update_template(templates=[required_template])
+        self.assertEqual(config.templates.count(), 1)
+        self.assertEqual(config.vpnclient_set.count(), 0)
+
     def test_vpn_template_switch(self):
         """
         Test switching between two VPN templates that use the same VPN server