From 3d1e4fde81e0c60a0ade64f0537e94b5239f62ed Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Fri, 28 May 2021 16:07:27 -0400 Subject: [PATCH 1/6] Initial work on #6284 --- netbox/extras/context_managers.py | 11 +++- netbox/extras/signals.py | 23 +++++-- netbox/extras/tests/test_webhooks.py | 90 ++++++++++++++-------------- netbox/extras/webhooks.py | 86 ++++++++++++++++---------- 4 files changed, 126 insertions(+), 84 deletions(-) diff --git a/netbox/extras/context_managers.py b/netbox/extras/context_managers.py index 4a33f28ef..25a49b325 100644 --- a/netbox/extras/context_managers.py +++ b/netbox/extras/context_managers.py @@ -4,6 +4,7 @@ from django.db.models.signals import m2m_changed, pre_delete, post_save from extras.signals import _handle_changed_object, _handle_deleted_object from utilities.utils import curry +from .webhooks import flush_webhooks @contextmanager @@ -14,9 +15,11 @@ def change_logging(request): :param request: WSGIRequest object with a unique `id` set """ + webhook_queue = [] + # Curry signals receivers to pass the current request - handle_changed_object = curry(_handle_changed_object, request) - handle_deleted_object = curry(_handle_deleted_object, request) + handle_changed_object = curry(_handle_changed_object, request, webhook_queue) + handle_deleted_object = curry(_handle_deleted_object, request, webhook_queue) # Connect our receivers to the post_save and post_delete signals. post_save.connect(handle_changed_object, dispatch_uid='handle_changed_object') @@ -30,3 +33,7 @@ def change_logging(request): post_save.disconnect(handle_changed_object, dispatch_uid='handle_changed_object') m2m_changed.disconnect(handle_changed_object, dispatch_uid='handle_changed_object') pre_delete.disconnect(handle_deleted_object, dispatch_uid='handle_deleted_object') + + # Flush queued webhooks to RQ + flush_webhooks(webhook_queue) + del webhook_queue diff --git a/netbox/extras/signals.py b/netbox/extras/signals.py index 2191c4397..d53985621 100644 --- a/netbox/extras/signals.py +++ b/netbox/extras/signals.py @@ -12,17 +12,20 @@ from prometheus_client import Counter from .choices import ObjectChangeActionChoices from .models import CustomField, ObjectChange -from .webhooks import enqueue_webhooks +from .webhooks import enqueue_object, serialize_for_webhook # # Change logging/webhooks # -def _handle_changed_object(request, sender, instance, **kwargs): +def _handle_changed_object(request, webhook_queue, sender, instance, **kwargs): """ Fires when an object is created or updated. """ + if not hasattr(instance, 'to_objectchange'): + return + m2m_changed = False # Determine the type of change being made @@ -53,8 +56,13 @@ def _handle_changed_object(request, sender, instance, **kwargs): objectchange.request_id = request.id objectchange.save() - # Enqueue webhooks - enqueue_webhooks(instance, request.user, request.id, action) + # If this is an M2M change, update the previously queued webhook (from post_save) + if m2m_changed and webhook_queue: + # TODO: Need more validation here + # TODO: Need to account for snapshot changes + webhook_queue[-1]['data'] = serialize_for_webhook(instance) + else: + enqueue_object(webhook_queue, instance, request.user, request.id, action) # Increment metric counters if action == ObjectChangeActionChoices.ACTION_CREATE: @@ -68,10 +76,13 @@ def _handle_changed_object(request, sender, instance, **kwargs): ObjectChange.objects.filter(time__lt=cutoff)._raw_delete(using=DEFAULT_DB_ALIAS) -def _handle_deleted_object(request, sender, instance, **kwargs): +def _handle_deleted_object(request, webhook_queue, sender, instance, **kwargs): """ Fires when an object is deleted. """ + if not hasattr(instance, 'to_objectchange'): + return + # Record an ObjectChange if applicable if hasattr(instance, 'to_objectchange'): objectchange = instance.to_objectchange(ObjectChangeActionChoices.ACTION_DELETE) @@ -80,7 +91,7 @@ def _handle_deleted_object(request, sender, instance, **kwargs): objectchange.save() # Enqueue webhooks - enqueue_webhooks(instance, request.user, request.id, ObjectChangeActionChoices.ACTION_DELETE) + enqueue_object(webhook_queue, instance, request.user, request.id, ObjectChangeActionChoices.ACTION_DELETE) # Increment metric counters model_deletes.labels(instance._meta.model_name).inc() diff --git a/netbox/extras/tests/test_webhooks.py b/netbox/extras/tests/test_webhooks.py index eff9cdb97..7f1242811 100644 --- a/netbox/extras/tests/test_webhooks.py +++ b/netbox/extras/tests/test_webhooks.py @@ -12,7 +12,7 @@ from rest_framework import status from dcim.models import Site from extras.choices import ObjectChangeActionChoices from extras.models import Webhook -from extras.webhooks import enqueue_webhooks, generate_signature +from extras.webhooks import enqueue_object, generate_signature from extras.webhooks_worker import process_webhook from utilities.testing import APITestCase @@ -96,46 +96,48 @@ class WebhookTest(APITestCase): self.assertEqual(job.kwargs['model_name'], 'site') self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_DELETE) - def test_webhooks_worker(self): - - request_id = uuid.uuid4() - - def dummy_send(_, request, **kwargs): - """ - A dummy implementation of Session.send() to be used for testing. - Always returns a 200 HTTP response. - """ - webhook = Webhook.objects.get(type_create=True) - signature = generate_signature(request.body, webhook.secret) - - # Validate the outgoing request headers - self.assertEqual(request.headers['Content-Type'], webhook.http_content_type) - self.assertEqual(request.headers['X-Hook-Signature'], signature) - self.assertEqual(request.headers['X-Foo'], 'Bar') - - # Validate the outgoing request body - body = json.loads(request.body) - self.assertEqual(body['event'], 'created') - self.assertEqual(body['timestamp'], job.kwargs['timestamp']) - self.assertEqual(body['model'], 'site') - self.assertEqual(body['username'], 'testuser') - self.assertEqual(body['request_id'], str(request_id)) - self.assertEqual(body['data']['name'], 'Site 1') - - return HttpResponse() - - # Enqueue a webhook for processing - site = Site.objects.create(name='Site 1', slug='site-1') - enqueue_webhooks( - instance=site, - user=self.user, - request_id=request_id, - action=ObjectChangeActionChoices.ACTION_CREATE - ) - - # Retrieve the job from queue - job = self.queue.jobs[0] - - # Patch the Session object with our dummy_send() method, then process the webhook for sending - with patch.object(Session, 'send', dummy_send) as mock_send: - process_webhook(**job.kwargs) + # TODO: Replace webhook worker test + # def test_webhooks_worker(self): + # + # request_id = uuid.uuid4() + # + # def dummy_send(_, request, **kwargs): + # """ + # A dummy implementation of Session.send() to be used for testing. + # Always returns a 200 HTTP response. + # """ + # webhook = Webhook.objects.get(type_create=True) + # signature = generate_signature(request.body, webhook.secret) + # + # # Validate the outgoing request headers + # self.assertEqual(request.headers['Content-Type'], webhook.http_content_type) + # self.assertEqual(request.headers['X-Hook-Signature'], signature) + # self.assertEqual(request.headers['X-Foo'], 'Bar') + # + # # Validate the outgoing request body + # body = json.loads(request.body) + # self.assertEqual(body['event'], 'created') + # self.assertEqual(body['timestamp'], job.kwargs['timestamp']) + # self.assertEqual(body['model'], 'site') + # self.assertEqual(body['username'], 'testuser') + # self.assertEqual(body['request_id'], str(request_id)) + # self.assertEqual(body['data']['name'], 'Site 1') + # + # return HttpResponse() + # + # # Enqueue a webhook for processing + # site = Site.objects.create(name='Site 1', slug='site-1') + # enqueue_webhooks( + # queue=[], + # instance=site, + # user=self.user, + # request_id=request_id, + # action=ObjectChangeActionChoices.ACTION_CREATE + # ) + # + # # Retrieve the job from queue + # job = self.queue.jobs[0] + # + # # Patch the Session object with our dummy_send() method, then process the webhook for sending + # with patch.object(Session, 'send', dummy_send) as mock_send: + # process_webhook(**job.kwargs) diff --git a/netbox/extras/webhooks.py b/netbox/extras/webhooks.py index bd645aca9..516f2f19d 100644 --- a/netbox/extras/webhooks.py +++ b/netbox/extras/webhooks.py @@ -12,6 +12,19 @@ from .models import Webhook from .registry import registry +def serialize_for_webhook(instance): + """ + Return a serialized representation of the given instance suitable for use in a webhook. + """ + serializer_class = get_serializer_for_model(instance.__class__) + serializer_context = { + 'request': None, + } + serializer = serializer_class(instance, context=serializer_context) + + return serializer.data + + def generate_signature(request_body, secret): """ Return a cryptographic signature that can be used to verify the authenticity of webhook data. @@ -24,10 +37,10 @@ def generate_signature(request_body, secret): return hmac_prep.hexdigest() -def enqueue_webhooks(instance, user, request_id, action): +def enqueue_object(queue, instance, user, request_id, action): """ - Find Webhook(s) assigned to this instance + action and enqueue them - to be processed + Enqueue a serialized representation of a created/updated/deleted object for the processing of + webhooks once the request has completed. """ # Determine whether this type of object supports webhooks app_label = instance._meta.app_label @@ -35,41 +48,50 @@ def enqueue_webhooks(instance, user, request_id, action): if model_name not in registry['model_features']['webhooks'].get(app_label, []): return - # Retrieve any applicable Webhooks - content_type = ContentType.objects.get_for_model(instance) - action_flag = { - ObjectChangeActionChoices.ACTION_CREATE: 'type_create', - ObjectChangeActionChoices.ACTION_UPDATE: 'type_update', - ObjectChangeActionChoices.ACTION_DELETE: 'type_delete', - }[action] - webhooks = Webhook.objects.filter(content_types=content_type, enabled=True, **{action_flag: True}) + # Gather pre- and post-change snapshots + snapshots = { + 'prechange': getattr(instance, '_prechange_snapshot', None), + 'postchange': serialize_object(instance) if action != ObjectChangeActionChoices.ACTION_DELETE else None, + } - if webhooks.exists(): + queue.append({ + 'content_type': ContentType.objects.get_for_model(instance), + 'object_id': instance.pk, + 'event': action, + 'data': serialize_for_webhook(instance), + 'snapshots': snapshots, + 'username': user.username, + 'request_id': request_id + }) - # Get the Model's API serializer class and serialize the object - serializer_class = get_serializer_for_model(instance.__class__) - serializer_context = { - 'request': None, - } - serializer = serializer_class(instance, context=serializer_context) - # Gather pre- and post-change snapshots - snapshots = { - 'prechange': getattr(instance, '_prechange_snapshot', None), - 'postchange': serialize_object(instance) if action != ObjectChangeActionChoices.ACTION_DELETE else None, - } +def flush_webhooks(queue): + """ + Flush a list of object representation to RQ for webhook processing. + """ + rq_queue = get_queue('default') + + for data in queue: + + # Collect Webhooks that apply for this object and action + content_type = data['content_type'] + action_flag = { + ObjectChangeActionChoices.ACTION_CREATE: 'type_create', + ObjectChangeActionChoices.ACTION_UPDATE: 'type_update', + ObjectChangeActionChoices.ACTION_DELETE: 'type_delete', + }[data['event']] + # TODO: Cache these so we're not calling multiple times for bulk operations + webhooks = Webhook.objects.filter(content_types=content_type, enabled=True, **{action_flag: True}) - # Enqueue the webhooks - webhook_queue = get_queue('default') for webhook in webhooks: - webhook_queue.enqueue( + rq_queue.enqueue( "extras.webhooks_worker.process_webhook", webhook=webhook, - model_name=instance._meta.model_name, - event=action, - data=serializer.data, - snapshots=snapshots, + model_name=content_type.model, + event=data['event'], + data=data['data'], + snapshots=data['snapshots'], timestamp=str(timezone.now()), - username=user.username, - request_id=request_id + username=data['username'], + request_id=data['request_id'] ) From c88dcef900e2e7629e64412e06cf499cf3544495 Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Tue, 1 Jun 2021 09:04:01 -0400 Subject: [PATCH 2/6] Extend webhook create/update/delete tests --- netbox/extras/signals.py | 1 + netbox/extras/tests/test_webhooks.py | 39 +++++++++++++++++++++------- 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/netbox/extras/signals.py b/netbox/extras/signals.py index d53985621..d996c2f4c 100644 --- a/netbox/extras/signals.py +++ b/netbox/extras/signals.py @@ -60,6 +60,7 @@ def _handle_changed_object(request, webhook_queue, sender, instance, **kwargs): if m2m_changed and webhook_queue: # TODO: Need more validation here # TODO: Need to account for snapshot changes + instance.refresh_from_db() # Ensure that we're working with fresh M2M assignments webhook_queue[-1]['data'] = serialize_for_webhook(instance) else: enqueue_object(webhook_queue, instance, request.user, request.id, action) diff --git a/netbox/extras/tests/test_webhooks.py b/netbox/extras/tests/test_webhooks.py index 7f1242811..60b8d7761 100644 --- a/netbox/extras/tests/test_webhooks.py +++ b/netbox/extras/tests/test_webhooks.py @@ -11,7 +11,7 @@ from rest_framework import status from dcim.models import Site from extras.choices import ObjectChangeActionChoices -from extras.models import Webhook +from extras.models import Tag, Webhook from extras.webhooks import enqueue_object, generate_signature from extras.webhooks_worker import process_webhook from utilities.testing import APITestCase @@ -20,11 +20,10 @@ from utilities.testing import APITestCase class WebhookTest(APITestCase): def setUp(self): - super().setUp() self.queue = django_rq.get_queue('default') - self.queue.empty() # Begin each test with an empty queue + self.queue.empty() @classmethod def setUpTestData(cls): @@ -34,38 +33,55 @@ class WebhookTest(APITestCase): DUMMY_SECRET = "LOOKATMEIMASECRETSTRING" webhooks = Webhook.objects.bulk_create(( - Webhook(name='Site Create Webhook', type_create=True, payload_url=DUMMY_URL, secret=DUMMY_SECRET, additional_headers='X-Foo: Bar'), - Webhook(name='Site Update Webhook', type_update=True, payload_url=DUMMY_URL, secret=DUMMY_SECRET), - Webhook(name='Site Delete Webhook', type_delete=True, payload_url=DUMMY_URL, secret=DUMMY_SECRET), + Webhook(name='Webhook 1', type_create=True, payload_url=DUMMY_URL, secret=DUMMY_SECRET, additional_headers='X-Foo: Bar'), + Webhook(name='Webhook 2', type_update=True, payload_url=DUMMY_URL, secret=DUMMY_SECRET), + Webhook(name='Webhook 3', type_delete=True, payload_url=DUMMY_URL, secret=DUMMY_SECRET), )) for webhook in webhooks: webhook.content_types.set([site_ct]) + Tag.objects.bulk_create(( + Tag(name='Foo', slug='foo'), + Tag(name='Bar', slug='bar'), + Tag(name='Baz', slug='baz'), + )) + def test_enqueue_webhook_create(self): # Create an object via the REST API data = { - 'name': 'Test Site', - 'slug': 'test-site', + 'name': 'Site 1', + 'slug': 'site-1', + 'tags': [ + {'name': 'Foo'}, + {'name': 'Bar'}, + ] } url = reverse('dcim-api:site-list') self.add_permissions('dcim.add_site') response = self.client.post(url, data, format='json', **self.header) self.assertHttpStatus(response, status.HTTP_201_CREATED) self.assertEqual(Site.objects.count(), 1) + self.assertEqual(Site.objects.first().tags.count(), 2) # Verify that a job was queued for the object creation webhook self.assertEqual(self.queue.count, 1) job = self.queue.jobs[0] self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_create=True)) self.assertEqual(job.kwargs['data']['id'], response.data['id']) + self.assertEqual(len(job.kwargs['data']['tags']), len(response.data['tags'])) self.assertEqual(job.kwargs['model_name'], 'site') self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_CREATE) def test_enqueue_webhook_update(self): - # Update an object via the REST API site = Site.objects.create(name='Site 1', slug='site-1') + site.tags.set(*Tag.objects.filter(name__in=['Foo', 'Bar'])) + + # Update an object via the REST API data = { 'comments': 'Updated the site', + 'tags': [ + {'name': 'Baz'} + ] } url = reverse('dcim-api:site-detail', kwargs={'pk': site.pk}) self.add_permissions('dcim.change_site') @@ -77,12 +93,15 @@ class WebhookTest(APITestCase): job = self.queue.jobs[0] self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_update=True)) self.assertEqual(job.kwargs['data']['id'], site.pk) + self.assertEqual(len(job.kwargs['data']['tags']), len(response.data['tags'])) self.assertEqual(job.kwargs['model_name'], 'site') self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_UPDATE) def test_enqueue_webhook_delete(self): - # Delete an object via the REST API site = Site.objects.create(name='Site 1', slug='site-1') + site.tags.set(*Tag.objects.filter(name__in=['Foo', 'Bar'])) + + # Delete an object via the REST API url = reverse('dcim-api:site-detail', kwargs={'pk': site.pk}) self.add_permissions('dcim.delete_site') response = self.client.delete(url, **self.header) From ba3ca6b00d8a1fddc9fa1d26a7dab64b708d1487 Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Tue, 1 Jun 2021 09:30:54 -0400 Subject: [PATCH 3/6] Update post-change snapshot for M2M changes --- netbox/extras/signals.py | 14 ++++++++++---- netbox/extras/tests/test_webhooks.py | 21 +++++++++++++++------ netbox/extras/webhooks.py | 15 ++++++++------- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/netbox/extras/signals.py b/netbox/extras/signals.py index d996c2f4c..2fc292294 100644 --- a/netbox/extras/signals.py +++ b/netbox/extras/signals.py @@ -12,7 +12,7 @@ from prometheus_client import Counter from .choices import ObjectChangeActionChoices from .models import CustomField, ObjectChange -from .webhooks import enqueue_object, serialize_for_webhook +from .webhooks import enqueue_object, get_snapshots, serialize_for_webhook # @@ -23,6 +23,13 @@ def _handle_changed_object(request, webhook_queue, sender, instance, **kwargs): """ Fires when an object is created or updated. """ + def is_same_object(instance, webhook_data): + return ( + ContentType.objects.get_for_model(instance) == webhook_data['content_type'] and + instance.pk == webhook_data['object_id'] and + request.id == webhook_data['request_id'] + ) + if not hasattr(instance, 'to_objectchange'): return @@ -57,11 +64,10 @@ def _handle_changed_object(request, webhook_queue, sender, instance, **kwargs): objectchange.save() # If this is an M2M change, update the previously queued webhook (from post_save) - if m2m_changed and webhook_queue: - # TODO: Need more validation here - # TODO: Need to account for snapshot changes + if m2m_changed and webhook_queue and is_same_object(instance, webhook_queue[-1]): instance.refresh_from_db() # Ensure that we're working with fresh M2M assignments webhook_queue[-1]['data'] = serialize_for_webhook(instance) + webhook_queue[-1]['snapshots']['postchange'] = get_snapshots(instance, action)['postchange'] else: enqueue_object(webhook_queue, instance, request.user, request.id, action) diff --git a/netbox/extras/tests/test_webhooks.py b/netbox/extras/tests/test_webhooks.py index 60b8d7761..d653d864f 100644 --- a/netbox/extras/tests/test_webhooks.py +++ b/netbox/extras/tests/test_webhooks.py @@ -67,10 +67,12 @@ class WebhookTest(APITestCase): self.assertEqual(self.queue.count, 1) job = self.queue.jobs[0] self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_create=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(job.kwargs['model_name'], 'site') self.assertEqual(job.kwargs['data']['id'], response.data['id']) self.assertEqual(len(job.kwargs['data']['tags']), len(response.data['tags'])) - self.assertEqual(job.kwargs['model_name'], 'site') - self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'Site 1') + self.assertEqual(job.kwargs['snapshots']['postchange']['tags'], ['Bar', 'Foo']) def test_enqueue_webhook_update(self): site = Site.objects.create(name='Site 1', slug='site-1') @@ -78,6 +80,7 @@ class WebhookTest(APITestCase): # Update an object via the REST API data = { + 'name': 'Site X', 'comments': 'Updated the site', 'tags': [ {'name': 'Baz'} @@ -92,10 +95,14 @@ class WebhookTest(APITestCase): self.assertEqual(self.queue.count, 1) job = self.queue.jobs[0] self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_update=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_UPDATE) + self.assertEqual(job.kwargs['model_name'], 'site') self.assertEqual(job.kwargs['data']['id'], site.pk) self.assertEqual(len(job.kwargs['data']['tags']), len(response.data['tags'])) - self.assertEqual(job.kwargs['model_name'], 'site') - self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_UPDATE) + self.assertEqual(job.kwargs['snapshots']['prechange']['name'], 'Site 1') + self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'Site X') + self.assertEqual(job.kwargs['snapshots']['postchange']['tags'], ['Baz']) def test_enqueue_webhook_delete(self): site = Site.objects.create(name='Site 1', slug='site-1') @@ -111,9 +118,11 @@ class WebhookTest(APITestCase): self.assertEqual(self.queue.count, 1) job = self.queue.jobs[0] self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_delete=True)) - self.assertEqual(job.kwargs['data']['id'], site.pk) - self.assertEqual(job.kwargs['model_name'], 'site') self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_DELETE) + self.assertEqual(job.kwargs['model_name'], 'site') + self.assertEqual(job.kwargs['data']['id'], site.pk) + self.assertEqual(job.kwargs['snapshots']['prechange']['name'], 'Site 1') + self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) # TODO: Replace webhook worker test # def test_webhooks_worker(self): diff --git a/netbox/extras/webhooks.py b/netbox/extras/webhooks.py index 516f2f19d..6c4598247 100644 --- a/netbox/extras/webhooks.py +++ b/netbox/extras/webhooks.py @@ -25,6 +25,13 @@ def serialize_for_webhook(instance): return serializer.data +def get_snapshots(instance, action): + return { + 'prechange': getattr(instance, '_prechange_snapshot', None), + 'postchange': serialize_object(instance) if action != ObjectChangeActionChoices.ACTION_DELETE else None, + } + + def generate_signature(request_body, secret): """ Return a cryptographic signature that can be used to verify the authenticity of webhook data. @@ -48,18 +55,12 @@ def enqueue_object(queue, instance, user, request_id, action): if model_name not in registry['model_features']['webhooks'].get(app_label, []): return - # Gather pre- and post-change snapshots - snapshots = { - 'prechange': getattr(instance, '_prechange_snapshot', None), - 'postchange': serialize_object(instance) if action != ObjectChangeActionChoices.ACTION_DELETE else None, - } - queue.append({ 'content_type': ContentType.objects.get_for_model(instance), 'object_id': instance.pk, 'event': action, 'data': serialize_for_webhook(instance), - 'snapshots': snapshots, + 'snapshots': get_snapshots(instance, action), 'username': user.username, 'request_id': request_id }) From be3cd2a4347e228511ed34054ad0513cf143d2c0 Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Tue, 1 Jun 2021 09:50:38 -0400 Subject: [PATCH 4/6] Add bulk operation tests for webhooks --- netbox/extras/tests/test_webhooks.py | 127 +++++++++++++++++++++++++++ 1 file changed, 127 insertions(+) diff --git a/netbox/extras/tests/test_webhooks.py b/netbox/extras/tests/test_webhooks.py index d653d864f..e720870cd 100644 --- a/netbox/extras/tests/test_webhooks.py +++ b/netbox/extras/tests/test_webhooks.py @@ -74,6 +74,52 @@ class WebhookTest(APITestCase): self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'Site 1') self.assertEqual(job.kwargs['snapshots']['postchange']['tags'], ['Bar', 'Foo']) + def test_enqueue_webhook_bulk_create(self): + # Create multiple objects via the REST API + data = [ + { + 'name': 'Site 1', + 'slug': 'site-1', + 'tags': [ + {'name': 'Foo'}, + {'name': 'Bar'}, + ] + }, + { + 'name': 'Site 2', + 'slug': 'site-2', + 'tags': [ + {'name': 'Foo'}, + {'name': 'Bar'}, + ] + }, + { + 'name': 'Site 3', + 'slug': 'site-3', + 'tags': [ + {'name': 'Foo'}, + {'name': 'Bar'}, + ] + }, + ] + url = reverse('dcim-api:site-list') + self.add_permissions('dcim.add_site') + response = self.client.post(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_201_CREATED) + self.assertEqual(Site.objects.count(), 3) + self.assertEqual(Site.objects.first().tags.count(), 2) + + # Verify that a webhook was queued for each object + self.assertEqual(self.queue.count, 3) + for i, job in enumerate(self.queue.jobs): + self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_create=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_CREATE) + self.assertEqual(job.kwargs['model_name'], 'site') + self.assertEqual(job.kwargs['data']['id'], response.data[i]['id']) + self.assertEqual(len(job.kwargs['data']['tags']), len(response.data[i]['tags'])) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], response.data[i]['name']) + self.assertEqual(job.kwargs['snapshots']['postchange']['tags'], ['Bar', 'Foo']) + def test_enqueue_webhook_update(self): site = Site.objects.create(name='Site 1', slug='site-1') site.tags.set(*Tag.objects.filter(name__in=['Foo', 'Bar'])) @@ -104,6 +150,58 @@ class WebhookTest(APITestCase): self.assertEqual(job.kwargs['snapshots']['postchange']['name'], 'Site X') self.assertEqual(job.kwargs['snapshots']['postchange']['tags'], ['Baz']) + def test_enqueue_webhook_bulk_update(self): + sites = ( + Site(name='Site 1', slug='site-1'), + Site(name='Site 2', slug='site-2'), + Site(name='Site 3', slug='site-3'), + ) + Site.objects.bulk_create(sites) + for site in sites: + site.tags.set(*Tag.objects.filter(name__in=['Foo', 'Bar'])) + + # Update three objects via the REST API + data = [ + { + 'id': sites[0].pk, + 'name': 'Site X', + 'tags': [ + {'name': 'Baz'} + ] + }, + { + 'id': sites[1].pk, + 'name': 'Site Y', + 'tags': [ + {'name': 'Baz'} + ] + }, + { + 'id': sites[2].pk, + 'name': 'Site Z', + 'tags': [ + {'name': 'Baz'} + ] + }, + ] + url = reverse('dcim-api:site-list') + self.add_permissions('dcim.change_site') + response = self.client.patch(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + + # Verify that a job was queued for the object update webhook + self.assertEqual(self.queue.count, 3) + for i, job in enumerate(self.queue.jobs): + self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_update=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_UPDATE) + self.assertEqual(job.kwargs['model_name'], 'site') + self.assertEqual(job.kwargs['data']['id'], data[i]['id']) + self.assertEqual(len(job.kwargs['data']['tags']), len(response.data[i]['tags'])) + self.assertEqual(job.kwargs['snapshots']['prechange']['name'], sites[i].name) + self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) + self.assertEqual(job.kwargs['snapshots']['postchange']['name'], response.data[i]['name']) + self.assertEqual(job.kwargs['snapshots']['postchange']['tags'], ['Baz']) + def test_enqueue_webhook_delete(self): site = Site.objects.create(name='Site 1', slug='site-1') site.tags.set(*Tag.objects.filter(name__in=['Foo', 'Bar'])) @@ -124,6 +222,35 @@ class WebhookTest(APITestCase): self.assertEqual(job.kwargs['snapshots']['prechange']['name'], 'Site 1') self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) + def test_enqueue_webhook_bulk_delete(self): + sites = ( + Site(name='Site 1', slug='site-1'), + Site(name='Site 2', slug='site-2'), + Site(name='Site 3', slug='site-3'), + ) + Site.objects.bulk_create(sites) + for site in sites: + site.tags.set(*Tag.objects.filter(name__in=['Foo', 'Bar'])) + + # Delete three objects via the REST API + data = [ + {'id': site.pk} for site in sites + ] + url = reverse('dcim-api:site-list') + self.add_permissions('dcim.delete_site') + response = self.client.delete(url, data, format='json', **self.header) + self.assertHttpStatus(response, status.HTTP_204_NO_CONTENT) + + # Verify that a job was queued for the object update webhook + self.assertEqual(self.queue.count, 3) + for i, job in enumerate(self.queue.jobs): + self.assertEqual(job.kwargs['webhook'], Webhook.objects.get(type_delete=True)) + self.assertEqual(job.kwargs['event'], ObjectChangeActionChoices.ACTION_DELETE) + self.assertEqual(job.kwargs['model_name'], 'site') + self.assertEqual(job.kwargs['data']['id'], sites[i].pk) + self.assertEqual(job.kwargs['snapshots']['prechange']['name'], sites[i].name) + self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) + # TODO: Replace webhook worker test # def test_webhooks_worker(self): # From 32cbc201088910a774cbc9a04344937a91299a65 Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Tue, 1 Jun 2021 12:52:25 -0400 Subject: [PATCH 5/6] Restore webhooks worker test --- netbox/extras/tests/test_webhooks.py | 93 ++++++++++++++-------------- 1 file changed, 47 insertions(+), 46 deletions(-) diff --git a/netbox/extras/tests/test_webhooks.py b/netbox/extras/tests/test_webhooks.py index e720870cd..57db6dd02 100644 --- a/netbox/extras/tests/test_webhooks.py +++ b/netbox/extras/tests/test_webhooks.py @@ -12,7 +12,7 @@ from rest_framework import status from dcim.models import Site from extras.choices import ObjectChangeActionChoices from extras.models import Tag, Webhook -from extras.webhooks import enqueue_object, generate_signature +from extras.webhooks import enqueue_object, flush_webhooks, generate_signature from extras.webhooks_worker import process_webhook from utilities.testing import APITestCase @@ -251,48 +251,49 @@ class WebhookTest(APITestCase): self.assertEqual(job.kwargs['snapshots']['prechange']['name'], sites[i].name) self.assertEqual(job.kwargs['snapshots']['prechange']['tags'], ['Bar', 'Foo']) - # TODO: Replace webhook worker test - # def test_webhooks_worker(self): - # - # request_id = uuid.uuid4() - # - # def dummy_send(_, request, **kwargs): - # """ - # A dummy implementation of Session.send() to be used for testing. - # Always returns a 200 HTTP response. - # """ - # webhook = Webhook.objects.get(type_create=True) - # signature = generate_signature(request.body, webhook.secret) - # - # # Validate the outgoing request headers - # self.assertEqual(request.headers['Content-Type'], webhook.http_content_type) - # self.assertEqual(request.headers['X-Hook-Signature'], signature) - # self.assertEqual(request.headers['X-Foo'], 'Bar') - # - # # Validate the outgoing request body - # body = json.loads(request.body) - # self.assertEqual(body['event'], 'created') - # self.assertEqual(body['timestamp'], job.kwargs['timestamp']) - # self.assertEqual(body['model'], 'site') - # self.assertEqual(body['username'], 'testuser') - # self.assertEqual(body['request_id'], str(request_id)) - # self.assertEqual(body['data']['name'], 'Site 1') - # - # return HttpResponse() - # - # # Enqueue a webhook for processing - # site = Site.objects.create(name='Site 1', slug='site-1') - # enqueue_webhooks( - # queue=[], - # instance=site, - # user=self.user, - # request_id=request_id, - # action=ObjectChangeActionChoices.ACTION_CREATE - # ) - # - # # Retrieve the job from queue - # job = self.queue.jobs[0] - # - # # Patch the Session object with our dummy_send() method, then process the webhook for sending - # with patch.object(Session, 'send', dummy_send) as mock_send: - # process_webhook(**job.kwargs) + def test_webhooks_worker(self): + + request_id = uuid.uuid4() + + def dummy_send(_, request, **kwargs): + """ + A dummy implementation of Session.send() to be used for testing. + Always returns a 200 HTTP response. + """ + webhook = Webhook.objects.get(type_create=True) + signature = generate_signature(request.body, webhook.secret) + + # Validate the outgoing request headers + self.assertEqual(request.headers['Content-Type'], webhook.http_content_type) + self.assertEqual(request.headers['X-Hook-Signature'], signature) + self.assertEqual(request.headers['X-Foo'], 'Bar') + + # Validate the outgoing request body + body = json.loads(request.body) + self.assertEqual(body['event'], 'created') + self.assertEqual(body['timestamp'], job.kwargs['timestamp']) + self.assertEqual(body['model'], 'site') + self.assertEqual(body['username'], 'testuser') + self.assertEqual(body['request_id'], str(request_id)) + self.assertEqual(body['data']['name'], 'Site 1') + + return HttpResponse() + + # Enqueue a webhook for processing + webhooks_queue = [] + site = Site.objects.create(name='Site 1', slug='site-1') + enqueue_object( + webhooks_queue, + instance=site, + user=self.user, + request_id=request_id, + action=ObjectChangeActionChoices.ACTION_CREATE + ) + flush_webhooks(webhooks_queue) + + # Retrieve the job from queue + job = self.queue.jobs[0] + + # Patch the Session object with our dummy_send() method, then process the webhook for sending + with patch.object(Session, 'send', dummy_send) as mock_send: + process_webhook(**job.kwargs) From 8afb7d654db835ed451d7a75d54b68da5a3d9b86 Mon Sep 17 00:00:00 2001 From: jeremystretch Date: Tue, 1 Jun 2021 12:57:31 -0400 Subject: [PATCH 6/6] Changelog for #6284 --- docs/release-notes/version-2.11.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/release-notes/version-2.11.md b/docs/release-notes/version-2.11.md index 2d5ff3b35..fa472cf8f 100644 --- a/docs/release-notes/version-2.11.md +++ b/docs/release-notes/version-2.11.md @@ -11,6 +11,7 @@ ### Bug Fixes * [#6064](https://github.com/netbox-community/netbox/issues/6064) - Fix object permission assignments for user and group models +* [#6284](https://github.com/netbox-community/netbox/issues/6284) - Avoid sending redundant webhooks when adding/removing tags * [#6496](https://github.com/netbox-community/netbox/issues/6496) - Fix upgrade script when Python installed in nonstandard path * [#6502](https://github.com/netbox-community/netbox/issues/6502) - Correct permissions evaluation for running a report via the REST API