diff --git a/CHANGES.rst b/CHANGES.rst index 8ba6d354e..c6b177a96 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -4,7 +4,14 @@ Changelog Version 1.3.0 [unreleased] -------------------------- -Work in progress. +Bugfixes +~~~~~~~~ + +- Fixed VPN peer cache desync when a VPN template is removed from a + device: replaced bulk ``QuerySet.delete()`` with per-instance deletion + so that ``post_delete`` signals fire, ensuring peer cache invalidation, + certificate revocation and IP address release `#1221 + `_ Version 1.2.2 [2026-03-06] -------------------------- diff --git a/openwisp_controller/config/api/serializers.py b/openwisp_controller/config/api/serializers.py index f07627e60..e26c9536a 100644 --- a/openwisp_controller/config/api/serializers.py +++ b/openwisp_controller/config/api/serializers.py @@ -200,9 +200,15 @@ def _update_config(self, device, config_data): old_templates = list(config.templates.values_list("id", flat=True)) if config_templates != old_templates: with transaction.atomic(): - vpn_list = config.templates.filter(type="vpn").values_list("vpn") - if vpn_list: - config.vpnclient_set.exclude(vpn__in=vpn_list).delete() + new_vpn_ids = Template.objects.filter( + pk__in=config_templates, type="vpn" + ).values_list("vpn", flat=True) + for vpnclient in ( + config.vpnclient_set.select_related("vpn", "cert", "ip") + .exclude(vpn__in=new_vpn_ids) + .iterator() + ): + vpnclient.delete() config.templates.set(config_templates, clear=True) config.save() except ValidationError as error: diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index efb42ff65..1470a9c27 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -338,7 +338,11 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): if instance.is_deactivating_or_deactivated(): # If the device is deactivated or in the process of deactivating, then # delete all vpn clients and return. - instance.vpnclient_set.all().delete() + with transaction.atomic(): + for vpnclient in instance.vpnclient_set.select_related( + "vpn", "cert", "ip" + ).iterator(): + vpnclient.delete() return vpn_client_model = cls.vpn.through @@ -370,9 +374,15 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): # 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() + with transaction.atomic(): + for vpnclient in ( + instance.vpnclient_set.select_related("vpn", "cert", "ip") + .exclude( + template_id__in=instance.templates.values_list("id", flat=True) + ) + .iterator() + ): + vpnclient.delete() if action == "post_add": for template in templates.filter(type="vpn"): diff --git a/openwisp_controller/config/tests/test_api.py b/openwisp_controller/config/tests/test_api.py index fe12fe2cf..f48daca81 100644 --- a/openwisp_controller/config/tests/test_api.py +++ b/openwisp_controller/config/tests/test_api.py @@ -26,6 +26,7 @@ Template = load_model("config", "Template") Vpn = load_model("config", "Vpn") VpnClient = load_model("config", "VpnClient") +Cert = load_model("django_x509", "Cert") Device = load_model("config", "Device") Config = load_model("config", "Config") DeviceGroup = load_model("config", "DeviceGroup") @@ -518,6 +519,50 @@ def test_device_patch_with_templates_of_different_org(self): f" do not match the organization of this configuration: {t3}", ) + def test_device_patch_vpn_template_removal_triggers_post_delete(self): + """Regression test for #1221: removing VPN template via PATCH API must + trigger VpnClient.post_delete so peer cache is invalidated and the + certificate is revoked. Tests both empty and non-empty replacement.""" + org = self._get_org() + vpn = self._create_vpn(organization=org) + t_vpn = self._create_template( + name="vpn-test", type="vpn", vpn=vpn, auto_cert=True, organization=org + ) + t_generic = self._create_template(name="generic-test", organization=org) + device = self._create_device(organization=org) + config = self._create_config(device=device) + path = reverse("config_api:device_detail", args=[device.pk]) + + with self.subTest("replace VPN template with generic template"): + config.templates.set([t_vpn]) + vpnclient = config.vpnclient_set.first() + self.assertIsNotNone(vpnclient) + cert_pk = vpnclient.cert.pk + data = {"config": {"templates": [str(t_generic.pk)]}} + with patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate: + response = self.client.patch( + path, data, content_type="application/json" + ) + mock_invalidate.assert_called_once() + self.assertEqual(response.status_code, 200) + self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists()) + self.assertTrue(Cert.objects.get(pk=cert_pk).revoked) + + with self.subTest("remove all templates (empty list)"): + config.templates.set([t_vpn]) + vpnclient = config.vpnclient_set.first() + self.assertIsNotNone(vpnclient) + cert_pk = vpnclient.cert.pk + data = {"config": {"templates": []}} + with patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate: + response = self.client.patch( + path, data, content_type="application/json" + ) + mock_invalidate.assert_called_once() + self.assertEqual(response.status_code, 200) + self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists()) + self.assertTrue(Cert.objects.get(pk=cert_pk).revoked) + def test_device_change_organization_required_templates(self): org1 = self._create_org(name="org1") org2 = self._create_org(name="org2") diff --git a/openwisp_controller/config/tests/test_vpn.py b/openwisp_controller/config/tests/test_vpn.py index 211ad4a06..e4c701062 100644 --- a/openwisp_controller/config/tests/test_vpn.py +++ b/openwisp_controller/config/tests/test_vpn.py @@ -286,6 +286,70 @@ def _assert_vpn_client_cert(cert, vpn_client, cert_ct, vpn_client_ct): vpnclient.save() _assert_vpn_client_cert(cert, vpnclient, 1, 0) + def test_vpn_client_post_delete_on_template_removal(self): + """Regression test for #1221: VpnClient.post_delete must fire + when a VPN template is removed so that peer cache is invalidated + and certificates are properly revoked.""" + org = self._get_org() + vpn = self._create_vpn() + t = self._create_template(name="vpn-test", type="vpn", vpn=vpn, auto_cert=True) + c = self._create_config(organization=org) + c.templates.add(t) + vpnclient = c.vpnclient_set.first() + self.assertIsNotNone(vpnclient) + cert_pk = vpnclient.cert.pk + with mock.patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate: + c.templates.remove(t) + mock_invalidate.assert_called_once() + self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists()) + # Certificate should be revoked (auto_cert=True) + self.assertTrue(Cert.objects.get(pk=cert_pk).revoked) + + def test_vpn_client_post_delete_on_device_deactivation(self): + """Regression test for #1221: VpnClient.post_delete must fire + when a device is deactivated so that peer cache is invalidated + and certificates are properly revoked.""" + org = self._get_org() + vpn = self._create_vpn() + t = self._create_template(name="vpn-test", type="vpn", vpn=vpn, auto_cert=True) + d = self._create_device(organization=org) + c = self._create_config(device=d) + c.templates.add(t) + vpnclient = c.vpnclient_set.first() + self.assertIsNotNone(vpnclient) + cert_pk = vpnclient.cert.pk + with mock.patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate: + d.deactivate() + mock_invalidate.assert_called_once() + self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists()) + # Certificate should be revoked (auto_cert=True) + self.assertTrue(Cert.objects.get(pk=cert_pk).revoked) + + def test_vpn_client_post_delete_multiple_clients(self): + """Regression test for #1221: when a device has multiple VPN templates, + removing all of them must delete every VpnClient, invalidate peer cache + for each VPN, and revoke all auto-created certificates.""" + org = self._get_org() + vpn1 = self._create_vpn(name="vpn1") + vpn2 = self._create_vpn(name="vpn2", ca=vpn1.ca) + t1 = self._create_template(name="vpn-t1", type="vpn", vpn=vpn1, auto_cert=True) + t2 = self._create_template(name="vpn-t2", type="vpn", vpn=vpn2, auto_cert=True) + d = self._create_device(organization=org) + c = self._create_config(device=d) + c.templates.add(t1, t2) + self.assertEqual(c.vpnclient_set.count(), 2) + vpnclient1 = c.vpnclient_set.get(vpn=vpn1) + vpnclient2 = c.vpnclient_set.get(vpn=vpn2) + cert_pk1 = vpnclient1.cert.pk + cert_pk2 = vpnclient2.cert.pk + with mock.patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate: + d.deactivate() + self.assertEqual(mock_invalidate.call_count, 2) + self.assertFalse(VpnClient.objects.filter(pk=vpnclient1.pk).exists()) + self.assertFalse(VpnClient.objects.filter(pk=vpnclient2.pk).exists()) + self.assertTrue(Cert.objects.get(pk=cert_pk1).revoked) + self.assertTrue(Cert.objects.get(pk=cert_pk2).revoked) + def test_vpn_client_get_common_name(self): vpn = self._create_vpn() d = self._create_device() @@ -728,6 +792,18 @@ def test_trigger_vpn_server_endpoint_invalid_vpn_id(self): f"VPN Server UUID: {vpn_id} does not exist." ) + def test_wireguard_vpnclient_ip_released_on_template_removal(self): + """Regression test for #1221: when a Wireguard VPN template is removed, + the allocated IP address must be released (deleted).""" + device, vpn, template = self._create_wireguard_vpn_template() + vpnclient = device.config.vpnclient_set.first() + self.assertIsNotNone(vpnclient) + self.assertIsNotNone(vpnclient.ip) + ip_pk = vpnclient.ip.pk + device.config.templates.remove(template) + self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists()) + self.assertFalse(IpAddress.objects.filter(pk=ip_pk).exists()) + class TestWireguardTransaction(BaseTestVpn, TestWireguardVpnMixin, TransactionTestCase): mock_response = mock.Mock(spec=requests.Response) diff --git a/tests/openwisp2/settings.py b/tests/openwisp2/settings.py index ea1a79a56..45c36c4cb 100644 --- a/tests/openwisp2/settings.py +++ b/tests/openwisp2/settings.py @@ -14,7 +14,7 @@ "default": { "ENGINE": "openwisp_utils.db.backends.spatialite", "NAME": os.path.join(BASE_DIR, "openwisp-controller.db"), - "OPTIONS": {"timeout": 10}, + "OPTIONS": {"timeout": 20}, } } if TESTING and "--exclude-tag=selenium_tests" not in sys.argv: