From 5754acf2fb9fcb60bb2ffd95bab3b0bcb1b162fd Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 19 Oct 2025 19:10:15 +0100 Subject: [PATCH 1/2] DB: Updated handling of deleted user ID handling in DB Updated uses of user ID to nullify on delete. Added testing to cover deletion of user relations. Added model factories to support changes and potential other tests. Cleans existing ID references in the DB via migration. --- app/Access/Mfa/MfaValue.php | 3 + app/Access/SocialAccount.php | 13 +- app/Activity/Models/Activity.php | 3 + app/Activity/Models/Favourite.php | 3 + app/Activity/Models/Watch.php | 3 + app/Entities/Models/Deletion.php | 3 + app/Entities/Models/PageRevision.php | 3 + app/Users/Models/User.php | 2 - app/Users/UserRepo.php | 65 ++++++--- .../factories/Access/Mfa/MfaValueFactory.php | 28 ++++ .../factories/Access/SocialAccountFactory.php | 29 ++++ .../Activity/Models/ActivityFactory.php | 34 +++++ .../Activity/Models/FavouriteFactory.php | 31 +++++ .../Activity/Models/WatchFactory.php | 33 +++++ .../Entities/Models/DeletionFactory.php | 29 ++++ .../factories/Entities/Models/PageFactory.php | 4 +- .../Entities/Models/PageRevisionFactory.php | 40 ++++++ ..._134751_update_entity_relation_columns.php | 3 +- ..._10_18_163331_clean_user_id_references.php | 80 +++++++++++ tests/User/UserManagementTest.php | 130 +++++++++++++++--- 20 files changed, 495 insertions(+), 44 deletions(-) create mode 100644 database/factories/Access/Mfa/MfaValueFactory.php create mode 100644 database/factories/Access/SocialAccountFactory.php create mode 100644 database/factories/Activity/Models/ActivityFactory.php create mode 100644 database/factories/Activity/Models/FavouriteFactory.php create mode 100644 database/factories/Activity/Models/WatchFactory.php create mode 100644 database/factories/Entities/Models/DeletionFactory.php create mode 100644 database/factories/Entities/Models/PageRevisionFactory.php create mode 100644 database/migrations/2025_10_18_163331_clean_user_id_references.php diff --git a/app/Access/Mfa/MfaValue.php b/app/Access/Mfa/MfaValue.php index 64d20eb18..dd3e04618 100644 --- a/app/Access/Mfa/MfaValue.php +++ b/app/Access/Mfa/MfaValue.php @@ -4,6 +4,7 @@ namespace BookStack\Access\Mfa; use BookStack\Users\Models\User; use Carbon\Carbon; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; /** @@ -16,6 +17,8 @@ use Illuminate\Database\Eloquent\Model; */ class MfaValue extends Model { + use HasFactory; + protected static $unguarded = true; const METHOD_TOTP = 'totp'; diff --git a/app/Access/SocialAccount.php b/app/Access/SocialAccount.php index b76dbb9ec..f52f74cc4 100644 --- a/app/Access/SocialAccount.php +++ b/app/Access/SocialAccount.php @@ -5,18 +5,23 @@ namespace BookStack\Access; use BookStack\Activity\Models\Loggable; use BookStack\App\Model; use BookStack\Users\Models\User; +use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Relations\BelongsTo; /** - * Class SocialAccount. - * * @property string $driver * @property User $user */ class SocialAccount extends Model implements Loggable { - protected $fillable = ['user_id', 'driver', 'driver_id', 'timestamps']; + use HasFactory; - public function user() + protected $fillable = ['user_id', 'driver', 'driver_id']; + + /** + * @return BelongsTo + */ + public function user(): BelongsTo { return $this->belongsTo(User::class); } diff --git a/app/Activity/Models/Activity.php b/app/Activity/Models/Activity.php index ac9fec517..898a6c93a 100644 --- a/app/Activity/Models/Activity.php +++ b/app/Activity/Models/Activity.php @@ -6,6 +6,7 @@ use BookStack\App\Model; use BookStack\Entities\Models\Entity; use BookStack\Permissions\Models\JointPermission; use BookStack\Users\Models\User; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; @@ -24,6 +25,8 @@ use Illuminate\Support\Str; */ class Activity extends Model { + use HasFactory; + /** * Get the loggable model related to this activity. * Currently only used for entities (previously entity_[id/type] columns). diff --git a/app/Activity/Models/Favourite.php b/app/Activity/Models/Favourite.php index 6f6079b07..6b5e97dee 100644 --- a/app/Activity/Models/Favourite.php +++ b/app/Activity/Models/Favourite.php @@ -4,11 +4,14 @@ namespace BookStack\Activity\Models; use BookStack\App\Model; use BookStack\Permissions\Models\JointPermission; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; class Favourite extends Model { + use HasFactory; + protected $fillable = ['user_id']; /** diff --git a/app/Activity/Models/Watch.php b/app/Activity/Models/Watch.php index dfb72cc0a..b088bd724 100644 --- a/app/Activity/Models/Watch.php +++ b/app/Activity/Models/Watch.php @@ -5,6 +5,7 @@ namespace BookStack\Activity\Models; use BookStack\Activity\WatchLevels; use BookStack\Permissions\Models\JointPermission; use Carbon\Carbon; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\HasMany; use Illuminate\Database\Eloquent\Relations\MorphTo; @@ -20,6 +21,8 @@ use Illuminate\Database\Eloquent\Relations\MorphTo; */ class Watch extends Model { + use HasFactory; + protected $guarded = []; public function watchable(): MorphTo diff --git a/app/Entities/Models/Deletion.php b/app/Entities/Models/Deletion.php index 55589f61e..c24c72d44 100644 --- a/app/Entities/Models/Deletion.php +++ b/app/Entities/Models/Deletion.php @@ -4,6 +4,7 @@ namespace BookStack\Entities\Models; use BookStack\Activity\Models\Loggable; use BookStack\Users\Models\User; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Relations\BelongsTo; use Illuminate\Database\Eloquent\Relations\MorphTo; @@ -17,6 +18,8 @@ use Illuminate\Database\Eloquent\Relations\MorphTo; */ class Deletion extends Model implements Loggable { + use HasFactory; + protected $hidden = []; /** diff --git a/app/Entities/Models/PageRevision.php b/app/Entities/Models/PageRevision.php index 1a6c980e1..4409afdc2 100644 --- a/app/Entities/Models/PageRevision.php +++ b/app/Entities/Models/PageRevision.php @@ -6,6 +6,7 @@ use BookStack\Activity\Models\Loggable; use BookStack\App\Model; use BookStack\Users\Models\User; use Carbon\Carbon; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Relations\BelongsTo; /** @@ -30,6 +31,8 @@ use Illuminate\Database\Eloquent\Relations\BelongsTo; */ class PageRevision extends Model implements Loggable { + use HasFactory; + protected $fillable = ['name', 'text', 'summary']; protected $hidden = ['html', 'markdown', 'text']; diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php index 0017653b5..8bbf11695 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -31,8 +31,6 @@ use Illuminate\Notifications\Notifiable; use Illuminate\Support\Collection; /** - * Class User. - * * @property int $id * @property string $name * @property string $slug diff --git a/app/Users/UserRepo.php b/app/Users/UserRepo.php index 79d9e1b9e..7a4fa5f98 100644 --- a/app/Users/UserRepo.php +++ b/app/Users/UserRepo.php @@ -5,8 +5,6 @@ namespace BookStack\Users; use BookStack\Access\UserInviteException; use BookStack\Access\UserInviteService; use BookStack\Activity\ActivityType; -use BookStack\Entities\EntityProvider; -use BookStack\Entities\Models\Entity; use BookStack\Exceptions\NotifyException; use BookStack\Exceptions\UserUpdateException; use BookStack\Facades\Activity; @@ -27,7 +25,6 @@ class UserRepo ) { } - /** * Get a user by their email address. */ @@ -161,15 +158,12 @@ class UserRepo * * @throws Exception */ - public function destroy(User $user, ?int $newOwnerId = null) + public function destroy(User $user, ?int $newOwnerId = null): void { $this->ensureDeletable($user); - $user->socialAccounts()->delete(); - $user->apiTokens()->delete(); - $user->favourites()->delete(); - $user->mfaValues()->delete(); - $user->watches()->delete(); + $this->removeUserDependantRelations($user); + $this->nullifyUserNonDependantRelations($user); $user->delete(); // Delete user profile images @@ -178,17 +172,53 @@ class UserRepo // Delete related activities setting()->deleteUserSettings($user->id); + // Migrate or nullify ownership + $newOwner = null; if (!empty($newOwnerId)) { $newOwner = User::query()->find($newOwnerId); - if (!is_null($newOwner)) { - $this->migrateOwnership($user, $newOwner); - } - // TODO - Should be be nullifying ownership instead? } + $this->migrateOwnership($user, $newOwner); Activity::add(ActivityType::USER_DELETE, $user); } + protected function removeUserDependantRelations(User $user): void + { + $user->apiTokens()->delete(); + $user->socialAccounts()->delete(); + $user->favourites()->delete(); + $user->mfaValues()->delete(); + $user->watches()->delete(); + + $tables = ['email_confirmations', 'user_invites', 'views']; + foreach ($tables as $table) { + DB::table($table)->where('user_id', '=', $user->id)->delete(); + } + } + protected function nullifyUserNonDependantRelations(User $user): void + { + $toNullify = [ + 'activities' => ['user_id'], + 'attachments' => ['created_by', 'updated_by'], + 'comments' => ['created_by', 'updated_by'], + 'deletions' => ['deleted_by'], + 'entities' => ['created_by', 'updated_by'], + 'images' => ['created_by', 'updated_by'], + 'imports' => ['created_by'], + 'joint_permissions' => ['owner_id'], + 'page_revisions' => ['created_by'], + 'sessions' => ['user_id'], + ]; + + foreach ($toNullify as $table => $columns) { + foreach ($columns as $column) { + DB::table($table) + ->where($column, '=', $user->id) + ->update([$column => null]); + } + } + } + /** * @throws NotifyException */ @@ -206,11 +236,12 @@ class UserRepo /** * Migrate ownership of items in the system from one user to another. */ - protected function migrateOwnership(User $fromUser, User $toUser): void + protected function migrateOwnership(User $fromUser, User|null $toUser): void { + $newOwnerValue = $toUser ? $toUser->id : null; DB::table('entities') ->where('owned_by', '=', $fromUser->id) - ->update(['owned_by' => $toUser->id]); + ->update(['owned_by' => $newOwnerValue]); } /** @@ -248,7 +279,7 @@ class UserRepo * * @throws UserUpdateException */ - protected function setUserRoles(User $user, array $roles) + protected function setUserRoles(User $user, array $roles): void { $roles = array_filter(array_values($roles)); @@ -261,7 +292,7 @@ class UserRepo /** * Check if the given user is the last admin and their new roles no longer - * contains the admin role. + * contain the admin role. */ protected function demotingLastAdmin(User $user, array $newRoles): bool { diff --git a/database/factories/Access/Mfa/MfaValueFactory.php b/database/factories/Access/Mfa/MfaValueFactory.php new file mode 100644 index 000000000..03036b608 --- /dev/null +++ b/database/factories/Access/Mfa/MfaValueFactory.php @@ -0,0 +1,28 @@ + + */ +class MfaValueFactory extends Factory +{ + protected $model = \BookStack\Access\Mfa\MfaValue::class; + + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + return [ + 'user_id' => User::factory(), + 'method' => 'totp', + 'value' => '123456', + ]; + } +} diff --git a/database/factories/Access/SocialAccountFactory.php b/database/factories/Access/SocialAccountFactory.php new file mode 100644 index 000000000..814f47b58 --- /dev/null +++ b/database/factories/Access/SocialAccountFactory.php @@ -0,0 +1,29 @@ + + */ +class SocialAccountFactory extends Factory +{ + protected $model = \BookStack\Access\SocialAccount::class; + + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + return [ + 'user_id' => User::factory(), + 'driver' => 'github', + 'driver_id' => '123456', + 'avatar' => '', + ]; + } +} diff --git a/database/factories/Activity/Models/ActivityFactory.php b/database/factories/Activity/Models/ActivityFactory.php new file mode 100644 index 000000000..06ec07ced --- /dev/null +++ b/database/factories/Activity/Models/ActivityFactory.php @@ -0,0 +1,34 @@ + + */ +class ActivityFactory extends Factory +{ + protected $model = \BookStack\Activity\Models\Activity::class; + + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + $activities = ActivityType::all(); + $activity = $activities[array_rand($activities)]; + return [ + 'type' => $activity, + 'detail' => 'Activity detail for ' . $activity, + 'user_id' => User::factory(), + 'ip' => $this->faker->ipv4(), + 'loggable_id' => null, + 'loggable_type' => null, + ]; + } +} diff --git a/database/factories/Activity/Models/FavouriteFactory.php b/database/factories/Activity/Models/FavouriteFactory.php new file mode 100644 index 000000000..75fba86b3 --- /dev/null +++ b/database/factories/Activity/Models/FavouriteFactory.php @@ -0,0 +1,31 @@ + + */ +class FavouriteFactory extends Factory +{ + protected $model = \BookStack\Activity\Models\Favourite::class; + + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + $book = Book::query()->first(); + + return [ + 'user_id' => User::factory(), + 'favouritable_id' => $book->id, + 'favouritable_type' => 'book', + ]; + } +} diff --git a/database/factories/Activity/Models/WatchFactory.php b/database/factories/Activity/Models/WatchFactory.php new file mode 100644 index 000000000..0f8b9e6f7 --- /dev/null +++ b/database/factories/Activity/Models/WatchFactory.php @@ -0,0 +1,33 @@ + + */ +class WatchFactory extends Factory +{ + protected $model = \BookStack\Activity\Models\Watch::class; + + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + $book = Book::factory()->create(); + + return [ + 'user_id' => User::factory(), + 'watchable_id' => $book->id, + 'watchable_type' => 'book', + 'level' => WatchLevels::NEW, + ]; + } +} diff --git a/database/factories/Entities/Models/DeletionFactory.php b/database/factories/Entities/Models/DeletionFactory.php new file mode 100644 index 000000000..2368447cd --- /dev/null +++ b/database/factories/Entities/Models/DeletionFactory.php @@ -0,0 +1,29 @@ + + */ +class DeletionFactory extends Factory +{ + protected $model = \BookStack\Entities\Models\Deletion::class; + + /** + * Define the model's default state. + * + * @return array + */ + public function definition(): array + { + return [ + 'deleted_by' => User::factory(), + 'deletable_id' => Page::factory(), + 'deletable_type' => 'page', + ]; + } +} diff --git a/database/factories/Entities/Models/PageFactory.php b/database/factories/Entities/Models/PageFactory.php index 47e5aa5db..538ee5f3e 100644 --- a/database/factories/Entities/Models/PageFactory.php +++ b/database/factories/Entities/Models/PageFactory.php @@ -17,10 +17,8 @@ class PageFactory extends Factory /** * Define the model's default state. - * - * @return array */ - public function definition() + public function definition(): array { $html = '

' . implode('

', $this->faker->paragraphs(5)) . '

'; diff --git a/database/factories/Entities/Models/PageRevisionFactory.php b/database/factories/Entities/Models/PageRevisionFactory.php new file mode 100644 index 000000000..333916931 --- /dev/null +++ b/database/factories/Entities/Models/PageRevisionFactory.php @@ -0,0 +1,40 @@ +' . implode('

', $this->faker->paragraphs(5)) . '

'; + $page = Page::query()->first(); + + return [ + 'page_id' => $page->id, + 'name' => $this->faker->sentence(), + 'html' => $html, + 'text' => strip_tags($html), + 'created_by' => User::factory(), + 'slug' => $page->slug, + 'book_slug' => $page->book->slug, + 'type' => 'version', + 'markdown' => strip_tags($html), + 'summary' => $this->faker->sentence(), + 'revision_number' => rand(1, 4000), + ]; + } +} diff --git a/database/migrations/2025_09_15_134751_update_entity_relation_columns.php b/database/migrations/2025_09_15_134751_update_entity_relation_columns.php index 267cd49f5..6fbeb1dd1 100644 --- a/database/migrations/2025_09_15_134751_update_entity_relation_columns.php +++ b/database/migrations/2025_09_15_134751_update_entity_relation_columns.php @@ -62,8 +62,9 @@ return new class extends Migration }); } - // Convert image zero values to null + // Convert image and activity zero values to null DB::table('images')->where('uploaded_to', '=', 0)->update(['uploaded_to' => null]); + DB::table('activities')->where('loggable_id', '=', 0)->update(['loggable_id' => null]); // Rebuild joint permissions if needed // This was moved here from 2023_01_24_104625_refactor_joint_permissions_storage since the changes diff --git a/database/migrations/2025_10_18_163331_clean_user_id_references.php b/database/migrations/2025_10_18_163331_clean_user_id_references.php new file mode 100644 index 000000000..75ff6af33 --- /dev/null +++ b/database/migrations/2025_10_18_163331_clean_user_id_references.php @@ -0,0 +1,80 @@ + ['user_id'], + 'attachments' => ['created_by', 'updated_by'], + 'comments' => ['created_by', 'updated_by'], + 'deletions' => ['deleted_by'], + 'entities' => ['created_by', 'updated_by', 'owned_by'], + 'images' => ['created_by', 'updated_by'], + 'imports' => ['created_by'], + 'joint_permissions' => ['owner_id'], + 'page_revisions' => ['created_by'], + ]; + + protected static array $toClean = [ + 'api_tokens' => ['user_id'], + 'email_confirmations' => ['user_id'], + 'favourites' => ['user_id'], + 'mfa_values' => ['user_id'], + 'role_user' => ['user_id'], + 'sessions' => ['user_id'], + 'social_accounts' => ['user_id'], + 'user_invites' => ['user_id'], + 'views' => ['user_id'], + 'watches' => ['user_id'], + ]; + + /** + * Run the migrations. + */ + public function up(): void + { + $idSelectQuery = DB::table('users')->select('id'); + + foreach (self::$toNullify as $tableName => $columns) { + Schema::table($tableName, function (Blueprint $table) use ($columns) { + foreach ($columns as $columnName) { + $table->unsignedInteger($columnName)->nullable()->change(); + } + }); + + foreach ($columns as $columnName) { + DB::table($tableName)->where($columnName, '=', 0)->update([$columnName => null]); + DB::table($tableName)->whereNotIn($columnName, $idSelectQuery)->update([$columnName => null]); + } + } + + foreach (self::$toClean as $tableName => $columns) { + foreach ($columns as $columnName) { + DB::table($tableName)->whereNotIn($columnName, $idSelectQuery)->delete(); + } + } + + // TODO - Ensure each is fully handled in user delete + // Start by writing tests for each of these columns + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + foreach (self::$toNullify as $tableName => $columns) { + foreach ($columns as $columnName) { + DB::table($tableName)->whereNull($columnName)->update([$columnName => 0]); + } + + Schema::table($tableName, function (Blueprint $table) use ($columns) { + foreach ($columns as $columnName) { + $table->unsignedInteger($columnName)->nullable(false)->change(); + } + }); + } + } +}; diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index 6d8b4d75a..a34a9e7f4 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -2,9 +2,21 @@ namespace Tests\User; +use BookStack\Access\Mfa\MfaValue; +use BookStack\Access\SocialAccount; use BookStack\Access\UserInviteException; use BookStack\Access\UserInviteService; use BookStack\Activity\ActivityType; +use BookStack\Activity\Models\Activity; +use BookStack\Activity\Models\Comment; +use BookStack\Activity\Models\Favourite; +use BookStack\Activity\Models\View; +use BookStack\Activity\Models\Watch; +use BookStack\Api\ApiToken; +use BookStack\Entities\Models\Deletion; +use BookStack\Entities\Models\PageRevision; +use BookStack\Exports\Import; +use BookStack\Uploads\Attachment; use BookStack\Uploads\Image; use BookStack\Users\Models\Role; use BookStack\Users\Models\User; @@ -28,10 +40,10 @@ class UserManagementTest extends TestCase $this->withHtml($resp)->assertElementContains('form[action="' . url('/settings/users/create') . '"]', 'Save'); $resp = $this->post('/settings/users/create', [ - 'name' => $user->name, - 'email' => $user->email, - 'password' => $user->password, - 'password-confirm' => $user->password, + 'name' => $user->name, + 'email' => $user->email, + 'password' => $user->password, + 'password-confirm' => $user->password, 'roles[' . $adminRole->id . ']' => 'true', ]); $resp->assertRedirect('/settings/users'); @@ -77,7 +89,7 @@ class UserManagementTest extends TestCase $this->get($userProfilePage)->assertSee('Password confirmation required'); $this->put($userProfilePage, [ - 'password' => 'newpassword', + 'password' => 'newpassword', 'password-confirm' => 'newpassword', ])->assertRedirect('/settings/users'); @@ -167,7 +179,7 @@ class UserManagementTest extends TestCase $this->asAdmin()->delete("settings/users/{$owner->id}", ['new_owner_id' => $newOwner->id])->assertRedirect(); $this->assertDatabaseHasEntityData('page', [ - 'id' => $page->id, + 'id' => $page->id, 'owned_by' => $newOwner->id, ]); } @@ -182,6 +194,90 @@ class UserManagementTest extends TestCase $this->assertSessionHas('success'); } + public function test_delete_with_empty_owner_migration_id_clears_relevant_id_uses() + { + $user = $this->users->editor(); + $page = $this->entities->page(); + $this->actingAs($user); + + // Create relations + $activity = Activity::factory()->create(['user_id' => $user->id]); + $attachment = Attachment::factory()->create(['created_by' => $user->id, 'updated_by' => $user->id]); + $comment = Comment::factory()->create(['created_by' => $user->id, 'updated_by' => $user->id]); + $deletion = Deletion::factory()->create(['deleted_by' => $user->id]); + $page->forceFill(['owned_by' => $user->id, 'created_by' => $user->id, 'updated_by' => $user->id])->save(); + $page->rebuildPermissions(); + $image = Image::factory()->create(['created_by' => $user->id, 'updated_by' => $user->id]); + $import = Import::factory()->create(['created_by' => $user->id]); + $revision = PageRevision::factory()->create(['created_by' => $user->id]); + + $apiToken = ApiToken::factory()->create(['user_id' => $user->id]); + \DB::table('email_confirmations')->insert(['user_id' => $user->id, 'token' => 'abc123']); + $favourite = Favourite::factory()->create(['user_id' => $user->id]); + $mfaValue = MfaValue::factory()->create(['user_id' => $user->id]); + $socialAccount = SocialAccount::factory()->create(['user_id' => $user->id]); + \DB::table('user_invites')->insert(['user_id' => $user->id, 'token' => 'abc123']); + View::incrementFor($page); + $watch = Watch::factory()->create(['user_id' => $user->id]); + + $userColumnsByTable = [ + 'activities' => ['user_id'], + 'api_tokens' => ['user_id'], + 'attachments' => ['created_by', 'updated_by'], + 'comments' => ['created_by', 'updated_by'], + 'deletions' => ['deleted_by'], + 'email_confirmations' => ['user_id'], + 'entities' => ['created_by', 'updated_by', 'owned_by'], + 'favourites' => ['user_id'], + 'images' => ['created_by', 'updated_by'], + 'imports' => ['created_by'], + 'joint_permissions' => ['owner_id'], + 'mfa_values' => ['user_id'], + 'page_revisions' => ['created_by'], + 'role_user' => ['user_id'], + 'social_accounts' => ['user_id'], + 'user_invites' => ['user_id'], + 'views' => ['user_id'], + 'watches' => ['user_id'], + ]; + + // Ensure columns have user id before deletion + foreach ($userColumnsByTable as $table => $columns) { + foreach ($columns as $column) { + $this->assertDatabaseHas($table, [$column => $user->id]); + } + } + + $resp = $this->asAdmin()->delete("settings/users/{$user->id}", ['new_owner_id' => '']); + $resp->assertRedirect('/settings/users'); + + // Ensure columns missing user id after deletion + foreach ($userColumnsByTable as $table => $columns) { + foreach ($columns as $column) { + $this->assertDatabaseMissing($table, [$column => $user->id]); + } + } + + // Check models exist where should be retained + $this->assertDatabaseHas('activities', ['id' => $activity->id, 'user_id' => null]); + $this->assertDatabaseHas('attachments', ['id' => $attachment->id, 'created_by' => null, 'updated_by' => null]); + $this->assertDatabaseHas('comments', ['id' => $comment->id, 'created_by' => null, 'updated_by' => null]); + $this->assertDatabaseHas('deletions', ['id' => $deletion->id, 'deleted_by' => null]); + $this->assertDatabaseHas('entities', ['id' => $page->id, 'created_by' => null, 'updated_by' => null, 'owned_by' => null]); + $this->assertDatabaseHas('images', ['id' => $image->id, 'created_by' => null, 'updated_by' => null]); + $this->assertDatabaseHas('imports', ['id' => $import->id, 'created_by' => null]); + $this->assertDatabaseHas('page_revisions', ['id' => $revision->id, 'created_by' => null]); + + // Check models no longer exist where should have been deleted with the user + $this->assertDatabaseMissing('api_tokens', ['id' => $apiToken->id]); + $this->assertDatabaseMissing('email_confirmations', ['token' => 'abc123']); + $this->assertDatabaseMissing('favourites', ['id' => $favourite->id]); + $this->assertDatabaseMissing('mfa_values', ['id' => $mfaValue->id]); + $this->assertDatabaseMissing('social_accounts', ['id' => $socialAccount->id]); + $this->assertDatabaseMissing('user_invites', ['token' => 'abc123']); + $this->assertDatabaseMissing('watches', ['id' => $watch->id]); + } + public function test_delete_removes_user_preferences() { $editor = $this->users->editor(); @@ -247,9 +343,9 @@ class UserManagementTest extends TestCase }); $this->asAdmin()->post('/settings/users/create', [ - 'name' => $user->name, - 'email' => $user->email, - 'send_invite' => 'true', + 'name' => $user->name, + 'email' => $user->email, + 'send_invite' => 'true', 'roles[' . $adminRole->id . ']' => 'true', ]); @@ -267,9 +363,9 @@ class UserManagementTest extends TestCase }); $this->asAdmin()->post('/settings/users/create', [ - 'name' => $user->name, - 'email' => $user->email, - 'send_invite' => 'true', + 'name' => $user->name, + 'email' => $user->email, + 'send_invite' => 'true', ]); $this->assertDatabaseMissing('activities', ['type' => 'USER_CREATE']); @@ -286,9 +382,9 @@ class UserManagementTest extends TestCase }); $resp = $this->asAdmin()->post('/settings/users/create', [ - 'name' => $user->name, - 'email' => $user->email, - 'send_invite' => 'true', + 'name' => $user->name, + 'email' => $user->email, + 'send_invite' => 'true', ]); $resp->assertRedirect('/settings/users/create'); @@ -314,8 +410,8 @@ class UserManagementTest extends TestCase // Both on create $resp = $this->post('/settings/users/create', [ 'language' => 'en 'My name', - 'email' => 'jimmy@example.com', + 'name' => 'My name', + 'email' => 'jimmy@example.com', ]); $resp->assertSessionHasErrors(['language' => 'The language may not be greater than 15 characters.']); $resp->assertSessionHasErrors(['language' => 'The language may only contain letters, numbers, dashes and underscores.']); From efff8700d47f83ab9c4c83b27143d9a27dc69c9a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 19 Oct 2025 19:52:15 +0100 Subject: [PATCH 2/2] DB: Addressed test issues for user ID changes Reverted change for activities table so that a record is retained of past activity, and added a check where the ID may be displayed to ensure it does not mislead and accidentially reference other, newer users. --- app/Users/UserRepo.php | 1 - ...025_10_18_163331_clean_user_id_references.php | 4 ---- resources/views/settings/audit.blade.php | 6 +++++- .../views/settings/parts/table-user.blade.php | 15 +++++---------- .../parts/recycle-bin-list-item.blade.php | 9 ++++++++- tests/Activity/AuditLogTest.php | 16 ++++++++++++++++ tests/User/UserManagementTest.php | 5 +++-- 7 files changed, 37 insertions(+), 19 deletions(-) diff --git a/app/Users/UserRepo.php b/app/Users/UserRepo.php index 7a4fa5f98..894d7c01f 100644 --- a/app/Users/UserRepo.php +++ b/app/Users/UserRepo.php @@ -198,7 +198,6 @@ class UserRepo protected function nullifyUserNonDependantRelations(User $user): void { $toNullify = [ - 'activities' => ['user_id'], 'attachments' => ['created_by', 'updated_by'], 'comments' => ['created_by', 'updated_by'], 'deletions' => ['deleted_by'], diff --git a/database/migrations/2025_10_18_163331_clean_user_id_references.php b/database/migrations/2025_10_18_163331_clean_user_id_references.php index 75ff6af33..42e670139 100644 --- a/database/migrations/2025_10_18_163331_clean_user_id_references.php +++ b/database/migrations/2025_10_18_163331_clean_user_id_references.php @@ -6,7 +6,6 @@ use Illuminate\Support\Facades\Schema; return new class extends Migration { protected static array $toNullify = [ - 'activities' => ['user_id'], 'attachments' => ['created_by', 'updated_by'], 'comments' => ['created_by', 'updated_by'], 'deletions' => ['deleted_by'], @@ -55,9 +54,6 @@ return new class extends Migration { DB::table($tableName)->whereNotIn($columnName, $idSelectQuery)->delete(); } } - - // TODO - Ensure each is fully handled in user delete - // Start by writing tests for each of these columns } /** diff --git a/resources/views/settings/audit.blade.php b/resources/views/settings/audit.blade.php index 8e4776680..0407275e0 100644 --- a/resources/views/settings/audit.blade.php +++ b/resources/views/settings/audit.blade.php @@ -88,7 +88,11 @@ @foreach($activities as $activity)
- @include('settings.parts.table-user', ['user' => $activity->user, 'user_id' => $activity->user_id]) + @if($activity->user && $activity->user->created_at <= $activity->created_at) + @include('settings.parts.table-user', ['user' => $activity->user]) + @else + [ID: {{ $activity->user_id }}] {{ trans('common.deleted_user') }} + @endif
{{ trans('settings.audit_table_event') }} diff --git a/resources/views/settings/parts/table-user.blade.php b/resources/views/settings/parts/table-user.blade.php index d29ad1979..affc7b6c4 100644 --- a/resources/views/settings/parts/table-user.blade.php +++ b/resources/views/settings/parts/table-user.blade.php @@ -1,12 +1,7 @@ {{-- -$user - User mode to display, Can be null. -$user_id - Id of user to show. Must be provided. +$user - User to display. --}} -@if($user) - -
{{ $user->name }}
-
{{ $user->name }}
- -@else - [ID: {{ $user_id }}] {{ trans('common.deleted_user') }} -@endif \ No newline at end of file + +
{{ $user->name }}
+
{{ $user->name }}
+ \ No newline at end of file diff --git a/resources/views/settings/recycle-bin/parts/recycle-bin-list-item.blade.php b/resources/views/settings/recycle-bin/parts/recycle-bin-list-item.blade.php index b18f9cbe0..2dad617dc 100644 --- a/resources/views/settings/recycle-bin/parts/recycle-bin-list-item.blade.php +++ b/resources/views/settings/recycle-bin/parts/recycle-bin-list-item.blade.php @@ -33,7 +33,14 @@ @endif
-
{{ trans('settings.recycle_bin_deleted_by') }}:
@include('settings.parts.table-user', ['user' => $deletion->deleter, 'user_id' => $deletion->deleted_by])
+
+ {{ trans('settings.recycle_bin_deleted_by') }}:
 + @if($deletion->deleter) + @include('settings.parts.table-user', ['user' => $deletion->deleter, 'user_id' => $deletion->deleted_by]) + @else + {{ trans('common.deleted_user') }} + @endif +
{{ trans('settings.recycle_bin_deleted_at') }}:
{{ $deletion->created_at }}
diff --git a/tests/Activity/AuditLogTest.php b/tests/Activity/AuditLogTest.php index 6b435544d..a6ba6be9f 100644 --- a/tests/Activity/AuditLogTest.php +++ b/tests/Activity/AuditLogTest.php @@ -83,6 +83,22 @@ class AuditLogTest extends TestCase $resp->assertSeeText("[ID: {$viewer->id}] Deleted User"); } + public function test_deleted_user_shows_if_user_created_date_is_later_than_activity() + { + $viewer = $this->users->viewer(); + $this->actingAs($viewer); + $page = $this->entities->page(); + $this->activityService->add(ActivityType::PAGE_CREATE, $page); + $viewer->created_at = Carbon::now()->addDay(); + $viewer->save(); + + $this->actingAs($this->users->admin()); + + $resp = $this->get('settings/audit'); + $resp->assertSeeText("[ID: {$viewer->id}] Deleted User"); + $resp->assertDontSee($viewer->name); + } + public function test_filters_by_key() { $this->actingAs($this->users->admin()); diff --git a/tests/User/UserManagementTest.php b/tests/User/UserManagementTest.php index a34a9e7f4..d50ac2087 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -221,7 +221,6 @@ class UserManagementTest extends TestCase $watch = Watch::factory()->create(['user_id' => $user->id]); $userColumnsByTable = [ - 'activities' => ['user_id'], 'api_tokens' => ['user_id'], 'attachments' => ['created_by', 'updated_by'], 'comments' => ['created_by', 'updated_by'], @@ -259,7 +258,6 @@ class UserManagementTest extends TestCase } // Check models exist where should be retained - $this->assertDatabaseHas('activities', ['id' => $activity->id, 'user_id' => null]); $this->assertDatabaseHas('attachments', ['id' => $attachment->id, 'created_by' => null, 'updated_by' => null]); $this->assertDatabaseHas('comments', ['id' => $comment->id, 'created_by' => null, 'updated_by' => null]); $this->assertDatabaseHas('deletions', ['id' => $deletion->id, 'deleted_by' => null]); @@ -276,6 +274,9 @@ class UserManagementTest extends TestCase $this->assertDatabaseMissing('social_accounts', ['id' => $socialAccount->id]); $this->assertDatabaseMissing('user_invites', ['token' => 'abc123']); $this->assertDatabaseMissing('watches', ['id' => $watch->id]); + + // Ensure activity remains using the old ID (Special case for auditing changes) + $this->assertDatabaseHas('activities', ['id' => $activity->id, 'user_id' => $user->id]); } public function test_delete_removes_user_preferences()