[feature] Allow shared subnets to have non shared child subnets #90

openwisp_ipam/admin.py

@@ -162,12 +162,13 @@ class Media:
 
 
 class IpAddressAdminForm(forms.ModelForm):
-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.fields['subnet'].help_text = _(
-            'Select a subnet and the first available IP address '
-            'will be automatically suggested in the ip address field'
-        )
+    class Meta:
+        help_texts = {
+            'subnet': _(
+                'Select a subnet and the first available IP address '
+                'will be automatically suggested in the ip address field'
+            )
+        }
 
 
 @admin.register(IpAddress)

openwisp_ipam/base/models.py

@@ -51,6 +51,7 @@ def clean(self):
             return
         self._validate_multitenant_uniqueness()
         self._validate_multitenant_master_subnet()
+        self._validate_multitenant_unique_child_subnet()
         self._validate_overlapping_subnets()
         self._validate_master_subnet_consistency()
 
@@ -82,24 +83,36 @@ def _validate_multitenant_master_subnet(self):
             return
         if self.master_subnet.organization:
             self._validate_org_relation('master_subnet', field_error='master_subnet')
-        elif self.organization:
+
+    def _validate_multitenant_unique_child_subnet(self):
+        if self.master_subnet is None or self.master_subnet.organization_id is not None:
+            return
+        qs = self._meta.model.objects.exclude(id=self.pk).filter(subnet=self.subnet)
+        if qs.exists():
             raise ValidationError(
                 {
-                    'master_subnet': _(
-                        'Please ensure that the organization of this subnet and '
-                        'the organization of the related subnet match.'
+                    'subnet': _(
+                        'This subnet is already assigned to another organization.'
                     )
                 }
             )
 
     def _validate_overlapping_subnets(self):
-        qs = self._meta.model.objects.only('subnet', 'master_subnet')
-        # if the subnet is not shared, include shared subnets in the overlap check
-        if self.organization:
-            conditions = Q(organization__isnull=True) | Q(
-                organization=self.organization
-            )
-            qs = qs.filter(conditions)
+        organization_query = Q(organization_id=self.organization_id) | Q(
+            organization_id__isnull=True
+        )
+        error_message = _('Subnet overlaps with {0}.')
+        if (
+            self.master_subnet and self.master_subnet.organization_id is None
+        ) or self.organization is None:
+            # The execution of above code implicitly ensures that
+            # organization of both master_subnet and current subnet are
+            # same. Otherwise, self._validate_multitenant_master_subnet
+            # would have raised a validation error
+            organization_query = Q()
+            error_message = _('Subnet overlaps with a subnet of another organization.')
+
+        qs = self._meta.model.objects.filter(organization_query).only('subnet')
         # exclude parent subnets
         exclude = [self.pk]
         parent_subnet = self.master_subnet
@@ -110,9 +123,7 @@ def _validate_overlapping_subnets(self):
         qs = qs.exclude(pk__in=exclude).exclude(subnet=self.subnet)
         for subnet in qs.iterator():
             if ip_network(self.subnet).overlaps(subnet.subnet):
-                raise ValidationError(
-                    {'subnet': _('Subnet overlaps with %s.') % subnet.subnet}
-                )
+                raise ValidationError({'subnet': error_message.format(subnet.subnet)})
 
     def _validate_master_subnet_consistency(self):
         if not self.master_subnet:

openwisp_ipam/tests/test_models.py

@@ -150,7 +150,10 @@ def test_overlapping_subnet(self):
                 self._create_subnet(subnet='10.0.0.0/8', organization=None)
             message_dict = context_manager.exception.message_dict
             self.assertIn('subnet', message_dict)
-            self.assertIn('Subnet overlaps with 10.0.0.0/16.', message_dict['subnet'])
+            self.assertIn(
+                'Subnet overlaps with a subnet of another organization.',
+                message_dict['subnet'],
+            )
 
         with self.subTest('non shared subnet overlaps with shared'):
             Subnet.objects.all().delete()
@@ -204,21 +207,51 @@ def test_master_subnet_validation(self):
                     master_subnet=master, subnet='10.0.0.0/24', organization=None
                 )
             message_dict = context_manager.exception.message_dict
-            self.assertIn('master_subnet', message_dict)
             self.assertIn(error_message, message_dict['master_subnet'])
 
-        with self.subTest('shared master, org1 children: rejected'):
+        with self.subTest('shared master, org1 children: ok'):
             org1 = master.organization
             master.organization = None
             master.full_clean()
             master.save()
+            org1_subnet = self._create_subnet(
+                master_subnet=master, subnet='10.0.0.0/24', organization=org1,
+            )
+            org1_subnet.delete()
+
+        with self.subTest(
+            'shared master, org1 children, org2 overlapping children: reject'
+        ):
+            overlap_error_message = (
+                'Subnet overlaps with a subnet of another organization.'
+            )
+            duplicate_error_message = (
+                'This subnet is already assigned to another organization.'
+            )
+            org1 = self._get_org()
+            org2 = self._create_org(name='test')
+            master.organization = None
+            master.full_clean()
+            master.save()
+            org1_subnet = self._create_subnet(
+                master_subnet=master, subnet='10.0.0.0/24', organization=org1,
+            )
+            # Tests for overlapping subnets
             with self.assertRaises(ValidationError) as context_manager:
                 self._create_subnet(
-                    master_subnet=master, subnet='10.0.0.0/24', organization=org1,
+                    master_subnet=master, subnet='10.0.0.0/25', organization=org2
                 )
             message_dict = context_manager.exception.message_dict
-            self.assertIn('master_subnet', message_dict)
-            self.assertIn(error_message, message_dict['master_subnet'])
+            self.assertIn(overlap_error_message, message_dict['subnet'])
+            # Tests for duplicate subnet
+            with self.assertRaises(ValidationError) as context_manager:
+                self._create_subnet(
+                    master_subnet=master, subnet='10.0.0.0/24', organization=org2
+                )
+            message_dict = context_manager.exception.message_dict
+            self.assertIn(duplicate_error_message, message_dict['subnet'])
+
+            org1_subnet.delete()
 
     def test_valid_subnet_relation_tree(self):
         subnet1 = self._create_subnet(subnet='12.0.56.0/24')
@@ -351,3 +384,16 @@ def test_unique_subnet_multitenancy(self):
             org2 = self._create_org(name='org2', slug='org2')
             subnet2 = self._create_subnet(subnet='10.0.0.0/24', organization=org2)
             self.assertEqual(subnet2.organization, org2)
+
+    def test_nested_overlapping_subnets(self):
+        # Tests overlapping validation for nested subnets
+        shared_level_0_subnet = self._create_subnet(
+            subnet='10.0.0.0/16', organization=None
+        )
+        shared_level_1_subnet = self._create_subnet(
+            subnet='10.0.1.0/24', master_subnet=shared_level_0_subnet, organization=None
+        )
+        org1_level_2_subnet = self._create_subnet(
+            subnet='10.0.1.0/28', master_subnet=shared_level_1_subnet
+        )
+        self._create_subnet(subnet='10.0.1.0/31', master_subnet=org1_level_2_subnet)