From d60ec138c6bd8b531935544748b4c89caf3fb9b7 Mon Sep 17 00:00:00 2001 From: prakash-kalwaniya Date: Mon, 16 Mar 2026 03:27:49 +0530 Subject: [PATCH 1/2] [fix] Ensure VpnClient.post_delete fires on template detach #1221 Replace bulk QuerySet.delete() with per-instance delete loops so that post_delete signals fire correctly, triggering peer cache invalidation, certificate revocation, and IP address release. --- openwisp_controller/config/api/serializers.py | 9 +++- openwisp_controller/config/base/config.py | 26 ++++++++-- openwisp_controller/config/tests/test_vpn.py | 48 +++++++++++++++++++ 3 files changed, 78 insertions(+), 5 deletions(-) diff --git a/openwisp_controller/config/api/serializers.py b/openwisp_controller/config/api/serializers.py index f07627e60..94a0c59fc 100644 --- a/openwisp_controller/config/api/serializers.py +++ b/openwisp_controller/config/api/serializers.py @@ -202,7 +202,14 @@ def _update_config(self, device, config_data): 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() + # Per-instance delete ensures post_delete signals fire + # (cache invalidation, cert revocation, IP release). + for vpnclient in ( + config.vpnclient_set.exclude(vpn__in=vpn_list) + .select_related("vpn", "cert", "ip") + .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..533cb5fe4 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -338,7 +338,15 @@ 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() + # Per-instance delete ensures post_delete signals fire + # (cache invalidation, cert revocation, IP release). + 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 +378,19 @@ 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() + # Per-instance delete ensures post_delete signals fire + # (cache invalidation, cert revocation, IP release). + with transaction.atomic(): + for vpnclient in ( + instance.vpnclient_set.exclude( + template_id__in=instance.templates.values_list( + "id", flat=True + ) + ) + .select_related("vpn", "cert", "ip") + .iterator() + ): + vpnclient.delete() if action == "post_add": for template in templates.filter(type="vpn"): diff --git a/openwisp_controller/config/tests/test_vpn.py b/openwisp_controller/config/tests/test_vpn.py index 211ad4a06..efa8bb4cd 100644 --- a/openwisp_controller/config/tests/test_vpn.py +++ b/openwisp_controller/config/tests/test_vpn.py @@ -880,6 +880,54 @@ def test_vpn_peers_changed(self): device.config.templates.remove(template) handler.assert_called_once() + def test_template_removal_fires_post_delete_signals(self): + """Regression test for #1221: removing a VPN template must trigger + VpnClient.post_delete so that the peer cache is invalidated, + certificates are revoked, and IP addresses are released.""" + device, vpn, template = self._create_wireguard_vpn_template() + vpn_client = device.config.vpnclient_set.first() + self.assertIsNotNone(vpn_client) + cert = vpn_client.cert + ip = vpn_client.ip + self.assertIsNotNone(ip) + initial_ip_count = IpAddress.objects.count() + + with self.subTest("peer cache invalidated on template removal"): + with catch_signal(vpn_peers_changed) as handler: + device.config.templates.remove(template) + handler.assert_called_once() + + with self.subTest("VpnClient deleted"): + self.assertEqual(device.config.vpnclient_set.count(), 0) + + with self.subTest("IP address released"): + self.assertLess(IpAddress.objects.count(), initial_ip_count) + + with self.subTest("certificate revoked for OpenVPN-style auto_cert"): + # For WireGuard there's no x509 cert to revoke, but the + # post_delete handler should still have run without error. + # The cert object should not exist anymore (CASCADE from VpnClient). + self.assertFalse( + VpnClient.objects.filter(pk=vpn_client.pk).exists() + ) + + def test_deactivation_fires_post_delete_signals(self): + """Regression test for #1221: deactivating a device must trigger + VpnClient.post_delete for each client (not bulk QuerySet.delete).""" + device, vpn, template = self._create_wireguard_vpn_template() + self.assertEqual(device.config.vpnclient_set.count(), 1) + initial_ip_count = IpAddress.objects.count() + + with catch_signal(vpn_peers_changed) as handler: + device.config.templates.clear() + # post_clear with deactivating state deletes vpn clients + # The signal should fire because per-instance delete is used + if device.config.vpnclient_set.count() == 0: + handler.assert_called() + + # Verify cleanup happened + self.assertEqual(device.config.vpnclient_set.count(), 0) + class TestVxlan(BaseTestVpn, TestVxlanWireguardVpnMixin, TestCase): def test_vxlan_config_creation(self): From eab33a6cc66caca6bc97759b502e85697e7e501f Mon Sep 17 00:00:00 2001 From: prakash-kalwaniya Date: Mon, 16 Mar 2026 07:40:53 +0530 Subject: [PATCH 2/2] [tests] Fix deactivation test to exercise correct code path #1221 test_deactivation_fires_post_delete_signals was calling device.config.templates.clear() directly without first setting the config status to deactivating. This meant the per-instance delete path guarded by is_deactivating_or_deactivated() in manage_vpn_clients was never exercised, defeating the purpose of the regression test. Replace templates.clear() with device.deactivate() which sets the status to deactivating before clearing templates. Also replace global IP count checks with object-level assertions, remove the unused cert variable (F841), and remove the redundant "certificate revoked for OpenVPN-style auto_cert" subTest that duplicated the VpnClient deletion assertion. Related to #1221 --- openwisp_controller/config/base/config.py | 12 +++---- openwisp_controller/config/tests/test_vpn.py | 35 ++++++++------------ 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/openwisp_controller/config/base/config.py b/openwisp_controller/config/base/config.py index 533cb5fe4..12e91e2a9 100644 --- a/openwisp_controller/config/base/config.py +++ b/openwisp_controller/config/base/config.py @@ -341,11 +341,9 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): # Per-instance delete ensures post_delete signals fire # (cache invalidation, cert revocation, IP release). with transaction.atomic(): - for vpnclient in ( - instance.vpnclient_set.select_related( - "vpn", "cert", "ip" - ).iterator() - ): + for vpnclient in instance.vpnclient_set.select_related( + "vpn", "cert", "ip" + ).iterator(): vpnclient.delete() return @@ -383,9 +381,7 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs): with transaction.atomic(): for vpnclient in ( instance.vpnclient_set.exclude( - template_id__in=instance.templates.values_list( - "id", flat=True - ) + template_id__in=instance.templates.values_list("id", flat=True) ) .select_related("vpn", "cert", "ip") .iterator() diff --git a/openwisp_controller/config/tests/test_vpn.py b/openwisp_controller/config/tests/test_vpn.py index efa8bb4cd..d510df8d8 100644 --- a/openwisp_controller/config/tests/test_vpn.py +++ b/openwisp_controller/config/tests/test_vpn.py @@ -883,14 +883,12 @@ def test_vpn_peers_changed(self): def test_template_removal_fires_post_delete_signals(self): """Regression test for #1221: removing a VPN template must trigger VpnClient.post_delete so that the peer cache is invalidated, - certificates are revoked, and IP addresses are released.""" + and IP addresses are released.""" device, vpn, template = self._create_wireguard_vpn_template() vpn_client = device.config.vpnclient_set.first() self.assertIsNotNone(vpn_client) - cert = vpn_client.cert - ip = vpn_client.ip - self.assertIsNotNone(ip) - initial_ip_count = IpAddress.objects.count() + vpn_client_pk = vpn_client.pk + ip_pk = vpn_client.ip.pk with self.subTest("peer cache invalidated on template removal"): with catch_signal(vpn_peers_changed) as handler: @@ -898,36 +896,29 @@ def test_template_removal_fires_post_delete_signals(self): handler.assert_called_once() with self.subTest("VpnClient deleted"): - self.assertEqual(device.config.vpnclient_set.count(), 0) + self.assertFalse(VpnClient.objects.filter(pk=vpn_client_pk).exists()) with self.subTest("IP address released"): - self.assertLess(IpAddress.objects.count(), initial_ip_count) - - with self.subTest("certificate revoked for OpenVPN-style auto_cert"): - # For WireGuard there's no x509 cert to revoke, but the - # post_delete handler should still have run without error. - # The cert object should not exist anymore (CASCADE from VpnClient). - self.assertFalse( - VpnClient.objects.filter(pk=vpn_client.pk).exists() - ) + self.assertFalse(IpAddress.objects.filter(pk=ip_pk).exists()) def test_deactivation_fires_post_delete_signals(self): """Regression test for #1221: deactivating a device must trigger VpnClient.post_delete for each client (not bulk QuerySet.delete).""" device, vpn, template = self._create_wireguard_vpn_template() - self.assertEqual(device.config.vpnclient_set.count(), 1) - initial_ip_count = IpAddress.objects.count() + vpn_client = device.config.vpnclient_set.first() + self.assertIsNotNone(vpn_client) + ip_pk = vpn_client.ip.pk with catch_signal(vpn_peers_changed) as handler: - device.config.templates.clear() - # post_clear with deactivating state deletes vpn clients - # The signal should fire because per-instance delete is used - if device.config.vpnclient_set.count() == 0: - handler.assert_called() + device.deactivate() + handler.assert_called_once() # Verify cleanup happened self.assertEqual(device.config.vpnclient_set.count(), 0) + with self.subTest("IP address released"): + self.assertFalse(IpAddress.objects.filter(pk=ip_pk).exists()) + class TestVxlan(BaseTestVpn, TestVxlanWireguardVpnMixin, TestCase): def test_vxlan_config_creation(self):