diff --git a/api/audit/constants.py b/api/audit/constants.py index 39672cf2aae2..c2bb05197143 100644 --- a/api/audit/constants.py +++ b/api/audit/constants.py @@ -58,4 +58,6 @@ CHANGE_REQUEST_COMMITTED_MESSAGE = "Change Request: %s committed" CHANGE_REQUEST_DELETED_MESSAGE = "Change Request: %s deleted" +ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE = "New version published for feature: %s" + DATETIME_FORMAT = "%d/%m/%Y %H:%M:%S" diff --git a/api/audit/related_object_type.py b/api/audit/related_object_type.py index 3c15d5858901..6fab2460890b 100644 --- a/api/audit/related_object_type.py +++ b/api/audit/related_object_type.py @@ -9,3 +9,4 @@ class RelatedObjectType(enum.Enum): CHANGE_REQUEST = "Change request" EDGE_IDENTITY = "Edge Identity" IMPORT_REQUEST = "Import request" + EF_VERSION = "Environment feature version" diff --git a/api/audit/tasks.py b/api/audit/tasks.py index 3d60c10927a7..0863580a50e3 100644 --- a/api/audit/tasks.py +++ b/api/audit/tasks.py @@ -159,9 +159,16 @@ def create_segment_priorities_changed_audit_log( if not feature_segments: return - # all feature segments should have the same value for feature and environment + # all feature segments should have the same value for feature, environment and + # environment feature version environment = feature_segments[0].environment feature = feature_segments[0].feature + environment_feature_version_id = feature_segments[0].environment_feature_version_id + + if environment_feature_version_id is not None: + # Don't create audit logs for FeatureSegments wrapped in a version + # as this is handled by the feature history instead. + return AuditLog.objects.create( log=f"Segment overrides re-ordered for feature '{feature.name}'.", diff --git a/api/features/versioning/migrations/0002_add_api_key_for_creation_and_publish.py b/api/features/versioning/migrations/0002_add_api_key_for_creation_and_publish.py new file mode 100644 index 000000000000..ea74f96c6d85 --- /dev/null +++ b/api/features/versioning/migrations/0002_add_api_key_for_creation_and_publish.py @@ -0,0 +1,35 @@ +# Generated by Django 3.2.25 on 2024-05-31 12:11 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('api_keys', '0003_masterapikey_is_admin'), + ('feature_versioning', '0001_add_environment_feature_state_version_logic'), + ] + + operations = [ + migrations.AddField( + model_name='environmentfeatureversion', + name='created_by_api_key', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='created_environment_feature_versions', to='api_keys.masterapikey'), + ), + migrations.AddField( + model_name='environmentfeatureversion', + name='published_by_api_key', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='published_environment_feature_versions', to='api_keys.masterapikey'), + ), + migrations.AddField( + model_name='historicalenvironmentfeatureversion', + name='created_by_api_key', + field=models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='api_keys.masterapikey'), + ), + migrations.AddField( + model_name='historicalenvironmentfeatureversion', + name='published_by_api_key', + field=models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to='api_keys.masterapikey'), + ), + ] diff --git a/api/features/versioning/models.py b/api/features/versioning/models.py index c38c15d23602..d62d0c4cc94b 100644 --- a/api/features/versioning/models.py +++ b/api/features/versioning/models.py @@ -12,6 +12,7 @@ from django.db.models import Index from django.utils import timezone +from api_keys.models import MasterAPIKey from features.versioning.exceptions import FeatureVersioningError from features.versioning.managers import EnvironmentFeatureVersionManager from features.versioning.signals import environment_feature_version_published @@ -47,6 +48,14 @@ class EnvironmentFeatureVersion( null=True, blank=True, ) + created_by_api_key = models.ForeignKey( + "api_keys.MasterAPIKey", + related_name="created_environment_feature_versions", + on_delete=models.SET_NULL, + null=True, + blank=True, + ) + published_by = models.ForeignKey( settings.AUTH_USER_MODEL, related_name="published_environment_feature_versions", @@ -54,6 +63,13 @@ class EnvironmentFeatureVersion( null=True, blank=True, ) + published_by_api_key = models.ForeignKey( + "api_keys.MasterAPIKey", + related_name="published_environment_feature_versions", + on_delete=models.SET_NULL, + null=True, + blank=True, + ) change_request = models.ForeignKey( "workflows_core.ChangeRequest", @@ -118,14 +134,21 @@ def get_previous_version(self) -> typing.Optional["EnvironmentFeatureVersion"]: def publish( self, published_by: typing.Union["FFAdminUser", None] = None, + published_by_api_key: MasterAPIKey | None = None, live_from: datetime.datetime | None = None, persist: bool = True, ) -> None: + assert not ( + published_by and published_by_api_key + ), "Version must be published by either a user or a MasterAPIKey" + now = timezone.now() self.live_from = live_from or (self.live_from or now) self.published_at = now self.published_by = published_by + self.published_by_api_key = published_by_api_key + if persist: self.save() environment_feature_version_published.send(self.__class__, instance=self) diff --git a/api/features/versioning/permissions.py b/api/features/versioning/permissions.py index 85d34a8647d3..f6add7839318 100644 --- a/api/features/versioning/permissions.py +++ b/api/features/versioning/permissions.py @@ -13,8 +13,8 @@ class EnvironmentFeatureVersionPermissions(BasePermission): def has_permission(self, request: Request, view: GenericViewSet) -> bool: - if view.action == "list": - # permissions for listing handled in view.get_queryset + if view.action in ("list", "retrieve"): + # permissions for listing and retrieving handled in view.get_queryset return True environment_pk = view.kwargs["environment_pk"] diff --git a/api/features/versioning/receivers.py b/api/features/versioning/receivers.py index 5fa6525c99e3..94670aa35dd2 100644 --- a/api/features/versioning/receivers.py +++ b/api/features/versioning/receivers.py @@ -5,7 +5,10 @@ from environments.tasks import rebuild_environment_document from features.versioning.models import EnvironmentFeatureVersion from features.versioning.signals import environment_feature_version_published -from features.versioning.tasks import trigger_update_version_webhooks +from features.versioning.tasks import ( + create_environment_feature_version_published_audit_log_task, + trigger_update_version_webhooks, +) @receiver(post_save, sender=EnvironmentFeatureVersion) @@ -50,3 +53,12 @@ def trigger_webhooks(instance: EnvironmentFeatureVersion, **kwargs) -> None: kwargs={"environment_feature_version_uuid": str(instance.uuid)}, delay_until=instance.live_from, ) + + +@receiver(environment_feature_version_published, sender=EnvironmentFeatureVersion) +def create_environment_feature_version_published_audit_log( + instance: EnvironmentFeatureVersion, **kwargs +) -> None: + create_environment_feature_version_published_audit_log_task.delay( + kwargs={"environment_feature_version_uuid": str(instance.uuid)} + ) diff --git a/api/features/versioning/serializers.py b/api/features/versioning/serializers.py index 5b145ba00873..fbfcd11f0c48 100644 --- a/api/features/versioning/serializers.py +++ b/api/features/versioning/serializers.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from api_keys.user import APIKeyUser from features.serializers import CreateSegmentOverrideFeatureStateSerializer from features.versioning.models import EnvironmentFeatureVersion from integrations.github.github import call_github_task @@ -64,6 +65,23 @@ class Meta: ) +class EnvironmentFeatureVersionRetrieveSerializer(EnvironmentFeatureVersionSerializer): + previous_version_uuid = serializers.SerializerMethodField() + + class Meta(EnvironmentFeatureVersionSerializer.Meta): + _fields = ("previous_version_uuid",) + + fields = EnvironmentFeatureVersionSerializer.Meta.fields + _fields + + def get_previous_version_uuid( + self, environment_feature_version: EnvironmentFeatureVersion + ) -> str | None: + previous_version = environment_feature_version.get_previous_version() + if not previous_version: + return None + return str(previous_version.uuid) + + class EnvironmentFeatureVersionPublishSerializer(serializers.Serializer): live_from = serializers.DateTimeField(required=False) @@ -71,9 +89,20 @@ def save(self, **kwargs): live_from = self.validated_data.get("live_from") request = self.context["request"] - published_by = request.user if isinstance(request.user, FFAdminUser) else None - self.instance.publish(live_from=live_from, published_by=published_by) + published_by = None + published_by_api_key = None + + if isinstance(request.user, FFAdminUser): + published_by = request.user + elif isinstance(request.user, APIKeyUser): + published_by_api_key = request.user.key + + self.instance.publish( + live_from=live_from, + published_by=published_by, + published_by_api_key=published_by_api_key, + ) return self.instance diff --git a/api/features/versioning/tasks.py b/api/features/versioning/tasks.py index d79088a77212..a16224b49955 100644 --- a/api/features/versioning/tasks.py +++ b/api/features/versioning/tasks.py @@ -3,6 +3,9 @@ from django.utils import timezone +from audit.constants import ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE +from audit.models import AuditLog +from audit.related_object_type import RelatedObjectType from features.models import FeatureState from features.versioning.models import EnvironmentFeatureVersion from features.versioning.schemas import ( @@ -131,3 +134,22 @@ def trigger_update_version_webhooks(environment_feature_version_uuid: str) -> No data=data, event_type=WebhookEventType.NEW_VERSION_PUBLISHED, ) + + +@register_task_handler() +def create_environment_feature_version_published_audit_log_task( + environment_feature_version_uuid: str, +) -> None: + environment_feature_version = EnvironmentFeatureVersion.objects.select_related( + "environment", "feature" + ).get(uuid=environment_feature_version_uuid) + + AuditLog.objects.create( + environment=environment_feature_version.environment, + related_object_type=RelatedObjectType.EF_VERSION.name, + related_object_uuid=environment_feature_version.uuid, + log=ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE + % environment_feature_version.feature.name, + author_id=environment_feature_version.published_by_id, + master_api_key_id=environment_feature_version.published_by_api_key_id, + ) diff --git a/api/features/versioning/views.py b/api/features/versioning/views.py index 3275db292843..c21032f52db5 100644 --- a/api/features/versioning/views.py +++ b/api/features/versioning/views.py @@ -8,6 +8,7 @@ CreateModelMixin, DestroyModelMixin, ListModelMixin, + RetrieveModelMixin, UpdateModelMixin, ) from rest_framework.permissions import IsAuthenticated @@ -30,6 +31,7 @@ EnvironmentFeatureVersionFeatureStateSerializer, EnvironmentFeatureVersionPublishSerializer, EnvironmentFeatureVersionQuerySerializer, + EnvironmentFeatureVersionRetrieveSerializer, EnvironmentFeatureVersionSerializer, ) from projects.permissions import VIEW_PROJECT @@ -44,11 +46,11 @@ ) class EnvironmentFeatureVersionViewSet( GenericViewSet, + RetrieveModelMixin, ListModelMixin, CreateModelMixin, DestroyModelMixin, ): - serializer_class = EnvironmentFeatureVersionSerializer permission_classes = [IsAuthenticated, EnvironmentFeatureVersionPermissions] def __init__(self, *args, **kwargs): @@ -62,6 +64,8 @@ def get_serializer_class(self): match self.action: case "publish": return EnvironmentFeatureVersionPublishSerializer + case "retrieve": + return EnvironmentFeatureVersionRetrieveSerializer case _: return EnvironmentFeatureVersionSerializer diff --git a/api/tests/unit/audit/test_unit_audit_tasks.py b/api/tests/unit/audit/test_unit_audit_tasks.py index dfac633c8180..f6ee29c250e0 100644 --- a/api/tests/unit/audit/test_unit_audit_tasks.py +++ b/api/tests/unit/audit/test_unit_audit_tasks.py @@ -14,6 +14,7 @@ ) from environments.models import Environment from features.models import Feature, FeatureSegment, FeatureState +from features.versioning.tasks import enable_v2_versioning from segments.models import Segment from users.models import FFAdminUser @@ -251,6 +252,56 @@ def test_create_segment_priorities_changed_audit_log( ).exists() +def test_create_segment_priorities_changed_audit_log_does_not_create_audit_log_for_versioned_feature_segments( + admin_user: FFAdminUser, + feature_segment: FeatureSegment, + feature: Feature, + segment_featurestate: FeatureState, + environment: Environment, +) -> None: + # Given + another_segment = Segment.objects.create( + project=environment.project, name="Another Segment" + ) + another_feature_segment = FeatureSegment.objects.create( + feature=feature, + environment=environment, + segment=another_segment, + ) + FeatureState.objects.create( + feature=feature, + environment=environment, + feature_segment=another_feature_segment, + ) + + now = timezone.now() + + enable_v2_versioning(environment.id) + + feature_segment.refresh_from_db() + another_feature_segment.refresh_from_db() + assert feature_segment.environment_feature_version_id is not None + assert another_feature_segment.environment_feature_version_id is not None + + # When + create_segment_priorities_changed_audit_log( + previous_id_priority_pairs=[ + (feature_segment.id, 0), + (another_feature_segment.id, 1), + ], + feature_segment_ids=[feature_segment.id, another_feature_segment.id], + user_id=admin_user.id, + changed_at=now.isoformat(), + ) + + # Then + assert not AuditLog.objects.filter( + environment=environment, + log=f"Segment overrides re-ordered for feature '{feature.name}'.", + created_date=now, + ).exists() + + def test_create_feature_state_went_live_audit_log( change_request_feature_state: FeatureState, ) -> None: diff --git a/api/tests/unit/features/versioning/test_unit_versioning_views.py b/api/tests/unit/features/versioning/test_unit_versioning_views.py index 76d7b471840c..e89fda4c50d3 100644 --- a/api/tests/unit/features/versioning/test_unit_versioning_views.py +++ b/api/tests/unit/features/versioning/test_unit_versioning_views.py @@ -9,6 +9,10 @@ from rest_framework import status from rest_framework.test import APIClient +from api_keys.models import MasterAPIKey +from audit.constants import ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE +from audit.models import AuditLog +from audit.related_object_type import RelatedObjectType from environments.models import Environment from environments.permissions.constants import VIEW_ENVIRONMENT from features.models import Feature, FeatureSegment, FeatureState @@ -122,6 +126,112 @@ def test_delete_feature_version( assert environment_feature_version.deleted is True +def test_retrieve_feature_version_with_no_previous_version( + feature: Feature, + environment_v2_versioning: Environment, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + environment_feature_version = EnvironmentFeatureVersion.objects.get( + feature=feature, environment=environment_v2_versioning + ) + + url = reverse( + "api-v1:versioning:environment-feature-versions-detail", + args=[ + environment_v2_versioning.id, + feature.id, + environment_feature_version.uuid, + ], + ) + + with_environment_permissions([VIEW_ENVIRONMENT]) + with_project_permissions([VIEW_PROJECT]) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(environment_feature_version.uuid) + assert response_json["previous_version_uuid"] is None + + +def test_retrieve_feature_version_with_previous_version( + feature: Feature, + environment_v2_versioning: Environment, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT]) + with_project_permissions([VIEW_PROJECT]) + + version_1 = EnvironmentFeatureVersion.objects.filter( + feature=feature, environment=environment_v2_versioning + ).get() + version_2 = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + version_2.publish(published_by=staff_user) + + url = reverse( + "api-v1:versioning:environment-feature-versions-detail", + args=[environment_v2_versioning.id, feature.id, version_2.uuid], + ) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(version_2.uuid) + assert response_json["previous_version_uuid"] == str(version_1.uuid) + + +def test_retrieve_feature_version_for_unpublished_version( + feature: Feature, + environment_v2_versioning: Environment, + staff_user: FFAdminUser, + staff_client: APIClient, + with_environment_permissions: WithEnvironmentPermissionsCallable, + with_project_permissions: WithProjectPermissionsCallable, +) -> None: + # Given + with_environment_permissions([VIEW_ENVIRONMENT]) + with_project_permissions([VIEW_PROJECT]) + + version_1 = EnvironmentFeatureVersion.objects.filter( + feature=feature, environment=environment_v2_versioning + ).get() + version_2 = EnvironmentFeatureVersion.objects.create( + feature=feature, environment=environment_v2_versioning + ) + + url = reverse( + "api-v1:versioning:environment-feature-versions-detail", + args=[environment_v2_versioning.id, feature.id, version_2.uuid], + ) + + # When + response = staff_client.get(url) + + # Then + assert response.status_code == status.HTTP_200_OK + + response_json = response.json() + assert response_json["uuid"] == str(version_2.uuid) + assert response_json["previous_version_uuid"] == str(version_1.uuid) + + def test_cannot_delete_live_feature_version( admin_client: APIClient, environment_v2_versioning: Environment, @@ -189,9 +299,18 @@ def test_publish_feature_version( environment_feature_version.live_from == now if live_from is None else live_from ) + # and an audit log record is created correctly + record = AuditLog.objects.filter( + related_object_type=RelatedObjectType.EF_VERSION.name, + related_object_uuid=environment_feature_version.uuid, + ).first() + assert record + assert record.log == ENVIRONMENT_FEATURE_VERSION_PUBLISHED_MESSAGE % feature.name + @pytest.mark.parametrize("live_from", (None, tomorrow)) def test_publish_feature_version_using_master_api_key( + admin_master_api_key: MasterAPIKey, admin_master_api_key_client: APIClient, environment_v2_versioning: Environment, feature: Feature, @@ -223,6 +342,7 @@ def test_publish_feature_version_using_master_api_key( assert environment_feature_version.is_live is True assert environment_feature_version.published is True assert environment_feature_version.published_by is None + assert environment_feature_version.published_by_api_key == admin_master_api_key[0] assert ( environment_feature_version.live_from == now if live_from is None else live_from )