Browse Source

Refactor user deletion logic in a User observer

Bubka 1 year ago
parent
commit
37e4711071

+ 9 - 28
app/Http/Controllers/Auth/UserController.php

@@ -24,6 +24,8 @@ class UserController extends Controller
         $user      = $request->user();
         $validated = $request->validated();
 
+        $this->authorize('update', $user);
+
         if (config('auth.defaults.guard') === 'reverse-proxy-guard' || $user->oauth_provider) {
             Log::notice('Account update rejected: reverse-proxy-guard enabled or account from external sso provider');
 
@@ -57,37 +59,16 @@ class UserController extends Controller
         $validated = $request->validated();
         $user      = Auth::user();
 
-        Log::info(sprintf('Deletion of user ID #%s requested', $user->id));
-
-        if ($user->isAdministrator() && User::admins()->count() == 1) {
-            return response()->json(['message' => __('errors.cannot_delete_the_only_admin')], 400);
-        }
-
         if (! Hash::check($validated['password'], Auth::user()->password)) {
             return response()->json(['message' => __('errors.wrong_current_password')], 400);
         }
 
-        try {
-            DB::transaction(function () use ($user) {
-                DB::table('twofaccounts')->where('user_id', $user->id)->delete();
-                DB::table('groups')->where('user_id', $user->id)->delete();
-                DB::table('webauthn_credentials')->where('authenticatable_id', $user->id)->delete();
-                DB::table(config('auth.passwords.webauthn.table'))->where('email', $user->email)->delete();
-                DB::table('oauth_access_tokens')->where('user_id', $user->id)->delete();
-                DB::table(config('auth.passwords.users.table'))->where('email', $user->email)->delete();
-                DB::table('users')->where('id', $user->id)->delete();
-            });
-        }
-        // @codeCoverageIgnoreStart
-        catch (\Throwable $e) {
-            Log::error(sprintf('Deletion of user ID #%s failed, transaction has been rolled-back', $user->id));
-
-            return response()->json(['message' => __('errors.user_deletion_failed')], 400);
-        }
-        // @codeCoverageIgnoreEnd
-
-        Log::info(sprintf('User ID #%s deleted', $user->id));
-
-        return response()->json(null, 204);
+        // This will delete the user and all its 2FAs & Groups thanks to the onCascadeDelete constrains.
+        // Deletion will not be done (and returns False) if the user is the only existing admin (see UserObserver clas)
+        return $user->delete() === false
+            ? response()->json([
+                'message' => __('errors.cannot_delete_the_only_admin'),
+            ], 400)
+            : response()->json(null, 204);
     }
 }

+ 97 - 0
app/Observers/UserObserver.php

@@ -0,0 +1,97 @@
+<?php
+
+namespace App\Observers;
+
+use App\Models\User;
+use Illuminate\Support\Facades\Auth;
+use Illuminate\Support\Facades\DB;
+use Illuminate\Support\Facades\Log;
+use Illuminate\Support\Facades\Password;
+use Illuminate\Support\Facades\Storage;
+
+class UserObserver
+{
+    /**
+     * Handle the User "created" event.
+     */
+    public function created(User $user): void
+    {
+        //
+    }
+
+    /**
+     * Handle the User "updated" event.
+     */
+    public function updated(User $user): void
+    {
+        //
+    }
+
+    /**
+     * Handle the User "deleting" event.
+     */
+    public function deleting(User $user): bool
+    {
+        Log::info(sprintf('Deletion of User ID #%s requested by User ID #%s', $user->id, Auth::user()->id ?? 'unknown'));
+
+        // Prevent deletion of the only administrator
+        $isLastAdmin = $user->isAdministrator() && User::admins()->count() == 1;
+
+        if ($isLastAdmin) {
+            Log::notice(sprintf('Deletion of user ID #%s refused, cannot delete the only administrator', $user->id));
+            
+            return false;
+        }
+
+        // Deleting user's twofaccounts icon
+        $iconPathes = $user->twofaccounts->filter(function ($twofaccount, $key) {
+            return $twofaccount->icon;
+        })->map(function ($twofaccount, $key) {
+            return $twofaccount->icon;
+        });
+        Storage::disk('icons')->delete($iconPathes->toArray());
+
+        return true;
+    }
+
+    /**
+     * Handle the User "deleted" event.
+     */
+    public function deleted(User $user): void
+    {
+        // DB has cascade delete enabled to flush 2FA and Groups but,
+        // for an unknown reason, SQLite refuses to delete these related.
+        // (despite DB_FOREIGN_KEYS=true which is supposed to enable it)
+        // So it ends with a direct db delete for SQLite...
+        if (DB::getDriverName() === 'sqlite') {
+            DB::table('twofaccounts')->where('user_id', $user->id)->delete();
+            DB::table('groups')->where('user_id', $user->id)->delete();
+        }
+
+        // We flush webauthn credentials & tokens
+        Password::broker('webauthn')->deleteToken($user);
+        $user->flushCredentials();
+
+        // And Passport tokens
+        Password::broker()->deleteToken($user);
+        DB::table('oauth_access_tokens')->where('user_id', $user->id)->delete();
+
+        Log::info(sprintf('User ID #%s and all user traces deleted', $user->id));
+    }
+
+    /**
+     * Handle the User "restored" event.
+     */
+    public function restored(User $user): void
+    {
+        //
+    }
+
+    /**
+     * Handle the User "force deleted" event.
+     */
+    public function forceDeleted(User $user): void
+    {
+        //
+    }
+}

+ 11 - 0
app/Providers/EventServiceProvider.php

@@ -11,6 +11,8 @@ use App\Listeners\DissociateTwofaccountFromGroup;
 use App\Listeners\RegisterOpenId;
 use App\Listeners\ReleaseRadar;
 use App\Listeners\ResetUsersPreference;
+use App\Models\User;
+use App\Observers\UserObserver;
 use Illuminate\Auth\Events\Registered;
 use Illuminate\Auth\Listeners\SendEmailVerificationNotification;
 use Illuminate\Foundation\Support\Providers\EventServiceProvider as ServiceProvider;
@@ -43,6 +45,15 @@ class EventServiceProvider extends ServiceProvider
             RegisterOpenId::class,
         ],
     ];
+    
+    /**
+    * The model observers for your application.
+    *
+    * @var array<string, string|object|array<int, string|object>>
+    */
+    protected $observers = [
+        User::class => [UserObserver::class],
+    ];
 
     /**
      * Register any events for your application.

+ 4 - 0
tests/Feature/Http/Auth/UserControllerTest.php

@@ -8,6 +8,8 @@ use App\Http\Requests\UserUpdateRequest;
 use App\Models\Group;
 use App\Models\TwoFAccount;
 use App\Models\User;
+use App\Observers\UserObserver;
+use App\Policies\UserPolicy;
 use Illuminate\Support\Facades\Config;
 use PHPUnit\Framework\Attributes\CoversClass;
 use Tests\FeatureTestCase;
@@ -16,6 +18,8 @@ use Tests\FeatureTestCase;
  * UserControllerTest test class
  */
 #[CoversClass(UserController::class)]
+#[CoversClass(UserObserver::class)]
+#[CoversClass(UserPolicy::class)]
 #[CoversClass(RejectIfDemoMode::class)]
 #[CoversClass(UserUpdateRequest::class)]
 class UserControllerTest extends FeatureTestCase