From e87e11377aba35155ca36197982723b910ca47b0 Mon Sep 17 00:00:00 2001 From: "julio.oliveira" Date: Mon, 15 Jan 2024 16:28:12 -0300 Subject: [PATCH 1/5] Fixes #14755: ValueError in web UI after REST API accepts invalid custom-field choice-set data --- netbox/extras/api/serializers.py | 12 +++- netbox/extras/tests/test_api.py | 10 ---- .../test_custom_field_choice_sets_endpoint.py | 59 +++++++++++++++++++ 3 files changed, 70 insertions(+), 11 deletions(-) create mode 100644 netbox/extras/tests/test_custom_field_choice_sets_endpoint.py diff --git a/netbox/extras/api/serializers.py b/netbox/extras/api/serializers.py index 60a30aed210..50cd022b570 100644 --- a/netbox/extras/api/serializers.py +++ b/netbox/extras/api/serializers.py @@ -179,6 +179,15 @@ class Meta: 'choices_count', 'created', 'last_updated', ] + def validate_extra_choices(self, value): + for choice in value: + if isinstance(choice, list): + if len(choice) < 2: + raise serializers.ValidationError('Each choice must have 2 elements.') + else: + raise serializers.ValidationError('Extra choice must be a list of two elements.') + return value + # # Custom links @@ -374,7 +383,8 @@ def validate(self, data): @extend_schema_field(serializers.JSONField(allow_null=True)) def get_assigned_object(self, instance): - serializer = get_serializer_for_model(instance.assigned_object_type.model_class(), prefix=NESTED_SERIALIZER_PREFIX) + serializer = get_serializer_for_model(instance.assigned_object_type.model_class(), + prefix=NESTED_SERIALIZER_PREFIX) context = {'request': self.context['request']} return serializer(instance.assigned_object, context=context).data diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index 93be2d2c4ba..fd9f5bafa93 100644 --- a/netbox/extras/tests/test_api.py +++ b/netbox/extras/tests/test_api.py @@ -14,14 +14,12 @@ from extras.scripts import BooleanVar, IntegerVar, Script, StringVar from utilities.testing import APITestCase, APIViewTestCases - User = get_user_model() class AppTest(APITestCase): def test_root(self): - url = reverse('extras-api:api-root') response = self.client.get('{}?format=api'.format(url), **self.header) @@ -52,7 +50,6 @@ class WebhookTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): - webhooks = ( Webhook( name='Webhook 1', @@ -505,7 +502,6 @@ class TagTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): - tags = ( Tag(name='Tag 1', slug='tag-1'), Tag(name='Tag 2', slug='tag-2'), @@ -632,7 +628,6 @@ class ConfigContextTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): - config_contexts = ( ConfigContext(name='Config Context 1', weight=100, data={'foo': 123}), ConfigContext(name='Config Context 2', weight=200, data={'bar': 456}), @@ -731,7 +726,6 @@ def setUpTestData(cls): class ReportTest(APITestCase): - class TestReport(Report): def test_foo(self): @@ -762,9 +756,7 @@ def test_get_report(self): class ScriptTest(APITestCase): - class TestScript(Script): - class Meta: name = "Test script" @@ -773,7 +765,6 @@ class Meta: var3 = BooleanVar() def run(self, data, commit=True): - self.log_info(data['var1']) self.log_success(data['var2']) self.log_failure(data['var3']) @@ -798,7 +789,6 @@ def setUp(self): ScriptViewSet._get_script = self.get_test_script def test_get_script(self): - url = reverse('extras-api:script-detail', kwargs={'pk': None}) response = self.client.get(url, **self.header) diff --git a/netbox/extras/tests/test_custom_field_choice_sets_endpoint.py b/netbox/extras/tests/test_custom_field_choice_sets_endpoint.py new file mode 100644 index 00000000000..5c9cb53b6a2 --- /dev/null +++ b/netbox/extras/tests/test_custom_field_choice_sets_endpoint.py @@ -0,0 +1,59 @@ +from django.contrib.auth import get_user_model +from rest_framework.test import APITestCase + +from users.models import Token + +User = get_user_model() + + +class CustomFieldChoiceSetsEndpointTest(APITestCase): + + def setUp(self): + self.super_user = User.objects.create_user(username='testuser', is_staff=True, is_superuser=True) + self.token = Token.objects.create(user=self.super_user) + self.header = {'HTTP_AUTHORIZATION': f'Token {self.token.key}'} + self.url = '/api/extras/custom-field-choice-sets/' + + def test_extra_choices_only_one_choice_element_return_400(self): + payload = { + "name": "test", + "extra_choices": [["choice1"]] + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 400) + + def test_extra_choices_two_wrong_choice_elements_return_400(self): + payload = { + "name": "test", + "extra_choices": [["choice1"], ["choice2"]] + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 400) + + def test_extra_choices_one_is_wrong_other_correct_choice_elements_return_400(self): + payload = { + "name": "test", + "extra_choices": [["1A", "choice1"], ["choice2"]] + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 400) + + def test_extra_choices_correct_choices_return_201(self): + payload = { + 'name': 'Choice Set', + 'extra_choices': [ + ['4A', 'Choice 1'], + ['4B', 'Choice 2'], + ['4C', 'Choice 3'], + ], + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 201) From f1322c932db251bf02893d0d61e7e159d0f97ead Mon Sep 17 00:00:00 2001 From: "julio.oliveira" Date: Tue, 16 Jan 2024 15:17:46 -0300 Subject: [PATCH 2/5] PR Comments Addressed --- netbox/extras/api/serializers.py | 5 +- netbox/extras/tests/test_api.py | 66 ++++++++++++++++++- .../test_custom_field_choice_sets_endpoint.py | 59 ----------------- 3 files changed, 68 insertions(+), 62 deletions(-) delete mode 100644 netbox/extras/tests/test_custom_field_choice_sets_endpoint.py diff --git a/netbox/extras/api/serializers.py b/netbox/extras/api/serializers.py index 50cd022b570..4bab228230a 100644 --- a/netbox/extras/api/serializers.py +++ b/netbox/extras/api/serializers.py @@ -3,6 +3,7 @@ from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import extend_schema_field from rest_framework import serializers +from rest_framework.fields import ListField from core.api.nested_serializers import NestedDataSourceSerializer, NestedDataFileSerializer, NestedJobSerializer from core.api.serializers import JobSerializer @@ -171,6 +172,7 @@ class CustomFieldChoiceSetSerializer(ValidatedModelSerializer): choices=CustomFieldChoiceSetBaseChoices, required=False ) + extra_choices = serializers.ListField(child=serializers.ListField(min_length=2)) class Meta: model = CustomFieldChoiceSet @@ -383,8 +385,7 @@ def validate(self, data): @extend_schema_field(serializers.JSONField(allow_null=True)) def get_assigned_object(self, instance): - serializer = get_serializer_for_model(instance.assigned_object_type.model_class(), - prefix=NESTED_SERIALIZER_PREFIX) + serializer = get_serializer_for_model(instance.assigned_object_type.model_class(), prefix=NESTED_SERIALIZER_PREFIX) context = {'request': self.context['request']} return serializer(instance.assigned_object, context=context).data diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index fd9f5bafa93..d3c89935151 100644 --- a/netbox/extras/tests/test_api.py +++ b/netbox/extras/tests/test_api.py @@ -4,7 +4,7 @@ from django.contrib.contenttypes.models import ContentType from django.urls import reverse from django.utils.timezone import make_aware -from rest_framework import status +from rest_framework import status, test from core.choices import ManagedFileRootPathChoices from dcim.models import Device, DeviceRole, DeviceType, Manufacturer, Rack, Location, RackRole, Site @@ -12,14 +12,17 @@ from extras.models import * from extras.reports import Report from extras.scripts import BooleanVar, IntegerVar, Script, StringVar +from users.models import Token from utilities.testing import APITestCase, APIViewTestCases + User = get_user_model() class AppTest(APITestCase): def test_root(self): + url = reverse('extras-api:api-root') response = self.client.get('{}?format=api'.format(url), **self.header) @@ -50,6 +53,7 @@ class WebhookTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): + webhooks = ( Webhook( name='Webhook 1', @@ -502,6 +506,7 @@ class TagTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): + tags = ( Tag(name='Tag 1', slug='tag-1'), Tag(name='Tag 2', slug='tag-2'), @@ -628,6 +633,7 @@ class ConfigContextTest(APIViewTestCases.APIViewTestCase): @classmethod def setUpTestData(cls): + config_contexts = ( ConfigContext(name='Config Context 1', weight=100, data={'foo': 123}), ConfigContext(name='Config Context 2', weight=200, data={'bar': 456}), @@ -726,6 +732,7 @@ def setUpTestData(cls): class ReportTest(APITestCase): + class TestReport(Report): def test_foo(self): @@ -756,7 +763,9 @@ def test_get_report(self): class ScriptTest(APITestCase): + class TestScript(Script): + class Meta: name = "Test script" @@ -765,6 +774,7 @@ class Meta: var3 = BooleanVar() def run(self, data, commit=True): + self.log_info(data['var1']) self.log_success(data['var2']) self.log_failure(data['var3']) @@ -789,6 +799,7 @@ def setUp(self): ScriptViewSet._get_script = self.get_test_script def test_get_script(self): + url = reverse('extras-api:script-detail', kwargs={'pk': None}) response = self.client.get(url, **self.header) @@ -886,3 +897,56 @@ def test_get_object(self): url = reverse('extras-api:contenttype-detail', kwargs={'pk': contenttype.pk}) self.assertHttpStatus(self.client.get(url, **self.header), status.HTTP_200_OK) + + +class CustomFieldChoiceSetsEndpointTest(test.APITestCase): + + def setUp(self): + self.super_user = User.objects.create_user(username='testuser', is_staff=True, is_superuser=True) + self.token = Token.objects.create(user=self.super_user) + self.header = {'HTTP_AUTHORIZATION': f'Token {self.token.key}'} + self.url = '/api/extras/custom-field-choice-sets/' + + def test_extra_choices_only_one_choice_element_return_400(self): + payload = { + "name": "test", + "extra_choices": [["choice1"]] + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 400) + + def test_extra_choices_two_wrong_choice_elements_return_400(self): + payload = { + "name": "test", + "extra_choices": [["choice1"], ["choice2"]] + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 400) + + def test_extra_choices_one_is_wrong_other_correct_choice_elements_return_400(self): + payload = { + "name": "test", + "extra_choices": [["1A", "choice1"], ["choice2"]] + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 400) + + def test_extra_choices_correct_choices_return_201(self): + payload = { + 'name': 'Choice Set', + 'extra_choices': [ + ['4A', 'Choice 1'], + ['4B', 'Choice 2'], + ['4C', 'Choice 3'], + ], + } + + response = self.client.post(self.url, payload, format='json', **self.header) + + self.assertEqual(response.status_code, 201) diff --git a/netbox/extras/tests/test_custom_field_choice_sets_endpoint.py b/netbox/extras/tests/test_custom_field_choice_sets_endpoint.py deleted file mode 100644 index 5c9cb53b6a2..00000000000 --- a/netbox/extras/tests/test_custom_field_choice_sets_endpoint.py +++ /dev/null @@ -1,59 +0,0 @@ -from django.contrib.auth import get_user_model -from rest_framework.test import APITestCase - -from users.models import Token - -User = get_user_model() - - -class CustomFieldChoiceSetsEndpointTest(APITestCase): - - def setUp(self): - self.super_user = User.objects.create_user(username='testuser', is_staff=True, is_superuser=True) - self.token = Token.objects.create(user=self.super_user) - self.header = {'HTTP_AUTHORIZATION': f'Token {self.token.key}'} - self.url = '/api/extras/custom-field-choice-sets/' - - def test_extra_choices_only_one_choice_element_return_400(self): - payload = { - "name": "test", - "extra_choices": [["choice1"]] - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 400) - - def test_extra_choices_two_wrong_choice_elements_return_400(self): - payload = { - "name": "test", - "extra_choices": [["choice1"], ["choice2"]] - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 400) - - def test_extra_choices_one_is_wrong_other_correct_choice_elements_return_400(self): - payload = { - "name": "test", - "extra_choices": [["1A", "choice1"], ["choice2"]] - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 400) - - def test_extra_choices_correct_choices_return_201(self): - payload = { - 'name': 'Choice Set', - 'extra_choices': [ - ['4A', 'Choice 1'], - ['4B', 'Choice 2'], - ['4C', 'Choice 3'], - ], - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 201) From bb72af864ba43240babf5361958c3d616f893eb5 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 19 Jan 2024 14:32:17 -0500 Subject: [PATCH 3/5] Set max_length=2 on extra_choices items; remove custom validation logic --- netbox/extras/api/serializers.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/netbox/extras/api/serializers.py b/netbox/extras/api/serializers.py index 4bab228230a..a40a03103fa 100644 --- a/netbox/extras/api/serializers.py +++ b/netbox/extras/api/serializers.py @@ -172,7 +172,12 @@ class CustomFieldChoiceSetSerializer(ValidatedModelSerializer): choices=CustomFieldChoiceSetBaseChoices, required=False ) - extra_choices = serializers.ListField(child=serializers.ListField(min_length=2)) + extra_choices = serializers.ListField( + child=serializers.ListField( + min_length=2, + max_length=2 + ) + ) class Meta: model = CustomFieldChoiceSet @@ -181,15 +186,6 @@ class Meta: 'choices_count', 'created', 'last_updated', ] - def validate_extra_choices(self, value): - for choice in value: - if isinstance(choice, list): - if len(choice) < 2: - raise serializers.ValidationError('Each choice must have 2 elements.') - else: - raise serializers.ValidationError('Extra choice must be a list of two elements.') - return value - # # Custom links From 73ab0425e873648fbd73ac399858a9d5f37a63b2 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 19 Jan 2024 14:38:14 -0500 Subject: [PATCH 4/5] Move test for invalid choices to CustomFieldChoiceSetTest --- netbox/extras/tests/test_api.py | 70 ++++++++------------------------- 1 file changed, 17 insertions(+), 53 deletions(-) diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index d3c89935151..45acb9580bd 100644 --- a/netbox/extras/tests/test_api.py +++ b/netbox/extras/tests/test_api.py @@ -252,6 +252,23 @@ def setUpTestData(cls): ) CustomFieldChoiceSet.objects.bulk_create(choice_sets) + def test_invalid_choice_items(self): + """ + Attempting to define each choice as a single-item list should return a 400 error. + """ + self.add_permissions('extras.add_customfieldchoiceset') + data = { + "name": "test", + "extra_choices": [ + ["choice1"], + ["choice2"], + ["choice3"], + ] + } + + response = self.client.post(self._get_list_url(), data, format='json', **self.header) + self.assertEqual(response.status_code, 400) + class CustomLinkTest(APIViewTestCases.APIViewTestCase): model = CustomLink @@ -897,56 +914,3 @@ def test_get_object(self): url = reverse('extras-api:contenttype-detail', kwargs={'pk': contenttype.pk}) self.assertHttpStatus(self.client.get(url, **self.header), status.HTTP_200_OK) - - -class CustomFieldChoiceSetsEndpointTest(test.APITestCase): - - def setUp(self): - self.super_user = User.objects.create_user(username='testuser', is_staff=True, is_superuser=True) - self.token = Token.objects.create(user=self.super_user) - self.header = {'HTTP_AUTHORIZATION': f'Token {self.token.key}'} - self.url = '/api/extras/custom-field-choice-sets/' - - def test_extra_choices_only_one_choice_element_return_400(self): - payload = { - "name": "test", - "extra_choices": [["choice1"]] - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 400) - - def test_extra_choices_two_wrong_choice_elements_return_400(self): - payload = { - "name": "test", - "extra_choices": [["choice1"], ["choice2"]] - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 400) - - def test_extra_choices_one_is_wrong_other_correct_choice_elements_return_400(self): - payload = { - "name": "test", - "extra_choices": [["1A", "choice1"], ["choice2"]] - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 400) - - def test_extra_choices_correct_choices_return_201(self): - payload = { - 'name': 'Choice Set', - 'extra_choices': [ - ['4A', 'Choice 1'], - ['4B', 'Choice 2'], - ['4C', 'Choice 3'], - ], - } - - response = self.client.post(self.url, payload, format='json', **self.header) - - self.assertEqual(response.status_code, 201) From b47dd157580c950bf18ae39a31a3b05dff759b5b Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Fri, 19 Jan 2024 14:41:18 -0500 Subject: [PATCH 5/5] Omit unused imports --- netbox/extras/tests/test_api.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/netbox/extras/tests/test_api.py b/netbox/extras/tests/test_api.py index 45acb9580bd..f40372a8f76 100644 --- a/netbox/extras/tests/test_api.py +++ b/netbox/extras/tests/test_api.py @@ -4,7 +4,7 @@ from django.contrib.contenttypes.models import ContentType from django.urls import reverse from django.utils.timezone import make_aware -from rest_framework import status, test +from rest_framework import status from core.choices import ManagedFileRootPathChoices from dcim.models import Device, DeviceRole, DeviceType, Manufacturer, Rack, Location, RackRole, Site @@ -12,10 +12,8 @@ from extras.models import * from extras.reports import Report from extras.scripts import BooleanVar, IntegerVar, Script, StringVar -from users.models import Token from utilities.testing import APITestCase, APIViewTestCases - User = get_user_model()