Skip to content

Commit 63a167f

Browse files
jeremystretchDanSheps
jeremystretch
and
DanSheps
authored
Fixes #15924: Fix API interface patch tagged all mode (#18759)
* Fixes: #15924 - Prevent API payload from allowing tagged_vlans while interface mode is set to taged-all * Prevent cleanup of tagged_vlans when no tagged_vlans set on interface * Fix test errors * Remove accidental debug statements * Update validation to model clean method instead of serializer * Remove clearing of tagged vlans from `save()` * Make changes to validation to account for M2M not being available under model in addition to not being able to check incoming vlans under same model. * Optimize untagged vlan check * Re-ordering statements in validators * Forgot to call super().clean() * Adjust logic for form and serializer. Add tests * Fix test failure * Fix ruff errors * Fix test by removing now invalid test * Update serializer, form and tests * Optimize API test for vlan fields * Optimize API serializer logic --------- Co-authored-by: Daniel Sheppard <[email protected]>
1 parent 09d867a commit 63a167f

File tree

5 files changed

+292
-15
lines changed

5 files changed

+292
-15
lines changed

‎netbox/dcim/api/serializers_/device_components.py‎

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
from django.utils.translation import gettext as _
12
from django.contrib.contenttypes.models import ContentType
23
from drf_spectacular.utils import extend_schema_field
34
from rest_framework import serializers
@@ -232,8 +233,56 @@ class Meta:
232233

233234
def validate(self, data):
234235

235-
# Validate many-to-many VLAN assignments
236236
if not self.nested:
237+
238+
# Validate 802.1q mode and vlan(s)
239+
mode = None
240+
tagged_vlans = []
241+
242+
# Gather Information
243+
if self.instance:
244+
mode = data.get('mode') if 'mode' in data.keys() else self.instance.mode
245+
untagged_vlan = data.get('untagged_vlan') if 'untagged_vlan' in data.keys() else \
246+
self.instance.untagged_vlan
247+
qinq_svlan = data.get('qinq_svlan') if 'qinq_svlan' in data.keys() else \
248+
self.instance.qinq_svlan
249+
tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else \
250+
self.instance.tagged_vlans.all()
251+
else:
252+
mode = data.get('mode', None)
253+
untagged_vlan = data.get('untagged_vlan') if 'untagged_vlan' in data.keys() else None
254+
qinq_svlan = data.get('qinq_svlan') if 'qinq_svlan' in data.keys() else None
255+
tagged_vlans = data.get('tagged_vlans') if 'tagged_vlans' in data.keys() else None
256+
257+
errors = {}
258+
259+
# Non Q-in-Q mode with service vlan set
260+
if mode != InterfaceModeChoices.MODE_Q_IN_Q and qinq_svlan:
261+
errors.update({
262+
'qinq_svlan': _("Interface mode does not support q-in-q service vlan")
263+
})
264+
# Routed mode
265+
if not mode:
266+
# Untagged vlan
267+
if untagged_vlan:
268+
errors.update({
269+
'untagged_vlan': _("Interface mode does not support untagged vlan")
270+
})
271+
# Tagged vlan
272+
if tagged_vlans:
273+
errors.update({
274+
'tagged_vlans': _("Interface mode does not support tagged vlans")
275+
})
276+
# Non-tagged mode
277+
elif mode in (InterfaceModeChoices.MODE_TAGGED_ALL, InterfaceModeChoices.MODE_ACCESS) and tagged_vlans:
278+
errors.update({
279+
'tagged_vlans': _("Interface mode does not support tagged vlans")
280+
})
281+
282+
if errors:
283+
raise serializers.ValidationError(errors)
284+
285+
# Validate many-to-many VLAN assignments
237286
device = self.instance.device if self.instance else data.get('device')
238287
for vlan in data.get('tagged_vlans', []):
239288
if vlan.site not in [device.site, None]:

‎netbox/dcim/forms/common.py‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,20 +43,14 @@ def clean(self):
4343
super().clean()
4444

4545
parent_field = 'device' if 'device' in self.cleaned_data else 'virtual_machine'
46-
tagged_vlans = self.cleaned_data.get('tagged_vlans')
47-
48-
# Untagged interfaces cannot be assigned tagged VLANs
49-
if self.cleaned_data['mode'] == InterfaceModeChoices.MODE_ACCESS and tagged_vlans:
50-
raise forms.ValidationError({
51-
'mode': _("An access interface cannot have tagged VLANs assigned.")
52-
})
53-
54-
# Remove all tagged VLAN assignments from "tagged all" interfaces
55-
elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED_ALL:
56-
self.cleaned_data['tagged_vlans'] = []
46+
if 'tagged_vlans' in self.fields.keys():
47+
tagged_vlans = self.cleaned_data.get('tagged_vlans') if self.is_bound else \
48+
self.get_initial_for_field(self.fields['tagged_vlans'], 'tagged_vlans')
49+
else:
50+
tagged_vlans = []
5751

