diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 1aa5e6382..bdadc120e 100644 --- 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 index 62c28b313..3531f6e71 100644 --- 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,