Skip to content

Commit efeefce

Browse files
mn-rammn-ram
committed
[fix] Ensure VpnClient post_delete signals fire on removal openwisp#1221
Replace bulk QuerySet.delete() calls with per-instance deletions to ensure post_delete signals fire properly. Each deletion is wrapped in its own transaction.atomic() with try/except logging so the loop continues on failure. Iteration uses .order_by("pk") for deterministic processing order. Applied isolation to all three code paths: device deactivation, template removal, and API serializer. Added regression tests for template removal, device deactivation, and partial-failure scenarios. Fixes openwisp#1221
1 parent f3c99c4 commit efeefce

File tree

3 files changed

+236
-7
lines changed

3 files changed

+236
-7
lines changed

‎openwisp_controller/config/api/serializers.py‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import logging
2+
13
from django.core.exceptions import ValidationError
24
from django.db import transaction
35
from django.db.models import Q
@@ -11,6 +13,8 @@
1113
from .. import settings as app_settings
1214
from ..whois.mixins import WHOISMixin
1315

16+
logger = logging.getLogger(__name__)
17+
1418
Template = load_model("config", "Template")
1519
Vpn = load_model("config", "Vpn")
1620
Device = load_model("config", "Device")
@@ -199,10 +203,29 @@ def _update_config(self, device, config_data):
199203
config = self._prepare_config(device, config_data)
200204
old_templates = list(config.templates.values_list("id", flat=True))
201205
if config_templates != old_templates:
206+
vpn_list = config.templates.filter(type="vpn").values_list("vpn")
207+
if vpn_list:
208+
# Use per-instance deletion to ensure post_delete
209+
# signals fire (peer cache, cert revocation, etc.)
210+
# Each deletion is wrapped in its own transaction so that
211+
# a failure in one does not roll back side effects
212+
# (cert revocation, IP release) of previously committed ones.
213+
for vpnclient in (
214+
config.vpnclient_set.select_related("vpn", "cert", "ip")
215+
.exclude(vpn__in=vpn_list)
216+
.order_by("pk")
217+
.iterator()
218+
):
219+
try:
220+
with transaction.atomic():
221+
vpnclient.delete()
222+
except Exception:
223+
logger.exception(
224+
"Failed to delete VpnClient (id: %s) " "for Config %s",
225+
vpnclient.pk,
226+
config,
227+
)
202228
with transaction.atomic():
203-
vpn_list = config.templates.filter(type="vpn").values_list("vpn")
204-
if vpn_list:
205-
config.vpnclient_set.exclude(vpn__in=vpn_list).delete()
206229
config.templates.set(config_templates, clear=True)
207230
config.save()
208231
except ValidationError as error:

‎openwisp_controller/config/base/config.py‎

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,26 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
338338
if instance.is_deactivating_or_deactivated():
339339
# If the device is deactivated or in the process of deactivating, then
340340
# delete all vpn clients and return.
341-
instance.vpnclient_set.all().delete()
341+
# Use per-instance deletion to ensure post_delete signals fire,
342+
# which handles peer cache invalidation, cert revocation,
343+
# ZeroTier member removal, and IP address release.
344+
# Each deletion is wrapped in its own transaction so that
345+
# a failure in one does not roll back side effects
346+
# (cert revocation, IP release) of previously committed ones.
347+
for vpnclient in (
348+
instance.vpnclient_set.select_related("vpn", "cert", "ip")
349+
.order_by("pk")
350+
.iterator()
351+
):
352+
try:
353+
with transaction.atomic():
354+
vpnclient.delete()
355+
except Exception:
356+
logger.exception(
357+
"Failed to delete VpnClient (id: %s) " "for Config %s",
358+
vpnclient.pk,
359+
instance,
360+
)
342361
return
343362

344363
vpn_client_model = cls.vpn.through
@@ -370,9 +389,29 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
370389
# signal is triggered again—after all templates, including the required
371390
# ones, have been fully added. At that point, we can identify and
372391
# delete VpnClient objects not linked to the final template set.
373-
instance.vpnclient_set.exclude(
374-
template_id__in=instance.templates.values_list("id", flat=True)
375-
).delete()
392+
# Use per-instance deletion to ensure post_delete signals fire,
393+
# which handles peer cache invalidation, cert revocation,
394+
# ZeroTier member removal, and IP address release.
395+
# Each deletion is wrapped in its own transaction so that
396+
# a failure in one does not roll back side effects
397+
# (cert revocation, IP release) of previously committed ones.
398+
for vpnclient in (
399+
instance.vpnclient_set.select_related("vpn", "cert", "ip")
400+
.exclude(
401+
template_id__in=instance.templates.values_list("id", flat=True)
402+
)
403+
.order_by("pk")
404+
.iterator()
405+
):
406+
try:
407+
with transaction.atomic():
408+
vpnclient.delete()
409+
except Exception:
410+
logger.exception(
411+
"Failed to delete VpnClient (id: %s) " "for Config %s",
412+
vpnclient.pk,
413+
instance,
414+
)
376415

