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