Repository: netbox-community/netbox
Pull request: 18706 Fix VLAN Assignment checking
### Fixes: #18706 

VLANs can currently be assigned to both a Site and SiteGroup. The SiteGroup has a scope assignment that can be to a Site or SiteGroup.  If a VLAN is assigned to both a Site and SiteGroup an error is raised if the SiteGroup assigned site doesn't match the VLAN site. However the check assumed the SiteGroup was only scoped to a Site, this fixes it to account that it can be scoped to either a Site or SiteGroup.

Problem statement:
Changing scope of VLAN group breaks possible VLAN association
### Deployment Type

NetBox Cloud

### NetBox Version

v4.2.2

### Python Version

3.12

### Steps to Reproduce

1. create a Site "New York"
2. create as Site Group "USA"
3. assign site "New York" to Site Group "USA"
4. create a VLAN group "example group" with scope Site and and site "New York"
5. create a VLAN "example VLAN" and assign both site "New York" and and group "example group"
6. change scope of VLAN group "example group" to Site-Group and select group "USA"
7. try to create a new VLAN "example with issue" and assign both site "New York" and and group "example group"

### Expected Behavior

That the VLAN "example with issue" can be assigned to VLAN group "example group" and site "New York" at the same time.

### Observed Behavior

Step 7 will fail, even though "example VLAN"  is assigned to exactly the same set of site and group

Error message:
```
VLAN is assigned to group example group (scope: USA); cannot also assign to site New York.
```

This could be solved by:
* changing VLAN to use scope as well: `dcim.site` and `ipam.vlangroup`
* permit both but add add constrains checking during add of a VLAN and at the change of the VLAN group scope to not violate the conditions of the currently assigned VLANs to that group
* make the usage of site and group exclusive for a VLAN. Either site or group, not both.

Hints from issue discussion:
Oh, and this error also occurs when we try to change unrelated attributes of described "example VLAN" in the description above:

```
Sending PATCH to 'http://netbox.test:80/api/ipam/vlans/49/' with data 'b'{"tags": [{"name": "NetBox-synced"}, {"name": "Source: vcsa"}]}''
```

Result:
```
ERROR: NetBox returned: PATCH /api/ipam/vlans/49/ Bad Request
ERROR: NetBox returned body: {'__all__': ['VLAN is assigned to group example group (scope: USA); cannot also assign to site New York.']}
```
It seems likely this behavior will also manifest in other structures that use scoping, not just VLAN groups/VLANs. Ideally the fix should address all such cases centrally, without redundancy, and note if possible all the object types affected by this issue.
Interestingly enough, this works for existing VLAN objects, that are already assigned to a site/VLAN group. You can change them without this error being triggered. However, newly created VLANs are indeed affected.

Commit message: 18706 add tests

Patch to review:
diff --git a/netbox/ipam/models/vlans.py b/netbox/ipam/models/vlans.py
index 91e39c6d3b6..8ebefe0838e 100644
--- a/netbox/ipam/models/vlans.py
+++ b/netbox/ipam/models/vlans.py
@@ -1,4 +1,5 @@
 from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
+from django.contrib.contenttypes.models import ContentType
 from django.contrib.postgres.fields import ArrayField, IntegerRangeField
 from django.core.exceptions import ValidationError
 from django.core.validators import MaxValueValidator, MinValueValidator
@@ -6,7 +7,7 @@
 from django.db.backends.postgresql.psycopg_any import NumericRange
 from django.utils.translation import gettext_lazy as _
 
-from dcim.models import Interface
+from dcim.models import Interface, Site, SiteGroup
 from ipam.choices import *
 from ipam.constants import *
 from ipam.querysets import VLANQuerySet, VLANGroupQuerySet
@@ -279,12 +280,20 @@ def clean(self):
         super().clean()
 
         # Validate VLAN group (if assigned)
-        if self.group and self.site and self.group.scope != self.site:
-            raise ValidationError(
-                _(
-                    "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}."
-                ).format(group=self.group, scope=self.group.scope, site=self.site)
-            )
+        if self.group and self.site and self.group.scope_type == ContentType.objects.get_for_model(Site):
+            if self.site != self.group.scope:
+                raise ValidationError(
+                    _(
+                        "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}."
+                    ).format(group=self.group, scope=self.group.scope, site=self.site)
+                )
+        if self.group and self.site and self.group.scope_type == ContentType.objects.get_for_model(SiteGroup):
+            if self.site not in self.group.scope.sites.all():
+                raise ValidationError(
+                    _(
+                        "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}."
+                    ).format(group=self.group, scope=self.group.scope, site=self.site)
+                )
 
         # Check that the VLAN ID is permitted in the assigned group (if any)
         if self.group:
