Skip to content

Commit 9cbc0ea

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. Added regression tests for template removal, device deactivation, and partial-failure scenarios. Fixes openwisp#1221
1 parent f3c99c4 commit 9cbc0ea

File tree

3 files changed

+215
-5
lines changed

3 files changed

+215
-5
lines changed

‎openwisp_controller/config/api/serializers.py‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,14 @@ def _update_config(self, device, config_data):
202202
with transaction.atomic():
203203
vpn_list = config.templates.filter(type="vpn").values_list("vpn")
204204
if vpn_list:
205-
config.vpnclient_set.exclude(vpn__in=vpn_list).delete()
205+
# Use per-instance deletion to ensure post_delete
206+
# signals fire (peer cache, cert revocation, etc.)
207+
for vpnclient in (
208+
config.vpnclient_set.select_related("vpn", "cert", "ip")
209+
.exclude(vpn__in=vpn_list)
210+
.iterator()
211+
):
212+
vpnclient.delete()
206213
config.templates.set(config_templates, clear=True)
207214
config.save()
208215
except ValidationError as error:

‎openwisp_controller/config/base/config.py‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,7 +338,24 @@ 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 instance.vpnclient_set.select_related(
348+
"vpn", "cert", "ip"
349+
).iterator():
350+
try:
351+
with transaction.atomic():
352+
vpnclient.delete()
353+
except Exception:
354+
logger.exception(
355+
"Failed to delete VpnClient (id: %s) " "for Config %s",
356+
vpnclient.pk,
357+
instance,
358+
)
342359
return
343360

344361
vpn_client_model = cls.vpn.through
@@ -370,9 +387,28 @@ def manage_vpn_clients(cls, action, instance, pk_set, **kwargs):
370387
# signal is triggered again—after all templates, including the required
371388
# ones, have been fully added. At that point, we can identify and
372389
# 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()
390+
# Use per-instance deletion to ensure post_delete signals fire,
391+
# which handles peer cache invalidation, cert revocation,
392+
# ZeroTier member removal, and IP address release.
393+
# Each deletion is wrapped in its own transaction so that
394+
# a failure in one does not roll back side effects
395+
# (cert revocation, IP release) of previously committed ones.
396+
for vpnclient in (
397+
instance.vpnclient_set.select_related("vpn", "cert", "ip")
398+
.exclude(
399+
template_id__in=instance.templates.values_list("id", flat=True)
400+
)
401+
.iterator()
402+
):
403+
try:
404+
with transaction.atomic():
405+
vpnclient.delete()
406+
except Exception:
407+
logger.exception(
408+
"Failed to delete VpnClient (id: %s) " "for Config %s",
409+
vpnclient.pk,
410+
instance,
411+
)
376412

377413
if action == "post_add":
378414
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)