fix(github-4555): use api_key name for changed_by (#4561)

api/api_keys/user.py

@@ -23,6 +23,9 @@ class APIKeyUser(UserABC):
     def __init__(self, key: MasterAPIKey):
         self.key = key
 
+    def __str__(self) -> str:
+        return self.key.name
+
     @property
     def is_authenticated(self) -> bool:
         return True

api/conftest.py

@@ -21,6 +21,7 @@
 from xdist import get_xdist_worker_id
 
 from api_keys.models import MasterAPIKey
+from api_keys.user import APIKeyUser
 from environments.identities.models import Identity
 from environments.identities.traits.models import Trait
 from environments.models import Environment, EnvironmentAPIKey
@@ -658,13 +659,28 @@ def master_api_key_object(
     return master_api_key[0]
 
 
+@pytest.fixture
+def master_api_key_id(master_api_key_object: MasterAPIKey) -> str:
+    return master_api_key_object.id
+
+
+@pytest.fixture
+def admin_user_id(admin_user: FFAdminUser) -> str:
+    return admin_user.id
+
+
 @pytest.fixture
 def admin_master_api_key_object(
     admin_master_api_key: typing.Tuple[MasterAPIKey, str]
 ) -> MasterAPIKey:
     return admin_master_api_key[0]
 
 
+@pytest.fixture
+def api_key_user(master_api_key_object: MasterAPIKey) -> APIKeyUser:
+    return APIKeyUser(master_api_key_object)
+
+
 @pytest.fixture()
 def admin_master_api_key_client(
     admin_master_api_key: typing.Tuple[MasterAPIKey, str]

api/edge_api/identities/models.py

@@ -6,7 +6,7 @@
 from flag_engine.features.models import FeatureStateModel
 from flag_engine.identities.models import IdentityFeaturesList, IdentityModel
 
-from api_keys.models import MasterAPIKey
+from api_keys.user import APIKeyUser
 from edge_api.identities.tasks import (
     generate_audit_log_records,
     sync_identity_document_features,
@@ -165,26 +165,22 @@ def remove_feature_override(self, feature_state: FeatureStateModel) -> None:
         with suppress(ValueError):  # ignore if feature state didn't exist
             self._engine_identity_model.identity_features.remove(feature_state)
 
-    def save(self, user: FFAdminUser = None, master_api_key: MasterAPIKey = None):
+    def save(self, user: FFAdminUser | APIKeyUser = None):
         self.dynamo_wrapper.put_item(self.to_document())
         changeset = self._get_changes()
         self._update_feature_overrides(
             changeset=changeset,
             user=user,
-            master_api_key=master_api_key,
         )
         self._reset_initial_state()
 
-    def delete(
-        self, user: FFAdminUser = None, master_api_key: MasterAPIKey = None
-    ) -> None:
+    def delete(self, user: FFAdminUser | APIKeyUser = None) -> None:
         self.dynamo_wrapper.delete_item(self._engine_identity_model.composite_key)
         self._engine_identity_model.identity_features.clear()
         changeset = self._get_changes()
         self._update_feature_overrides(
             changeset=changeset,
             user=user,
-            master_api_key=master_api_key,
         )
         self._reset_initial_state()
 
@@ -200,23 +196,21 @@ def to_document(self) -> dict:
         return map_engine_identity_to_identity_document(self._engine_identity_model)
 
     def _update_feature_overrides(
-        self,
-        changeset: IdentityChangeset,
-        user: FFAdminUser = None,
-        master_api_key: MasterAPIKey = None,
+        self, changeset: IdentityChangeset, user: FFAdminUser | APIKeyUser
     ) -> None:
         if changeset["feature_overrides"]:
             # TODO: would this be simpler if we put a wrapper around FeatureStateModel instead?
-            generate_audit_log_records.delay(
-                kwargs={
-                    "environment_api_key": self.environment_api_key,
-                    "identifier": self.identifier,
-                    "user_id": getattr(user, "id", None),
-                    "changes": changeset,
-                    "identity_uuid": str(self.identity_uuid),
-                    "master_api_key_id": getattr(master_api_key, "id", None),
-                }
-            )
+            kwargs = {
+                "environment_api_key": self.environment_api_key,
+                "identifier": self.identifier,
+                "user_id": getattr(user, "id", None),
+                "changes": changeset,
+                "identity_uuid": str(self.identity_uuid),
+                "master_api_key_id": (
+                    user.pk if getattr(user, "is_master_api_key_user", False) else None
+                ),
+            }
+            generate_audit_log_records.delay(kwargs=kwargs)
             update_flagsmith_environments_v2_identity_overrides.delay(
                 kwargs={
                     "environment_api_key": self.environment_api_key,

api/edge_api/identities/serializers.py

@@ -171,10 +171,7 @@ def save(self, **kwargs):
             self.instance.multivariate_feature_state_values,
         )
 
-        identity.save(
-            user=request.user,
-            master_api_key=getattr(request, "master_api_key", None),
-        )
+        identity.save(user=request.user)
 
         new_value = self.instance.get_value(identity.id)
         previous_value = (
@@ -189,7 +186,7 @@ def save(self, **kwargs):
                 "environment_api_key": identity.environment_api_key,
                 "identity_id": identity.id,
                 "identity_identifier": identity.identifier,
-                "changed_by_user_id": request.user.id,
+                "changed_by": str(request.user),
                 "new_enabled_state": self.instance.enabled,
                 "new_value": new_value,
                 "previous_enabled_state": getattr(previous_state, "enabled", None),

api/edge_api/identities/tasks.py

@@ -11,7 +11,6 @@
 from environments.dynamodb import DynamoEnvironmentV2Wrapper
 from environments.models import Environment, Webhook
 from features.models import Feature, FeatureState
-from users.models import FFAdminUser
 from util.mappers import map_identity_changeset_to_identity_override_changeset
 from webhooks.webhooks import WebhookEventType, call_environment_webhooks
 
@@ -24,7 +23,7 @@ def call_environment_webhook_for_feature_state_change(
     environment_api_key: str,
     identity_id: typing.Union[id, str],
     identity_identifier: str,
-    changed_by_user_id: int,
+    changed_by: str,
     timestamp: str,
     new_enabled_state: bool = None,
     new_value: typing.Union[bool, int, str] = None,
@@ -40,10 +39,9 @@ def call_environment_webhook_for_feature_state_change(
         return
 
     feature = Feature.objects.get(id=feature_id)
-    changed_by = FFAdminUser.objects.get(id=changed_by_user_id)
 
     data = {
-        "changed_by": changed_by.email,
+        "changed_by": changed_by,
         "timestamp": timestamp,
         "new_state": None,
     }

api/edge_api/identities/views.py

@@ -161,10 +161,7 @@ def get_environment_from_request(self):
 
     def perform_destroy(self, instance):
         edge_identity = EdgeIdentity.from_identity_document(instance)
-        edge_identity.delete(
-            user=self.request.user,
-            master_api_key=getattr(self.request, "master_api_key", None),
-        )
+        edge_identity.delete(user=self.request.user)
 
     @swagger_auto_schema(
         responses={200: EdgeIdentityTraitsSerializer(many=True)},
@@ -284,10 +281,7 @@ def list(self, request, *args, **kwargs):
 
     def perform_destroy(self, instance):
         self.identity.remove_feature_override(instance)
-        self.identity.save(
-            user=self.request.user,
-            master_api_key=getattr(self.request, "master_api_key", None),
-        )
+        self.identity.save(user=self.request.user)
 
     @swagger_auto_schema(responses={200: IdentityAllFeatureStatesSerializer(many=True)})
     @action(detail=False, methods=["GET"])
@@ -332,10 +326,7 @@ def clone_from_given_identity(self, request, *args, **kwargs) -> Response:
         )
 
         self.identity.clone_flag_states_from(source_identity)
-        self.identity.save(
-            user=request.user.id,
-            master_api_key=getattr(request, "master_api_key", None),
-        )
+        self.identity.save(user=request.user)
 
         return self.all(request, *args, **kwargs)
 
@@ -391,10 +382,7 @@ def delete(self, request, *args, **kwargs):
         feature_state = self.identity.get_feature_state_by_feature_name_or_id(feature)
         if feature_state:
             self.identity.remove_feature_override(feature_state)
-            self.identity.save(
-                user=request.user.id,
-                master_api_key=getattr(request, "master_api_key", None),
-            )
+            self.identity.save(user=request.user)
         return Response(status=status.HTTP_204_NO_CONTENT)
 
 

api/tests/integration/edge_api/identities/test_edge_identity_featurestates_viewset.py

@@ -1,6 +1,7 @@
 import json
 import typing
 import uuid
+from unittest.mock import MagicMock
 
 import pytest
 from core.constants import BOOLEAN, INTEGER, STRING
@@ -351,15 +352,15 @@ def test_edge_identities_create_featurestate_returns_400_if_feature_state_alread
 
 
 def test_edge_identities_create_featurestate(
-    dynamodb_wrapper_v2,
-    admin_client,
-    environment,
-    environment_api_key,
-    identity_document_without_fs,
-    edge_identity_dynamo_wrapper_mock,
-    feature,
-    feature_name,
-    webhook_mock,
+    dynamodb_wrapper_v2: DynamoEnvironmentV2Wrapper,
+    admin_client_new: APIClient,
+    environment: int,
+    environment_api_key: str,
+    identity_document_without_fs: dict,
+    edge_identity_dynamo_wrapper_mock: MagicMock,
+    feature: int,
+    feature_name: str,
+    webhook_mock: MagicMock,
 ):
     # Given
     edge_identity_dynamo_wrapper_mock.get_item_from_uuid_or_404.return_value = (
@@ -381,7 +382,7 @@ def test_edge_identities_create_featurestate(
     }
 
     # When
-    response = admin_client.post(
+    response = admin_client_new.post(
         url, data=json.dumps(data), content_type="application/json"
     )
 

api/tests/unit/api_keys/test_user.py

@@ -17,6 +17,17 @@ def test_is_authenticated(master_api_key_object):
     assert user.is_authenticated is True
 
 
+def test__str__returns_name(master_api_key_object):
+    # Given
+    user = APIKeyUser(master_api_key_object)
+
+    # When
+    result = str(user)
+
+    # Then
+    assert result == master_api_key_object.name
+
+
 @pytest.mark.parametrize(
     "for_organisation, expected_result",
     [

api/tests/unit/edge_api/identities/test_edge_api_identities_serializers.py

@@ -1,13 +1,20 @@
+import pytest
+from django.test import RequestFactory
 from django.utils import timezone
 from flag_engine.features.models import FeatureModel, FeatureStateModel
+from pytest_lazyfixture import lazy_fixture
+from pytest_mock import MockerFixture
 
+from api_keys.user import APIKeyUser
 from edge_api.identities.models import EdgeIdentity
 from edge_api.identities.serializers import EdgeIdentityFeatureStateSerializer
+from environments.identities.models import Identity
 from environments.identities.serializers import (
     IdentityAllFeatureStatesSerializer,
 )
 from features.feature_types import STANDARD
-from features.models import FeatureState
+from features.models import Feature, FeatureState
+from users.models import FFAdminUser
 from util.mappers import map_identity_to_identity_document
 from webhooks.constants import WEBHOOK_DATETIME_FORMAT
 
@@ -51,15 +58,27 @@ def test_edge_identity_feature_state_serializer_save_allows_missing_mvfsvs(
     assert saved_identity_feature_state["feature"]["id"] == feature.id
 
 
+@pytest.mark.parametrize(
+    "user",
+    [
+        lazy_fixture("api_key_user"),
+        lazy_fixture("admin_user"),
+    ],
+)
 def test_edge_identity_feature_state_serializer_save_calls_webhook_for_new_override(
-    mocker, identity, feature, admin_user
+    mocker: MockerFixture,
+    identity: Identity,
+    feature: Feature,
+    user: FFAdminUser | APIKeyUser,
+    rf: RequestFactory,
 ):
     # Given
     identity_model = EdgeIdentity.from_identity_document(
         map_identity_to_identity_document(identity)
     )
     view = mocker.MagicMock(identity=identity_model)
-    request = mocker.MagicMock(user=admin_user, master_api_key=None)
+    request = rf.post("/")
+    request.user = user
 
     new_enabled_state = True
     new_value = "foo"
@@ -93,7 +112,7 @@ def test_edge_identity_feature_state_serializer_save_calls_webhook_for_new_overr
             "environment_api_key": identity.environment.api_key,
             "identity_id": identity.id,
             "identity_identifier": identity.identifier,
-            "changed_by_user_id": admin_user.id,
+            "changed_by": str(user),
             "new_enabled_state": new_enabled_state,
             "new_value": new_value,
             "previous_enabled_state": None,
@@ -154,7 +173,7 @@ def test_edge_identity_feature_state_serializer_save_calls_webhook_for_update(
             "environment_api_key": identity.environment.api_key,
             "identity_id": identity.id,
             "identity_identifier": identity.identifier,
-            "changed_by_user_id": admin_user.id,
+            "changed_by": str(admin_user),
             "new_enabled_state": new_enabled_state,
             "new_value": new_value,
             "previous_enabled_state": previous_enabled_state,