5852
# Validate tagged VLANs; must be a global VLAN or in the same site
59-
elif self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED and tagged_vlans:
53+
if self.cleaned_data['mode'] == InterfaceModeChoices.MODE_TAGGED and tagged_vlans:
6054
valid_sites = [None, self.cleaned_data[parent_field].site]
6155
invalid_vlans = [str(v) for v in tagged_vlans if v.site not in valid_sites]
6256

‎netbox/dcim/models/device_components.py‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -934,6 +934,8 @@ def clean(self):
934934
raise ValidationError({'rf_channel_width': _("Cannot specify custom width with channel selected.")})
935935

936936
# VLAN validation
937+
if not self.mode and self.untagged_vlan:
938+
raise ValidationError({'untagged_vlan': _("Interface mode does not support an untagged vlan.")})
937939

938940
# Validate untagged VLAN
939941
if self.untagged_vlan and self.untagged_vlan.site not in [self.device.site, None]:

‎netbox/dcim/tests/test_api.py‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import json
2+
13
from django.test import override_settings
24
from django.urls import reverse
35
from django.utils.translation import gettext as _
@@ -1748,6 +1750,23 @@ def setUpTestData(cls):
17481750
},
17491751
]
17501752

1753+
def _perform_interface_test_with_invalid_data(self, mode: str = None, invalid_data: dict = {}):
1754+
device = Device.objects.first()
1755+
data = {
1756+
'device': device.pk,
1757+
'name': 'Interface 1',
1758+
'type': InterfaceTypeChoices.TYPE_1GE_FIXED,
1759+
}
1760+
data.update({'mode': mode})
1761+
data.update(invalid_data)
1762+
1763+
response = self.client.post(self._get_list_url(), data, format='json', **self.header)
1764+
self.assertHttpStatus(response, status.HTTP_400_BAD_REQUEST)
1765+
content = json.loads(response.content)
1766+
for key in invalid_data.keys():
1767+
self.assertIn(key, content)
1768+
self.assertIsNone(content.get('data'))
1769+
17511770
def test_bulk_delete_child_interfaces(self):
17521771
interface1 = Interface.objects.get(name='Interface 1')
17531772
device = interface1.device
@@ -1775,6 +1794,57 @@ def test_bulk_delete_child_interfaces(self):
17751794
self.client.delete(self._get_list_url(), data, format='json', **self.header)
17761795
self.assertEqual(device.interfaces.count(), 2) # Child & parent were both deleted
17771796

1797+
def test_create_child_interfaces_mode_invalid_data(self):
1798+
"""
1799+
POST data to test interface mode check and invalid tagged/untagged VLANS.
1800+
"""
1801+
self.add_permissions('dcim.add_interface')
1802+
1803+
vlans = VLAN.objects.all()[0:3]
1804+
1805+
# Routed mode, untagged, tagged and qinq service vlan
1806+
invalid_data = {
1807+
'untagged_vlan': vlans[0].pk,
1808+
'tagged_vlans': [vlans[1].pk, vlans[2].pk],
1809+
'qinq_svlan': vlans[2].pk
1810+
}
1811+
self._perform_interface_test_with_invalid_data(None, invalid_data)
1812+
1813+
# Routed mode, untagged and tagged vlan
1814+
invalid_data = {
1815+
'untagged_vlan': vlans[0].pk,
1816+
'tagged_vlans': [vlans[1].pk, vlans[2].pk],
1817+
}
1818+
self._perform_interface_test_with_invalid_data(None, invalid_data)
1819+
1820+
# Routed mode, untagged vlan
1821+
invalid_data = {
1822+
'untagged_vlan': vlans[0].pk,
1823+
}
1824+
self._perform_interface_test_with_invalid_data(None, invalid_data)
1825+
1826+
invalid_data = {
1827+
'tagged_vlans': [vlans[1].pk, vlans[2].pk],
1828+
}
1829+
# Routed mode, qinq service vlan
1830+
self._perform_interface_test_with_invalid_data(None, invalid_data)
1831+
# Access mode, tagged vlans
1832+
self._perform_interface_test_with_invalid_data(InterfaceModeChoices.MODE_ACCESS, invalid_data)
1833+
# All tagged mode, tagged vlans
1834+
self._perform_interface_test_with_invalid_data(InterfaceModeChoices.MODE_TAGGED_ALL, invalid_data)
1835+
1836+
invalid_data = {
1837+
'qinq_svlan': vlans[0].pk,
1838+
}
1839+
# Routed mode, qinq service vlan
1840+
self._perform_interface_test_with_invalid_data(None, invalid_data)
1841+
# Access mode, qinq service vlan
1842+
self._perform_interface_test_with_invalid_data(InterfaceModeChoices.MODE_ACCESS, invalid_data)
1843+
# Tagged mode, qinq service vlan
1844+
self._perform_interface_test_with_invalid_data(InterfaceModeChoices.MODE_TAGGED, invalid_data)
1845+
# Tagged-all mode, qinq service vlan
1846+
self._perform_interface_test_with_invalid_data(InterfaceModeChoices.MODE_TAGGED_ALL, invalid_data)
1847+
17781848

