diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 4b3b00e7a2..848adb66ff 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/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')))); } /** diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 8df5842929..e88572b586 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/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')); diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php new file mode 100644 index 0000000000..8136bd68e2 --- /dev/null +++ b/app/Http/Requests/DeleteUserRequest.php @@ -0,0 +1,92 @@ +|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; + } +} diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index a3bb7364bd..0f5fbdcec7 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/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'); }