enterprise/audit: fix audit logging with m2m relations (#9571)

authentik/enterprise/audit/middleware.py

@@ -2,11 +2,12 @@
 
 from copy import deepcopy
 from functools import partial
+from typing import Any
 
 from django.apps.registry import apps
 from django.core.files import File
 from django.db import connection
-from django.db.models import Model
+from django.db.models import ManyToManyRel, Model
 from django.db.models.expressions import BaseExpression, Combinable
 from django.db.models.signals import post_init
 from django.http import HttpRequest
@@ -44,7 +45,7 @@ class EnterpriseAuditMiddleware(AuditMiddleware):
         post_init.disconnect(dispatch_uid=request.request_id)
 
     def serialize_simple(self, model: Model) -> dict:
-        """Serialize a model in a very simple way. No ForeginKeys or other relationships are
+        """Serialize a model in a very simple way. No ForeignKeys or other relationships are
         resolved"""
         data = {}
         deferred_fields = model.get_deferred_fields()
@@ -70,6 +71,9 @@ class EnterpriseAuditMiddleware(AuditMiddleware):
         for key, value in before.items():
             if after.get(key) != value:
                 diff[key] = {"previous_value": value, "new_value": after.get(key)}
+        for key, value in after.items():
+            if key not in before and key not in diff and before.get(key) != value:
+                diff[key] = {"previous_value": before.get(key), "new_value": value}
         return sanitize_item(diff)
 
     def post_init_handler(self, request: HttpRequest, sender, instance: Model, **_):
@@ -98,8 +102,37 @@ class EnterpriseAuditMiddleware(AuditMiddleware):
         thread_kwargs = {}
         if hasattr(instance, "_previous_state") or created:
             prev_state = getattr(instance, "_previous_state", {})
+            if created:
+                prev_state = {}
             # Get current state
             new_state = self.serialize_simple(instance)
             diff = self.diff(prev_state, new_state)
             thread_kwargs["diff"] = diff
         return super().post_save_handler(request, sender, instance, created, thread_kwargs, **_)
+
+    def m2m_changed_handler(  # noqa: PLR0913
+        self,
+        request: HttpRequest,
+        sender,
+        instance: Model,
+        action: str,
+        pk_set: set[Any],
+        thread_kwargs: dict | None = None,
+        **_,
+    ):
+        thread_kwargs = {}
+        m2m_field = None
+        # For the audit log we don't care about `pre_` or `post_` so we trim that part off
+        _, _, action_direction = action.partition("_")
+        # resolve the "through" model to an actual field
+        for field in instance._meta.get_fields():
+            if not isinstance(field, ManyToManyRel):
+                continue
+            if field.through == sender:
+                m2m_field = field
+        if m2m_field:
+            # If we're clearing we just set the "flag" to True
+            if action_direction == "clear":
+                pk_set = True
+            thread_kwargs["diff"] = {m2m_field.related_name: {action_direction: pk_set}}
+        return super().m2m_changed_handler(request, sender, instance, action, thread_kwargs)

authentik/enterprise/audit/tests.py

@@ -1,9 +1,22 @@
+from unittest.mock import PropertyMock, patch
+
 from django.apps import apps
 from django.conf import settings
-from django.test import TestCase
+from django.urls import reverse
+from rest_framework.test import APITestCase
+
+from authentik.core.models import Group, User
+from authentik.core.tests.utils import create_test_admin_user
+from authentik.events.models import Event, EventAction
+from authentik.events.utils import sanitize_item
+from authentik.lib.generators import generate_id
 
 
-class TestEnterpriseAudit(TestCase):
+class TestEnterpriseAudit(APITestCase):
+    """Test audit middleware"""
+
+    def setUp(self) -> None:
+        self.user = create_test_admin_user()
 
     def test_import(self):
         """Ensure middleware is imported when app.ready is called"""
@@ -16,3 +29,182 @@ class TestEnterpriseAudit(TestCase):
         self.assertIn(
             "authentik.enterprise.audit.middleware.EnterpriseAuditMiddleware", settings.MIDDLEWARE
         )
+
+    @patch(
+        "authentik.enterprise.audit.middleware.EnterpriseAuditMiddleware.enabled",
+        PropertyMock(return_value=True),
+    )
+    def test_create(self):
+        """Test create audit log"""
+        self.client.force_login(self.user)
+        username = generate_id()
+        response = self.client.post(
+            reverse("authentik_api:user-list"),
+            data={"name": generate_id(), "username": username, "groups": [], "path": "foo"},
+        )
+        user = User.objects.get(username=username)
+        self.assertEqual(response.status_code, 201)
+        events = Event.objects.filter(
+            action=EventAction.MODEL_CREATED,
+            context__model__model_name="user",
+            context__model__app="authentik_core",
+            context__model__pk=user.pk,
+        )
+        event = events.first()
+        self.assertIsNotNone(event)
+        self.assertIsNotNone(event.context["diff"])
+        diff = event.context["diff"]
+        self.assertEqual(
+            diff,
+            {
+                "name": {
+                    "new_value": user.name,
+                    "previous_value": None,
+                },
+                "path": {"new_value": "foo", "previous_value": None},
+                "type": {"new_value": "internal", "previous_value": None},
+                "uuid": {
+                    "new_value": user.uuid.hex,
+                    "previous_value": None,
+                },
+                "email": {"new_value": "", "previous_value": None},
+                "username": {
+                    "new_value": user.username,
+                    "previous_value": None,
+                },
+                "is_active": {"new_value": True, "previous_value": None},
+                "attributes": {"new_value": {}, "previous_value": None},
+                "date_joined": {
+                    "new_value": sanitize_item(user.date_joined),
+                    "previous_value": None,
+                },
+                "first_name": {"new_value": "", "previous_value": None},
+                "id": {"new_value": user.pk, "previous_value": None},
+                "last_name": {"new_value": "", "previous_value": None},
+                "password": {"new_value": "********************", "previous_value": None},
+                "password_change_date": {
+                    "new_value": sanitize_item(user.password_change_date),
+                    "previous_value": None,
+                },
+            },
+        )
+
+    @patch(
+        "authentik.enterprise.audit.middleware.EnterpriseAuditMiddleware.enabled",
+        PropertyMock(return_value=True),
+    )
+    def test_update(self):
+        """Test update audit log"""
+        self.client.force_login(self.user)
+        user = create_test_admin_user()
+        current_name = user.name
+        new_name = generate_id()
+        response = self.client.patch(
+            reverse("authentik_api:user-detail", kwargs={"pk": user.id}),
+            data={"name": new_name},
+        )
+        user.refresh_from_db()
+        self.assertEqual(response.status_code, 200)
+        events = Event.objects.filter(
+            action=EventAction.MODEL_UPDATED,
+            context__model__model_name="user",
+            context__model__app="authentik_core",
+            context__model__pk=user.pk,
+        )
+        event = events.first()
+        self.assertIsNotNone(event)
+        self.assertIsNotNone(event.context["diff"])
+        diff = event.context["diff"]
+        self.assertEqual(
+            diff,
+            {
+                "name": {
+                    "new_value": new_name,
+                    "previous_value": current_name,
+                },
+            },
+        )
+
+    @patch(
+        "authentik.enterprise.audit.middleware.EnterpriseAuditMiddleware.enabled",
+        PropertyMock(return_value=True),
+    )
+    def test_delete(self):
+        """Test delete audit log"""
+        self.client.force_login(self.user)
+        user = create_test_admin_user()
+        response = self.client.delete(
+            reverse("authentik_api:user-detail", kwargs={"pk": user.id}),
+        )
+        self.assertEqual(response.status_code, 204)
+        events = Event.objects.filter(
+            action=EventAction.MODEL_DELETED,
+            context__model__model_name="user",
+            context__model__app="authentik_core",
+            context__model__pk=user.pk,
+        )
+        event = events.first()
+        self.assertIsNotNone(event)
+        self.assertNotIn("diff", event.context)
+
+    @patch(
+        "authentik.enterprise.audit.middleware.EnterpriseAuditMiddleware.enabled",
+        PropertyMock(return_value=True),
+    )
+    def test_m2m_add(self):
+        """Test m2m add audit log"""
+        self.client.force_login(self.user)
+        user = create_test_admin_user()
+        group = Group.objects.create(name=generate_id())
+        response = self.client.post(
+            reverse("authentik_api:group-add-user", kwargs={"pk": group.group_uuid}),
+            data={
+                "pk": user.pk,
+            },
+        )
+        self.assertEqual(response.status_code, 204)
+        events = Event.objects.filter(
+            action=EventAction.MODEL_UPDATED,
+            context__model__model_name="group",
+            context__model__app="authentik_core",
+            context__model__pk=group.pk.hex,
+        )
+        event = events.first()
+        self.assertIsNotNone(event)
+        self.assertIsNotNone(event.context["diff"])
+        diff = event.context["diff"]
+        self.assertEqual(
+            diff,
+            {"users": {"add": [user.pk]}},
+        )
+
+    @patch(
+        "authentik.enterprise.audit.middleware.EnterpriseAuditMiddleware.enabled",
+        PropertyMock(return_value=True),
+    )
+    def test_m2m_remove(self):
+        """Test m2m remove audit log"""
+        self.client.force_login(self.user)
+        user = create_test_admin_user()
+        group = Group.objects.create(name=generate_id())
+        response = self.client.post(
+            reverse("authentik_api:group-remove-user", kwargs={"pk": group.group_uuid}),
+            data={
+                "pk": user.pk,
+            },
+        )
+        self.assertEqual(response.status_code, 204)
+        events = Event.objects.filter(
+            action=EventAction.MODEL_UPDATED,
+            context__model__model_name="group",
+            context__model__app="authentik_core",
+            context__model__pk=group.pk.hex,
+        )
+        event = events.first()
+        self.assertIsNotNone(event)
+        self.assertIsNotNone(event.context["diff"])
+        diff = event.context["diff"]
+        self.assertEqual(
+            diff,
+            {"users": {"remove": [user.pk]}},
+        )

authentik/events/middleware.py

@@ -214,7 +214,15 @@ class AuditMiddleware:
             model=model_to_dict(instance),
         ).run()
 
-    def m2m_changed_handler(self, request: HttpRequest, sender, instance: Model, action: str, **_):
+    def m2m_changed_handler(
+        self,
+        request: HttpRequest,
+        sender,
+        instance: Model,
+        action: str,
+        thread_kwargs: dict | None = None,
+        **_,
+    ):
         """Signal handler for all object's m2m_changed"""
         if action not in ["pre_add", "pre_remove", "post_clear"]:
             return
@@ -229,4 +237,5 @@ class AuditMiddleware:
             request,
             user=user,
             model=model_to_dict(instance),
+            **thread_kwargs,
         ).run()