17791849
class FrontPortTest(APIViewTestCases.APIViewTestCase):
17801850
model = FrontPort

‎netbox/dcim/tests/test_forms.py‎

Lines changed: 164 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
from django.test import TestCase
22

3-
from dcim.choices import DeviceFaceChoices, DeviceStatusChoices, InterfaceTypeChoices
3+
from dcim.choices import DeviceFaceChoices, DeviceStatusChoices, InterfaceTypeChoices, InterfaceModeChoices
44
from dcim.forms import *
55
from dcim.models import *
6+
from ipam.models import VLAN
67
from utilities.testing import create_test_device
78
from virtualization.models import Cluster, ClusterGroup, ClusterType
89

@@ -117,11 +118,23 @@ def test_non_racked_device_with_position(self):
117118
self.assertIn('position', form.errors)
118119

119120

120-
class LabelTestCase(TestCase):
121+
class InterfaceTestCase(TestCase):
121122

122123
@classmethod
123124
def setUpTestData(cls):
124125
cls.device = create_test_device('Device 1')
126+
cls.vlans = (
127+
VLAN(name='VLAN 1', vid=1),
128+
VLAN(name='VLAN 2', vid=2),
129+
VLAN(name='VLAN 3', vid=3),
130+
)
131+
VLAN.objects.bulk_create(cls.vlans)
132+
cls.interface = Interface.objects.create(
133+
device=cls.device,
134+
name='Interface 1',
135+
type=InterfaceTypeChoices.TYPE_1GE_GBIC,
136+
mode=InterfaceModeChoices.MODE_TAGGED,
137+
)
125138

