From c665063fd22b7cf740eb5b27981c8633a577c470 Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Mon, 8 Jan 2024 13:57:55 +0000 Subject: [PATCH] feat(audit): add change details to AuditLog (#3218) --- api/audit/models.py | 4 +- api/audit/serializers.py | 46 ++++- api/audit/signals.py | 4 +- api/audit/views.py | 16 +- api/core/models.py | 30 +++- api/environments/models.py | 6 +- .../integration/audit/test_audit_logs.py | 161 ++++++++++++++++++ api/tests/integration/conftest.py | 16 +- .../unit/audit/test_unit_audit_models.py | 10 +- 9 files changed, 274 insertions(+), 19 deletions(-) diff --git a/api/audit/models.py b/api/audit/models.py index 0aaa68bb5638..379124ea75d4 100644 --- a/api/audit/models.py +++ b/api/audit/models.py @@ -79,9 +79,9 @@ def environment_document_updated(self) -> bool: return "send_environments_to_dynamodb" not in skip_signals_and_hooks @property - def history_record(self): + def history_record(self) -> typing.Optional[Model]: klass = self.get_history_record_model_class(self.history_record_class_path) - return klass.objects.get(id=self.history_record_id) + return klass.objects.filter(history_id=self.history_record_id).first() @property def environment_name(self) -> str: diff --git a/api/audit/serializers.py b/api/audit/serializers.py index a936a137fe1e..feccc2a76e79 100644 --- a/api/audit/serializers.py +++ b/api/audit/serializers.py @@ -1,3 +1,6 @@ +import typing + +from drf_yasg.utils import swagger_serializer_method from rest_framework import serializers from audit.models import AuditLog @@ -6,7 +9,7 @@ from users.serializers import UserListSerializer -class AuditLogSerializer(serializers.ModelSerializer): +class AuditLogListSerializer(serializers.ModelSerializer): author = UserListSerializer() environment = EnvironmentSerializerLight() project = ProjectListSerializer() @@ -26,6 +29,47 @@ class Meta: ) +class AuditLogChangeDetailsSerializer(serializers.Serializer): + field = serializers.ReadOnlyField() + old = serializers.ReadOnlyField() + new = serializers.ReadOnlyField() + + +class AuditLogRetrieveSerializer(serializers.ModelSerializer): + author = UserListSerializer() + environment = EnvironmentSerializerLight() + project = ProjectListSerializer() + change_details = serializers.SerializerMethodField() + + class Meta: + model = AuditLog + fields = ( + "id", + "created_date", + "log", + "author", + "environment", + "project", + "related_object_id", + "related_object_type", + "is_system_event", + "change_details", + ) + + @swagger_serializer_method( + serializer_or_field=AuditLogChangeDetailsSerializer(many=True) + ) + def get_change_details( + self, instance: AuditLog + ) -> typing.List[typing.Dict[str, typing.Any]]: + if history_record := instance.history_record: + return AuditLogChangeDetailsSerializer( + instance=history_record.get_change_details(), many=True + ).data + + return [] + + class AuditLogsQueryParamSerializer(serializers.Serializer): project = serializers.IntegerField(required=False) environments = serializers.ListField( diff --git a/api/audit/signals.py b/api/audit/signals.py index a0cb322ce80a..36e45753043d 100644 --- a/api/audit/signals.py +++ b/api/audit/signals.py @@ -4,7 +4,7 @@ from django.dispatch import receiver from audit.models import AuditLog, RelatedObjectType -from audit.serializers import AuditLogSerializer +from audit.serializers import AuditLogListSerializer from integrations.datadog.datadog import DataDogWrapper from integrations.dynatrace.dynatrace import DynatraceWrapper from integrations.new_relic.new_relic import NewRelicWrapper @@ -16,7 +16,7 @@ @receiver(post_save, sender=AuditLog) def call_webhooks(sender, instance, **kwargs): - data = AuditLogSerializer(instance=instance).data + data = AuditLogListSerializer(instance=instance).data if not (instance.project or instance.environment): logger.warning("Audit log without project or environment. Not sending webhook.") diff --git a/api/audit/views.py b/api/audit/views.py index 07e68c511799..62fadf74d788 100644 --- a/api/audit/views.py +++ b/api/audit/views.py @@ -10,7 +10,11 @@ OrganisationAuditLogPermissions, ProjectAuditLogPermissions, ) -from audit.serializers import AuditLogSerializer, AuditLogsQueryParamSerializer +from audit.serializers import ( + AuditLogListSerializer, + AuditLogRetrieveSerializer, + AuditLogsQueryParamSerializer, +) from organisations.models import OrganisationRole @@ -18,8 +22,9 @@ name="list", decorator=swagger_auto_schema(query_serializer=AuditLogsQueryParamSerializer()), ) -class _BaseAuditLogViewSet(mixins.ListModelMixin, viewsets.GenericViewSet): - serializer_class = AuditLogSerializer +class _BaseAuditLogViewSet( + mixins.ListModelMixin, mixins.RetrieveModelMixin, viewsets.GenericViewSet +): pagination_class = CustomPagination def get_queryset(self) -> QuerySet[AuditLog]: @@ -46,6 +51,11 @@ def get_queryset(self) -> QuerySet[AuditLog]: def _get_base_filters(self) -> Q: return Q() + def get_serializer_class(self): + return {"retrieve": AuditLogRetrieveSerializer}.get( + self.action, AuditLogListSerializer + ) + class AllAuditLogViewSet(_BaseAuditLogViewSet): def _get_base_filters(self) -> Q: diff --git a/api/core/models.py b/api/core/models.py index a41f3fff638b..dc5dd00a5983 100644 --- a/api/core/models.py +++ b/api/core/models.py @@ -5,7 +5,7 @@ from django.db import models from django.db.models import Manager from django.http import HttpRequest -from simple_history.models import HistoricalRecords +from simple_history.models import HistoricalRecords, ModelChange from softdelete.models import SoftDeleteManager, SoftDeleteObject from audit.related_object_type import RelatedObjectType @@ -52,7 +52,7 @@ class Meta: abstract = True -class BaseHistoricalModel(models.Model): +class _BaseHistoricalModel(models.Model): include_in_audit = True master_api_key = models.ForeignKey( @@ -62,6 +62,29 @@ class BaseHistoricalModel(models.Model): class Meta: abstract = True + def get_change_details(self) -> typing.Optional[typing.List[ModelChange]]: + if not self.history_type == "~": + # we only return the change details for updates + return + + return [ + change + for change in self.diff_against(self.prev_record).changes + if change.field not in self._change_details_excluded_fields + ] + + +def base_historical_model_factory( + change_details_excluded_fields: typing.Sequence[str], +) -> typing.Type[_BaseHistoricalModel]: + class BaseHistoricalModel(_BaseHistoricalModel): + _change_details_excluded_fields = set(change_details_excluded_fields) + + class Meta: + abstract = True + + return BaseHistoricalModel + class _AbstractBaseAuditableModel(models.Model): """ @@ -145,10 +168,11 @@ def get_history_user( def abstract_base_auditable_model_factory( historical_records_excluded_fields: typing.List[str] = None, + change_details_excluded_fields: typing.Sequence[str] = None, ) -> typing.Type[_AbstractBaseAuditableModel]: class Base(_AbstractBaseAuditableModel): history = HistoricalRecords( - bases=[BaseHistoricalModel], + bases=[base_historical_model_factory(change_details_excluded_fields or [])], excluded_fields=historical_records_excluded_fields or [], get_user=get_history_user, inherit=True, diff --git a/api/environments/models.py b/api/environments/models.py index 6497aa3e4155..6b666f238ab5 100644 --- a/api/environments/models.py +++ b/api/environments/models.py @@ -65,7 +65,11 @@ class Environment( - LifecycleModel, abstract_base_auditable_model_factory(), SoftDeleteObject + LifecycleModel, + abstract_base_auditable_model_factory( + change_details_excluded_fields=["updated_at"] + ), + SoftDeleteObject, ): history_record_class_path = "environments.models.HistoricalEnvironment" related_object_type = RelatedObjectType.ENVIRONMENT diff --git a/api/tests/integration/audit/test_audit_logs.py b/api/tests/integration/audit/test_audit_logs.py index f5b1363a3f78..ac3913d6a051 100644 --- a/api/tests/integration/audit/test_audit_logs.py +++ b/api/tests/integration/audit/test_audit_logs.py @@ -1,5 +1,8 @@ +import json + from django.urls import reverse from rest_framework import status +from rest_framework.test import APIClient def test_audit_logs_only_makes_two_queries( @@ -17,3 +20,161 @@ def test_audit_logs_only_makes_two_queries( assert res.status_code == status.HTTP_200_OK assert res.json()["count"] == 3 + + +def test_retrieve_audit_log_for_environment_change( + admin_client: APIClient, + project: int, + environment_api_key: str, + environment_name: str, + environment: int, +) -> None: + # Given + # we update the environment + update_environment_url = reverse( + "api-v1:environments:environment-detail", + args=[environment_api_key], + ) + new_name = "some new name!" + data = {"name": new_name} + admin_client.patch( + update_environment_url, data=json.dumps(data), content_type="application/json" + ) + + # we list the audit log to get an audit record that was created when we update the feature above + list_audit_log_url = reverse("api-v1:audit-list") + list_response = admin_client.get(list_audit_log_url) + + # When + # We retrieve the record directly where we updated the environment + environment_update_result = next( + filter( + lambda r: r["log"].startswith("Environment updated"), + list_response.json()["results"], + ) + ) + retrieve_audit_log_url = reverse( + "api-v1:audit-detail", args=[environment_update_result["id"]] + ) + retrieve_response = admin_client.get(retrieve_audit_log_url) + retrieve_response_json = retrieve_response.json() + assert len(retrieve_response_json["change_details"]) == 1 + assert retrieve_response_json["change_details"][0]["field"] == "name" + assert retrieve_response_json["change_details"][0]["new"] == new_name + assert retrieve_response_json["change_details"][0]["old"] == environment_name + + +def test_retrieve_audit_log_for_feature_state_enabled_change( + admin_client: APIClient, + environment_api_key: str, + environment: int, + feature: int, + feature_state: int, +) -> None: + # Given + # we update the state and value of a feature in an environment + update_feature_state_url = reverse( + "api-v1:environments:environment-featurestates-detail", + args=[environment_api_key, feature_state], + ) + data = {"enabled": True} + admin_client.patch( + update_feature_state_url, data=json.dumps(data), content_type="application/json" + ) + + # we list the audit log to get an audit record that was created when we update the feature above + list_audit_log_url = reverse("api-v1:audit-list") + list_response = admin_client.get(list_audit_log_url) + list_results = list_response.json()["results"] + + # When + # We retrieve the records directly where we updated the feature state + flag_state_update_result = next( + filter( + lambda r: r["log"].startswith("Flag state updated"), + list_results, + ) + ) + retrieve_audit_log_url = reverse( + "api-v1:audit-detail", args=[flag_state_update_result["id"]] + ) + retrieve_response = admin_client.get(retrieve_audit_log_url) + + # Then + retrieve_response_json = retrieve_response.json() + assert len(retrieve_response_json["change_details"]) == 1 + assert retrieve_response_json["change_details"][0]["field"] == "enabled" + assert retrieve_response_json["change_details"][0]["new"] is True + assert retrieve_response_json["change_details"][0]["old"] is False + + +def test_retrieve_audit_log_for_feature_state_value_change( + admin_client: APIClient, + environment_api_key: str, + environment: int, + feature: int, + feature_state: int, + default_feature_value: str, +) -> None: + # Given + # we update the state and value of a feature in an environment + new_value = "foobar" + update_feature_state_url = reverse( + "api-v1:environments:environment-featurestates-detail", + args=[environment_api_key, feature_state], + ) + data = {"feature_state_value": new_value} + admin_client.patch( + update_feature_state_url, data=json.dumps(data), content_type="application/json" + ) + + # we list the audit log to get an audit record that was created when we update the feature above + list_audit_log_url = reverse("api-v1:audit-list") + list_response = admin_client.get(list_audit_log_url) + list_results = list_response.json()["results"] + + # When + # We retrieve the records directly where we updated the feature state + flag_state_update_result = next( + filter( + lambda r: r["log"].startswith("Remote config value updated"), + list_results, + ) + ) + retrieve_audit_log_url = reverse( + "api-v1:audit-detail", args=[flag_state_update_result["id"]] + ) + retrieve_response = admin_client.get(retrieve_audit_log_url) + + # Then + retrieve_response_json = retrieve_response.json() + assert len(retrieve_response_json["change_details"]) == 1 + assert retrieve_response_json["change_details"][0]["field"] == "string_value" + assert retrieve_response_json["change_details"][0]["new"] == new_value + assert retrieve_response_json["change_details"][0]["old"] == default_feature_value + + +def test_retrieve_audit_log_does_not_include_change_details_for_non_update( + admin_client: APIClient, project: int, environment: str +) -> None: + # Given + # we list the audit log to get an audit record that was created when + # the environment was created in the fixture + list_audit_log_url = reverse("api-v1:audit-list") + list_response = admin_client.get(list_audit_log_url) + + environment_update_result = next( + filter( + lambda r: r["log"].startswith("New Environment created"), + list_response.json()["results"], + ) + ) + + # When + retrieve_audit_log_url = reverse( + "api-v1:audit-detail", args=[environment_update_result["id"]] + ) + retrieve_response = admin_client.get(retrieve_audit_log_url) + + # Then + assert retrieve_response.json()["change_details"] == [] diff --git a/api/tests/integration/conftest.py b/api/tests/integration/conftest.py index 2db92a024d1f..7affdcda1965 100644 --- a/api/tests/integration/conftest.py +++ b/api/tests/integration/conftest.py @@ -4,6 +4,7 @@ import pytest from django.test import Client as DjangoClient from django.urls import reverse +from pytest_django.fixtures import SettingsWrapper from rest_framework import status from rest_framework.test import APIClient from tests.integration.helpers import create_mv_option_with_api @@ -72,10 +73,21 @@ def environment_api_key(): @pytest.fixture() -def environment(admin_client, project, environment_api_key, settings) -> int: +def environment_name() -> str: + return "Test Environment" + + +@pytest.fixture() +def environment( + admin_client: APIClient, + project: int, + environment_api_key: str, + environment_name: str, + settings: SettingsWrapper, +) -> int: settings.EDGE_RELEASE_DATETIME = None environment_data = { - "name": "Test Environment", + "name": environment_name, "api_key": environment_api_key, "project": project, "allow_client_traits": True, diff --git a/api/tests/unit/audit/test_unit_audit_models.py b/api/tests/unit/audit/test_unit_audit_models.py index b8a4efe86fdc..d4f402d5ed49 100644 --- a/api/tests/unit/audit/test_unit_audit_models.py +++ b/api/tests/unit/audit/test_unit_audit_models.py @@ -1,6 +1,6 @@ from audit.models import AuditLog from audit.related_object_type import RelatedObjectType -from audit.serializers import AuditLogSerializer +from audit.serializers import AuditLogListSerializer from integrations.datadog.models import DataDogConfiguration from webhooks.webhooks import WebhookEventType @@ -18,7 +18,7 @@ def test_organisation_webhooks_are_called_when_audit_log_saved(project, mocker): mock_call_webhooks.delay.assert_called_once_with( args=( project.organisation.id, - AuditLogSerializer(instance=audit_log).data, + AuditLogListSerializer(instance=audit_log).data, WebhookEventType.AUDIT_LOG_CREATED.value, ) ) @@ -145,15 +145,15 @@ def test_audit_log_history_record(mocker): mocked_model_class = mocker.MagicMock() mocked_module = mocker.MagicMock(**{model_class_name: mocked_model_class}) mocker.patch("audit.models.import_module", return_value=mocked_module) - mocked_model_class.objects.get.return_value = mocked_model + mocked_model_class.objects.filter.return_value.first.return_value = mocked_model # When record = audit_log.history_record # Then assert record == mocked_model - mocked_model_class.objects.get.assert_called_once_with( - id=audit_log.history_record_id + mocked_model_class.objects.filter.assert_called_once_with( + history_id=audit_log.history_record_id )