From a888f291eafd3b113233cf30b19594a37f8fb13a Mon Sep 17 00:00:00 2001 From: Matthew Elwell Date: Fri, 2 Feb 2024 15:37:23 +0000 Subject: [PATCH] fix(audit): add details for override creation (#3359) --- api/audit/models.py | 3 +- api/audit/serializers.py | 12 ++ api/core/models.py | 52 ++++-- api/features/managers.py | 34 +++- api/features/models.py | 32 +++- api/features/versioning/versioning_service.py | 40 +---- .../integration/audit/test_audit_logs.py | 149 ++++++++++++++++++ .../unit/features/test_unit_features_views.py | 2 +- 8 files changed, 277 insertions(+), 47 deletions(-) diff --git a/api/audit/models.py b/api/audit/models.py index 533b9955884b..700fc8085f36 100644 --- a/api/audit/models.py +++ b/api/audit/models.py @@ -1,4 +1,5 @@ import typing +from functools import cached_property from importlib import import_module from django.db import models @@ -78,7 +79,7 @@ def environment_document_updated(self) -> bool: ) return "send_environments_to_dynamodb" not in skip_signals_and_hooks - @property + @cached_property def history_record(self) -> typing.Optional[Model]: if not (self.history_record_class_path and self.history_record_id): # There are still AuditLog records that will not have this detail diff --git a/api/audit/serializers.py b/api/audit/serializers.py index feccc2a76e79..202ef42f800f 100644 --- a/api/audit/serializers.py +++ b/api/audit/serializers.py @@ -40,6 +40,7 @@ class AuditLogRetrieveSerializer(serializers.ModelSerializer): environment = EnvironmentSerializerLight() project = ProjectListSerializer() change_details = serializers.SerializerMethodField() + change_type = serializers.SerializerMethodField() class Meta: model = AuditLog @@ -54,6 +55,7 @@ class Meta: "related_object_type", "is_system_event", "change_details", + "change_type", ) @swagger_serializer_method( @@ -69,6 +71,16 @@ def get_change_details( return [] + def get_change_type(self, instance: AuditLog) -> str: + if not (history_record := instance.history_record): + return "UNKNOWN" + + return { + "+": "CREATE", + "-": "DELETE", + "~": "UPDATE", + }.get(history_record.history_type) + class AuditLogsQueryParamSerializer(serializers.Serializer): project = serializers.IntegerField(required=False) diff --git a/api/core/models.py b/api/core/models.py index dc5dd00a5983..e1082e49a3cc 100644 --- a/api/core/models.py +++ b/api/core/models.py @@ -4,6 +4,7 @@ from django.db import models from django.db.models import Manager +from django.forms import model_to_dict from django.http import HttpRequest from simple_history.models import HistoricalRecords, ModelChange from softdelete.models import SoftDeleteManager, SoftDeleteObject @@ -54,6 +55,7 @@ class Meta: class _BaseHistoricalModel(models.Model): include_in_audit = True + _show_change_details_for_create = False master_api_key = models.ForeignKey( "api_keys.MasterAPIKey", blank=True, null=True, on_delete=models.DO_NOTHING @@ -63,22 +65,32 @@ 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 - ] + if self.history_type == "~": + return [ + change + for change in self.diff_against(self.prev_record).changes + if change.field not in self._change_details_excluded_fields + ] + elif self.history_type == "+" and self._show_change_details_for_create: + return [ + ModelChange(field_name=key, old_value=None, new_value=value) + for key, value in self.instance.to_dict().items() + if key not in self._change_details_excluded_fields + ] + elif self.history_type == "-": + # Ignore deletes because they get painful due to cascade deletes + # Maybe we can resolve this in the future but for now it's not + # critical. + return [] def base_historical_model_factory( change_details_excluded_fields: typing.Sequence[str], + show_change_details_for_create: bool = False, ) -> typing.Type[_BaseHistoricalModel]: class BaseHistoricalModel(_BaseHistoricalModel): _change_details_excluded_fields = set(change_details_excluded_fields) + _show_change_details_for_create = show_change_details_for_create class Meta: abstract = True @@ -102,6 +114,9 @@ class _AbstractBaseAuditableModel(models.Model): history_record_class_path = None related_object_type = None + to_dict_excluded_fields: typing.Sequence[str] = None + to_dict_included_fields: typing.Sequence[str] = None + class Meta: abstract = True @@ -150,6 +165,17 @@ def get_audit_log_related_object_type(self, history_instance) -> RelatedObjectTy """ return self.related_object_type + def to_dict(self) -> dict[str, typing.Any]: + # by default, we exclude the id and any foreign key fields from the response + return model_to_dict( + instance=self, + fields=[ + f.name + for f in self._meta.fields + if f.name != "id" and not f.related_model + ], + ) + def _get_environment(self) -> typing.Optional["Environment"]: """Return the related environment for this model.""" return None @@ -169,10 +195,16 @@ 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, + show_change_details_for_create: bool = False, ) -> typing.Type[_AbstractBaseAuditableModel]: class Base(_AbstractBaseAuditableModel): history = HistoricalRecords( - bases=[base_historical_model_factory(change_details_excluded_fields or [])], + bases=[ + base_historical_model_factory( + change_details_excluded_fields or [], + show_change_details_for_create, + ) + ], excluded_fields=historical_records_excluded_fields or [], get_user=get_history_user, inherit=True, diff --git a/api/features/managers.py b/api/features/managers.py index 7e3c2b115b71..b7fd39155210 100644 --- a/api/features/managers.py +++ b/api/features/managers.py @@ -1,9 +1,17 @@ from __future__ import unicode_literals +import typing + from core.models import UUIDNaturalKeyManagerMixin +from django.db.models import Q, QuerySet +from django.utils import timezone from ordered_model.models import OrderedModelManager from softdelete.models import SoftDeleteManager +if typing.TYPE_CHECKING: + from environments.models import Environment + from features.models import FeatureState + class FeatureSegmentManager(UUIDNaturalKeyManagerMixin, OrderedModelManager): pass @@ -14,7 +22,31 @@ class FeatureManager(UUIDNaturalKeyManagerMixin, SoftDeleteManager): class FeatureStateManager(UUIDNaturalKeyManagerMixin, SoftDeleteManager): - pass + def get_live_feature_states( + self, environment: "Environment", additional_filters: Q = None, **kwargs + ) -> QuerySet["FeatureState"]: + # TODO: replace additional_filters with just using kwargs in calling locations + + now = timezone.now() + + qs_filter = Q(environment=environment, deleted_at__isnull=True) + if environment.use_v2_feature_versioning: + qs_filter &= Q( + environment_feature_version__isnull=False, + environment_feature_version__published_at__isnull=False, + environment_feature_version__live_from__lte=now, + ) + else: + qs_filter &= Q( + live_from__isnull=False, + live_from__lte=now, + version__isnull=False, + ) + + if additional_filters: + qs_filter &= additional_filters + + return self.filter(qs_filter, **kwargs) class FeatureStateValueManager(UUIDNaturalKeyManagerMixin, SoftDeleteManager): diff --git a/api/features/models.py b/api/features/models.py index 1f0db254b3b7..39be0ea2519e 100644 --- a/api/features/models.py +++ b/api/features/models.py @@ -373,7 +373,11 @@ def _get_environment(self) -> "Environment": class FeatureState( SoftDeleteExportableModel, LifecycleModelMixin, - abstract_base_auditable_model_factory(["uuid"]), + abstract_base_auditable_model_factory( + historical_records_excluded_fields=["uuid"], + change_details_excluded_fields=["live_from", "version"], + show_change_details_for_create=True, + ), ): history_record_class_path = "features.models.HistoricalFeatureState" related_object_type = RelatedObjectType.FEATURE_STATE @@ -846,6 +850,17 @@ def get_skip_create_audit_log(self) -> bool: return False def get_create_log_message(self, history_instance) -> typing.Optional[str]: + if ( + history_instance.history_type == "+" + and (self.identity_id or self.feature_segment_id) + and self.enabled == self.get_environment_default().enabled + ): + # Don't create an Audit Log for overrides that are created which don't differ + # from the environment default. This likely means that an override was created + # for a remote config value, and hence there will be an AuditLog message + # created for the FeatureStateValue model change. + return + if self.identity_id: return audit_helpers.get_identity_override_created_audit_message(self) elif self.feature_segment_id: @@ -900,6 +915,21 @@ def get_extra_audit_log_kwargs(self, history_instance) -> dict: return kwargs + def get_environment_default(self) -> typing.Optional["FeatureState"]: + if self.feature_segment_id or self.identity_id: + return ( + self.__class__.objects.get_live_feature_states( + environment=self.environment, + feature_id=self.feature_id, + feature_segment_id__isnull=True, + identity_id__isnull=True, + ) + .order_by("-version", "-environment_feature_version__live_from") + .first() + ) + + return None + def _get_environment(self) -> typing.Optional["Environment"]: return self.environment diff --git a/api/features/versioning/versioning_service.py b/api/features/versioning/versioning_service.py index b559632950a6..bcac998faebe 100644 --- a/api/features/versioning/versioning_service.py +++ b/api/features/versioning/versioning_service.py @@ -38,15 +38,14 @@ def get_environment_flags_list( feature states. The logic to grab the latest version is then handled in python by building a dictionary. Returns a list of FeatureState objects. """ - qs_filter = _build_environment_flags_qs_filter( - environment, feature_name, additional_filters - ) - additional_select_related_args = additional_select_related_args or tuple() additional_prefetch_related_args = additional_prefetch_related_args or tuple() feature_states = ( - FeatureState.objects.select_related( + FeatureState.objects.get_live_feature_states( + environment=environment, additional_filters=additional_filters + ) + .select_related( "environment", "feature", "feature_state_value", @@ -55,9 +54,11 @@ def get_environment_flags_list( *additional_select_related_args, ) .prefetch_related(*additional_prefetch_related_args) - .filter(qs_filter) ) + if feature_name: + feature_states = feature_states.filter(feature__name__iexact=feature_name) + # Build up a dictionary in the form # {(feature_id, feature_segment_id, identity_id): feature_state} # and only keep the latest version for each feature. @@ -88,30 +89,3 @@ def get_current_live_environment_feature_version( .order_by("-live_from") .first() ) - - -def _build_environment_flags_qs_filter( - environment: "Environment", feature_name: str = None, additional_filters: Q = None -) -> Q: - now = timezone.now() - - qs_filter = Q(environment=environment, deleted_at__isnull=True) - if environment.use_v2_feature_versioning: - qs_filter &= Q( - environment_feature_version__isnull=False, - environment_feature_version__published_at__isnull=False, - environment_feature_version__live_from__lte=now, - ) - else: - qs_filter &= Q( - live_from__isnull=False, - live_from__lte=now, - version__isnull=False, - ) - - if feature_name: - qs_filter &= Q(feature__name__iexact=feature_name) - if additional_filters: - qs_filter &= additional_filters - - return qs_filter diff --git a/api/tests/integration/audit/test_audit_logs.py b/api/tests/integration/audit/test_audit_logs.py index ac3913d6a051..9ee71f325486 100644 --- a/api/tests/integration/audit/test_audit_logs.py +++ b/api/tests/integration/audit/test_audit_logs.py @@ -178,3 +178,152 @@ def test_retrieve_audit_log_does_not_include_change_details_for_non_update( # Then assert retrieve_response.json()["change_details"] == [] + + +def test_retrieve_audit_log_includes_changes_when_segment_override_created_and_deleted_for_enabled_state( + admin_client: APIClient, + project: int, + feature: int, + environment_api_key: str, + environment: int, + segment: int, +) -> None: + # First, let's create a segment override + data = { + "feature_segment": {"segment": segment}, + "enabled": True, + "feature_state_value": {}, + } + create_segment_override_url = reverse( + "api-v1:environments:create-segment-override", + args=[environment_api_key, feature], + ) + create_segment_override_response = admin_client.post( + create_segment_override_url, + data=json.dumps(data), + content_type="application/json", + ) + assert create_segment_override_response.status_code == status.HTTP_201_CREATED + segment_override_feature_segment_id = create_segment_override_response.json()[ + "feature_segment" + ]["id"] + + # Now, that should have created an audit log, let's check + get_audit_logs_url = "%s?environment=%s" % ( + reverse("api-v1:audit-list"), + environment, + ) + get_audit_logs_response = admin_client.get(get_audit_logs_url) + assert get_audit_logs_response.status_code == status.HTTP_200_OK + results = get_audit_logs_response.json()["results"] + + # the first audit log in the list (i.e. most recent) should be the one that we want + audit_log_id = results[0]["id"] + get_create_override_audit_log_detail_url = reverse( + "api-v1:audit-detail", args=[audit_log_id] + ) + get_create_override_audit_log_detail_response = admin_client.get( + get_create_override_audit_log_detail_url + ) + assert ( + get_create_override_audit_log_detail_response.status_code == status.HTTP_200_OK + ) + create_override_audit_log_details = ( + get_create_override_audit_log_detail_response.json() + ) + + # now let's check that we have some information about the change + assert create_override_audit_log_details["change_type"] == "CREATE" + assert create_override_audit_log_details["change_details"] == [ + {"field": "enabled", "old": None, "new": True}, + ] + + # now let's delete the segment override + delete_segment_override_url = reverse( + "api-v1:features:feature-segment-detail", + args=[segment_override_feature_segment_id], + ) + response = admin_client.delete(delete_segment_override_url) + assert response.status_code == status.HTTP_204_NO_CONTENT + + # now we should have one more audit log record + get_audit_logs_response_2 = admin_client.get(get_audit_logs_url) + assert get_audit_logs_response_2.status_code == status.HTTP_200_OK + results = get_audit_logs_response_2.json()["results"] + assert len(results) == 5 + + # and the first one in the list should be for the deletion of the segment override + delete_override_audit_log_id = results[0]["id"] + get_delete_override_audit_log_detail_url = reverse( + "api-v1:audit-detail", args=[delete_override_audit_log_id] + ) + get_delete_override_audit_log_detail_response = admin_client.get( + get_delete_override_audit_log_detail_url + ) + assert ( + get_delete_override_audit_log_detail_response.status_code == status.HTTP_200_OK + ) + delete_override_audit_log_details = ( + get_delete_override_audit_log_detail_response.json() + ) + + # now let's check that we have some information about the change + assert delete_override_audit_log_details["change_type"] == "DELETE" + assert delete_override_audit_log_details["change_details"] == [] + + +def test_retrieve_audit_log_includes_changes_when_segment_override_created_for_feature_value( + admin_client: APIClient, + project: int, + feature: int, + default_feature_value: str, + environment_api_key: str, + environment: int, + segment: int, +) -> None: + # First, let's create a segment override + data = { + "feature_segment": {"segment": segment}, + "feature_state_value": {"value_type": "unicode", "string_value": "foo"}, + } + create_segment_override_url = reverse( + "api-v1:environments:create-segment-override", + args=[environment_api_key, feature], + ) + create_segment_override_response = admin_client.post( + create_segment_override_url, + data=json.dumps(data), + content_type="application/json", + ) + assert create_segment_override_response.status_code == status.HTTP_201_CREATED + + # Now, that should have created an audit log, let's check + get_audit_logs_url = "%s?environment=%s" % ( + reverse("api-v1:audit-list"), + environment, + ) + get_audit_logs_response = admin_client.get(get_audit_logs_url) + assert get_audit_logs_response.status_code == status.HTTP_200_OK + results = get_audit_logs_response.json()["results"] + + # and we should only have one audit log in the list related to the segment override + # (since the FeatureState hasn't changed) + # 1 for creating the feature + 1 for creating the environment + 1 for creating the segment + # + 1 for the segment override = 4 + assert len(results) == 4 + + # the first audit log in the list (i.e. most recent) should be the one that we want + audit_log_id = results[0]["id"] + get_audit_log_detail_url = reverse("api-v1:audit-detail", args=[audit_log_id]) + get_audit_log_detail_response = admin_client.get(get_audit_log_detail_url) + assert get_audit_log_detail_response.status_code == status.HTTP_200_OK + audit_log_details = get_audit_log_detail_response.json() + + # now let's check that we have some information about the change + # This is treated as an update since the FeatureStateValue is created + # automatically when the FeatureState is created, and then updated + # with the value in the request. + assert audit_log_details["change_type"] == "UPDATE" + assert audit_log_details["change_details"] == [ + {"field": "string_value", "old": "default_value", "new": "foo"}, + ] diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index b9c951b4e7a7..751ffa8eeb5d 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -1024,7 +1024,7 @@ def test_list_feature_states_nested_environment_view_set( "client", [lazy_fixture("admin_master_api_key_client"), lazy_fixture("admin_client")], ) -def test_environment_feature_states_filter_using_feataure_name( +def test_environment_feature_states_filter_using_feature_name( environment, project, feature, client ): # Given