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.

Author: ssddanbrown

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';

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<User, $this>
+     */
+    public function user(): BelongsTo
     {
         return $this->belongsTo(User::class);
     }

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).

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'];
 
     /**

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

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 = [];
 
     /**

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'];
 

app/Users/Models/User.php

@@ -31,8 +31,6 @@
 use Illuminate\Support\Collection;
 
 /**
- * Class User.
- *
  * @property int        $id
  * @property string     $name
  * @property string     $slug

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
     {

database/factories/Access/Mfa/MfaValueFactory.php

@@ -0,0 +1,28 @@
+<?php
+
+namespace Database\Factories\Access\Mfa;
+
+use BookStack\Users\Models\User;
+use Illuminate\Database\Eloquent\Factories\Factory;
+
+/**
+ * @extends \Illuminate\Database\Eloquent\Factories\Factory<\BookStack\Access\Mfa\MfaValue>
+ */
+class MfaValueFactory extends Factory
+{
+    protected $model = \BookStack\Access\Mfa\MfaValue::class;
+
+    /**
+     * Define the model's default state.
+     *
+     * @return array<string, mixed>
+     */
+    public function definition(): array
+    {
+        return [
+            'user_id' => User::factory(),
+            'method' => 'totp',
+            'value' => '123456',
+        ];
+    }
+}