From 17898625c57a5e42ad9e8bef32d189b548141dbc Mon Sep 17 00:00:00 2001 From: Daniel Sheppard Date: Mon, 23 Sep 2024 17:17:41 -0500 Subject: [PATCH] Adjust logic for form and serializer. Add tests --- .../api/serializers_/device_components.py | 21 +-- netbox/dcim/forms/common.py | 9 +- netbox/dcim/forms/model_forms.py | 31 ---- netbox/dcim/models/device_components.py | 2 +- netbox/dcim/tests/test_api.py | 30 +++- netbox/dcim/tests/test_forms.py | 143 +++++++++++++++++- 6 files changed, 183 insertions(+), 53 deletions(-) diff --git a/netbox/dcim/api/serializers_/device_components.py b/netbox/dcim/api/serializers_/device_components.py index 867781995..d2f1954bd 100644 --- a/netbox/dcim/api/serializers_/device_components.py +++ b/netbox/dcim/api/serializers_/device_components.py @@ -239,25 +239,16 @@ class InterfaceSerializer(NetBoxModelSerializer, CabledObjectSerializer, Connect tagged_vlans = [] if self.instance: - if 'mode' in data.keys(): - mode = data.get('mode') if 'mode' in data.keys() else self.instance.get('mode') - if 'tagged_vlans' in data.keys(): - tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else \ - self.instance.tagged_vlans.all() - if 'untagged_vlan' in data.keys(): - untagged_vlan = data.get('untagged_vlan') if 'untagged_vlan' in data.keys() else \ - self.instance.untagged_vlan + mode = data.get('mode') if 'mode' in data.keys() else self.instance.mode + tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else \ + self.instance.tagged_vlans.all() else: - if 'mode' in data.keys(): - mode = data.get('mode') - if 'tagged_vlans' in data.keys(): - tagged_vlans = data.get('tagged_vlans') - if 'untagged_vlan' in data.keys(): - untagged_vlan = data.get('untagged_vlan') + mode = data.get('mode', None) + tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else None if mode != InterfaceModeChoices.MODE_TAGGED and tagged_vlans: raise serializers.ValidationError({ - 'tagged_vlans': _("Interface mode does not support including tagged vlans") + 'tagged_vlans': _("Interface mode does not support tagged vlans") }) # Validate many-to-many VLAN assignments diff --git a/netbox/dcim/forms/common.py b/netbox/dcim/forms/common.py index 4341ec041..f943fc49b 100644 --- a/netbox/dcim/forms/common.py +++ b/netbox/dcim/forms/common.py @@ -42,7 +42,8 @@ class InterfaceCommonForm(forms.Form): super().clean() parent_field = 'device' if 'device' in self.cleaned_data else 'virtual_machine' - tagged_vlans = self.cleaned_data.get('tagged_vlans') + interface_mode = get_field_value(self, parent_field) + tagged_vlans = get_field_value(self, 'tagged_vlans') if 'tagged_vlans' in self.fields.keys() else [] # Untagged interfaces cannot be assigned tagged VLANs if self.cleaned_data['mode'] == InterfaceModeChoices.MODE_ACCESS and tagged_vlans: @@ -51,8 +52,10 @@ class InterfaceCommonForm(forms.Form): }) # Remove all tagged VLAN assignments from "tagged all" interfaces - elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED_ALL: - self.cleaned_data['tagged_vlans'] = [] + elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED_ALL and tagged_vlans: + raise forms.ValidationError({ + 'mode': _("An tagged-all interface cannot have tagged VLANs assigned.") + }) # Validate tagged VLANs; must be a global VLAN or in the same site elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED and tagged_vlans: diff --git a/netbox/dcim/forms/model_forms.py b/netbox/dcim/forms/model_forms.py index 7ef655278..7dc6d5239 100644 --- a/netbox/dcim/forms/model_forms.py +++ b/netbox/dcim/forms/model_forms.py @@ -1415,37 +1415,6 @@ class InterfaceForm(InterfaceCommonForm, ModularDeviceComponentForm): 'mode': '802.1Q Mode', } - def clean(self): - super().clean() - - # Validate VLAN config - mode = None - tagged_vlans = [] - untagged_vlan = None - - if self.instance: - if 'mode' in self.cleaned_data.keys(): - mode = self.cleaned_data.get('mode') if 'mode' in self.cleaned_data.keys() else\ - self.instance.get('mode') - if 'tagged_vlans' in self.cleaned_data.keys(): - tagged_vlans = self.cleaned_data.get('tagged_vlans') if 'tagged_vlans' in self.cleaned_data.keys() else\ - self.instance.tagged_vlans.all() - if 'untagged_vlan' in self.cleaned_data.keys(): - untagged_vlan = self.cleaned_data.get('untagged_vlan') if 'untagged_vlan' in self.cleaned_data.keys()\ - else self.instance.untagged_vlan - else: - if 'mode' in self.cleaned_data.keys(): - mode = self.cleaned_data.get('mode') - if 'tagged_vlans' in self.cleaned_data.keys(): - tagged_vlans = self.cleaned_data.get('tagged_vlans') - if 'untagged_vlan' in self.cleaned_data.keys(): - untagged_vlan = self.cleaned_data.get('untagged_vlan') - - if mode != InterfaceModeChoices.MODE_TAGGED and tagged_vlans: - raise forms.ValidationError({ - 'tagged_vlans': _("Interface mode does not support including tagged vlans") - }) - class FrontPortForm(ModularDeviceComponentForm): rear_port = DynamicModelChoiceField( diff --git a/netbox/dcim/models/device_components.py b/netbox/dcim/models/device_components.py index 445a36be5..d3b8ef6bf 100644 --- a/netbox/dcim/models/device_components.py +++ b/netbox/dcim/models/device_components.py @@ -895,7 +895,7 @@ class Interface(ModularComponentModel, BaseInterface, CabledObjectModel, PathEnd # VLAN validation if not self.mode and self.untagged_vlan: - raise ValidationError({'untagged_vlan': _("Interface mode does not support including an untagged vlan.")}) + raise ValidationError({'untagged_vlan': _("Interface mode does not support an untagged vlan.")}) # Validate untagged VLAN if self.untagged_vlan and self.untagged_vlan.site not in [self.device.site, None]: diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 573fdbb96..6397d3502 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -1,3 +1,5 @@ +import json + from django.test import override_settings from django.urls import reverse from django.utils.translation import gettext as _ @@ -11,7 +13,7 @@ from ipam.models import ASN, RIR, VLAN, VRF from netbox.api.serializers import GenericObjectSerializer from tenancy.models import Tenant from users.models import User -from utilities.testing import APITestCase, APIViewTestCases, create_test_device +from utilities.testing import APITestCase, APIViewTestCases, create_test_device, disable_warnings from virtualization.models import Cluster, ClusterType from wireless.choices import WirelessChannelChoices from wireless.models import WirelessLAN @@ -1718,6 +1720,32 @@ class InterfaceTest(Mixins.ComponentTraceMixin, APIViewTestCases.APIViewTestCase self.client.delete(self._get_list_url(), data, format='json', **self.header) self.assertEqual(device.interfaces.count(), 2) # Child & parent were both deleted + def test_create_child_interfaces_mode_invalid_data(self): + """ + POST a single object without permission. + """ + self.add_permissions('dcim.add_interface') + url = self._get_list_url() + initial_count = self._get_queryset().count() + + device = Device.objects.first() + vlans = VLAN.objects.all()[0:3] + + create_data = { + 'device': device.pk, + 'name': 'Untagged Interface 1', + 'type': InterfaceTypeChoices.TYPE_1GE_FIXED, + 'mode': InterfaceModeChoices.MODE_ACCESS, + 'tagged_vlans': [vlans[0].pk, vlans[1].pk], + 'untagged_vlan': vlans[2].pk, + } + + response = self.client.post(self._get_list_url(), create_data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST) + content = json.loads(response.content) + self.assertIn('tagged_vlans', content) + self.assertIsNone(content.get('data')) + class FrontPortTest(APIViewTestCases.APIViewTestCase): model = FrontPort diff --git a/netbox/dcim/tests/test_forms.py b/netbox/dcim/tests/test_forms.py index 7a57bf3f0..57056bcb9 100644 --- a/netbox/dcim/tests/test_forms.py +++ b/netbox/dcim/tests/test_forms.py @@ -1,8 +1,9 @@ from django.test import TestCase -from dcim.choices import DeviceFaceChoices, DeviceStatusChoices, InterfaceTypeChoices +from dcim.choices import DeviceFaceChoices, DeviceStatusChoices, InterfaceTypeChoices, InterfaceModeChoices from dcim.forms import * from dcim.models import * +from ipam.models import VLAN from utilities.testing import create_test_device from virtualization.models import Cluster, ClusterGroup, ClusterType @@ -117,11 +118,23 @@ class DeviceTestCase(TestCase): self.assertIn('position', form.errors) -class LabelTestCase(TestCase): +class InterfaceTestCase(TestCase): @classmethod def setUpTestData(cls): cls.device = create_test_device('Device 1') + cls.vlans = ( + VLAN(name='VLAN 1', vid=1), + VLAN(name='VLAN 2', vid=2), + VLAN(name='VLAN 3', vid=3), + ) + VLAN.objects.bulk_create(cls.vlans) + cls.interface = Interface.objects.create( + device=cls.device, + name='Interface 1', + type=InterfaceTypeChoices.TYPE_1GE_GBIC, + mode=InterfaceModeChoices.MODE_TAGGED, + ) def test_interface_label_count_valid(self): """ @@ -151,3 +164,129 @@ class LabelTestCase(TestCase): self.assertFalse(form.is_valid()) self.assertIn('label', form.errors) + + def test_create_interface_mode_valid_data(self): + """ + Test that saving valid interface mode and tagged/untagged vlans works properly + """ + + # Validate access mode + data = { + 'device': self.device.pk, + 'name': 'ethernet1/1', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_ACCESS, + 'untagged_vlan': self.vlans[0].pk + } + form = InterfaceCreateForm(data) + + self.assertTrue(form.is_valid()) + + # Validate tagged vlans + data = { + 'device': self.device.pk, + 'name': 'ethernet1/2', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_TAGGED, + 'untagged_vlan': self.vlans[0].pk, + 'tagged_vlans': [self.vlans[1].pk, self.vlans[2].pk] + } + form = InterfaceCreateForm(data) + self.assertTrue(form.is_valid()) + + # Validate tagged vlans + data = { + 'device': self.device.pk, + 'name': 'ethernet1/3', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_TAGGED_ALL, + 'untagged_vlan': self.vlans[0].pk, + } + form = InterfaceCreateForm(data) + self.assertTrue(form.is_valid()) + + def test_create_interface_mode_access_invalid_data(self): + """ + Test that saving invalid interface mode and tagged/untagged vlans works properly + """ + data = { + 'device': self.device.pk, + 'name': 'ethernet1/4', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_ACCESS, + 'untagged_vlan': self.vlans[0].pk, + 'tagged_vlans': [self.vlans[1].pk, self.vlans[2].pk] + } + form = InterfaceCreateForm(data) + + self.assertTrue(form.is_valid()) + + def test_edit_interface_mode_access_invalid_data(self): + """ + Test that saving invalid interface mode and tagged/untagged vlans works properly + """ + data = { + 'device': self.device.pk, + 'name': 'Ethernet 1/5', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_ACCESS, + 'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk] + } + form = InterfaceForm(data, instance=self.interface) + + self.assertTrue(form.is_valid()) + + def test_create_interface_mode_tagged_all_invalid_data(self): + """ + Test that saving invalid interface mode and tagged/untagged vlans works properly + """ + data = { + 'device': self.device.pk, + 'name': 'ethernet1/6', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_TAGGED_ALL, + 'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2]] + } + form = InterfaceCreateForm(data) + + self.assertTrue(form.is_valid()) + + def test_edit_interface_mode_tagged_all_invalid_data(self): + """ + Test that saving invalid interface mode and tagged/untagged vlans works properly + """ + data = { + 'device': self.device.pk, + 'name': 'Ethernet 1/7', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_TAGGED_ALL, + 'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2]] + } + form = InterfaceForm(data, instance=self.interface) + form.is_valid() + self.assertTrue(form.is_valid()) + + def test_edit_interface_mode_tagged_all_existing_invalid_data(self): + """ + Test that saving invalid interface mode and tagged/untagged vlans works properly + """ + self.interface.tagged_vlans.add(self.vlans[0]) + self.interface.tagged_vlans.add(self.vlans[1]) + data = { + 'device': self.device.pk, + 'name': 'Ethernet 1/8', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_TAGGED_ALL, + } + form = InterfaceForm(data, instance=self.interface) + self.assertFalse(form.is_valid()) + + data = { + 'device': self.device.pk, + 'name': 'Ethernet 1/9', + 'type': InterfaceTypeChoices.TYPE_1GE_GBIC, + 'mode': InterfaceModeChoices.MODE_ACCESS, + } + form = InterfaceForm(data, instance=self.interface) + self.assertFalse(form.is_valid()) + self.interface.tagged_vlans.clear()