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 64d20eb18f6..dd3e04618f7 100644 --- a/app/Access/Mfa/MfaValue.php +++ b/app/Access/Mfa/MfaValue.php @@ -4,6 +4,7 @@ use BookStack\Users\Models\User; use Carbon\Carbon; +use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; /** @@ -16,6 +17,8 @@ */ 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 b76dbb9ec85..f52f74cc48c 100644 --- a/app/Access/SocialAccount.php +++ b/app/Access/SocialAccount.php @@ -5,18 +5,23 @@ 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 ac9fec5178b..898a6c93a09 100644 --- a/app/Activity/Models/Activity.php +++ b/app/Activity/Models/Activity.php @@ -6,6 +6,7 @@ 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 @@ */ 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 6f6079b0766..6b5e97deeee 100644 --- a/app/Activity/Models/Favourite.php +++ b/app/Activity/Models/Favourite.php @@ -4,11 +4,14 @@ 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 dfb72cc0a36..b088bd72458 100644 --- a/app/Activity/Models/Watch.php +++ b/app/Activity/Models/Watch.php @@ -5,6 +5,7 @@ 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 @@ */ 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 55589f61ec9..c24c72d442a 100644 --- a/app/Entities/Models/Deletion.php +++ b/app/Entities/Models/Deletion.php @@ -4,6 +4,7 @@ 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 @@ */ 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 1a6c980e109..4409afdc222 100644 --- a/app/Entities/Models/PageRevision.php +++ b/app/Entities/Models/PageRevision.php @@ -6,6 +6,7 @@ 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 @@ */ 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 0017653b53d..8bbf11695e2 100644 --- a/app/Users/Models/User.php +++ b/app/Users/Models/User.php @@ -31,8 +31,6 @@ 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 79d9e1b9eb2..7a4fa5f98d2 100644 --- a/app/Users/UserRepo.php +++ b/app/Users/UserRepo.php @@ -5,8 +5,6 @@ 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 @@ public function __construct( ) { } - /** * Get a user by their email address. */ @@ -161,15 +158,12 @@ public function update(User $user, array $data, bool $manageUsersAllowed): User * * @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 @@ public function destroy(User $user, ?int $newOwnerId = null) // 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 @@ protected function ensureDeletable(User $user): void /** * 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 @@ protected function isOnlyAdmin(User $user): bool * * @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 @@ protected function setUserRoles(User $user, array $roles) /** * 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 00000000000..03036b6086e --- /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 00000000000..814f47b5884 --- /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 00000000000..06ec07ced06 --- /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 00000000000..75fba86b371 --- /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 00000000000..0f8b9e6f759 --- /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 00000000000..2368447cd72 --- /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 47e5aa5db2b..538ee5f3ec2 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 00000000000..333916931f5 --- /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 267cd49f551..6fbeb1dd111 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 @@ public function up(): void }); } - // 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 00000000000..75ff6af33cb --- /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 6d8b4d75a53..a34a9e7f4c4 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 @@ public function test_user_creation() $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 @@ public function test_user_password_update() $this->get($userProfilePage)->assertSee('Password confirmation required'); $this->put($userProfilePage, [ - 'password' => 'newpassword', + 'password' => 'newpassword', 'password-confirm' => 'newpassword', ])->assertRedirect('/settings/users'); @@ -167,7 +179,7 @@ public function test_delete_with_new_owner_id_changes_ownership() $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 @@ public function test_delete_with_empty_owner_migration_id_works() $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 @@ public function test_user_creation_is_not_performed_if_the_invitation_sending_fa }); $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 @@ public function test_user_create_activity_is_not_persisted_if_the_invitation_sen }); $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 @@ public function test_return_to_form_with_warning_if_the_invitation_sending_fails }); $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 @@ public function test_user_create_update_fails_if_locale_is_invalid() // 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 7a4fa5f98d2..894d7c01f7d 100644 --- a/app/Users/UserRepo.php +++ b/app/Users/UserRepo.php @@ -198,7 +198,6 @@ protected function removeUserDependantRelations(User $user): void 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 75ff6af33cb..42e67013900 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 @@ 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 @@ public function up(): void 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 8e477668040..0407275e059 100644 --- a/resources/views/settings/audit.blade.php +++ b/resources/views/settings/audit.blade.php @@ -88,7 +88,11 @@ class="text-item">{{ $type }} @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 d29ad1979a0..affc7b6c4c4 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 b18f9cbe08c..2dad617dcad 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 6b435544df7..a6ba6be9f62 100644 --- a/tests/Activity/AuditLogTest.php +++ b/tests/Activity/AuditLogTest.php @@ -83,6 +83,22 @@ public function test_shows_activity_for_deleted_users() $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 a34a9e7f4c4..d50ac208791 100644 --- a/tests/User/UserManagementTest.php +++ b/tests/User/UserManagementTest.php @@ -221,7 +221,6 @@ public function test_delete_with_empty_owner_migration_id_clears_relevant_id_use $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 @@ public function test_delete_with_empty_owner_migration_id_clears_relevant_id_use } // 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 @@ public function test_delete_with_empty_owner_migration_id_clears_relevant_id_use $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()