Merge pull request #14937 from snipe/fixes/user_improvements

Fixed #14935 - improvements and more tests around user deletion

app/Http/Controllers/Api/UsersController.php

@@ -13,6 +13,8 @@ use App\Http\Transformers\SelectlistTransformer;
 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] [<snipe@snipe.net>]
      * @since [v4.0]
      *
-     * @return \Illuminate\Http\Response
+     * @return array
      */
     public function index(Request $request)
     {
@@ -359,7 +361,7 @@ class UsersController extends Controller
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @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 @@ class UsersController extends Controller
      *
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @param  int  $id
-     * @return \Illuminate\Http\Response
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function show($id)
     {
@@ -429,7 +431,7 @@ class UsersController extends Controller
      * @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 @@ class UsersController extends Controller
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @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 @@ class UsersController extends Controller
 
                 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 @@ class UsersController extends Controller
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @since [v3.0]
      * @param $userId
-     * @return string JSON
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function assets(Request $request, $id)
     {
@@ -626,14 +627,14 @@ class UsersController extends Controller
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @since [v3.0]
      * @param $userId
-     * @return string JSON
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function consumables(Request $request, $id)
     {
         $this->authorize('view', User::class);
         $this->authorize('view', Consumable::class);
         $user = User::findOrFail($id);
-        $this->authorize('update', $user);
+        $this->authorize('view', $user);
         $consumables = $user->consumables;
         return (new ConsumablesTransformer)->transformConsumables($consumables, $consumables->count(), $request);
     }
@@ -644,7 +645,7 @@ class UsersController extends Controller
      * @author [A. Gianotto] [<snipe@snipe.net>]
      * @since [v4.6.14]
      * @param $userId
-     * @return string JSON
+     * @return array
      */
     public function accessories($id)
     {
@@ -663,7 +664,7 @@ class UsersController extends Controller
      * @author [N. Mathar] [<snipe@snipe.net>]
      * @since [v5.0]
      * @param $userId
-     * @return string JSON
+     * @return array | \Illuminate\Http\JsonResponse
      */
     public function licenses($id)
     {
@@ -726,7 +727,7 @@ class UsersController extends Controller
      * @author [Juan Font] [<juanfontalonso@gmail.com>]
      * @since [v4.4.2]
      * @param  \Illuminate\Http\Request  $request
-     * @return \Illuminate\Http\Response
+     * @return array
      */
     public function getCurrentUserInfo(Request $request)
     {
@@ -739,12 +740,14 @@ class UsersController extends Controller
      * @author [E. Taylor] [<dev@evantaylor.name>]
      * @param int $userId
      * @since [v6.0.0]
-     * @return JsonResponse
+     * @return \Illuminate\Http\JsonResponse
      */
-    public function restore($userId = null)
+    public function restore($userId)
     {
+        $this->authorize('delete', User::class);
 
         if ($user = User::withTrashed()->find($userId)) {
+
             $this->authorize('delete', $user);
 
             if ($user->deleted_at == '') {
@@ -763,8 +766,6 @@ class UsersController extends Controller
                 return response()->json(Helper::formatStandardApiResponse('success', null, trans('admin/users/message.success.restored')), 200);
             }
 
-            // Check validation to make sure we're not restoring a user with the same username as an existing user
-            return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors()));
         }
 
         return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found')), 200);

app/Http/Controllers/Users/UsersController.php

@@ -17,7 +17,9 @@ use App\Notifications\WelcomeNotification;
 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 @@ class UsersController extends Controller
      */
     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 == '')) {
+            $this->authorize('delete', $user);
-            // Delete the user
+
-            $user->delete();
+            if ($user->delete()) {
-            return redirect()->route('users.index')->with('success', trans('admin/users/message.success.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'));
-        return redirect()->route('users.index')
-            ->with('error', trans('admin/users/message.user_not_found', compact('id')));
 
     }
 

app/Http/Requests/DeleteUserRequest.php

@@ -6,7 +6,7 @@ use Illuminate\Foundation\Http\FormRequest;
 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', User::class);
     }
 
-    /**
+    public function prepareForValidation(): void
-     * 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::withTrashed()->find(request()->route('user'));
-        $user_to_delete = User::find(request()->route('user'));
 
         if ($user_to_delete) {
             $this->merge([
@@ -41,30 +35,41 @@ class DeleteUserRequest extends FormRequest
                 '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'));
+        $user_to_delete = User::withTrashed()->find(request()->route('user'));
-        $messages = ['id.exists' => trans('admin/users/message.user_not_found')];
+        $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 @@ class DeleteUserRequest extends FormRequest
                 // 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 @@ class UserFactory extends Factory
         ];
     }
 
+    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,40 +78,64 @@ class DeleteUserTest extends TestCase
             ->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->settings->enableMultipleFullCompanySupport();
+        $this->actingAsForApi(User::factory()->deleteUsers()->create())
-        [$companyA, $companyB] = Company::factory()->count(2)->create();
+            ->deleteJson(route('api.users.destroy', User::factory()->create()))
-
-        $superUser = $companyA->users()->save(User::factory()->superuser()->make());
-        $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)
-            ->json();
-
-        $this->actingAsForApi($userInCompanyB)
-            ->deleteJson(route('api.users.destroy', $userInCompanyA))
-            ->assertStatus(403)
-            ->json();
-
-        $this->actingAsForApi($superUser)
-            ->deleteJson(route('api.users.destroy', $userInCompanyA))
             ->assertOk()
             ->assertStatus(200)
             ->assertStatusMessageIs('success')
             ->json();
+    }
+
+    
+    public function testPermissionsForDeletingIfNotInSameCompanyAndNotSuperadmin()
+    {
+        $this->settings->enableMultipleFullCompanySupport();
+
+        [$companyA, $companyB] = Company::factory()->count(2)->create();
+
+        $superuser = User::factory()->superuser()->create();
+        $userFromA = User::factory()->deleteUsers()->for($companyA)->create();
+        $userFromB = User::factory()->deleteUsers()->for($companyB)->create();
+
+        $this->actingAsForApi($userFromA)
+            ->deleteJson(route('api.users.destroy', ['user' => $userFromB->id]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+
+        $userFromB->refresh();
+        $this->assertNull($userFromB->deleted_at);
+
+        $this->actingAsForApi($userFromB)
+            ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+
+        $userFromA->refresh();
+        $this->assertNull($userFromA->deleted_at);
+
+        $this->actingAsForApi($superuser)
+            ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
+
+        $userFromA->refresh();
+        $this->assertNotNull($userFromA->deleted_at);
 
     }
 

tests/Feature/Users/Api/RestoreUserTest.php

@@ -0,0 +1,105 @@
+<?php
+
+namespace Tests\Feature\Users\Api;
+
+use App\Models\Company;
+use App\Models\LicenseSeat;
+use App\Models\Location;
+use App\Models\User;
+use Tests\TestCase;
+
+class RestoreUserTest extends TestCase
+{
+
+
+    public function testErrorReturnedViaApiIfUserDoesNotExist()
+    {
+        $this->actingAsForApi(User::factory()->deleteUsers()->create())
+            ->postJson(route('api.users.restore', 'invalid-id'))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+    }
+
+    public function testErrorReturnedViaApiIfUserIsNotDeleted()
+    {
+        $user = User::factory()->create();
+        $this->actingAsForApi(User::factory()->deleteUsers()->create())
+            ->postJson(route('api.users.restore', $user->id))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+    }
+
+
+    public function testDeniedPermissionsForRestoringUserViaApi()
+    {
+        $this->actingAsForApi(User::factory()->create())
+            ->postJson(route('api.users.restore', User::factory()->deletedUser()->create()))
+            ->assertStatus(403)
+            ->json();
+    }
+
+    public function testSuccessPermissionsForRestoringUserViaApi()
+    {
+        $deleted_user = User::factory()->deletedUser()->create();
+
+        $this->actingAsForApi(User::factory()->admin()->create())
+            ->postJson(route('api.users.restore', ['user' => $deleted_user]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
+
+        $deleted_user->refresh();
+        $this->assertNull($deleted_user->deleted_at);
+    }
+
+    public function testPermissionsForRestoringIfNotInSameCompanyAndNotSuperadmin()
+    {
+        $this->settings->enableMultipleFullCompanySupport();
+
+        [$companyA, $companyB] = Company::factory()->count(2)->create();
+
+        $superuser = User::factory()->superuser()->create();
+        $userFromA = User::factory()->deletedUser()->deleteUsers()->for($companyA)->create();
+        $userFromB = User::factory()->deletedUser()->deleteUsers()->for($companyB)->create();
+
+        $this->actingAsForApi($userFromA)
+            ->postJson(route('api.users.restore', ['user' => $userFromB->id]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+
+        $userFromB->refresh();
+        $this->assertNotNull($userFromB->deleted_at);
+
+        $this->actingAsForApi($userFromB)
+            ->postJson(route('api.users.restore', ['user' => $userFromA->id]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('error')
+            ->json();
+
+        $userFromA->refresh();
+        $this->assertNotNull($userFromA->deleted_at);
+
+        $this->actingAsForApi($superuser)
+            ->postJson(route('api.users.restore', ['user' => $userFromA->id]))
+            ->assertOk()
+            ->assertStatus(200)
+            ->assertStatusMessageIs('success')
+            ->json();
+
+        $userFromA->refresh();
+        $this->assertNull($userFromA->deleted_at);
+
+    }
+
+
+
+
+}

tests/Feature/Users/Api/ViewUserTest.php

@@ -20,42 +20,4 @@ class ViewUserTest extends TestCase
             ->assertOk();
     }
 
-    public function testPermissionsWithCompanyableToDeleteUser()
-    {
-
-        $this->settings->enableMultipleFullCompanySupport();
-
-        [$companyA, $companyB] = Company::factory()->count(2)->create();
-
-        $superuser = User::factory()->superuser()->create();
-        $userFromA = User::factory()->for($companyA)->create();
-        $userFromB = User::factory()->for($companyB)->create();
-
-        $this->actingAsForApi(User::factory()->deleteUsers()->for($companyA)->create())
-            ->deleteJson(route('api.users.destroy', $userFromA->id))
-            ->assertOk()
-            ->assertStatus(200)
-            ->assertStatusMessageIs('success')
-            ->json();
-
-        $this->actingAsForApi(User::factory()->deleteUsers()->for($companyB)->create())
-            ->deleteJson(route('api.users.destroy', $userFromA->id))
-            ->assertStatus(403);
-
-        $this->actingAsForApi($superuser)
-            ->deleteJson(route('api.users.destroy', $userFromA->id))
-            ->assertOk()
-            ->assertStatus(200)
-            ->assertStatusMessageIs('success')
-            ->json();
-
-        $this->actingAsForApi($superuser)
-            ->deleteJson(route('api.users.destroy', $userFromB->id))
-            ->assertOk()
-            ->assertStatus(200)
-            ->assertStatusMessageIs('success')
-            ->json();
-
-    }
-
 }

tests/Feature/Users/Ui/DeleteUserTest.php

@@ -14,7 +14,42 @@ use App\Models\Asset;
 class DeleteUserTest extends TestCase
 {
 
-    public function testPermissionsToDeleteUser()
+    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 testFmcsPermissionsToDeleteUser()
     {
 
         $this->settings->enableMultipleFullCompanySupport();
@@ -22,17 +57,35 @@ class DeleteUserTest extends TestCase
         [$companyA, $companyB] = Company::factory()->count(2)->create();
 
         $superuser = User::factory()->superuser()->create();
-        $userFromA = User::factory()->for($companyA)->create();
+        $userFromA = User::factory()->deleteUsers()->for($companyA)->create();
-        $userFromB = User::factory()->for($companyB)->create();
+        $userFromB = User::factory()->deleteUsers()->for($companyB)->create();
 
-        $this->followingRedirects()->actingAs(User::factory()->deleteUsers()->for($companyA)->create())
+        $response =  $this->followingRedirects()->actingAs($userFromA)
             ->delete(route('users.destroy', ['user' => $userFromB->id]))
             ->assertStatus(403);
+        $this->followRedirects($response)->assertSee('sad-panda.png');
 
-        $this->actingAs(User::factory()->deleteUsers()->for($companyA)->create())
+        $userFromB->refresh();
+        $this->assertNull($userFromB->deleted_at);
+
+
+        $response = $this->actingAs($userFromB)
             ->delete(route('users.destroy', ['user' => $userFromA->id]))
             ->assertStatus(302)
             ->assertRedirect(route('users.index'));
+        $this->followRedirects($response)->assertSee('sad-panda.png');
+
+        $userFromA->refresh();
+        $this->assertNull($userFromA->deleted_at);
+
+        $response = $this->actingAs($superuser)
+            ->delete(route('users.destroy', ['user' => $userFromA->id]))
+            ->assertStatus(302)
+            ->assertRedirect(route('users.index'));
+        $this->followRedirects($response)->assertSee('Success');
+
+        $userFromA->refresh();
+        $this->assertNotNull($userFromA->deleted_at);
 
     }