mirror of
https://github.com/solidtime-io/solidtime.git
synced 2026-10-07 21:33:18 +01:00
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.
This commit is contained in:
committed by
Constantin Graf
parent
af54b0db73
commit
135984f9ef
@@ -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<AuditFactory> */
|
||||
use HasFactory;
|
||||
|
||||
/**
|
||||
* @return BelongsTo<User, $this>
|
||||
*/
|
||||
public function ownerUser(): BelongsTo
|
||||
{
|
||||
return $this->belongsTo(User::class, 'owner_user_id');
|
||||
}
|
||||
|
||||
/**
|
||||
* @return BelongsTo<Organization, $this>
|
||||
*/
|
||||
public function ownerOrganization(): BelongsTo
|
||||
{
|
||||
return $this->belongsTo(Organization::class, 'owner_organization_id');
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<int|string, string>
|
||||
*/
|
||||
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<string, mixed> $data
|
||||
* @return array<string, mixed>
|
||||
*/
|
||||
public function transformAudit(array $data): array
|
||||
{
|
||||
$data['owner_organization_id'] = $this->getAuditOwnerOrganizationId();
|
||||
$data['owner_user_id'] = $this->getAuditOwnerUserId();
|
||||
|
||||
return $data;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
*
|
||||
|
||||
@@ -32,7 +32,7 @@ return [
|
||||
*/
|
||||
|
||||
'user' => [
|
||||
'morph_prefix' => 'user',
|
||||
'morph_prefix' => 'actor',
|
||||
'guards' => [
|
||||
'web',
|
||||
'api',
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -0,0 +1,42 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Database\Schema\Blueprint;
|
||||
use Illuminate\Support\Facades\Schema;
|
||||
|
||||
return new class extends Migration
|
||||
{
|
||||
/**
|
||||
* Run the migrations.
|
||||
*
|
||||
* Note: Renaming columns and adding nullable columns without default are metadata-only operations in PostgreSQL,
|
||||
* so this migration is fast even for a large audits table.
|
||||
* The indexes and foreign keys for the new columns are added in a separate non-transactional migration.
|
||||
*/
|
||||
public function up(): void
|
||||
{
|
||||
Schema::table('audits', function (Blueprint $table): void {
|
||||
$table->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');
|
||||
});
|
||||
}
|
||||
};
|
||||
@@ -0,0 +1,94 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
use Illuminate\Database\Migrations\Migration;
|
||||
use Illuminate\Support\Facades\DB;
|
||||
|
||||
return new class extends Migration
|
||||
{
|
||||
/**
|
||||
* PostgreSQL cannot build an index concurrently inside a transaction.
|
||||
* Keeping this migration non-transactional prevents long write locks on the (large) audits table in production.
|
||||
* Every step is idempotent, so the migration can be re-run if it fails halfway.
|
||||
*
|
||||
* @var bool
|
||||
*/
|
||||
public $withinTransaction = false;
|
||||
|
||||
/**
|
||||
* Run the migrations.
|
||||
*/
|
||||
public function up(): void
|
||||
{
|
||||
$this->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' : '';
|
||||
}
|
||||
};
|
||||
122
tests/Unit/Model/AuditModelTest.php
Normal file
122
tests/Unit/Model/AuditModelTest.php
Normal file
@@ -0,0 +1,122 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace Tests\Unit\Model;
|
||||
|
||||
use App\Models\Audit;
|
||||
use App\Models\Member;
|
||||
use App\Models\Organization;
|
||||
use App\Models\Project;
|
||||
use App\Models\ProjectMember;
|
||||
use App\Models\TimeEntry;
|
||||
use App\Models\User;
|
||||
use PHPUnit\Framework\Attributes\CoversClass;
|
||||
|
||||
#[CoversClass(Audit::class)]
|
||||
class AuditModelTest extends ModelTestAbstract
|
||||
{
|
||||
public function test_it_belongs_to_an_owner_organization(): void
|
||||
{
|
||||
// Arrange
|
||||
$organization = Organization::factory()->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());
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user