Merge pull request #14830 from snipe/chore/sc-25756/better_validation_for_relations_on_delete

Better validation for relations on delete

app/Http/Controllers/Api/UsersController.php

@@ -22,6 +22,7 @@ use Illuminate\Http\Request;
 use Illuminate\Support\Facades\Storage;
 use Illuminate\Support\Facades\Validator;
 use Illuminate\Support\Facades\Log;
+use App\Http\Requests\DeleteUserRequest;
 
 class UsersController extends Controller
 {
@@ -530,40 +531,16 @@ class UsersController extends Controller
      * @param  int  $id
      * @return \Illuminate\Http\Response
      */
-    public function destroy($id)
+    public function destroy(DeleteUserRequest $request, $id)
     {
         $this->authorize('delete', User::class);
-        $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed();
+        $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc');
         $user = Company::scopeCompanyables($user)->find($id);
         $this->authorize('delete', $user);
 
+
         if ($user) {
             
-            if ($user->id === Auth::id()) {
-                // Redirect to the user management page
-                return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.error.cannot_delete_yourself')));
-            }
-
-            if (($user->assets) && ($user->assets->count() > 0)) {
-                return response()->json(Helper::formatStandardApiResponse('error', null, trans_choice('admin/users/message.error.delete_has_assets_var', $user->assets()->count(), ['count'=> $user->assets()->count()])));
-            }
-
-            if (($user->licenses) && ($user->licenses->count() > 0)) {
-                return response()->json(Helper::formatStandardApiResponse('error', null, trans_choice('admin/users/message.error.delete_has_licenses_var', $user->licenses()->count(), ['count'=> $user->licenses()->count()])));
-            }
-
-            if (($user->accessories) && ($user->accessories->count() > 0)) {
-                return response()->json(Helper::formatStandardApiResponse('error', null, trans_choice('admin/users/message.error.delete_has_accessories_var', $user->accessories()->count(), ['count'=> $user->accessories()->count()])));
-            }
-
-            if (($user->managedLocations()) && ($user->managedLocations()->count() > 0)) {
-                return response()->json(Helper::formatStandardApiResponse('error', null, trans_choice('admin/users/message.error.delete_has_locations_var', $user->managedLocations()->count(), ['count'=> $user->managedLocations()->count()])));
-            }
-
-            if (($user->managesUsers()) && ($user->managesUsers()->count() > 0)) {
-                return response()->json(Helper::formatStandardApiResponse('error', null, trans_choice('admin/users/message.error.delete_has_users_var', $user->managesUsers()->count(), ['count'=> $user->managesUsers()->count()])));
-            }
-
             if ($user->delete()) {
 
                 // Remove the user's avatar if they have one
@@ -579,7 +556,7 @@ class UsersController extends Controller
             }
         }
 
-        return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.error.delete')));
+        return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id'))));
     }
 
     /**

app/Http/Controllers/Users/UsersController.php

@@ -4,6 +4,7 @@ namespace App\Http\Controllers\Users;
 
 use App\Helpers\Helper;
 use App\Http\Controllers\Controller;
+use App\Http\Requests\DeleteUserRequest;
 use App\Http\Requests\ImageUploadRequest;
 use App\Http\Requests\SaveUserRequest;
 use App\Models\Actionlog;
@@ -333,48 +334,14 @@ class UsersController extends Controller
      * @return \Illuminate\Http\RedirectResponse
      * @throws \Illuminate\Auth\Access\AuthorizationException
      */
-    public function destroy($id = null)
+    public function destroy(DeleteUserRequest $request, $id = null)
     {
 
         $this->authorize('delete', User::class);
-        $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed();
+        $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc');
         $user = Company::scopeCompanyables($user)->find($id);
 
-
-        if ($user) {
-            // Check if we are not trying to delete ourselves
-            if ($user->id === Auth::id()) {
-                // Redirect to the user management page
-                return redirect()->route('users.index')
-                    ->with('error', trans('admin/users/message.error.cannot_delete_yourself'));
-            }
-
-            if (($user->assets()) && ($user->assets()->count() > 0)) {
-                // Redirect to the user management page
-                return redirect()->route('users.index')
-                    ->with('error', trans_choice('admin/users/message.error.delete_has_assets_var', $user->assets()->count(), ['count'=> $user->assets()->count()]));
-            }
-
-            if (($user->licenses()) && ($user->licenses()->count() > 0)) {
-                return redirect()->route('users.index')->with('error', trans_choice('admin/users/message.error.delete_has_licenses_var', $user->licenses()->count(), ['count'=> $user->licenses()->count()]));
-            }
-
-            if (($user->accessories()) && ($user->accessories()->count() > 0)) {
-                // Redirect to the user management page
-                return redirect()->route('users.index')->with('error', trans_choice('admin/users/message.error.delete_has_accessories_var', $user->accessories()->count(), ['count'=> $user->accessories()->count()]));
-            }
-
-            if (($user->managedLocations()) && ($user->managedLocations()->count() > 0)) {
-                // Redirect to the user management page
-                return redirect()->route('users.index')
-                    ->with('error', trans_choice('admin/users/message.error.delete_has_locations_var', $user->managedLocations()->count(), ['count'=> $user->managedLocations()->count()]));
-            }
-
-            if (($user->managesUsers()) && ($user->managesUsers()->count() > 0)) {
-                return redirect()->route('users.index')
-                    ->with('error', trans_choice('admin/users/message.error.delete_has_users_var', $user->managesUsers()->count(), ['count'=> $user->managesUsers()->count()]));
-            }
-
+        if (($user) && ($user->deleted_at = '')) {
             // Delete the user
             $user->delete();
             return redirect()->route('users.index')->with('success', trans('admin/users/message.success.delete'));

app/Http/Requests/DeleteUserRequest.php

@@ -0,0 +1,92 @@
+<?php
+
+namespace App\Http\Requests;
+
+use Illuminate\Foundation\Http\FormRequest;
+use Illuminate\Validation\Rule;
+use Illuminate\Support\Facades\Auth;
+use App\Models\User;
+use Illuminate\Http\Request;
+
+
+class DeleteUserRequest extends FormRequest
+{
+
+    protected $redirectRoute = 'users.index';
+
+    /**
+     * Determine if the user is authorized to make this request.
+     */
+    public function authorize(): bool
+    {
+        return true;
+    }
+
+    /**
+     * Get the validation rules that apply to the request.
+     *
+     * @return array<string, \Illuminate\Contracts\Validation\ValidationRule|array<mixed>|string>
+     */
+    public function rules(): array
+    {
+
+        $user_to_delete = User::find(request()->route('user'));
+
+        if ($user_to_delete) {
+            $this->merge([
+                'user' => request()->route('user'),
+                'admin_id' => Auth::user()->id,
+                'managed_users' => $user_to_delete->managesUsers()->count(),
+                'managed_locations' => $user_to_delete->managedLocations()->count(),
+                'assigned_assets' => $user_to_delete->assets()->count(),
+                'assigned_licenses' => $user_to_delete->licenses()->count(),
+                'assigned_accessories' => $user_to_delete->accessories()->count(),
+            ]);
+        }
+
+        return [
+            'id' => ['exists:users,id'],
+            'user' =>  Rule::notIn([Auth::user()->id]),
+            'managed_users' =>  Rule::in([0]),
+            'managed_locations' => Rule::in([0]),
+            'assigned_assets' => Rule::in([0]),
+            'assigned_licenses' => Rule::in([0]),
+            'assigned_accessories' => Rule::in([0]),
+        ];
+    }
+
+    public function messages(): array
+    {
+
+        $user_to_delete = User::find(request()->route('user'));
+        $messages = ['id.exists' => trans('admin/users/message.user_not_found')];
+
+        if ($user_to_delete) {
+
+            $messages = array_merge([
+
+                // Cannot delete yourself
+                'user.not_in' => trans('admin/users/message.error.cannot_delete_yourself'),
+
+                // managed users is not 0
+                'managed_users.in' => trans_choice('admin/users/message.error.delete_has_users_var', $user_to_delete->managesUsers()->count(), ['count' => $user_to_delete->managesUsers()->count()]),
+
+                // managed locations is not 0
+                'managed_locations.in' => trans_choice('admin/users/message.error.delete_has_locations_var', $user_to_delete->managedLocations()->count(), ['count' => $user_to_delete->managedLocations()->count()]),
+
+
+                // assigned_assets is not 0
+                'assigned_assets.in' => trans_choice('admin/users/message.error.delete_has_assets_var', $user_to_delete->assets()->count(), ['count' => $user_to_delete->assets()->count()]),
+
+                // assigned licenses is not 0
+                'assigned_licenses.in' => trans_choice('admin/users/message.error.delete_has_licenses_var', $user_to_delete->licenses()->count(), ['count' => $user_to_delete->licenses()->count()]),
+
+                // assigned accessories is not 0
+                'assigned_accessories.in' => trans_choice('admin/users/message.error.delete_has_accessories_var', $user_to_delete->accessories()->count(), ['count' => $user_to_delete->accessories()->count()]),
+
+            ], $messages);
+        }
+
+        return $messages;
+    }
+}

tests/Feature/Users/Ui/DeleteUserTest.php

@@ -2,9 +2,13 @@
 
 namespace Tests\Feature\Users\Ui;
 
-use App\Models\Location;
-use App\Models\User;
 use Tests\TestCase;
+use App\Models\LicenseSeat;
+use App\Models\Location;
+use App\Models\Accessory;
+use App\Models\User;
+
+use App\Models\Asset;
 
 class DeleteUserTest extends TestCase
 {
@@ -13,9 +17,9 @@ class DeleteUserTest extends TestCase
     public function testDisallowUserDeletionIfStillManagingPeople()
     {
         $manager = User::factory()->create();
-        User::factory()->count(3)->create(['manager_id' => $manager->id]);
+        User::factory()->count(1)->create(['manager_id' => $manager->id]);
 
-        $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable());
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertFalse($manager->isDeletable());
 
         $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
             ->delete(route('users.destroy', $manager->id))
@@ -28,9 +32,9 @@ class DeleteUserTest extends TestCase
     public function testDisallowUserDeletionIfStillManagingLocations()
     {
         $manager = User::factory()->create();
-        Location::factory()->count(3)->create(['manager_id' => $manager->id]);
+        Location::factory()->count(2)->create(['manager_id' => $manager->id]);
 
-        $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable());
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertFalse($manager->isDeletable());
 
         $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
             ->delete(route('users.destroy', $manager->id))
@@ -40,10 +44,41 @@ class DeleteUserTest extends TestCase
            $this->followRedirects($response)->assertSee('Error');
     }
 
+    public function testDisallowUserDeletionIfStillHaveAccessories()
+    {
+        $user = User::factory()->create();
+        Accessory::factory()->count(3)->checkedOutToUser($user)->create();
+
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertFalse($user->isDeletable());
+
+        $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
+            ->delete(route('users.destroy', $user->id))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+
+        $this->followRedirects($response)->assertSee('Error');
+    }
+
+    public function testDisallowUserDeletionIfStillHaveLicenses()
+    {
+        $user = User::factory()->create();
+        LicenseSeat::factory()->count(4)->create(['assigned_to' => $user->id]);
+
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertFalse($user->isDeletable());
+
+        $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
+            ->delete(route('users.destroy', $user->id))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+
+        $this->followRedirects($response)->assertSee('Error');
+    }
+
+
     public function testAllowUserDeletionIfNotManagingLocations()
     {
         $manager = User::factory()->create();
-        $this->actingAs(User::factory()->deleteUsers()->create())->assertTrue($manager->isDeletable());
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertTrue($manager->isDeletable());
 
         $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
             ->delete(route('users.destroy', $manager->id))
@@ -58,7 +93,43 @@ class DeleteUserTest extends TestCase
     {
         $manager = User::factory()->create();
         Location::factory()->create(['manager_id' => $manager->id]);
-        $this->actingAs(User::factory()->editUsers()->create())->assertFalse($manager->isDeletable());
+        $this->actingAs(User::factory()->editUsers()->viewUsers()->create())->assertFalse($manager->isDeletable());
+    }
+
+    public function testDisallowUserDeletionIfTheyStillHaveAssets()
+    {
+        $user = User::factory()->create();
+        $asset = Asset::factory()->create();
+
+        $this->actingAs(User::factory()->checkoutAssets()->create())
+            ->post(route('hardware.checkout.store', $asset->id), [
+                'checkout_to_type' => 'user',
+                'assigned_user' => $user->id,
+                'name' => 'Changed Name',
+            ]);
+
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertFalse($user->isDeletable());
+
+        $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
+            ->delete(route('users.destroy', $user->id))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+
+        $this->followRedirects($response)->assertSee('Error');
+    }
+
+
+    public function testUsersCannotDeleteThemselves()
+    {
+        $manager = User::factory()->deleteUsers()->viewUsers()->create();
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertTrue($manager->isDeletable());
+
+        $response = $this->actingAs($manager)
+            ->delete(route('users.destroy', $manager->id))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+
+        $this->followRedirects($response)->assertSee('Error');
     }