diff --git a/netbox/ipam/tests/test_models.py b/netbox/ipam/tests/test_models.py
index 62eb741238b..7996636ad96 100644
--- a/netbox/ipam/tests/test_models.py
+++ b/netbox/ipam/tests/test_models.py
@@ -1,8 +1,10 @@
+from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import ValidationError
 from django.test import TestCase, override_settings
 from netaddr import IPNetwork, IPSet
 from utilities.data import string_to_ranges
 
+from dcim.models import Site, SiteGroup
 from ipam.choices import *
 from ipam.models import *
 
@@ -645,3 +647,54 @@ def test_qinq_role(self):
         )
         with self.assertRaises(ValidationError):
             vlan.full_clean()
+
+    def test_vlan_group_site_validation(self):
+        sitegroup = SiteGroup.objects.create(
+            name='Site Group 1',
+            slug='site-group-1',
+        )
+        sites = Site.objects.bulk_create((
+            Site(
+                name='Site 1',
+                slug='site-1',
+            ),
+            Site(
+                name='Site 2',
+                slug='site-2',
+            ),
+        ))
+        sitegroup.sites.add(sites[0])
+        vlangroups = VLANGroup.objects.bulk_create((
+            VLANGroup(
+                name='VLAN Group 1',
+                slug='vlan-group-1',
+                scope=sitegroup,
+                scope_type=ContentType.objects.get_for_model(SiteGroup),
+            ),
+            VLANGroup(
+                name='VLAN Group 2',
+                slug='vlan-group-2',
+                scope=sites[0],
+                scope_type=ContentType.objects.get_for_model(Site),
+            ),
+            VLANGroup(
+                name='VLAN Group 2',
+                slug='vlan-group-2',
+                scope=sites[1],
+                scope_type=ContentType.objects.get_for_model(Site),
+            ),
+        ))
+        vlan = VLAN(
+            name='VLAN 1',
+            vid=1,
+            group=vlangroups[0],
+            site=sites[0],
+        )
+
+        # VLAN Group 0 and 1 should be valid
+        vlan.full_clean()
+        vlan.group = vlangroups[1]
+        vlan.full_clean()
+        vlan.group = vlangroups[2]
+        with self.assertRaises(ValidationError):
+            vlan.full_clean()


Code review comments:
diff_hunk: @@ -279,12 +280,20 @@ def clean(self):
         super().clean()
 
         # Validate VLAN group (if assigned)
-        if self.group and self.site and self.group.scope != self.site:
-            raise ValidationError(
-                _(
-                    "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}."
-                ).format(group=self.group, scope=self.group.scope, site=self.site)
-            )
+        if self.group and self.site and self.group.scope_type == ContentType.objects.get_for_model(Site):
+            if self.site != self.group.scope:
+                raise ValidationError(
+                    _(
+                        "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}."
+                    ).format(group=self.group, scope=self.group.scope, site=self.site)
+                )
+        if self.group and self.site and self.group.scope_type == ContentType.objects.get_for_model(SiteGroup):
+            if self.site not in self.group.scope.sites.all():
+                raise ValidationError(
+                    _(
+                        "VLAN is assigned to group {group} (scope: {scope}); cannot also assign to site {site}."
path: netbox/ipam/models/vlans.py
text: @user1:
This error message should specifically indicate the issue is that the assigned site is not a member of the assigned group.

diff_hunk: @@ -645,3 +647,54 @@ def test_qinq_role(self):
         )
         with self.assertRaises(ValidationError):
             vlan.full_clean()
+
+    def test_vlan_group_site_validation(self):
+        sitegroup = SiteGroup.objects.create(
+            name='Site Group 1',
+            slug='site-group-1',
+        )
+        sites = Site.objects.bulk_create((
+            Site(
+                name='Site 1',
+                slug='site-1',
+            ),
+            Site(
+                name='Site 2',
+                slug='site-2',
+            ),
+        ))
+        sitegroup.sites.add(sites[0])
+        vlangroups = VLANGroup.objects.bulk_create((
+            VLANGroup(
+                name='VLAN Group 1',
+                slug='vlan-group-1',
+                scope=sitegroup,
+                scope_type=ContentType.objects.get_for_model(SiteGroup),
+            ),
+            VLANGroup(
+                name='VLAN Group 2',
+                slug='vlan-group-2',
+                scope=sites[0],
+                scope_type=ContentType.objects.get_for_model(Site),
+            ),
+            VLANGroup(
+                name='VLAN Group 2',
+                slug='vlan-group-2',
+                scope=sites[1],
+                scope_type=ContentType.objects.get_for_model(Site),
+            ),
+        ))
+        vlan = VLAN(
+            name='VLAN 1',
+            vid=1,
+            group=vlangroups[0],
+            site=sites[0],
+        )
+
+        # VLAN Group 0 and 1 should be valid
path: netbox/ipam/tests/test_models.py
text: @user1:
```suggestion
        # VLAN Group 1 and 2 should be valid
```