[fix] Fix VPN-client template switch bug with same VPN server #973

Closes #973. --------- Co-authored-by: Gagan Deep <[email protected]> Co-authored-by: Federico Capoano <[email protected]>
openwisp · Feb 21, 2025 · aba8beb · aba8beb
1 parent c39c916
commit aba8beb
Show file tree

Hide file tree

Showing 2 changed files with 127 additions and 22 deletions.
diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py
@@ -256,7 +256,7 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
 
         if action == 'post_clear':
             if instance.is_deactivating_or_deactivated():
-                # If the device is deactivated or in the process of deactivatiing, then
+                # If the device is deactivated or in the process of deactivating, then
                 # delete all vpn clients and return.
                 instance.vpnclient_set.all().delete()
             return
@@ -265,37 +265,35 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
         # coming from signal
         if isinstance(pk_set, set):
             template_model = cls.get_template_model()
-            # Ordering the queryset here doesn't affect the functionality
-            # since pk_set is a set. Though ordering the queryset is required
-            # for tests.
             templates = template_model.objects.filter(pk__in=list(pk_set)).order_by(
                 'created'
             )
         # coming from admin ModelForm
         else:
             templates = pk_set
-        # delete VPN clients which have been cleared
-        # by sortedm2m and have not been added back
-        if action == 'post_add':
-            vpn_list = instance.templates.filter(type='vpn').values_list('vpn')
-            instance.vpnclient_set.exclude(vpn__in=vpn_list).delete()
-        # when adding or removing specific templates
+
+        # Get current VPNs in use by any template
+        current_vpns = set(
+            instance.templates.filter(type='vpn').values_list('vpn_id', flat=True)
+        )
+
+        # Handle template actions
         for template in templates.filter(type='vpn'):
             if action == 'post_add':
-                if vpn_client_model.objects.filter(
+                # Create VPN client if needed
+                if not vpn_client_model.objects.filter(
                     config=instance, vpn=template.vpn
                 ).exists():
-                    continue
-                client = vpn_client_model(
-                    config=instance,
-                    vpn=template.vpn,
-                    auto_cert=template.auto_cert,
-                )
-                client.full_clean()
-                client.save()
-            elif action == 'post_remove':
-                for client in instance.vpnclient_set.filter(vpn=template.vpn):
-                    client.delete()
+                    client = vpn_client_model(
+                        config=instance,
+                        vpn=template.vpn,
+                        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/tests/test_admin.py b/openwisp_controller/config/tests/test_admin.py
@@ -2047,6 +2047,113 @@ def test_device_changelist_activate_deactivate_admin_action_security(
             )
             self.assertEqual(mocked_deactivate.call_count, 1)
 
+    def test_vpn_template_switch(self):
+        """
+        Test switching between two VPN templates that use the same VPN server
+        Verifies that:
+        1. Only one VpnClient exists at a time
+        2. VPN config variables are correctly resolved
+        3. Switching back and forth works properly
+        """
+        vpn = self._create_vpn()
+        template1 = self._create_template(
+            name='vpn-test-1',
+            type='vpn',
+            vpn=vpn,
+            config={},
+            auto_cert=True,
+        )
+        template1.config['openvpn'][0]['dev'] = 'tun0'
+        template1.full_clean()
+        template1.save()
+        template2 = self._create_template(
+            name='vpn-test-2',
+            type='vpn',
+            vpn=vpn,
+            config={},
+            auto_cert=True,
+        )
+        template2.config['openvpn'][0]['dev'] = 'tun1'
+        template2.full_clean()
+        template2.save()
+
+        # Add device with default template (template1)
+        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)
+        self.assertEqual(response.status_code, 200)
+        config = Config.objects.get(device__name=params['name'])
+
+        # Add template1 to the device
+        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-0-templates': str(template1.pk),
+                'config-INITIAL_FORMS': 1,
+                '_continue': True,
+            }
+        )
+        response = self.client.post(path, data=params, follow=True)
+        self.assertEqual(response.status_code, 200)
+        config.refresh_from_db()
+
+        # Ensure all works as expected
+        self.assertEqual(config.templates.count(), 1)
+        self.assertEqual(config.vpnclient_set.count(), 1)
+        self.assertEqual(
+            config.backend_instance.config['openvpn'][0]['cert'],
+            f'/etc/x509/client-{vpn.pk.hex}.pem',
+        )
+        self.assertEqual(
+            config.backend_instance.config['openvpn'][0]['dev'],
+            'tun0',
+        )
+
+        with self.subTest('Switch device to template2'):
+            path = reverse(
+                f'admin:{self.app_label}_device_change', args=[config.device_id]
+            )
+            params.update(
+                {
+                    'config-0-templates': str(template2.pk),
+                }
+            )
+            response = self.client.post(path, data=params, follow=True)
+            self.assertEqual(response.status_code, 200)
+            config.refresh_from_db()
+            del config.backend_instance
+            self.assertEqual(
+                config.backend_instance.config['openvpn'][0]['cert'],
+                f'/etc/x509/client-{vpn.pk.hex}.pem',
+            )
+            self.assertEqual(
+                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(
+                {
+                    'config-0-templates': str(template1.pk),
+                }
+            )
+            response = self.client.post(path, data=params, follow=True)
+            self.assertEqual(response.status_code, 200)
+            config.refresh_from_db()
+            del config.backend_instance
+            self.assertEqual(
+                config.backend_instance.config['openvpn'][0]['cert'],
+                f'/etc/x509/client-{vpn.pk.hex}.pem',
+            )
+            self.assertEqual(
+                config.backend_instance.config['openvpn'][0]['dev'],
+                'tun0',
+            )
+            self.assertEqual(config.vpnclient_set.count(), 1)
+
 
 class TestTransactionAdmin(
     CreateConfigTemplateMixin,