From ec32ce7dfc86ccb350f2a9e1af8c3f85f6c154b7 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 2 Feb 2024 15:36:53 +0000 Subject: [PATCH] fix(webhooks): prevent unnecessary organisation webhook tasks (#3365) --- api/audit/signals.py | 24 ++++-- .../unit/audit/test_unit_audit_models.py | 15 +++- .../unit/audit/test_unit_audit_signals.py | 83 +++++++++++++++++++ 3 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 api/tests/unit/audit/test_unit_audit_signals.py diff --git a/api/audit/signals.py b/api/audit/signals.py index 36e45753043d..19591bea7be4 100644 --- a/api/audit/signals.py +++ b/api/audit/signals.py @@ -1,5 +1,6 @@ import logging +from django.conf import settings from django.db.models.signals import post_save from django.dispatch import receiver @@ -9,6 +10,7 @@ from integrations.dynatrace.dynatrace import DynatraceWrapper from integrations.new_relic.new_relic import NewRelicWrapper from integrations.slack.slack import SlackWrapper +from organisations.models import OrganisationWebhook from webhooks.webhooks import WebhookEventType, call_organisation_webhooks logger = logging.getLogger(__name__) @@ -16,21 +18,27 @@ @receiver(post_save, sender=AuditLog) def call_webhooks(sender, instance, **kwargs): - data = AuditLogListSerializer(instance=instance).data + if settings.DISABLE_WEBHOOKS: + return - if not (instance.project or instance.environment): + if not (instance.project_id or instance.environment_id): logger.warning("Audit log without project or environment. Not sending webhook.") return - organisation = ( - instance.project.organisation + organisation_id = ( + instance.project.organisation_id if instance.project - else instance.environment.project.organisation - ) - call_organisation_webhooks.delay( - args=(organisation.id, data, WebhookEventType.AUDIT_LOG_CREATED.value) + else instance.environment.project.organisation_id ) + if OrganisationWebhook.objects.filter( + organisation_id=organisation_id, enabled=True + ).exists(): + data = AuditLogListSerializer(instance=instance).data + call_organisation_webhooks.delay( + args=(organisation_id, data, WebhookEventType.AUDIT_LOG_CREATED.value) + ) + def _get_integration_config(instance, integration_name): if hasattr(instance.project, integration_name): diff --git a/api/tests/unit/audit/test_unit_audit_models.py b/api/tests/unit/audit/test_unit_audit_models.py index 4e0e17039262..a6b49229b8f7 100644 --- a/api/tests/unit/audit/test_unit_audit_models.py +++ b/api/tests/unit/audit/test_unit_audit_models.py @@ -1,16 +1,29 @@ +from pytest_mock import MockerFixture + from audit.models import AuditLog from audit.related_object_type import RelatedObjectType from audit.serializers import AuditLogListSerializer from integrations.datadog.models import DataDogConfiguration +from organisations.models import Organisation, OrganisationWebhook +from projects.models import Project from webhooks.webhooks import WebhookEventType -def test_organisation_webhooks_are_called_when_audit_log_saved(project, mocker): +def test_organisation_webhooks_are_called_when_audit_log_saved( + project: Project, mocker: MockerFixture, organisation: Organisation +) -> None: # Given mock_call_webhooks = mocker.patch("audit.signals.call_organisation_webhooks") audit_log = AuditLog(project=project, log="Some audit log") + OrganisationWebhook.objects.create( + organisation=organisation, + url="http://example.com/webhook", + enabled=True, + name="example webhook", + ) + # When audit_log.save() diff --git a/api/tests/unit/audit/test_unit_audit_signals.py b/api/tests/unit/audit/test_unit_audit_signals.py new file mode 100644 index 000000000000..3fd924141635 --- /dev/null +++ b/api/tests/unit/audit/test_unit_audit_signals.py @@ -0,0 +1,83 @@ +from pytest_django.fixtures import SettingsWrapper +from pytest_mock import MockerFixture + +from audit.models import AuditLog +from audit.signals import call_webhooks +from organisations.models import Organisation, OrganisationWebhook +from projects.models import Project +from webhooks.webhooks import WebhookEventType + + +def test_call_webhooks_does_not_create_task_if_webhooks_disabled( + organisation: Organisation, + project: Project, + settings: SettingsWrapper, + mocker: MockerFixture, +) -> None: + # Given + audit_log = AuditLog(project=project) + settings.DISABLE_WEBHOOKS = True + + mocked_call_organisation_webhooks = mocker.patch( + "audit.signals.call_organisation_webhooks" + ) + + # When + call_webhooks(sender=AuditLog, instance=audit_log) + + # Then + mocked_call_organisation_webhooks.delay.assert_not_called() + + +def test_call_webhooks_does_not_create_task_if_organisation_has_no_webhooks( + organisation: Organisation, + project: Project, + settings: SettingsWrapper, + mocker: MockerFixture, +) -> None: + # Given + audit_log = AuditLog(project=project) + settings.DISABLE_WEBHOOKS = False + + assert organisation.webhooks.count() == 0 + + mocked_call_organisation_webhooks = mocker.patch( + "audit.signals.call_organisation_webhooks" + ) + + # When + call_webhooks(sender=AuditLog, instance=audit_log) + + # Then + mocked_call_organisation_webhooks.delay.assert_not_called() + + +def test_call_webhooks_creates_task_if_organisation_has_webhooks( + organisation: Organisation, + project: Project, + settings: SettingsWrapper, + mocker: MockerFixture, +) -> None: + # Given + audit_log = AuditLog.objects.create(project=project) + OrganisationWebhook.objects.create( + organisation=organisation, + name="Example webhook", + url="http://example.com/webhook", + enabled=True, + ) + settings.DISABLE_WEBHOOKS = False + + mocked_call_organisation_webhooks = mocker.patch( + "audit.signals.call_organisation_webhooks" + ) + + # When + call_webhooks(sender=AuditLog, instance=audit_log) + + # Then + mocked_call_organisation_webhooks.delay.assert_called_once() + mock_call = mocked_call_organisation_webhooks.delay.call_args.kwargs + assert mock_call["args"][0] == organisation.id + assert mock_call["args"][1]["id"] == audit_log.id + assert mock_call["args"][2] == WebhookEventType.AUDIT_LOG_CREATED.name