From d61528e4647e5d9064ab1a732550995a76975978 Mon Sep 17 00:00:00 2001 From: Martin Hauser Date: Thu, 6 Aug 2026 22:08:41 +0200 Subject: [PATCH] Fixes #22821: Prevent Tenant Group deletion from creating duplicate ungrouped Tenant names or slugs (#22830) --- docs/models/tenancy/tenantgroup.md | 2 + netbox/tenancy/models/tenants.py | 20 +++++- netbox/tenancy/tests/test_api.py | 53 ++++++++++++++- netbox/tenancy/tests/test_models.py | 102 +++++++++++++++++++++++++++- 4 files changed, 173 insertions(+), 4 deletions(-) diff --git a/docs/models/tenancy/tenantgroup.md b/docs/models/tenancy/tenantgroup.md index f9929ff66..22441f789 100644 --- a/docs/models/tenancy/tenantgroup.md +++ b/docs/models/tenancy/tenantgroup.md @@ -4,6 +4,8 @@ Tenant groups may be nested recursively to achieve a multi-level hierarchy. For example, you might have a group called "Customers" containing subgroups of individual tenants grouped by product or account team. +A tenant group cannot be deleted if ungrouping its tenants, including those of any nested groups, would result in duplicate tenant names or slugs among ungrouped tenants. + ## Fields ### Parent diff --git a/netbox/tenancy/models/tenants.py b/netbox/tenancy/models/tenants.py index 923c9a9ec..d6c7e8c2d 100644 --- a/netbox/tenancy/models/tenants.py +++ b/netbox/tenancy/models/tenants.py @@ -1,5 +1,5 @@ from django.db import models -from django.db.models import Q +from django.db.models import Count, ProtectedError, Q from django.utils.translation import gettext_lazy as _ from netbox.models import NestedGroupModel, PrimaryModel @@ -35,6 +35,24 @@ class TenantGroup(NestedGroupModel): verbose_name = _('tenant group') verbose_name_plural = _('tenant groups') + def delete(self, *args, **kwargs): + # Ungrouping the tenants of this group and its descendants can violate tenant name and slug uniqueness. + ungrouped = Tenant.objects.filter( + Q(group__isnull=True) | Q(group__in=self.get_descendants(include_self=True)) + ) + duplicate_names = ungrouped.values('name').annotate(count=Count('pk')).filter(count__gt=1).values('name') + duplicate_slugs = ungrouped.values('slug').annotate(count=Count('pk')).filter(count__gt=1).values('slug') + if conflicts := set(ungrouped.filter(Q(name__in=duplicate_names) | Q(slug__in=duplicate_slugs))): + raise ProtectedError( + _( + "Unable to delete tenant group {tenant_group}. Ungrouping its tenants, including those of any " + "nested groups, would create duplicate tenant names or slugs." + ).format(tenant_group=self), + conflicts, + ) + + return super().delete(*args, **kwargs) + class Tenant(ContactsMixin, PrimaryModel): """ diff --git a/netbox/tenancy/tests/test_api.py b/netbox/tenancy/tests/test_api.py index 36235a56b..dcba0025b 100644 --- a/netbox/tenancy/tests/test_api.py +++ b/netbox/tenancy/tests/test_api.py @@ -1,9 +1,14 @@ +import json +import logging + +from django.test import tag from django.urls import reverse +from rest_framework import status from dcim.models import Site from tenancy.choices import * from tenancy.models import * -from utilities.testing import APITestCase, APIViewTestCases +from utilities.testing import APITestCase, APIViewTestCases, disable_logging class AppTestCase(APITestCase): @@ -60,6 +65,52 @@ class TenantGroupTestCase(APIViewTestCases.APIViewTestCase): }, ] + @tag('regression') # Ref: #22821 + def test_delete_tenant_group_with_conflicting_tenants(self): + """ + Attempt and fail to delete a tenant group whose tenants cannot be ungrouped. + """ + group = TenantGroup.objects.create(name='Tenant Group 7', slug='tenant-group-7') + Tenant.objects.create(name='Tenant 1', slug='tenant-1a', group=group) + Tenant.objects.create(name='Tenant 1', slug='tenant-1b') + + self.add_permissions('tenancy.delete_tenantgroup') + url = reverse('tenancy-api:tenantgroup-detail', kwargs={'pk': group.pk}) + with disable_logging(level=logging.WARNING): + response = self.client.delete(url, **self.header) + + self.assertHttpStatus(response, status.HTTP_409_CONFLICT) + + content = json.loads(response.content.decode('utf-8')) + self.assertIn('detail', content) + self.assertTrue(content['detail'].startswith('Unable to delete object.')) + self.assertTrue(TenantGroup.objects.filter(pk=group.pk).exists()) + + @tag('regression') # Ref: #22821 + def test_bulk_delete_tenant_groups_with_conflicting_tenants(self): + """ + Attempt and fail to bulk delete two tenant groups whose tenants conflict only once both are + ungrouped, leaving every group and assignment intact. + """ + group1 = TenantGroup.objects.create(name='Tenant Group 8', slug='tenant-group-8') + group2 = TenantGroup.objects.create(name='Tenant Group 9', slug='tenant-group-9') + tenant1 = Tenant.objects.create(name='Tenant 2', slug='tenant-2a', group=group1) + tenant2 = Tenant.objects.create(name='Tenant 2', slug='tenant-2b', group=group2) + + self.add_permissions('tenancy.delete_tenantgroup') + data = [{'id': group1.pk}, {'id': group2.pk}] + with disable_logging(level=logging.WARNING): + response = self.client.delete(self._get_list_url(), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_409_CONFLICT) + + # The rolled back batch must not leave the first group deleted + self.assertEqual(TenantGroup.objects.filter(pk__in=(group1.pk, group2.pk)).count(), 2) + tenant1.refresh_from_db() + tenant2.refresh_from_db() + self.assertEqual(tenant1.group, group1) + self.assertEqual(tenant2.group, group2) + class TenantTestCase(APIViewTestCases.APIViewTestCase): model = Tenant diff --git a/netbox/tenancy/tests/test_models.py b/netbox/tenancy/tests/test_models.py index 00f7c124f..438830d49 100644 --- a/netbox/tenancy/tests/test_models.py +++ b/netbox/tenancy/tests/test_models.py @@ -1,6 +1,104 @@ -from django.test import TestCase +from django.db.models import ProtectedError +from django.test import TestCase, tag -from tenancy.models import Contact, ContactGroup +from tenancy.models import Contact, ContactGroup, Tenant, TenantGroup + + +class TenantGroupTestCase(TestCase): + + @tag('regression') # Ref: #22821 + def test_tenantgroup_deletion_blocked_by_duplicate_ungrouped_name(self): + """ + Deleting a tenant group must raise ProtectedError when ungrouping its tenant would duplicate + the name of an already ungrouped tenant. + """ + group = TenantGroup.objects.create(name='Tenant Group 1', slug='tenant-group-1') + tenant1 = Tenant.objects.create(name='Tenant 1', slug='tenant-1a', group=group) + tenant2 = Tenant.objects.create(name='Tenant 1', slug='tenant-1b') + + with self.assertRaises(ProtectedError) as cm: + group.delete() + + self.assertEqual( + cm.exception.args[0], + 'Unable to delete tenant group Tenant Group 1. Ungrouping its tenants, including those of any nested ' + 'groups, would create duplicate tenant names or slugs.' + ) + self.assertEqual(set(cm.exception.protected_objects), {tenant1, tenant2}) + + # The failed deletion must leave the group and its tenant assignment intact + self.assertTrue(TenantGroup.objects.filter(pk=group.pk).exists()) + tenant1.refresh_from_db() + self.assertEqual(tenant1.group, group) + + @tag('regression') # Ref: #22821 + def test_tenantgroup_deletion_blocked_by_duplicate_ungrouped_slug(self): + """ + Deleting a tenant group must raise ProtectedError for a slug collision alone, when the + colliding tenants have differing names. + """ + group = TenantGroup.objects.create(name='Tenant Group 2', slug='tenant-group-2') + tenant1 = Tenant.objects.create(name='Tenant 2', slug='duplicate-slug', group=group) + tenant2 = Tenant.objects.create(name='Tenant 3', slug='duplicate-slug') + + with self.assertRaises(ProtectedError) as cm: + group.delete() + + self.assertEqual(set(cm.exception.protected_objects), {tenant1, tenant2}) + self.assertTrue(TenantGroup.objects.filter(pk=group.pk).exists()) + + @tag('regression') # Ref: #22821 + def test_tenantgroup_deletion_blocked_by_duplicate_name_in_descendants(self): + """ + Deleting a parent tenant group must raise ProtectedError when ungrouping the tenants of its + descendant groups would duplicate a name. + """ + parent = TenantGroup.objects.create(name='Parent Group', slug='parent-group') + child1 = TenantGroup.objects.create(name='Child Group 1', slug='child-group-1', parent=parent) + child2 = TenantGroup.objects.create(name='Child Group 2', slug='child-group-2', parent=parent) + tenant1 = Tenant.objects.create(name='Tenant 4', slug='tenant-4a', group=child1) + tenant2 = Tenant.objects.create(name='Tenant 4', slug='tenant-4b', group=child2) + + with self.assertRaises(ProtectedError) as cm: + parent.delete() + + self.assertEqual(set(cm.exception.protected_objects), {tenant1, tenant2}) + self.assertTrue(TenantGroup.objects.filter(pk=parent.pk).exists()) + self.assertEqual(TenantGroup.objects.filter(pk__in=(child1.pk, child2.pk)).count(), 2) + + def test_tenantgroup_deletion_ungroups_tenants(self): + """ + Deleting a tenant group whose tenants can be ungrouped safely must succeed and clear the group + assignment across the whole subtree. + """ + parent = TenantGroup.objects.create(name='Parent Group', slug='parent-group') + child = TenantGroup.objects.create(name='Child Group', slug='child-group', parent=parent) + tenant1 = Tenant.objects.create(name='Tenant 5', slug='tenant-5', group=parent) + tenant2 = Tenant.objects.create(name='Tenant 6', slug='tenant-6', group=child) + + parent.delete() + + self.assertFalse(TenantGroup.objects.filter(pk__in=(parent.pk, child.pk)).exists()) + tenant1.refresh_from_db() + tenant2.refresh_from_db() + self.assertIsNone(tenant1.group) + self.assertIsNone(tenant2.group) + + def test_tenantgroup_deletion_ignores_tenants_in_unrelated_groups(self): + """ + Deleting a tenant group must succeed when a tenant outside its subtree shares a name and slug + with one of its own tenants, as that tenant remains grouped. + """ + group = TenantGroup.objects.create(name='Tenant Group 3', slug='tenant-group-3') + unrelated = TenantGroup.objects.create(name='Unrelated Group', slug='unrelated-group') + tenant = Tenant.objects.create(name='Tenant 7', slug='tenant-7', group=group) + Tenant.objects.create(name='Tenant 7', slug='tenant-7', group=unrelated) + + group.delete() + + self.assertFalse(TenantGroup.objects.filter(pk=group.pk).exists()) + tenant.refresh_from_db() + self.assertIsNone(tenant.group) class ContactGroupTestCase(TestCase):