From d384136045db4beb231645077b14a3662a7a56fe Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 11 Aug 2026 11:22:17 -0400 Subject: [PATCH] Return a 403 when attempting to alter non-permitted objects --- netbox/core/api/schema.py | 23 +++++ netbox/core/tests/test_openapi_schema.py | 20 +++- netbox/dcim/tests/test_api.py | 106 +++++++++++++++++++ netbox/netbox/api/serializers/bulk.py | 2 +- netbox/netbox/api/viewsets/mixins.py | 126 ++++++++++++++++++----- 5 files changed, 251 insertions(+), 26 deletions(-) diff --git a/netbox/core/api/schema.py b/netbox/core/api/schema.py index d69f4a92f..5383f80a9 100644 --- a/netbox/core/api/schema.py +++ b/netbox/core/api/schema.py @@ -204,6 +204,13 @@ class NetBoxAutoSchema(AutoSchema): "modified: a bulk update is an all-or-none operation." ), ), + '403': OpenApiResponse( + response=BulkOperationErrorSerializer, + description=_( + "The requesting user is not permitted to apply one or more of the " + "modifications specified. No objects were modified." + ), + ), } if action == 'bulk_destroy': @@ -216,6 +223,13 @@ class NetBoxAutoSchema(AutoSchema): "rule. No objects were deleted." ), ), + '403': OpenApiResponse( + response=BulkOperationErrorSerializer, + description=_( + "The requesting user is not permitted to delete one or more of the objects " + "specified. No objects were deleted." + ), + ), '409': OpenApiResponse( response=BulkOperationErrorSerializer, description=_( @@ -244,6 +258,15 @@ class NetBoxAutoSchema(AutoSchema): "were created: a bulk creation is an all-or-none operation." ), ), + # A 403 always carries a `detail`, and BulkOperationError's `errors` is optional, so + # the one component covers both the single-object and the bulk shape here. + '403': OpenApiResponse( + response=BulkOperationErrorSerializer, + description=_( + "The requesting user is not permitted to create one or more of the objects " + "specified. No objects were created." + ), + ), } return {} diff --git a/netbox/core/tests/test_openapi_schema.py b/netbox/core/tests/test_openapi_schema.py index 521557279..acfb50611 100644 --- a/netbox/core/tests/test_openapi_schema.py +++ b/netbox/core/tests/test_openapi_schema.py @@ -154,8 +154,8 @@ class OpenAPISchemaTestCase(TestCase): def test_bulk_delete_documents_error_responses(self): """ - Bulk delete operations should document both the 400 (unresolvable request) and the 409 - (dependency or protection rule) responses. + Bulk delete operations should document the 400 (unresolvable request or protection rule), the + 403 (not permitted) and the 409 (dependent object) responses. Refs: #20054 """ @@ -164,8 +164,23 @@ class OpenAPISchemaTestCase(TestCase): for path in ('/api/dcim/sites/', '/api/ipam/prefixes/', '/api/users/users/'): with self.subTest(path=path): self.assertEqual(self._get_response_schema(path, 'delete', '400'), ref) + self.assertEqual(self._get_response_schema(path, 'delete', '403'), ref) self.assertEqual(self._get_response_schema(path, 'delete', '409'), ref) + def test_bulk_write_operations_document_forbidden_response(self): + """ + Every bulk write should document the 403 returned when an object-level permission refuses one + of the objects specified. + + Refs: #20054 + """ + ref = {'$ref': '#/components/schemas/BulkOperationError'} + + for path in ('/api/dcim/sites/', '/api/ipam/prefixes/', '/api/users/users/'): + for method in ('post', 'put', 'patch', 'delete'): + with self.subTest(path=path, method=method): + self.assertEqual(self._get_response_schema(path, method, '403'), ref) + def test_create_documents_error_response_for_either_shape(self): """ A POST to a list endpoint accepts either a single object or a list, so its 400 response @@ -196,6 +211,7 @@ class OpenAPISchemaTestCase(TestCase): with self.subTest(method=method): responses = self.schema['paths'][path][method]['responses'] self.assertNotIn('409', responses) + self.assertNotIn('403', responses) for code, response in responses.items(): schema = response.get('content', {}).get('application/json', {}).get('schema', {}) self.assertNotEqual( diff --git a/netbox/dcim/tests/test_api.py b/netbox/dcim/tests/test_api.py index 6b93068ce..d33203bd8 100644 --- a/netbox/dcim/tests/test_api.py +++ b/netbox/dcim/tests/test_api.py @@ -25,6 +25,7 @@ from utilities.testing import ( create_test_device, create_test_nat_ip_pair, disable_logging, + disable_warnings, ) from virtualization.models import Cluster, ClusterType from wireless.choices import WirelessChannelChoices @@ -739,6 +740,111 @@ class SiteTestCase(APIViewTestCases.APIViewTestCase): self.assertTrue(Site.objects.filter(pk=site1.pk).exists(), 'Site 1 should not have been deleted') self.assertTrue(Site.objects.filter(pk=site2.pk).exists(), 'Site 2 should not have been deleted') + def test_bulk_update_objects_permission_constraint(self): + """ + PATCH a set of objects where the update would move one of them outside the requesting user's + object-level permissions. The offending object must be named, rather than the whole batch + failing with an opaque 403, and nothing may be modified. + """ + site1 = Site.objects.get(slug='site-1') + site2 = Site.objects.get(slug='site-2') + Site.objects.filter(pk__in=(site1.pk, site2.pk)).update(status=SiteStatusChoices.STATUS_ACTIVE) + + # Only active sites may be changed, so setting Site 2's status to "planned" saves the object + # and then fails _validate_objects(), which perform_update() reports as PermissionDenied. + obj_perm = ObjectPermission( + name='Test permission', + actions=['change'], + constraints={'status': SiteStatusChoices.STATUS_ACTIVE}, + ) + obj_perm.save() + obj_perm.users.add(self.user) + obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model)) + + data = [ + {'id': site1.pk, 'description': 'Permitted'}, + {'id': site2.pk, 'status': SiteStatusChoices.STATUS_PLANNED}, + ] + with disable_warnings('django.request'): + response = self.client.patch(self._get_list_url(), data, format='json', **self.header) + + # Still a 403, as the single-object endpoint returns, but now correlated + self.assertHttpStatus(response, status.HTTP_403_FORBIDDEN) + self.assertIn('detail', response.data) + self.assertEqual([e['id'] for e in response.data['errors']], [site2.pk]) + self.assertIsInstance(response.data['errors'][0]['errors']['__all__'], list) + + # Neither site may have been modified, including the permitted one + site1.refresh_from_db() + site2.refresh_from_db() + self.assertEqual(site1.description, '', 'Site 1 should not have been updated') + self.assertEqual(site2.status, SiteStatusChoices.STATUS_ACTIVE, 'Site 2 should not have been updated') + + def test_bulk_update_objects_permission_constraint_and_validation_error(self): + """ + PATCH a set of objects where one entry is invalid and another is refused by object-level + permissions. Both must be reported, and the authorization failure must determine the status + code: it is the failure which would remain were the invalid entry corrected. + """ + site1 = Site.objects.get(slug='site-1') + site2 = Site.objects.get(slug='site-2') + Site.objects.filter(pk__in=(site1.pk, site2.pk)).update(status=SiteStatusChoices.STATUS_ACTIVE) + + obj_perm = ObjectPermission( + name='Test permission', + actions=['change'], + constraints={'status': SiteStatusChoices.STATUS_ACTIVE}, + ) + obj_perm.save() + obj_perm.users.add(self.user) + obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model)) + + data = [ + {'id': site1.pk, 'status': 'not-a-valid-status'}, # Fails validation (400) + {'id': site2.pk, 'status': SiteStatusChoices.STATUS_PLANNED}, # Not permitted (403) + ] + with disable_warnings('django.request'): + response = self.client.patch(self._get_list_url(), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_403_FORBIDDEN) + self.assertEqual([e['id'] for e in response.data['errors']], [site1.pk, site2.pk]) + + site1.refresh_from_db() + site2.refresh_from_db() + self.assertEqual(site1.status, SiteStatusChoices.STATUS_ACTIVE) + self.assertEqual(site2.status, SiteStatusChoices.STATUS_ACTIVE) + + def test_bulk_create_objects_permission_constraint(self): + """ + POST a set of objects where one falls outside the requesting user's object-level permissions. + The offending object must be correlated by its position, and nothing may be created. + """ + obj_perm = ObjectPermission( + name='Test permission', + actions=['add'], + constraints={'status': SiteStatusChoices.STATUS_ACTIVE}, + ) + obj_perm.save() + obj_perm.users.add(self.user) + obj_perm.object_types.add(ObjectType.objects.get_for_model(self.model)) + + initial_count = self._get_queryset().count() + data = [ + {'name': 'Site 20', 'slug': 'site-20', 'status': SiteStatusChoices.STATUS_ACTIVE}, + {'name': 'Site 21', 'slug': 'site-21', 'status': SiteStatusChoices.STATUS_PLANNED}, + ] + with disable_warnings('django.request'): + response = self.client.post(self._get_list_url(), data, format='json', **self.header) + + self.assertHttpStatus(response, status.HTTP_403_FORBIDDEN) + self.assertEqual([e['index'] for e in response.data['errors']], [1]) + self.assertIsInstance(response.data['errors'][0]['errors']['__all__'], list) + + self.assertEqual( + self._get_queryset().count(), initial_count, + 'No objects should be created when any sibling is not permitted', + ) + def test_bulk_delete_objects_conflict_and_abort_request(self): """ DELETE a set of objects where one is blocked by a dependent object and another by a diff --git a/netbox/netbox/api/serializers/bulk.py b/netbox/netbox/api/serializers/bulk.py index 57c2a27a1..ad0cb5660 100644 --- a/netbox/netbox/api/serializers/bulk.py +++ b/netbox/netbox/api/serializers/bulk.py @@ -61,7 +61,7 @@ class BulkOperationErrorSerializer(serializers.Serializer): responsible for it. """ detail = serializers.CharField( - help_text=_('A summary of the failure, e.g. "1 of 3 objects failed validation."') + help_text=_('A summary of the failure, e.g. "1 of 3 objects could not be updated."') ) errors = BulkOperationEntryErrorSerializer( many=True, diff --git a/netbox/netbox/api/viewsets/mixins.py b/netbox/netbox/api/viewsets/mixins.py index c3b4fb802..362f2b0f9 100644 --- a/netbox/netbox/api/viewsets/mixins.py +++ b/netbox/netbox/api/viewsets/mixins.py @@ -2,7 +2,7 @@ import warnings from collections import Counter from contextlib import contextmanager -from django.core.exceptions import ObjectDoesNotExist +from django.core.exceptions import ObjectDoesNotExist, PermissionDenied from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError from django.http import Http404 @@ -24,6 +24,7 @@ from utilities.request import copy_safe_request from utilities.rqworker import any_workers_for_queue __all__ = ( + 'BULK_ERROR_STATUSES', 'BackgroundOperationMixin', 'BulkCreateModelMixin', 'BulkDestroyModelMixin', @@ -37,8 +38,44 @@ __all__ = ( 'get_invalid_entries_response', 'get_missing_objects_response', 'get_non_list_response', + 'resolve_bulk_error_status', ) +# The status codes with which a failed bulk operation may be reported, in order of precedence: where +# the per-object failures within one batch imply more than one of these, the earliest applies, being +# the one which would still stand were the others corrected. An authorization failure thus outranks a +# conflict with the current state of the database, which in turn outranks a rejection of the request. +BULK_ERROR_STATUSES = ( + status.HTTP_403_FORBIDDEN, + status.HTTP_409_CONFLICT, + status.HTTP_400_BAD_REQUEST, +) + +# Reported for an object whose write was undone because the object it produced falls outside the +# queryset permitted to the requesting user (see ObjectValidationMixin._validate_objects). Which +# constraint was violated is deliberately not disclosed, consistent with the single-object endpoints. +PERMISSION_DENIED_MESSAGE = _("You do not have permission to perform this action on this object.") + + +def resolve_bulk_error_status(error_statuses): + """ + Return the single status code with which to report a bulk operation whose per-object failures + imply the given ones, or None if there were no failures. + + :param error_statuses: The set of status codes implied by the failures within one batch, each + drawn from BULK_ERROR_STATUSES (which documents how they are ranked). + """ + if not error_statuses: + return None + + for error_status in BULK_ERROR_STATUSES: + if error_status in error_statuses: + return error_status + + # A code with no defined precedence (a subclass may report its own) is not silently ranked; + # fall back to the generic client error. + return status.HTTP_400_BAD_REQUEST + def get_non_list_response(data): """ @@ -357,18 +394,18 @@ class BulkCreateModelMixin: ] """ def bulk_create(self, request, *args, **kwargs): - created_pks, errors = self.perform_bulk_create(request.data) + created_pks, errors, error_status = self.perform_bulk_create(request.data) if errors: return Response( { - 'detail': _('{failed_count} of {total} objects failed validation.').format( + 'detail': _('{failed_count} of {total} objects could not be created.').format( failed_count=len(errors), total=len(request.data), ), 'errors': errors, }, - status=status.HTTP_400_BAD_REQUEST, + status=error_status, ) # Re-fetch the new objects to serialize them with their related objects prefetched. Order by PK @@ -380,8 +417,16 @@ class BulkCreateModelMixin: return Response(serializer.data, status=status.HTTP_201_CREATED) def perform_bulk_create(self, data): + """ + Validate and create each of the given objects, rolling the entire batch back if any one of + them could not be created. + + Returns the PKs of the objects created, the per-object errors, and the status code with + which to report them (None if there were none). See resolve_bulk_error_status(). + """ created_pks = [] errors = [] + error_statuses = set() using = router.db_for_write(self.queryset.model) with transaction.atomic(using=using), discard_events_on_rollback(self, using=using): # Validate and save each object in turn, rather than validating the entire batch up front, @@ -404,10 +449,12 @@ class BulkCreateModelMixin: ], }, }) + error_statuses.add(status.HTTP_400_BAD_REQUEST) continue serializer = self.get_serializer(data=item) if not serializer.is_valid(): errors.append({'index': i, 'errors': serializer.errors}) + error_statuses.add(status.HTTP_400_BAD_REQUEST) continue try: # Provisionally create even when a prior item failed, so subsequent @@ -421,11 +468,19 @@ class BulkCreateModelMixin: # and the remaining objects in the batch can still be evaluated. The message is # coerced to a string because a few receivers pass an exception rather than text. errors.append({'index': i, 'errors': {'__all__': [str(e.message)]}}) + error_statuses.add(status.HTTP_400_BAD_REQUEST) + except PermissionDenied: + # Raised by perform_create() when the object it saved falls outside the queryset + # permitted to the requesting user. Reported per object so that the offending + # entry is named, but still as a 403, which is what the single-object endpoint + # returns for the same rejection. + errors.append({'index': i, 'errors': {'__all__': [PERMISSION_DENIED_MESSAGE]}}) + error_statuses.add(status.HTTP_403_FORBIDDEN) else: created_pks.append(serializer.instance.pk) if errors: transaction.set_rollback(True) - return created_pks, errors + return created_pks, errors, resolve_bulk_error_status(error_statuses) # TODO: Remove this in NetBox v5.0 @@ -516,18 +571,21 @@ class BulkUpdateModelMixin: for object_id, item in zip(object_ids, request.data, strict=True) } - object_pks, errors = self.perform_bulk_update(qs, update_data, partial=partial) + object_pks, errors, error_status = self.perform_bulk_update(qs, update_data, partial=partial) if errors: return Response( { - 'detail': _('{failed_count} of {total} objects failed validation.').format( + 'detail': _('{failed_count} of {total} objects could not be updated.').format( failed_count=len(errors), + # Every object named was matched and attempted, the duplicate and missing-ID + # checks above having rejected the batch otherwise, so this equals the number + # of objects submitted. total=len(object_pks) + len(errors), ), 'errors': errors, }, - status=status.HTTP_400_BAD_REQUEST, + status=error_status, ) # Prefetch related objects for all updated instances @@ -537,8 +595,16 @@ class BulkUpdateModelMixin: return Response(serializer.data, status=status.HTTP_200_OK) def perform_bulk_update(self, objects, update_data, partial): + """ + Validate and apply the given attributes to each of the given objects, rolling the entire + batch back if any one of them could not be updated. + + Returns the PKs of the objects updated, the per-object errors, and the status code with + which to report them (None if there were none). See resolve_bulk_error_status(). + """ updated_pks = [] errors = [] + error_statuses = set() using = router.db_for_write(self.queryset.model) with transaction.atomic(using=using), discard_events_on_rollback(self, using=using): # Validate and save each object in turn so subsequent validations see the DB @@ -551,6 +617,7 @@ class BulkUpdateModelMixin: serializer = self.get_serializer(obj, data=data, partial=partial) if not serializer.is_valid(): errors.append({'id': obj.pk, 'errors': serializer.errors}) + error_statuses.add(status.HTTP_400_BAD_REQUEST) continue try: self.perform_update(serializer) @@ -561,11 +628,20 @@ class BulkUpdateModelMixin: # and the remaining objects in the batch can still be evaluated. The message is # coerced to a string because a few receivers pass an exception rather than text. errors.append({'id': obj.pk, 'errors': {'__all__': [str(e.message)]}}) + error_statuses.add(status.HTTP_400_BAD_REQUEST) + except PermissionDenied: + # Raised by perform_update() when the object, as modified, falls outside the + # queryset permitted to the requesting user -- so unlike the check made before + # the batch begins (see get_missing_objects_response), this depends on the + # attributes submitted. Reported per object so that the offending entry is + # named, but still as a 403, as the single-object endpoint returns. + errors.append({'id': obj.pk, 'errors': {'__all__': [PERMISSION_DENIED_MESSAGE]}}) + error_statuses.add(status.HTTP_403_FORBIDDEN) else: updated_pks.append(obj.pk) if errors: transaction.set_rollback(True) - return updated_pks, errors + return updated_pks, errors, resolve_bulk_error_status(error_statuses) def get_bulk_update_serializer_class(self, *, partial=False): return get_bulk_update_serializer_class( @@ -637,14 +713,9 @@ class BulkDestroyModelMixin: o['id']: o.get('changelog_message') for o in serializer.validated_data } - errors, total, has_conflict = self.perform_bulk_destroy(qs, changelog_messages) + errors, total, error_status = self.perform_bulk_destroy(qs, changelog_messages) if errors: - # A dependency conflict reports 409, as it is a conflict with the current state of the - # database; every other failure reports 400, as it is a rejection of the request. This - # matches the single-object endpoint, where dispatch() maps the same two exception - # classes to the same two status codes. Where a batch hit both, the conflict takes - # precedence: it is the failure which would remain were the request itself corrected. return Response( { 'detail': _('{failed_count} of {total} objects could not be deleted.').format( @@ -653,7 +724,7 @@ class BulkDestroyModelMixin: ), 'errors': errors, }, - status=status.HTTP_409_CONFLICT if has_conflict else status.HTTP_400_BAD_REQUEST, + status=error_status, ) return Response(status=status.HTTP_204_NO_CONTENT) @@ -663,15 +734,18 @@ class BulkDestroyModelMixin: Attempt to delete each of the given objects, rolling the entire batch back if any one of them could not be deleted. - Returns the per-object errors, the number of objects processed, and whether any of the - failures was a conflict with the current state of the database (a dependent object) rather - than a rejection of the request (a protection rule, or any other signal receiver raising - AbortRequest). The caller uses the last of these to select a status code. + Returns the per-object errors, the number of objects processed, and the status code with + which to report the errors (None if there were none). A dependency conflict yields a 409, as + it is a conflict with the current state of the database, whereas a protection rule (or any + other signal receiver raising AbortRequest) yields a 400, being a rejection of the request: + this matches the single-object endpoint, where dispatch() maps the same exception classes to + the same status codes. See resolve_bulk_error_status() for how a batch hitting more than one + of these is resolved. """ changelog_messages = changelog_messages or {} errors = [] total = 0 - has_conflict = False + error_statuses = set() using = router.db_for_write(self.queryset.model) with transaction.atomic(using=using), discard_events_on_rollback(self, using=using): for obj in objects: @@ -683,7 +757,7 @@ class BulkDestroyModelMixin: try: self.perform_destroy(obj) except (ProtectedError, RestrictedError) as e: - has_conflict = True + error_statuses.add(status.HTTP_409_CONFLICT) protected = list( e.protected_objects if isinstance(e, ProtectedError) else e.restricted_objects ) @@ -709,9 +783,15 @@ class BulkDestroyModelMixin: # is rolled back to that savepoint and the remaining objects in the batch can # still be evaluated. errors.append({'id': pk, 'errors': {'__all__': [str(e.message)]}}) + error_statuses.add(status.HTTP_400_BAD_REQUEST) + except PermissionDenied: + # Raised by perform_destroy() when the object falls outside the queryset + # permitted to the requesting user (reachable via the If-Match re-check). + errors.append({'id': pk, 'errors': {'__all__': [PERMISSION_DENIED_MESSAGE]}}) + error_statuses.add(status.HTTP_403_FORBIDDEN) if errors: transaction.set_rollback(True) - return errors, total, has_conflict + return errors, total, resolve_bulk_error_status(error_statuses) class ObjectValidationMixin: