[fix] Do not allow applying more than 1 templates of same VPN server #832

[pandafy]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• N/A I have updated the documentation.

Reference to Existing Issue

Fixes #832

Screenshot

[pandafy]

This check makes the following test fail - when users switches the template of the device and both the templates belongs to the same VPN.

It happens because pre_add action is sent before pre_clear, thus the db still has old template assigned to the config and this logic raises an error.

openwisp-controller/openwisp_controller/config/tests/test_admin.py

Lines 2056 to 2163 in ccf380c

	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()
	self.assertEqual(config.vpnclient_set.count(), 1)
	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',
	)
	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)

[coveralls]

coverage: 98.816% (-0.01%) from 98.827%
when pulling 257732f on issues/832-multiple-template-same-vpn
into ba733cf on master.

[nemesifier]

I tried replicating #832 on the current master and I couldn't do so, I get an uncaught exception instead:

Are you sure all the new logic is 100% needed? It seems recent changes already half-handled this.

Trying the same operation on this branch gives me the following result which is a lot better and matches your testing:

I have other observations below.

[pandafy]

I tried replicating #832 on the current master and I couldn't do so, I get an uncaught exception instead:

Are you sure all the new logic is 100% needed? It seems recent changes already half-handled this.

The ValidationError you saw is raised by Config.manage_vpn_clients (due to client.full_clean()):

openwisp-controller/openwisp_controller/config/base/config.py

Lines 333 to 346 in dc499d2

	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, template=template
	).exists():
	client = vpn_client_model(
	config=instance,
	vpn=template.vpn,
	template=template,
	auto_cert=template.auto_cert,
	)
	client.full_clean()
	client.save()

But, this is called in post_add (i.e after executing Config.save()). Django does not expect errors to be called such late in the flow (Django Admin does not handle errors raised from save()), thus it returns HTTP 500 response.

This PR performs validation of VPN Client templates and ensures the error is caught before save().