web/src/admin/events/EventListPage.ts

@@ -97,7 +97,7 @@ export class EventListPage extends TablePage<Event> {
     }
 
     renderExpanded(item: Event): TemplateResult {
-        return html` <td role="cell" colspan="3">
+        return html` <td role="cell" colspan="5">
                 <div class="pf-c-table__expandable-row-content">
                     <ak-event-info .event=${item as EventWithContext}></ak-event-info>
                 </div>

web/src/components/ak-event-info.ts

@@ -18,6 +18,7 @@ import PFDescriptionList from "@patternfly/patternfly/components/DescriptionList
 import PFList from "@patternfly/patternfly/components/List/list.css";
 import PFTable from "@patternfly/patternfly/components/Table/table.css";
 import PFFlex from "@patternfly/patternfly/layouts/Flex/flex.css";
+import PFSplit from "@patternfly/patternfly/layouts/Split/split.css";
 import PFBase from "@patternfly/patternfly/patternfly-base.css";
 
 import { EventActions, FlowsApi } from "@goauthentik/api";
@@ -81,6 +82,7 @@ export class EventInfo extends AKElement {
             PFCard,
             PFTable,
             PFList,
+            PFSplit,
             PFDescriptionList,
             css`
                 code {
@@ -246,11 +248,17 @@ export class EventInfo extends AKElement {
 
     renderModelChanged() {
         const diff = this.event.context.diff as unknown as {
-            [key: string]: { new_value: unknown; previous_value: unknown };
+            [key: string]: {
+                new_value: unknown;
+                previous_value: unknown;
+                add?: unknown[];
+                remove?: unknown[];
+                clear?: boolean;
+            };
         };
         let diffBody = html``;
         if (diff) {
-            diffBody = html`<div class="pf-l-flex__item">
+            diffBody = html`<div class="pf-l-split__item pf-m-fill">
                     <div class="pf-c-card__title">${msg("Changes made:")}</div>
                     <table class="pf-c-table pf-m-compact pf-m-grid-md" role="grid">
                         <thead>
@@ -262,16 +270,36 @@ export class EventInfo extends AKElement {
                         </thead>
                         <tbody role="rowgroup">
                             ${Object.keys(diff).map((key) => {
+                                const value = diff[key];
+                                const previousCol = value.previous_value
+                                    ? JSON.stringify(value.previous_value, null, 4)
+                                    : msg("-");
+                                let newCol = html``;
+                                if (value.add || value.remove) {
+                                    newCol = html`<ul class="pf-c-list">
+                                        ${(value.add || value.remove)?.map((item) => {
+                                            let itemLabel = "";
+                                            if (value.add) {
+                                                itemLabel = msg(str`Added ID ${item}`);
+                                            } else if (value.remove) {
+                                                itemLabel = msg(str`Removed ID ${item}`);
+                                            }
+                                            return html`<li>${itemLabel}</li>`;
+                                        })}
+                                    </ul>`;
+                                } else if (value.clear) {
+                                    newCol = html`${msg("Cleared")}`;
+                                } else {
+                                    newCol = html`<pre>
+${JSON.stringify(value.new_value, null, 4)}</pre
+                                    >`;
+                                }
                                 return html` <tr role="row">
                                     <td role="cell"><pre>${key}</pre></td>
                                     <td role="cell">
-                                        <pre>
+                                        <pre>${previousCol}</pre>
-${JSON.stringify(diff[key].previous_value, null, 4)}</pre
-                                        >
-                                    </td>
-                                    <td role="cell">
-                                        <pre>${JSON.stringify(diff[key].new_value, null, 4)}</pre>
                                     </td>
+                                    <td role="cell">${newCol}</td>
                                 </tr>`;
                             })}
                         </tbody>
@@ -280,8 +308,8 @@ ${JSON.stringify(diff[key].previous_value, null, 4)}</pre
                 </div>`;
         }
         return html`
-            <div class="pf-l-flex">
+            <div class="pf-l-split">
-                <div class="pf-l-flex__item">
+                <div class="pf-l-split__item pf-m-fill">
                     <div class="pf-c-card__title">${msg("Affected model:")}</div>
                     <div class="pf-c-card__body">
                         ${this.getModelInfo(this.event.context?.model as EventModel)}