[change/fix] Added relation from VpnClient objects to Template openwisp#831

pandafy · web-flow · commit 5186ad996386 · 2025-03-20T17:49:16.000-03:00
Make sure VpnClient objects are deleted when VPN-client templates are deleted. Fixes openwisp#831
diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py
@@ -272,29 +272,26 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
         else:
             templates = pk_set
 
-        # Get current VPNs in use by any template
-        current_vpns = set(
-            instance.templates.filter(type='vpn').values_list('vpn_id', flat=True)
-        )
+        # 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()
 
-        # Handle template actions
-        for template in templates.filter(type='vpn'):
-            if action == 'post_add':
+        if action == 'post_add':
+            for template in templates.filter(type='vpn'):
                 # Create VPN client if needed
                 if not vpn_client_model.objects.filter(
-                    config=instance, vpn=template.vpn
+                    config=instance, vpn=template.vpn, template=template
                 ).exists():
                     client = vpn_client_model(
                         config=instance,
                         vpn=template.vpn,
+                        template=template,
                         auto_cert=template.auto_cert,
                     )
                     client.full_clean()
                     client.save()
 
-        # Clean up any VPN clients that aren't associated with current templates
-        instance.vpnclient_set.exclude(vpn_id__in=current_vpns).delete()
-
     @classmethod
     def clean_templates_org(cls, action, instance, pk_set, raw_data=None, **kwargs):
         """
diff --git a/openwisp_controller/config/base/vpn.py b/openwisp_controller/config/base/vpn.py
@@ -796,6 +796,10 @@ class AbstractVpnClient(models.Model):
     config = models.ForeignKey(
         get_model_name('config', 'Config'), on_delete=models.CASCADE
     )
+    template = models.ForeignKey(
+        get_model_name('config', 'Template'),
+        on_delete=models.CASCADE,
+    )
     vpn = models.ForeignKey(get_model_name('config', 'Vpn'), on_delete=models.CASCADE)
     cert = models.OneToOneField(
         get_model_name('django_x509', 'Cert'),
diff --git a/openwisp_controller/config/migrations/0056_vpnclient_template.py b/openwisp_controller/config/migrations/0056_vpnclient_template.py
@@ -0,0 +1,24 @@
+# Generated by Django 4.2.18 on 2025-03-18 15:53
+
+import django.db.models.deletion
+from django.conf import settings
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("config", "0055_alter_config_status"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="vpnclient",
+            name="template",
+            field=models.ForeignKey(
+                default=None,
+                null=True,
+                on_delete=django.db.models.deletion.CASCADE,
+                to=settings.CONFIG_TEMPLATE_MODEL,
+            ),
+        ),
+    ]
diff --git a/openwisp_controller/config/migrations/0057_populate_vpnclient_template.py b/openwisp_controller/config/migrations/0057_populate_vpnclient_template.py
@@ -0,0 +1,25 @@
+# Generated by Django 3.2.15 on 2022-09-16 11:45
+
+
+from django.db import migrations
+
+
+def populate_vpnclient_template(apps, schema_editor):
+    VpnClient = apps.get_model('config', 'VpnClient')
+
+    for vpn_client in VpnClient.objects.iterator():
+        if vpn_client.template is None:
+            vpn_client.template = vpn_client.config.templates.get(
+                vpn_id=vpn_client.vpn_id
+            )
+            vpn_client.save()
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('config', '0056_vpnclient_template'),
+    ]
+
+    operations = [
+        migrations.RunPython(populate_vpnclient_template, migrations.RunPython.noop)
+    ]
diff --git a/openwisp_controller/config/migrations/0058_alter_vpnclient_template.py b/openwisp_controller/config/migrations/0058_alter_vpnclient_template.py
@@ -0,0 +1,22 @@
+# Generated by Django 4.2.18 on 2025-03-18 15:55
+
+import django.db.models.deletion
+from django.conf import settings
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("config", "0057_populate_vpnclient_template"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="vpnclient",
+            name="template",
+            field=models.ForeignKey(
+                on_delete=django.db.models.deletion.CASCADE,
+                to=settings.CONFIG_TEMPLATE_MODEL,
+            ),
+        ),
+    ]
diff --git a/openwisp_controller/config/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py
@@ -2129,6 +2129,7 @@ def test_vpn_template_switch(self):
             response = self.client.post(path, data=params, follow=True)
             self.assertEqual(response.status_code, 200)
             config.refresh_from_db()
+            self.assertEqual(config.vpnclient_set.count(), 1)
             del config.backend_instance
             self.assertEqual(
                 config.backend_instance.config['openvpn'][0]['cert'],
@@ -2138,7 +2139,6 @@ def test_vpn_template_switch(self):
                 config.backend_instance.config['openvpn'][0]['dev'],
                 'tun1',
             )
-            self.assertEqual(config.vpnclient_set.count(), 1)
 
         with self.subTest('Switch device back to template1'):
             params.update(
diff --git a/openwisp_controller/config/tests/test_config.py b/openwisp_controller/config/tests/test_config.py
@@ -424,6 +424,16 @@ def test_clear_vpnclient(self):
         self.assertIsNotNone(vpnclient)
         self.assertNotEqual(c.vpnclient_set.count(), 0)
 
+    def test_deleting_template_deletes_vpnclient(self):
+        template = self._create_template(
+            name='test-network', type='vpn', vpn=self._create_vpn(), default=True
+        )
+        config = self._create_config(device=self._create_device())
+        self.assertEqual(config.vpnclient_set.count(), 1)
+        template.delete()
+        self.assertEqual(config.templates.count(), 0)
+        self.assertEqual(config.vpnclient_set.count(), 0)
+
     def test_multiple_vpn_clients(self):
         vpn1 = self._create_vpn(name='vpn1')
         vpn2 = self._create_vpn(name='vpn2')
diff --git a/openwisp_controller/config/tests/test_vpn.py b/openwisp_controller/config/tests/test_vpn.py
@@ -405,7 +405,9 @@ def test_auto_create_cert_with_long_device_name(self):
         vpn = self._create_vpn(organization=org)
         d = self._create_device(organization=org, name=device_name)
         c = self._create_config(device=d)
-        client = VpnClient(vpn=vpn, config=c, auto_cert=True)
+        client = VpnClient(
+            vpn=vpn, config=c, auto_cert=True, template=self._create_template()
+        )
         client.full_clean()
         client.save()
         # The last 9 characters gets truncated and replaced with unique id
@@ -818,10 +820,17 @@ def test_vpn_peers_changed(self):
 class TestVxlan(BaseTestVpn, TestVxlanWireguardVpnMixin, TestCase):
     def test_vxlan_config_creation(self):
         tunnel, subnet = self._create_vxlan_tunnel()
+        template = self._create_template(
+            name='vxlan-wireguard',
+            type='vpn',
+            vpn=tunnel,
+            organization=tunnel.organization,
+            auto_cert=True,
+        )
         with self.subTest('vni 1'):
             d1 = self._create_device()
             c1 = self._create_config(device=d1)
-            client = VpnClient(vpn=tunnel, config=c1, auto_cert=True)
+            client = VpnClient(vpn=tunnel, config=c1, auto_cert=True, template=template)
             client.full_clean()
             client.save()
             client.refresh_from_db()
@@ -830,7 +839,7 @@ def test_vxlan_config_creation(self):
         with self.subTest('vni 2'):
             d2 = self._create_device(name='d2', mac_address='16:DB:7F:E8:50:01')
             c2 = self._create_config(device=d2)
-            client = VpnClient(vpn=tunnel, config=c2, auto_cert=True)
+            client = VpnClient(vpn=tunnel, config=c2, auto_cert=True, template=template)
             client.full_clean()
             client.save()
             client.refresh_from_db()
@@ -853,23 +862,32 @@ def test_vxlan_config_creation(self):
         with self.subTest('auto_cert=False'):
             d3 = self._create_device(name='d3', mac_address='16:DB:7F:E8:50:03')
             c3 = self._create_config(device=d3)
-            client = VpnClient(vpn=tunnel, config=c3, auto_cert=False)
+            client = VpnClient(
+                vpn=tunnel, config=c3, auto_cert=False, template=template
+            )
             client.full_clean()
             client.save()
             client.refresh_from_db()
             self.assertEqual(client.vni, None)
 
     def test_duplicate_vxlan_tunnels_same_vni(self):
         tunnel, subnet = self._create_vxlan_tunnel()
+        template = self._create_template(
+            name='vxlan-wireguard',
+            type='vpn',
+            vpn=tunnel,
+            organization=tunnel.organization,
+            auto_cert=True,
+        )
         d1 = self._create_device()
         c1 = self._create_config(device=d1)
-        client = VpnClient(vpn=tunnel, config=c1, vni=1)
+        client = VpnClient(vpn=tunnel, config=c1, vni=1, template=template)
         client.full_clean()
         client.save()
         with self.subTest('Test server configuration does not define VNI'):
             d2 = self._create_device(name='d2', mac_address='16:DB:7F:E8:50:01')
             c2 = self._create_config(device=d2)
-            client = VpnClient(vpn=tunnel, config=c2, vni=1)
+            client = VpnClient(vpn=tunnel, config=c2, vni=1, template=template)
             with self.assertRaises(ValidationError) as context_manager:
                 client.full_clean()
             message_dict = context_manager.exception.message_dict
@@ -883,7 +901,7 @@ def test_duplicate_vxlan_tunnels_same_vni(self):
             tunnel.config['vxlan'] = [{'interface': 'vxlan1', 'vni': 1}]
             tunnel.full_clean()
             tunnel.save()
-            client = VpnClient(vpn=tunnel, config=c2, vni=1)
+            client = VpnClient(vpn=tunnel, config=c2, vni=1, template=template)
             client.full_clean()
             client.save()
 
@@ -1573,13 +1591,6 @@ def _reset_mocks():
             organization=zt_vpn1.organization,
             auto_cert=True,
         )
-        zt_1_copy = self._create_template(
-            name='test-zt-template-1-copy',
-            type='vpn',
-            vpn=zt_vpn1,
-            organization=zt_vpn1.organization,
-            auto_cert=True,
-        )
         zt2 = self._create_template(
             name='test-zt-template-2',
             type='vpn',
@@ -1655,27 +1666,6 @@ def _reset_mocks():
             self.assertEqual(mock_requests.post.call_count, 1)
         _reset_mocks()
 
-        with self.subTest(
-            (
-                'Test zt no identity generation when same zt vpn server '
-                'vpn client template is already applied to the device'
-            )
-        ):
-            device.config.templates.add(zt_1_copy)
-            # No new zt vpn client object
-            self.assertEqual(vpnclient_qs.count(), 1)
-            # Make sure only previously applied 'zt_vpn1' vpn client object is exist
-            self.assertEqual(vpnclient_qs.first().vpn, zt_vpn1)
-            self.assertEqual(IpAddress.objects.count(), 4)
-            # Make sure subprocess is not called for identity generation
-            self.assertEqual(mock_subprocess.run.call_count, 0)
-            # No configuration changed
-            self.assertEqual(mock_info.call_count, 0)
-            self.assertEqual(mock_warn.call_count, 0)
-            self.assertEqual(mock_error.call_count, 0)
-            self.assertEqual(mock_requests.post.call_count, 0)
-        _reset_mocks()
-
         with self.subTest(
             (
                 'Test zt no identity generation when different '
diff --git a/tests/openwisp2/sample_config/migrations/0001_initial.py b/tests/openwisp2/sample_config/migrations/0001_initial.py
@@ -630,6 +630,14 @@ class Migration(migrations.Migration):
                 to='sample_config.TemplateTag',
             ),
         ),
+        migrations.AddField(
+            model_name="vpnclient",
+            name="template",
+            field=models.ForeignKey(
+                on_delete=django.db.models.deletion.CASCADE,
+                to='sample_config.template',
+            ),
+        ),
         migrations.CreateModel(
             name='OrganizationConfigSettings',
             fields=[
