Fixed #14935 - improvements and more tests around user deletion

app/Http/Controllers/Api/UsersController.php

@@ -13,6 +13,8 @@
 use App\Http\Transformers\UsersTransformer;
 use App\Models\Actionlog;
 use App\Models\Asset;
+use App\Models\Accessory;
+use App\Models\Consumable;
 use App\Models\License;
 use App\Models\User;
 use App\Notifications\CurrentInventory;
@@ -31,7 +33,7 @@ class UsersController extends Controller
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v4.0]
      *
-     * @return \Illuminate\Http\Response
+     * @return array
      */
     public function index(Request $request)
     {
@@ -359,7 +361,7 @@ public function selectlist(Request $request)
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v4.0]
      * @param  \Illuminate\Http\Request  $request
-     * @return \Illuminate\Http\Response
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function store(SaveUserRequest $request)
     {
@@ -406,7 +408,7 @@ public function store(SaveUserRequest $request)
      *
      * @author [A. Gianotto] [<[email protected]>]
      * @param  int  $id
-     * @return \Illuminate\Http\Response
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function show($id)
     {
@@ -429,7 +431,7 @@ public function show($id)
      * @since [v4.0]
      * @param  \Illuminate\Http\Request  $request
      * @param  int  $id
-     * @return \Illuminate\Http\Response
+     * @return \Illuminate\Http\JsonResponse
      */
     public function update(SaveUserRequest $request, $id)
     {
@@ -514,18 +516,16 @@ public function update(SaveUserRequest $request, $id)
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v4.0]
      * @param  int  $id
-     * @return \Illuminate\Http\Response
+     * @return \Illuminate\Http\JsonResponse
      */
     public function destroy(DeleteUserRequest $request, $id)
     {
         $this->authorize('delete', User::class);
-        $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id);
 
-        $this->authorize('delete', $user);
+        if ($user = User::withTrashed()->find($id)) {
 
+            $this->authorize('delete', $user);
 
-        if ($user) {
-
             if ($user->delete()) {
 
                 // Remove the user's avatar if they have one
@@ -539,11 +539,12 @@ public function destroy(DeleteUserRequest $request, $id)
 
                 return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.success.delete')));
             }
+
             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'))));
+        return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found')));
 
     }
 
@@ -553,7 +554,7 @@ public function destroy(DeleteUserRequest $request, $id)
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v3.0]
      * @param $userId
-     * @return string JSON
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function assets(Request $request, $id)
     {
@@ -626,7 +627,7 @@ public function emailAssetList(Request $request, $id)
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v3.0]
      * @param $userId
-     * @return string JSON
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function consumables(Request $request, $id)
     {
@@ -644,7 +645,7 @@ public function consumables(Request $request, $id)
      * @author [A. Gianotto] [<[email protected]>]
      * @since [v4.6.14]
      * @param $userId
-     * @return string JSON
+     * @return array
      */
     public function accessories($id)
     {
@@ -663,7 +664,7 @@ public function accessories($id)
      * @author [N. Mathar] [<[email protected]>]
      * @since [v5.0]
      * @param $userId
-     * @return string JSON
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function licenses($id)
     {
@@ -726,7 +727,7 @@ public function postTwoFactorReset(Request $request)
      * @author [Juan Font] [<[email protected]>]
      * @since [v4.4.2]
      * @param  \Illuminate\Http\Request  $request
-     * @return \Illuminate\Http\Response
+     * @return array
      */
     public function getCurrentUserInfo(Request $request)
     {
@@ -739,7 +740,7 @@ public function getCurrentUserInfo(Request $request)
      * @author [E. Taylor] [<[email protected]>]
      * @param int $userId
      * @since [v6.0.0]
-     * @return JsonResponse
+     * @return \Illuminate\Http\JsonResponse
      */
     public function restore($userId = null)
     {

app/Http/Controllers/Users/UsersController.php

@@ -17,7 +17,9 @@
 use Illuminate\Support\Facades\Auth;
 use Illuminate\Database\Eloquent\ModelNotFoundException;
 use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Facades\Password;
+use Illuminate\Support\Facades\Storage;
 use Redirect;
 use Str;
 use Symfony\Component\HttpFoundation\StreamedResponse;
@@ -333,19 +335,24 @@ public function update(SaveUserRequest $request, $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()->first();
+        if ($user = User::find($id)) {
 
-        if (($user) && ($user->deleted_at == '')) {
-            // Delete the user
-            $user->delete();
-            return redirect()->route('users.index')->with('success', trans('admin/users/message.success.delete'));
-        }
+            $this->authorize('delete', $user);
 
-        return redirect()->route('users.index')
-            ->with('error', trans('admin/users/message.user_not_found', compact('id')));
+            if ($user->delete()) {
+                if (Storage::disk('public')->exists('avatars/' . $user->avatar)) {
+                    try {
+                        Storage::disk('public')->delete('avatars/' . $user->avatar);
+                    } catch (\Exception $e) {
+                        Log::debug($e);
+                    }
+                }
+                return redirect()->route('users.index')->with('success', trans('admin/users/message.success.delete'));
+            }
+        }
+        return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found'));
 
     }
 

app/Http/Requests/DeleteUserRequest.php

@@ -6,7 +6,7 @@
 use Illuminate\Validation\Rule;
 use Illuminate\Support\Facades\Auth;
 use App\Models\User;
-use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Gate;
 
 
 class DeleteUserRequest extends FormRequest
@@ -19,18 +19,12 @@ class DeleteUserRequest extends FormRequest
      */
     public function authorize(): bool
     {
-        return true;
+        return Gate::allows('delete', new User);
     }
 
-    /**
-     * Get the validation rules that apply to the request.
-     *
-     * @return array<string, \Illuminate\Contracts\Validation\ValidationRule|array<mixed>|string>
-     */
-    public function rules(): array
+    public function prepareForValidation(): void
     {
-
-        $user_to_delete = User::find(request()->route('user'));
+        $user_to_delete = User::withTrashed()->find(request()->route('user'));
 
         if ($user_to_delete) {
             $this->merge([
@@ -41,30 +35,41 @@ public function rules(): array
                 'assigned_assets' => $user_to_delete->assets()->count(),
                 'assigned_licenses' => $user_to_delete->licenses()->count(),
                 'assigned_accessories' => $user_to_delete->accessories()->count(),
+                'deleted_at' => $user_to_delete->deleted_at,
             ]);
         }
+    }
 
+    /**
+     * Get the validation rules that apply to the request.
+     *
+     * @return array<string, \Illuminate\Contracts\Validation\ValidationRule|array<mixed>|string>
+     */
+    public function rules(): array
+    {
         return [
-            'id' => ['exists:users,id'],
-            'user' =>  Rule::notIn([Auth::user()->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]),
+            'deleted_at' => Rule::in([null]),
         ];
     }
 
     public function messages(): array
     {
 
-        $user_to_delete = User::find(request()->route('user'));
-        $messages = ['id.exists' => trans('admin/users/message.user_not_found')];
+        $user_to_delete = User::withTrashed()->find(request()->route('user'));
+        $messages = [];
 
         if ($user_to_delete) {
 
             $messages = array_merge([
 
+                'user.exists' => trans('admin/users/message.user_not_found'),
+
                 // Cannot delete yourself
                 'user.not_in' => trans('admin/users/message.error.cannot_delete_yourself'),
 
@@ -84,6 +89,8 @@ public function messages(): array
                 // 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()]),
 
+                'deleted_at.in' => trans('admin/users/message.user_deleted_warning'),
+
             ], $messages);
         }
 

database/factories/UserFactory.php

@@ -38,6 +38,16 @@ public function definition()
         ];
     }
 
+    public function deletedUser()
+    {
+        return $this->state(function () {
+            return [
+                'deleted_at' => $this->faker->dateTime(),
+            ];
+        });
+    }
+
+
     public function firstAdmin()
     {
         return $this->state(function () {

tests/Feature/Users/Api/DeleteUserTest.php

@@ -12,6 +12,28 @@ class DeleteUserTest extends TestCase
 {
 
 
+//    public function testErrorReturnedViaApiIfUserDoesNotExist()
+//    {
+//        $this->actingAsForApi(User::factory()->deleteUsers()->create())
+//            ->deleteJson(route('api.users.destroy', 'invalid-id'))
+//            ->assertOk()
+//            ->assertStatus(200)
+//            ->assertStatusMessageIs('error')
+//            ->json();
+//    }
+
+    public function testErrorReturnedViaApiIfUserIsAlreadyDeleted()
+    {
+        $user = User::factory()->deletedUser()->create();
+        $this->actingAsForApi(User::factory()->deleteUsers()->create())
+            ->deleteJson(route('api.users.destroy', $user->id))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+    }
+
+
     public function testDisallowUserDeletionViaApiIfStillManagingPeople()
     {
         $manager = User::factory()->create();
@@ -56,16 +78,28 @@ public function testDisallowUserDeletionViaApiIfStillHasLicenses()
             ->json();
     }
 
-    public function testPermissionsForDeletingUsers()
+    public function testDeniedPermissionsForDeletingUserViaApi()
     {
-
         $this->actingAsForApi(User::factory()->create())
             ->deleteJson(route('api.users.destroy', User::factory()->create()))
             ->assertStatus(403)
             ->json();
     }
 
-    public function testPermissionsIfNotInSameCompanyAndNotSuperadmin()
+    public function testSuccessPermissionsForDeletingUserViaApi()
+    {
+        $this->actingAsForApi(User::factory()->deleteUsers()->create())
+            ->deleteJson(route('api.users.destroy', User::factory()->create()))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
+    }
+
+
+
+
+    public function testPermissionsForDeletingIfNotInSameCompanyAndNotSuperadmin()
     {
         $this->settings->enableMultipleFullCompanySupport();
         [$companyA, $companyB] = Company::factory()->count(2)->create();
@@ -74,6 +108,7 @@ public function testPermissionsIfNotInSameCompanyAndNotSuperadmin()
         $userInCompanyA = $companyA->users()->save(User::factory()->deleteUsers()->make());
         $userInCompanyB = $companyB->users()->save(User::factory()->deleteUsers()->make());
 
+
         $this->actingAsForApi($userInCompanyA)
             ->deleteJson(route('api.users.destroy', $userInCompanyB))
             ->assertStatus(403)

tests/Feature/Users/Ui/DeleteUserTest.php

@@ -14,6 +14,41 @@
 class DeleteUserTest extends TestCase
 {
 
+    public function testUserCanDeleteAnotherUser()
+    {
+        $user = User::factory()->deleteUsers()->viewUsers()->create();
+        $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertTrue($user->isDeletable());
+
+        $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
+            ->delete(route('users.destroy', ['user' => $user->id]))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+
+        $this->followRedirects($response)->assertSee(trans('general.notification_success'));
+    }
+
+
+    public function testErrorReturnedIfUserDoesNotExist()
+    {
+        $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
+            ->delete(route('users.destroy', ['user' => '40596803548609346']))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+        $this->followRedirects($response)->assertSee(trans('alert-danger'));
+    }
+
+    public function testErrorReturnedIfUserIsAlreadyDeleted()
+    {
+        $user = User::factory()->deletedUser()->viewUsers()->create();
+        $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())
+            ->delete(route('users.destroy', $user->id))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+
+          $this->followRedirects($response)->assertSee(trans('general.error'));
+    }
+
+
     public function testPermissionsToDeleteUser()
     {