From 135984f9efba0ad616b75347ea4df2b70a5535ef Mon Sep 17 00:00:00 2001 From: Constantin Graf Date: Wed, 7 Oct 2026 11:49:51 +0200 Subject: [PATCH] Add owner organization and owner user to audits - Rename the audit actor columns user_type/user_id to actor_type/actor_id - Add owner_organization_id and owner_user_id to the audits table as foreign keys with cascade on delete, so that the audits of an organization or user are deleted together with it. The indexes and foreign keys are created without blocking writes on the large table. - Fill the owner columns for new audits via CustomAuditable. Organizations and users no longer record their own deletion audit, since it would reference the already deleted owner. --- app/Models/Audit.php | 25 +++- app/Models/Concerns/CustomAuditable.php | 59 +++++++++ app/Models/Organization.php | 10 ++ app/Models/ProjectMember.php | 8 ++ app/Models/User.php | 10 ++ config/audit.php | 2 +- .../2024_09_02_094105_create_audits_table.php | 3 +- ...0001_add_owner_columns_to_audits_table.php | 42 ++++++ ...add_owner_foreign_keys_to_audits_table.php | 94 ++++++++++++++ tests/Unit/Model/AuditModelTest.php | 122 ++++++++++++++++++ tests/Unit/Service/DeletionServiceTest.php | 25 ++++ 11 files changed, 396 insertions(+), 4 deletions(-) create mode 100644 database/migrations/2026_10_06_000001_add_owner_columns_to_audits_table.php create mode 100644 database/migrations/2026_10_06_000002_add_owner_foreign_keys_to_audits_table.php create mode 100644 tests/Unit/Model/AuditModelTest.php diff --git a/app/Models/Audit.php b/app/Models/Audit.php index c7466715..312ff0db 100644 --- a/app/Models/Audit.php +++ b/app/Models/Audit.php @@ -6,13 +6,14 @@ namespace App\Models; use Database\Factories\AuditFactory; use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Support\Carbon; use OwenIt\Auditing\Models\Audit as PackageAuditModel; /** * @property int $id - * @property string|null $user_type - * @property string|null $user_id + * @property string|null $actor_type + * @property string|null $actor_id * @property string $event * @property string $auditable_type * @property string $auditable_id @@ -22,8 +23,12 @@ use OwenIt\Auditing\Models\Audit as PackageAuditModel; * @property string|null $ip_address * @property string|null $user_agent * @property string|null $tags + * @property string|null $owner_user_id + * @property string|null $owner_organization_id * @property Carbon|null $created_at * @property Carbon|null $updated_at + * @property-read User|null $ownerUser + * @property-read Organization|null $ownerOrganization * * @method static AuditFactory factory() */ @@ -31,4 +36,20 @@ class Audit extends PackageAuditModel { /** @use HasFactory */ use HasFactory; + + /** + * @return BelongsTo + */ + public function ownerUser(): BelongsTo + { + return $this->belongsTo(User::class, 'owner_user_id'); + } + + /** + * @return BelongsTo + */ + public function ownerOrganization(): BelongsTo + { + return $this->belongsTo(Organization::class, 'owner_organization_id'); + } } diff --git a/app/Models/Concerns/CustomAuditable.php b/app/Models/Concerns/CustomAuditable.php index af03c25d..150bfb21 100644 --- a/app/Models/Concerns/CustomAuditable.php +++ b/app/Models/Concerns/CustomAuditable.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Models\Concerns; +use Illuminate\Support\Facades\Config; use OwenIt\Auditing\Auditable; trait CustomAuditable @@ -19,4 +20,62 @@ trait CustomAuditable { $this->auditEvents = []; } + + /** + * The organization that owns the audited model. + * The audits of the model are deleted (via foreign key cascade) when the organization is deleted. + */ + public function getAuditOwnerOrganizationId(): ?string + { + return $this->getAttributes()['organization_id'] ?? null; + } + + /** + * The user that owns the audited model. + * The audits of the model are deleted (via foreign key cascade) when the user is deleted. + */ + public function getAuditOwnerUserId(): ?string + { + return null; + } + + /** + * Models that are the owner of their own audits can not record the deletion audit, + * since the audit would reference the already deleted model and therefore violate the foreign key. + */ + protected function isAuditOwnerOfItself(): bool + { + return false; + } + + /** + * @return array + */ + public function getAuditEvents(): array + { + $events = $this->auditEvents ?? Config::get('audit.events', [ + 'created', + 'updated', + 'deleted', + 'restored', + ]); + + if ($this->isAuditOwnerOfItself()) { + $events = array_filter($events, fn (string $value, int|string $key): bool => (is_int($key) ? $value : $key) !== 'deleted', ARRAY_FILTER_USE_BOTH); + } + + return $events; + } + + /** + * @param array $data + * @return array + */ + public function transformAudit(array $data): array + { + $data['owner_organization_id'] = $this->getAuditOwnerOrganizationId(); + $data['owner_user_id'] = $this->getAuditOwnerUserId(); + + return $data; + } } diff --git a/app/Models/Organization.php b/app/Models/Organization.php index 06d13c3e..b5f8fc92 100644 --- a/app/Models/Organization.php +++ b/app/Models/Organization.php @@ -96,6 +96,16 @@ class Organization extends Model implements AuditableContract protected $attributes = [ ]; + public function getAuditOwnerOrganizationId(): ?string + { + return $this->getKey(); + } + + protected function isAuditOwnerOfItself(): bool + { + return true; + } + /** * Get all the users that belong to the team. * diff --git a/app/Models/ProjectMember.php b/app/Models/ProjectMember.php index 698f5bb7..5d45095d 100644 --- a/app/Models/ProjectMember.php +++ b/app/Models/ProjectMember.php @@ -82,4 +82,12 @@ class ProjectMember extends Model implements AuditableContract $query->whereBelongsTo($organization, 'organization'); }); } + + public function getAuditOwnerOrganizationId(): ?string + { + /** @var string|null $organizationId */ + $organizationId = Project::query()->whereKey($this->project_id)->value('organization_id'); + + return $organizationId; + } } diff --git a/app/Models/User.php b/app/Models/User.php index 3ba34234..9cfd8b9d 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -124,6 +124,16 @@ class User extends Authenticatable implements AuditableContract, FilamentUser, M 'send_time_entry_still_running_email' => true, ]; + public function getAuditOwnerUserId(): ?string + { + return $this->getKey(); + } + + protected function isAuditOwnerOfItself(): bool + { + return true; + } + /** * Get the URL to the user's profile photo. * diff --git a/config/audit.php b/config/audit.php index 81fc8076..b0e4ac14 100644 --- a/config/audit.php +++ b/config/audit.php @@ -32,7 +32,7 @@ return [ */ 'user' => [ - 'morph_prefix' => 'user', + 'morph_prefix' => 'actor', 'guards' => [ 'web', 'api', diff --git a/database/migrations/2024_09_02_094105_create_audits_table.php b/database/migrations/2024_09_02_094105_create_audits_table.php index c8550764..c504b04e 100644 --- a/database/migrations/2024_09_02_094105_create_audits_table.php +++ b/database/migrations/2024_09_02_094105_create_audits_table.php @@ -18,7 +18,8 @@ class CreateAuditsTable extends Migration Schema::connection($connection)->create($table, function (Blueprint $table): void { - $morphPrefix = config('audit.user.morph_prefix', 'user'); + // Note: The morph prefix is hardcoded, since the columns are renamed in a later migration + $morphPrefix = 'user'; $table->bigIncrements('id'); $table->string($morphPrefix.'_type')->nullable(); diff --git a/database/migrations/2026_10_06_000001_add_owner_columns_to_audits_table.php b/database/migrations/2026_10_06_000001_add_owner_columns_to_audits_table.php new file mode 100644 index 00000000..06692ed7 --- /dev/null +++ b/database/migrations/2026_10_06_000001_add_owner_columns_to_audits_table.php @@ -0,0 +1,42 @@ +renameColumn('user_type', 'actor_type'); + $table->renameColumn('user_id', 'actor_id'); + $table->renameIndex('audits_user_id_user_type_index', 'audits_actor_id_actor_type_index'); + $table->uuid('owner_user_id')->nullable(); + $table->uuid('owner_organization_id')->nullable(); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::table('audits', function (Blueprint $table): void { + $table->dropColumn('owner_user_id'); + $table->dropColumn('owner_organization_id'); + $table->renameIndex('audits_actor_id_actor_type_index', 'audits_user_id_user_type_index'); + $table->renameColumn('actor_type', 'user_type'); + $table->renameColumn('actor_id', 'user_id'); + }); + } +}; diff --git a/database/migrations/2026_10_06_000002_add_owner_foreign_keys_to_audits_table.php b/database/migrations/2026_10_06_000002_add_owner_foreign_keys_to_audits_table.php new file mode 100644 index 00000000..7aad0f90 --- /dev/null +++ b/database/migrations/2026_10_06_000002_add_owner_foreign_keys_to_audits_table.php @@ -0,0 +1,94 @@ +createIndex('audits_owner_user_id_index', 'owner_user_id'); + $this->createIndex('audits_owner_organization_id_index', 'owner_organization_id'); + + $this->createForeignKey('audits_owner_user_id_foreign', 'owner_user_id', 'users'); + $this->createForeignKey('audits_owner_organization_id_foreign', 'owner_organization_id', 'organizations'); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + DB::statement('ALTER TABLE audits DROP CONSTRAINT IF EXISTS audits_owner_user_id_foreign'); + DB::statement('ALTER TABLE audits DROP CONSTRAINT IF EXISTS audits_owner_organization_id_foreign'); + DB::statement('DROP INDEX'.$this->concurrently().' IF EXISTS audits_owner_user_id_index'); + DB::statement('DROP INDEX'.$this->concurrently().' IF EXISTS audits_owner_organization_id_index'); + } + + private function createIndex(string $index, string $column): void + { + $state = DB::selectOne( + <<<'SQL' + SELECT pg_index.indisvalid::int AS valid + FROM pg_index + JOIN pg_class ON pg_class.oid = pg_index.indexrelid + JOIN pg_namespace ON pg_namespace.oid = pg_class.relnamespace + WHERE pg_namespace.nspname = current_schema() + AND pg_class.relname = ? + SQL, + [$index], + ); + + if ($state !== null && (bool) $state->valid) { + return; + } + + // A failed concurrent index build leaves an invalid index behind, that needs to be dropped before rebuilding + if ($state !== null) { + DB::statement('DROP INDEX'.$this->concurrently().' '.$index); + } + + DB::statement('CREATE INDEX'.$this->concurrently().' '.$index.' ON audits ('.$column.')'); + } + + private function createForeignKey(string $constraint, string $column, string $referencedTable): void + { + $exists = DB::selectOne( + <<<'SQL' + SELECT 1 + FROM pg_constraint + JOIN pg_namespace ON pg_namespace.oid = pg_constraint.connamespace + WHERE pg_namespace.nspname = current_schema() + AND pg_constraint.conname = ? + SQL, + [$constraint], + ) !== null; + + // Note: Adding the constraint as NOT VALID only needs a short lock, the validation of the existing rows + // afterward does not block reads or writes on the audits table. + if (! $exists) { + DB::statement('ALTER TABLE audits ADD CONSTRAINT '.$constraint.' FOREIGN KEY ('.$column.') REFERENCES '.$referencedTable.' (id) ON DELETE CASCADE NOT VALID'); + } + + DB::statement('ALTER TABLE audits VALIDATE CONSTRAINT '.$constraint); + } + + private function concurrently(): string + { + return DB::transactionLevel() === 0 ? ' CONCURRENTLY' : ''; + } +}; diff --git a/tests/Unit/Model/AuditModelTest.php b/tests/Unit/Model/AuditModelTest.php new file mode 100644 index 00000000..bc465c7c --- /dev/null +++ b/tests/Unit/Model/AuditModelTest.php @@ -0,0 +1,122 @@ +create(); + $audit = Audit::factory()->create([ + 'owner_organization_id' => $organization->getKey(), + ]); + + // Act + $audit->refresh(); + $ownerOrganizationRel = $audit->ownerOrganization; + + // Assert + $this->assertNotNull($ownerOrganizationRel); + $this->assertTrue($ownerOrganizationRel->is($organization)); + } + + public function test_it_belongs_to_an_owner_user(): void + { + // Arrange + $user = User::factory()->create(); + $audit = Audit::factory()->create([ + 'owner_user_id' => $user->getKey(), + ]); + + // Act + $audit->refresh(); + $ownerUserRel = $audit->ownerUser; + + // Assert + $this->assertNotNull($ownerUserRel); + $this->assertTrue($ownerUserRel->is($user)); + } + + public function test_audits_of_models_with_organization_have_the_organization_as_owner(): void + { + // Arrange + $organization = Organization::factory()->create(); + $member = Member::factory()->forOrganization($organization)->create(); + + // Act + $timeEntry = TimeEntry::factory()->forOrganization($organization)->forMember($member)->create(); + + // Assert + $audit = Audit::query()->where('auditable_id', $timeEntry->getKey())->sole(); + $this->assertSame($organization->getKey(), $audit->owner_organization_id); + $this->assertNull($audit->owner_user_id); + } + + public function test_audits_of_project_members_have_the_organization_of_the_project_as_owner(): void + { + // Arrange + $organization = Organization::factory()->create(); + $project = Project::factory()->forOrganization($organization)->create(); + $member = Member::factory()->forOrganization($organization)->create(); + + // Act + $projectMember = ProjectMember::factory()->forProject($project)->forMember($member)->create(); + + // Assert + $audit = Audit::query()->where('auditable_id', $projectMember->getKey())->sole(); + $this->assertSame($organization->getKey(), $audit->owner_organization_id); + $this->assertNull($audit->owner_user_id); + } + + public function test_audits_of_an_organization_have_the_organization_itself_as_owner(): void + { + // Act + $organization = Organization::factory()->create(); + + // Assert + $audit = Audit::query()->where('auditable_id', $organization->getKey())->sole(); + $this->assertSame($organization->getKey(), $audit->owner_organization_id); + $this->assertNull($audit->owner_user_id); + } + + public function test_audits_of_a_user_have_the_user_itself_as_owner(): void + { + // Act + $user = User::factory()->create(); + + // Assert + $audit = Audit::query()->where('auditable_id', $user->getKey())->sole(); + $this->assertSame($user->getKey(), $audit->owner_user_id); + $this->assertNull($audit->owner_organization_id); + } + + public function test_deleting_an_owner_deletes_its_audits_and_does_not_create_a_deletion_audit(): void + { + // Arrange + $user = User::factory()->create(); + $organization = Organization::factory()->create(); + $otherUser = User::factory()->create(); + + // Act + $user->delete(); + $organization->delete(); + + // Assert + $this->assertSame(0, Audit::query()->where('auditable_id', $user->getKey())->count()); + $this->assertSame(0, Audit::query()->where('auditable_id', $organization->getKey())->count()); + $this->assertSame(1, Audit::query()->where('auditable_id', $otherUser->getKey())->count()); + } +} diff --git a/tests/Unit/Service/DeletionServiceTest.php b/tests/Unit/Service/DeletionServiceTest.php index 30b4ccc9..ad767abb 100644 --- a/tests/Unit/Service/DeletionServiceTest.php +++ b/tests/Unit/Service/DeletionServiceTest.php @@ -7,6 +7,7 @@ namespace Tests\Unit\Service; use App\Enums\Role; use App\Events\BeforeOrganizationDeletion; use App\Exceptions\Api\CanNotDeleteUserWhoIsOwnerOfOrganizationWithMultipleMembers; +use App\Models\Audit; use App\Models\Client; use App\Models\Member; use App\Models\Organization; @@ -182,6 +183,8 @@ class DeletionServiceTest extends TestCaseWithDatabase // Assert $this->assertOrganizationDeleted($organization->organization); $this->assertOrganizationNothingDeleted($otherOrganization->organization); + $this->assertSame(0, Audit::query()->where('owner_organization_id', $organization->organization->getKey())->count()); + $this->assertGreaterThan(0, Audit::query()->where('owner_organization_id', $otherOrganization->organization->getKey())->count()); Log::assertLoggedTimes(fn (LogEntry $log) => $log->level === 'debug' && $log->message === 'Start deleting organization' && $log->context['organization_id'] === $organization->organization->getKey(), @@ -318,6 +321,10 @@ class DeletionServiceTest extends TestCaseWithDatabase $this->assertDatabaseMissing(Member::class, [ 'user_id' => $user->getKey(), ]); + $this->assertSame(0, Audit::query()->where('owner_user_id', $user->getKey())->count()); + $this->assertSame(0, Audit::query()->where('owner_organization_id', $user->current_team_id)->count()); + $this->assertGreaterThan(0, Audit::query()->where('owner_user_id', $otherUser->getKey())->count()); + $this->assertGreaterThan(0, Audit::query()->where('owner_organization_id', $otherUser->current_team_id)->count()); Storage::disk(config('filesystems.public'))->assertMissing($user->profile_photo_path); Storage::disk(config('filesystems.public'))->assertExists($otherUser->profile_photo_path); Log::assertLoggedTimes(fn (LogEntry $log) => $log->level === 'debug' @@ -332,6 +339,24 @@ class DeletionServiceTest extends TestCaseWithDatabase ); } + public function test_delete_user_keeps_audits_of_other_organizations_where_the_user_is_the_actor(): void + { + // Arrange + $user = User::factory()->withPersonalOrganization()->create(); + $otherOrganization = Organization::factory()->create(); + $audit = Audit::factory()->auditUser($user)->auditFor($otherOrganization)->create([ + 'owner_organization_id' => $otherOrganization->getKey(), + ]); + + // Act + $this->deletionService->deleteUser($user); + + // Assert + $audit->refresh(); + $this->assertSame($user->getKey(), $audit->actor_id); + $this->assertSame($otherOrganization->getKey(), $audit->owner_organization_id); + } + public function test_delete_user_deletes_owned_organizations_that_have_only_one_member_and_makes_makes_the_user_placeholder_in_not_owned_organizations(): void { // Arrange