126139
def test_interface_label_count_valid(self):
127140
"""
@@ -151,3 +164,152 @@ def test_interface_label_count_mismatch(self):
151164

152165
self.assertFalse(form.is_valid())
153166
self.assertIn('label', form.errors)
167+
168+
def test_create_interface_mode_valid_data(self):
169+
"""
170+
Test that saving valid interface mode and tagged/untagged vlans works properly
171+
"""
172+
173+
# Validate access mode
174+
data = {
175+
'device': self.device.pk,
176+
'name': 'ethernet1/1',
177+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
178+
'mode': InterfaceModeChoices.MODE_ACCESS,
179+
'untagged_vlan': self.vlans[0].pk
180+
}
181+
form = InterfaceCreateForm(data)
182+
183+
self.assertTrue(form.is_valid())
184+
185+
# Validate tagged vlans
186+
data = {
187+
'device': self.device.pk,
188+
'name': 'ethernet1/2',
189+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
190+
'mode': InterfaceModeChoices.MODE_TAGGED,
191+
'untagged_vlan': self.vlans[0].pk,
192+
'tagged_vlans': [self.vlans[1].pk, self.vlans[2].pk]
193+
}
194+
form = InterfaceCreateForm(data)
195+
self.assertTrue(form.is_valid())
196+
197+
# Validate tagged vlans
198+
data = {
199+
'device': self.device.pk,
200+
'name': 'ethernet1/3',
201+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
202+
'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
203+
'untagged_vlan': self.vlans[0].pk,
204+
}
205+
form = InterfaceCreateForm(data)
206+
self.assertTrue(form.is_valid())
207+
208+
def test_create_interface_mode_access_invalid_data(self):
209+
"""
210+
Test that saving invalid interface mode and tagged/untagged vlans works properly
211+
"""
212+
data = {
213+
'device': self.device.pk,
214+
'name': 'ethernet1/4',
215+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
216+
'mode': InterfaceModeChoices.MODE_ACCESS,
217+
'untagged_vlan': self.vlans[0].pk,
218+
'tagged_vlans': [self.vlans[1].pk, self.vlans[2].pk]
219+
}
220+
form = InterfaceCreateForm(data)
221+
222+
self.assertTrue(form.is_valid())
223+
self.assertIn('untagged_vlan', form.cleaned_data.keys())
224+
self.assertNotIn('tagged_vlans', form.cleaned_data.keys())
225+
self.assertNotIn('qinq_svlan', form.cleaned_data.keys())
226+
227+
def test_edit_interface_mode_access_invalid_data(self):
228+
"""
229+
Test that saving invalid interface mode and tagged/untagged vlans works properly
230+
"""
231+
data = {
232+
'device': self.device.pk,
233+
'name': 'Ethernet 1/5',
234+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
235+
'mode': InterfaceModeChoices.MODE_ACCESS,
236+
'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk]
237+
}
238+
form = InterfaceForm(data, instance=self.interface)
239+
240+
self.assertTrue(form.is_valid())
241+
self.assertIn('untagged_vlan', form.cleaned_data.keys())
242+
self.assertNotIn('tagged_vlans', form.cleaned_data.keys())
243+
self.assertNotIn('qinq_svlan', form.cleaned_data.keys())
244+
245+
def test_create_interface_mode_tagged_all_invalid_data(self):
246+
"""
247+
Test that saving invalid interface mode and tagged/untagged vlans works properly
248+
"""
249+
data = {
250+
'device': self.device.pk,
251+
'name': 'ethernet1/6',
252+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
253+
'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
254+
'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk]
255+
}
256+
form = InterfaceCreateForm(data)
257+
258+
self.assertTrue(form.is_valid())
259+
self.assertIn('untagged_vlan', form.cleaned_data.keys())
260+
self.assertNotIn('tagged_vlans', form.cleaned_data.keys())
261+
self.assertNotIn('qinq_svlan', form.cleaned_data.keys())
262+
263+
def test_edit_interface_mode_tagged_all_invalid_data(self):
264+
"""
265+
Test that saving invalid interface mode and tagged/untagged vlans works properly
266+
"""
267+
data = {
268+
'device': self.device.pk,
269+
'name': 'Ethernet 1/7',
270+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
271+
'mode': InterfaceModeChoices.MODE_TAGGED_ALL,
272+
'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk]
273+
}
274+
form = InterfaceForm(data)
275+
self.assertTrue(form.is_valid())
276+
self.assertIn('untagged_vlan', form.cleaned_data.keys())
277+
self.assertNotIn('tagged_vlans', form.cleaned_data.keys())
278+
self.assertNotIn('qinq_svlan', form.cleaned_data.keys())
279+
280+
def test_create_interface_mode_routed_invalid_data(self):
281+
"""
282+
Test that saving invalid interface mode (routed) and tagged/untagged vlans works properly
283+
"""
284+
data = {
285+
'device': self.device.pk,
286+
'name': 'ethernet1/6',
287+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
288+
'mode': None,
289+
'untagged_vlan': self.vlans[0].pk,
290+
'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk]
291+
}
292+
form = InterfaceCreateForm(data)
293+
294+
self.assertTrue(form.is_valid())
295+
self.assertNotIn('untagged_vlan', form.cleaned_data.keys())
296+
self.assertNotIn('tagged_vlans', form.cleaned_data.keys())
297+
self.assertNotIn('qinq_svlan', form.cleaned_data.keys())
298+
299+
def test_edit_interface_mode_routed_invalid_data(self):
300+
"""
301+
Test that saving invalid interface mode (routed) and tagged/untagged vlans works properly
302+
"""
303+
data = {
304+
'device': self.device.pk,
305+
'name': 'Ethernet 1/7',
306+
'type': InterfaceTypeChoices.TYPE_1GE_GBIC,
307+
'mode': None,
308+
'untagged_vlan': self.vlans[0].pk,
309+
'tagged_vlans': [self.vlans[0].pk, self.vlans[1].pk, self.vlans[2].pk]
310+
}
311+
form = InterfaceForm(data)
312+
self.assertTrue(form.is_valid())
313+
self.assertNotIn('untagged_vlan', form.cleaned_data.keys())
314+
self.assertNotIn('tagged_vlans', form.cleaned_data.keys())
315+
self.assertNotIn('qinq_svlan', form.cleaned_data.keys())

0 commit comments

Comments
 (0)