377416
if action == "post_add":
378417
for template in templates.filter(type="vpn"):

‎openwisp_controller/config/tests/test_vpn.py‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,173 @@ def _assert_vpn_client_cert(cert, vpn_client, cert_ct, vpn_client_ct):
286286
vpnclient.save()
287287
_assert_vpn_client_cert(cert, vpnclient, 1, 0)
288288

289+
def test_vpn_client_post_delete_on_template_removal(self):
290+
"""Regression test for #1221: VpnClient.post_delete must fire
291+
when a VPN template is removed so that peer cache is invalidated
292+
and certificates are properly revoked."""
293+
org = self._get_org()
294+
vpn = self._create_vpn()
295+
t = self._create_template(name="vpn-test", type="vpn", vpn=vpn, auto_cert=True)
296+
c = self._create_config(organization=org)
297+
c.templates.add(t)
298+
vpnclient = c.vpnclient_set.first()
299+
self.assertIsNotNone(vpnclient)
300+
cert_pk = vpnclient.cert.pk
301+
with mock.patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate:
302+
c.templates.remove(t)
303+
mock_invalidate.assert_called()
304+
self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists())
305+
# Certificate should be revoked (auto_cert=True)
306+
self.assertTrue(Cert.objects.get(pk=cert_pk).revoked)
307+
308+
def test_vpn_client_post_delete_on_device_deactivation(self):
309+
"""Regression test for #1221: VpnClient.post_delete must fire
310+
when a device is deactivated so that peer cache is invalidated
311+
and certificates are properly revoked."""
312+
org = self._get_org()
313+
vpn = self._create_vpn()
314+
t = self._create_template(name="vpn-test", type="vpn", vpn=vpn, auto_cert=True)
315+
d = self._create_device(organization=org)
316+
c = self._create_config(device=d)
317+
c.templates.add(t)
318+
vpnclient = c.vpnclient_set.first()
319+
self.assertIsNotNone(vpnclient)
320+
cert_pk = vpnclient.cert.pk
321+
with mock.patch.object(Vpn, "_invalidate_peer_cache") as mock_invalidate:
322+
d.deactivate()
323+
mock_invalidate.assert_called()
324+
self.assertFalse(VpnClient.objects.filter(pk=vpnclient.pk).exists())
325+
# Certificate should be revoked (auto_cert=True)
326+
self.assertTrue(Cert.objects.get(pk=cert_pk).revoked)
327+
328+
def test_vpnclient_delete_partial_failure_on_deactivation(self):
329+
"""Regression test for #1221: if one VpnClient deletion fails
330+
during device deactivation, remaining VpnClients should still
331+
be deleted with their side effects (cert revocation) intact."""
332+
org = self._get_org()
333+
vpn1 = self._create_vpn(name="vpn1")
334+
vpn2 = self._create_vpn(name="vpn2")
335+
t1 = self._create_template(
336+
name="vpn-test-1", type="vpn", vpn=vpn1, auto_cert=True
337+
)
338+
t2 = self._create_template(
339+
name="vpn-test-2", type="vpn", vpn=vpn2, auto_cert=True
340+
)
341+
d = self._create_device(organization=org)
342+
c = self._create_config(device=d)
343+
c.templates.add(t1, t2)
344+
vc1, vc2 = list(c.vpnclient_set.order_by("pk"))
345+
vc1_pk = vc1.pk
346+
vc2_pk = vc2.pk
347+
cert2_pk = vc2.cert.pk
348+
349+
original_delete = VpnClient.delete
350+
call_count = 0
351+
352+
def failing_delete(self):
353+
nonlocal call_count
354+
call_count += 1
355+
if call_count == 1:
356+
raise Exception("simulated failure")
357+
return original_delete(self)
358+
359+
with mock.patch.object(VpnClient, "delete", failing_delete):
360+
with self.assertLogs(
361+
"openwisp_controller.config.base.config", level="ERROR"
362+
) as log_cm:
363+
d.deactivate()
364+
# First VpnClient delete failed, so it should still exist
365+
self.assertTrue(VpnClient.objects.filter(pk=vc1_pk).exists())
366+
# Second VpnClient should be deleted
367+
self.assertFalse(VpnClient.objects.filter(pk=vc2_pk).exists())
368+
# Second cert should be revoked
369+
self.assertTrue(Cert.objects.get(pk=cert2_pk).revoked)
370+
# Error log should contain the VpnClient PK
371+
self.assertTrue(
372+
any(str(vc1_pk) in msg for msg in log_cm.output),
373+
f"Expected VpnClient PK {vc1_pk} in log output",
374+
)
375+
376+
def test_vpnclient_delete_partial_failure_on_template_removal(self):
377+
"""Regression test for #1221: if one VpnClient deletion fails
378+
during template removal, remaining VpnClients should still
379+
be deleted with their side effects (cert revocation) intact."""
380+
org = self._get_org()
381+
vpn1 = self._create_vpn(name="vpn1")
382+
vpn2 = self._create_vpn(name="vpn2")
383+
t1 = self._create_template(
384+
name="vpn-test-1", type="vpn", vpn=vpn1, auto_cert=True
385+
)
386+
t2 = self._create_template(
387+
name="vpn-test-2", type="vpn", vpn=vpn2, auto_cert=True
388+
)
389+
c = self._create_config(organization=org)
390+
c.templates.add(t1, t2)
391+
vc1, vc2 = list(c.vpnclient_set.order_by("pk"))
392+
vc1_pk = vc1.pk
393+
vc2_pk = vc2.pk
394+
cert2_pk = vc2.cert.pk
395+
396+
original_delete = VpnClient.delete
397+
call_count = 0
398+
399+
def failing_delete(self):
400+
nonlocal call_count
401+
call_count += 1
402+
if call_count == 1:
403+
raise Exception("simulated failure")
404+
return original_delete(self)
405+
406+
with mock.patch.object(VpnClient, "delete", failing_delete):
407+
with self.assertLogs(
408+
"openwisp_controller.config.base.config", level="ERROR"
409+
) as log_cm:
410+
c.templates.remove(t1, t2)
411+
# First VpnClient delete failed, so it should still exist
412+
self.assertTrue(VpnClient.objects.filter(pk=vc1_pk).exists())
413+
# Second VpnClient should be deleted
414+
self.assertFalse(VpnClient.objects.filter(pk=vc2_pk).exists())
415+
# Second cert should be revoked
416+
self.assertTrue(Cert.objects.get(pk=cert2_pk).revoked)
417+
# Error log should contain the VpnClient PK
418+
self.assertTrue(
419+
any(str(vc1_pk) in msg for msg in log_cm.output),
420+
f"Expected VpnClient PK {vc1_pk} in log output",
421+
)
422+
423+
def test_vpnclient_delete_all_fail_no_crash(self):
424+
"""Regression test for #1221: if all VpnClient deletions fail,
425+
the method should not raise and should log each failure."""
426+
org = self._get_org()
427+
vpn1 = self._create_vpn(name="vpn1")
428+
vpn2 = self._create_vpn(name="vpn2")
429+
t1 = self._create_template(
430+
name="vpn-test-1", type="vpn", vpn=vpn1, auto_cert=True
431+
)
432+
t2 = self._create_template(
433+
name="vpn-test-2", type="vpn", vpn=vpn2, auto_cert=True
434+
)
435+
d = self._create_device(organization=org)
436+
c = self._create_config(device=d)
437+
c.templates.add(t1, t2)
438+
vc1_pk, vc2_pk = list(
439+
c.vpnclient_set.order_by("pk").values_list("pk", flat=True)
440+
)
441+
442+
with mock.patch.object(
443+
VpnClient, "delete", side_effect=Exception("simulated failure")
444+
):
445+
with self.assertLogs(
446+
"openwisp_controller.config.base.config", level="ERROR"
447+
) as log_cm:
448+
d.deactivate()
449+
# Both VpnClients should still exist (all deletes failed)
450+
self.assertTrue(VpnClient.objects.filter(pk=vc1_pk).exists())
451+
self.assertTrue(VpnClient.objects.filter(pk=vc2_pk).exists())
452+
# Should have logged 2 errors
453+
error_msgs = [msg for msg in log_cm.output if "Failed to delete" in msg]
454+
self.assertEqual(len(error_msgs), 2)
455+
289456
def test_vpn_client_get_common_name(self):
290457
vpn = self._create_vpn()
291458
d = self._create_device()

0 commit comments

Comments
 (0)