Просмотр исходного кода

fix(api): Disable background writes for Tokens

Bulk Token creation via ?background=true captured the plaintext v2 Token
into the job result, where any core.view_job holder could read it.
BackgroundOperationMixin gains a background_enabled flag, checked both
when accepting work and when a queued job executes.

Fixes #23196
Martin Hauser 7 часов назад
Родитель
Сommit
b135beec3f

+ 2 - 0
docs/integrations/rest-api.md

@@ -799,6 +799,8 @@ A `202` response indicates that the request was accepted and queued, not that it
 
 Background processing applies only to bulk operations (a JSON list) on a model's list endpoint. For a single-object write the `background` parameter is ignored and the request is processed synchronously. It cannot be combined with an [`If-Match`](#if-match) precondition (which cannot be evaluated reliably once execution is deferred); such a request is rejected with an `HTTP 400` response. If no background worker is running to service the queue, the request is rejected with an `HTTP 503` response rather than enqueuing a job that would never run.
 
+Some endpoints do not support background processing at all, because their response carries a value that exists only once and cannot be delivered by an `HTTP 202` response. The API tokens endpoint (`/api/users/tokens/`) is one such endpoint: a created token's plaintext is returned only by the request that creates it. A bulk write to such an endpoint with `background=true` is rejected with an `HTTP 400` response and no job is enqueued.
+
 Two behaviors differ from a synchronous request and may change in a future release: field selection via [`fields`/`omit`](#specifying-fields) (and brief mode) is not applied to the stored result, and the authorization captured when the request is accepted is not re-checked if the token is later disabled or expires before the job runs.
 
 ## Changelog Messages

+ 15 - 0
docs/plugins/development/rest-api.md

@@ -75,6 +75,21 @@ class MyModelViewSet(NetBoxModelViewSet):
     serializer_class = MyModelSerializer
 ```
 
+### Background Processing
+
+Viewsets derived from `NetBoxModelViewSet` accept `?background=true` on bulk write requests, which defers the write to a background job and returns `HTTP 202 Accepted`. The job's result, including the serialized response body, is stored on the job record and is readable by any user permitted to view jobs.
+
+If a viewset's response can contain a value that must not be retained, such as a secret returned only at creation time, disable background processing for that endpoint:
+
+```python
+class MyModelViewSet(NetBoxModelViewSet):
+    queryset = MyModel.objects.all()
+    serializer_class = MyModelSerializer
+    background_enabled = False
+```
+
+A bulk write to that endpoint with `background=true` is then rejected with an `HTTP 400` response.
+
 ## Routers
 
 Routers map URLs to REST API views (endpoints). NetBox does not provide any custom components for this; the [`DefaultRouter`](https://www.django-rest-framework.org/api-guide/routers/#defaultrouter) class provided by DRF should suffice for most use cases.

+ 11 - 0
netbox/netbox/api/viewsets/mixins.py

@@ -289,6 +289,15 @@ class BackgroundOperationMixin:
     This mixin overrides no framework methods; the bulk action methods call its helpers.
     """
 
+    # False where the response carries a write-once secret: a 202 can only return it via the job record.
+    background_enabled = True
+
+    def _check_background_enabled(self):
+        if not self.background_enabled:
+            raise ValidationError({
+                'detail': _("Background processing is not supported for this endpoint.")
+            })
+
     def _background_requested(self, request):
         """Return True if background processing was requested for this write."""
         if request.method not in ('POST', 'PUT', 'PATCH', 'DELETE'):
@@ -314,6 +323,8 @@ class BackgroundOperationMixin:
         Enqueue an AsyncAPIJob to perform the given bulk action in the background and return
         a 202 response containing the job ID and polling URL.
         """
+        self._check_background_enabled()
+
         # Reject conditional requests: an If-Match precondition cannot be meaningfully
         # honored when the write is deferred to a worker (the TOCTOU window is unbounded).
         if request.META.get('HTTP_IF_MATCH'):

+ 3 - 0
netbox/netbox/jobs.py

@@ -385,6 +385,9 @@ class AsyncAPIJob(JobRunner):
         #   - AbortRequest / ProtectedError / RestrictedError -> exception_to_response()
         with apply_request_processors(drf_request):
             try:
+                # A job queued before the endpoint opted out carries no query parameter to catch.
+                if check_background_enabled := getattr(viewset, '_check_background_enabled', None):
+                    check_background_enabled()
                 response = getattr(viewset, action)(drf_request, **action_kwargs)
             except (APIException, Http404, PermissionDenied) as e:
                 response = viewset.handle_exception(e)

+ 59 - 4
netbox/netbox/tests/test_api_background.py

@@ -21,6 +21,7 @@ from core.exceptions import JobFailed
 from core.models import Job, ObjectChange
 from dcim.api.views import RegionViewSet
 from dcim.models import DeviceType, Manufacturer, Region
+from netbox.jobs import AsyncAPIJob
 from users.models import ObjectPermission
 from utilities.request import copy_safe_request
 from utilities.testing.api import APITestCase
@@ -305,6 +306,37 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
         self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE)
         self.assertEqual(Job.objects.count(), 0)
 
+    def test_background_disabled_rejected_before_worker_check(self):
+        # The rejection precedes the worker-liveness probe, so it does not depend on a live worker.
+        self.grant('add', 'change', 'delete', 'view')
+        requests = (
+            (self.client.post, [{'name': 'Region A', 'slug': 'region-a'}]),
+            (self.client.put, [{'id': self.regions[0].pk, 'name': 'X', 'slug': 'x'}]),
+            (self.client.patch, [{'id': self.regions[0].pk, 'description': 'x'}]),
+            (self.client.delete, [{'id': self.regions[0].pk}]),
+        )
+        with patch.object(RegionViewSet, 'background_enabled', False):
+            with patch(
+                'netbox.api.viewsets.mixins.any_workers_for_queue', return_value=True
+            ) as any_workers:
+                for method, payload in requests:
+                    with self.subTest(method=method.__name__):
+                        response = method(
+                            '/api/dcim/regions/?background=true', payload,
+                            format='json', **self.header
+                        )
+                        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+                        self.assertEqual(
+                            response.data['detail'],
+                            'Background processing is not supported for this endpoint.'
+                        )
+
+        any_workers.assert_not_called()
+        self.assertEqual(Job.objects.count(), 0)
+        self.assertEqual(Region.objects.count(), len(self.regions))
+        self.regions[0].refresh_from_db()
+        self.assertEqual(self.regions[0].description, '')
+
     def test_get_with_background_is_ignored(self):
         self.grant('view')
         response = self.client.get('/api/dcim/regions/?background=true', **self.header)
@@ -375,8 +407,6 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
         # user was active at enqueue), but the user is deactivated before the worker runs.
         # We drive AsyncAPIJob directly because the HTTP layer would otherwise reject an
         # inactive user's token at authentication time, never reaching the worker.
-        from netbox.jobs import AsyncAPIJob
-
         self.grant('change', 'view')
         self.user.is_active = False
         self.user.save()
@@ -404,13 +434,38 @@ class BackgroundBulkWriteTests(RQQueueTestMixin, APITestCase):
 
     # ------------------------------------------------------------------ host parsing
 
+    def test_background_disabled_fails_queued_job(self):
+        # Models the upgrade window: enqueued while the endpoint still accepted background work.
+        self.grant('add', 'view')
+
+        factory = RequestFactory()
+        raw_request = factory.post('/api/dcim/regions/', data=[], content_type='application/json')
+        raw_request.user = self.user
+        request_copy = copy_safe_request(raw_request)
+
+        job = Job.objects.create(name='Bulk create regions', user=self.user, job_id=uuid.uuid4())
+        with patch.object(RegionViewSet, 'background_enabled', False):
+            AsyncAPIJob.handle(
+                job=job,
+                viewset_class='dcim.api.views.RegionViewSet',
+                action='bulk_create',
+                payload=[{'name': 'Region A', 'slug': 'region-a'}],
+                user_pk=self.user.pk,
+                request=request_copy,
+                scheme='http',
+            )
+
+        job.refresh_from_db()
+        self.assertEqual(job.status, JobStatusChoices.STATUS_FAILED)
+        self.assertEqual(job.data['status_code'], status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(job.error, 'Background processing is not supported for this endpoint.')
+        self.assertFalse(Region.objects.filter(slug='region-a').exists())
+
     def test_ipv6_host_builds_correct_request(self):
         # A bracketed IPv6 host:port must round-trip through the request snapshot without being
         # split on its inner colons. copy_safe_request() carries SERVER_NAME/SERVER_PORT/HTTP_HOST
         # verbatim (already separated when the original request was received), so _build_request()
         # needs no host parsing of its own.
-        from netbox.jobs import AsyncAPIJob
-
         factory = RequestFactory()
         raw_request = factory.patch(
             '/api/dcim/regions/', data=[], content_type='application/json',

+ 2 - 0
netbox/users/api/views.py

@@ -51,6 +51,8 @@ class TokenViewSet(NetBoxModelViewSet):
     queryset = Token.objects.all()
     serializer_class = serializers.TokenSerializer
     filterset_class = filtersets.TokenFilterSet
+    # A created v2 Token's plaintext must not be retained in a job record.
+    background_enabled = False
 
 
 class TokenProvisionView(APIView):

+ 153 - 2
netbox/users/tests/test_api.py

@@ -1,10 +1,15 @@
-from django.test import override_settings
+import uuid
+
+from django.test import RequestFactory, override_settings
 from django.urls import reverse
 
-from core.models import ObjectType
+from core.exceptions import JobFailed
+from core.models import Job, ObjectType
+from netbox.jobs import AsyncAPIJob
 from users.constants import TOKEN_DEFAULT_LENGTH
 from users.models import Group, ObjectPermission, Owner, OwnerGroup, Token, User
 from utilities.data import deepmerge
+from utilities.request import copy_safe_request
 from utilities.testing import APITestCase, APIViewTestCases, create_test_user
 
 
@@ -460,6 +465,152 @@ class TokenTestCase(
         # Each token should be unique
         self.assertEqual(len(plaintexts), len(data))
 
+    def test_background_bulk_create_tokens_rejected(self):
+        """
+        Bulk Token creation cannot be backgrounded. The plaintext of a created v2 Token would
+        otherwise be captured into the job's data, where it is readable for the job's lifetime.
+        """
+        self.add_permissions('users.add_token')
+        users = [
+            User.objects.create_user(username='token_bg_user1'),
+            User.objects.create_user(username='token_bg_user2'),
+        ]
+        data = [{'user': u.pk} for u in users]
+        url = reverse('users-api:token-list')
+
+        response = self.client.post(f'{url}?background=true', data, format='json', **self.header)
+
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            response.data['detail'], 'Background processing is not supported for this endpoint.'
+        )
+        self.assertEqual(Job.objects.count(), 0)
+        for user in users:
+            self.assertFalse(Token.objects.filter(user=user).exists())
+
+    def test_background_bulk_update_and_delete_tokens_rejected(self):
+        """
+        The opt-out covers the whole Token endpoint, not only creation, and the requested
+        modifications and deletions do not occur.
+        """
+        self.add_permissions('users.change_token', 'users.delete_token')
+        # Token orders by '-created', so first() would return the test user's own auth Token.
+        token = Token.objects.get(user__username='User 1')
+        url = reverse('users-api:token-list')
+
+        for method in (self.client.put, self.client.patch):
+            with self.subTest(method=method.__name__):
+                response = method(
+                    f'{url}?background=true', [{'id': token.pk, 'description': 'bg'}],
+                    format='json', **self.header
+                )
+                self.assertEqual(response.status_code, 400)
+
+        response = self.client.delete(
+            f'{url}?background=true', [{'id': token.pk}], format='json', **self.header
+        )
+        self.assertEqual(response.status_code, 400)
+
+        self.assertEqual(Job.objects.count(), 0)
+        token.refresh_from_db()
+        self.assertNotEqual(token.description, 'bg')
+
+    def test_synchronous_token_creation_unaffected_by_opt_out(self):
+        """
+        The opt-out only refuses background processing. A single-object write with
+        `background=true` and a bulk write with `background=false` both run synchronously and
+        still return a usable plaintext.
+        """
+        self.add_permissions('users.add_token')
+        url = reverse('users-api:token-list')
+
+        single_user = User.objects.create_user(username='token_bg_single_user')
+        response = self.client.post(
+            f'{url}?background=true', {'user': single_user.pk}, format='json', **self.header
+        )
+        self.assertEqual(response.status_code, 201)
+        token = Token.objects.get(pk=response.data['id'])
+        self.assertTrue(token.validate(response.data['token']))
+
+        bulk_users = [
+            User.objects.create_user(username='token_bg_false_user1'),
+            User.objects.create_user(username='token_bg_false_user2'),
+        ]
+        response = self.client.post(
+            f'{url}?background=false', [{'user': u.pk} for u in bulk_users],
+            format='json', **self.header
+        )
+        self.assertEqual(response.status_code, 201)
+        for obj in response.data:
+            self.assertEqual(len(obj['token']), TOKEN_DEFAULT_LENGTH)
+            self.assertTrue(Token.objects.get(pk=obj['id']).validate(obj['token']))
+
+        self.assertEqual(Job.objects.count(), 0)
+
+    def test_token_plaintext_is_not_returned_on_subsequent_reads(self):
+        """
+        A v2 Token's plaintext is returned only by the creating response. Any later read of the
+        same object returns no usable value.
+        """
+        self.add_permissions('users.add_token', 'users.view_token')
+        user = User.objects.create_user(username='token_reread_user')
+
+        response = self.client.post(
+            reverse('users-api:token-list'), {'user': user.pk, 'version': 2},
+            format='json', **self.header
+        )
+        self.assertEqual(response.status_code, 201)
+        plaintext = response.data['token']
+        self.assertEqual(len(plaintext), TOKEN_DEFAULT_LENGTH)
+
+        detail = self.client.get(
+            reverse('users-api:token-detail', kwargs={'pk': response.data['id']}), **self.header
+        )
+        self.assertEqual(detail.status_code, 200)
+        self.assertFalse(detail.data.get('token'))
+
+    def test_queued_background_token_job_creates_nothing(self):
+        """
+        A Token job enqueued before the endpoint opted out is refused when the worker picks it
+        up, so no Token is created and no plaintext is captured into the job's data.
+        """
+        self.add_permissions('users.add_token')
+        user = User.objects.create_user(username='token_queued_user')
+        token_count = Token.objects.count()
+
+        raw_request = RequestFactory().post(
+            '/api/users/tokens/', data=[], content_type='application/json'
+        )
+        raw_request.user = self.user
+        request_copy = copy_safe_request(raw_request)
+
+        for action in ('create', 'bulk_create'):
+            with self.subTest(action=action):
+                job = Job.objects.create(
+                    name='Bulk create tokens', user=self.user, job_id=uuid.uuid4()
+                )
+                with self.assertRaises(JobFailed):
+                    AsyncAPIJob(job).run(
+                        viewset_class='users.api.views.TokenViewSet',
+                        action=action,
+                        payload=[{'user': user.pk}],
+                        user_pk=self.user.pk,
+                        request=request_copy,
+                        scheme='http',
+                    )
+
+                job.refresh_from_db()
+                self.assertEqual(job.data['status_code'], 400)
+                self.assertEqual(
+                    job.error, 'Background processing is not supported for this endpoint.'
+                )
+                # Pinning the whole body is what fails if the guard moves below the action.
+                self.assertEqual(
+                    job.data['data'],
+                    {'detail': 'Background processing is not supported for this endpoint.'}
+                )
+                self.assertEqual(Token.objects.count(), token_count)
+
     def test_create_token_ignores_client_supplied_plaintext(self):
         """
         A client must not be able to choose a Token's plaintext value. Any `token` value supplied in a