diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 5a37c50e1b..4b3b00e7a2 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -79,6 +79,10 @@ class UsersController extends Controller ->withCount('assets as assets_count', 'licenses as licenses_count', 'accessories as accessories_count', 'consumables as consumables_count', 'managesUsers as manages_users_count', 'managedLocations as manages_locations_count'); + if ($request->filled('search') != '') { + $users = $users->TextSearch($request->input('search')); + } + if ($request->filled('activated')) { $users = $users->where('users.activated', '=', $request->input('activated')); } @@ -201,8 +205,12 @@ class UsersController extends Controller if ($request->filled('location_id') != '') { $users = $users->UserLocation($request->input('location_id'), $request->input('search')); - } else { - $users = $users->TextSearch($request->input('search')); + } + + if (($request->filled('deleted')) && ($request->input('deleted') == 'true')) { + $users = $users->onlyTrashed(); + } elseif (($request->filled('all')) && ($request->input('all') == 'true')) { + $users = $users->withTrashed(); } $order = $request->input('order') === 'asc' ? 'asc' : 'desc'; @@ -254,7 +262,7 @@ class UsersController extends Controller 'licenses_count', 'consumables_count', 'accessories_count', - 'manages_user_count', + 'manages_users_count', 'manages_locations_count', 'phone', 'address', @@ -274,16 +282,12 @@ class UsersController extends Controller 'website', ]; - $sort = in_array($request->get('sort'), $allowed_columns) ? $request->get('sort') : 'first_name'; + $sort = in_array($request->input('sort'), $allowed_columns) ? $request->input('sort') : 'first_name'; $users = $users->orderBy($sort, $order); break; } - if (($request->filled('deleted')) && ($request->input('deleted') == 'true')) { - $users = $users->onlyTrashed(); - } elseif (($request->filled('all')) && ($request->input('all') == 'true')) { - $users = $users->withTrashed(); - } + // Apply companyable scope $users = Company::scopeCompanyables($users); @@ -535,20 +539,29 @@ class UsersController extends Controller 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('admin/users/message.error.delete_has_assets'))); + 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, 'This user still has ' . $user->licenses->count() . ' license(s) associated with them and cannot be deleted.')); + 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, 'This user still has ' . $user->accessories->count() . ' accessories associated with them.')); + 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, 'This user still has ' . $user->managedLocations()->count() . ' locations that they manage.')); + 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()) { diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 9624753b16..8df5842929 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -346,31 +346,33 @@ class UsersController extends Controller if ($user->id === Auth::id()) { // Redirect to the user management page return redirect()->route('users.index') - ->with('error', 'We would feel really bad if you deleted yourself, please reconsider.'); + ->with('error', trans('admin/users/message.error.cannot_delete_yourself')); } - if (($user->assets()) && (($assetsCount = $user->assets()->count()) > 0)) { + if (($user->assets()) && ($user->assets()->count() > 0)) { // Redirect to the user management page return redirect()->route('users.index') - ->with('error', 'This user still has '.$assetsCount.' assets associated with them.'); + ->with('error', trans_choice('admin/users/message.error.delete_has_assets_var', $user->assets()->count(), ['count'=> $user->assets()->count()])); } - if (($user->licenses()) && (($licensesCount = $user->licenses()->count())) > 0) { - // Redirect to the user management page - return redirect()->route('users.index') - ->with('error', 'This user still has '.$licensesCount.' licenses associated with them.'); + 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()) && (($accessoriesCount = $user->accessories()->count()) > 0)) { + if (($user->accessories()) && ($user->accessories()->count() > 0)) { // Redirect to the user management page - return redirect()->route('users.index') - ->with('error', 'This user still has '.$accessoriesCount.' accessories associated with them.'); + 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()) && (($managedLocationsCount = $user->managedLocations()->count())) > 0) { + if (($user->managedLocations()) && ($user->managedLocations()->count() > 0)) { // Redirect to the user management page return redirect()->route('users.index') - ->with('error', 'This user still has '.$managedLocationsCount.' locations that they manage.'); + ->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()])); } // Delete the user diff --git a/resources/lang/en-US/admin/users/message.php b/resources/lang/en-US/admin/users/message.php index b7c0a29f14..4d014775bd 100644 --- a/resources/lang/en-US/admin/users/message.php +++ b/resources/lang/en-US/admin/users/message.php @@ -37,10 +37,16 @@ return array( 'update' => 'There was an issue updating the user. Please try again.', 'delete' => 'There was an issue deleting the user. Please try again.', 'delete_has_assets' => 'This user has items assigned and could not be deleted.', + 'delete_has_assets_var' => 'This user still has an asset assigned. Please check it in first.|This user still has :count assets assigned. Please check their assets in first.', + 'delete_has_licenses_var' => 'This user still has a license seats assigned. Please check it in first.|This user still has :count license seats assigned. Please check them in first.', + 'delete_has_accessories_var' => 'This user still has an accessory assigned. Please check it in first.|This user still has :count accessories assigned. Please check their assets in first.', + 'delete_has_locations_var' => 'This user still manages a location. Please select another manager first.|This user still manages :count locations. Please select another manager first.', + 'delete_has_users_var' => 'This user still manages another user. Please select another manager for that user first.|This user still manages :count users. Please select another manager for them first.', 'unsuspend' => 'There was an issue unsuspending the user. Please try again.', 'import' => 'There was an issue importing users. Please try again.', 'asset_already_accepted' => 'This asset has already been accepted.', 'accept_or_decline' => 'You must either accept or decline this asset.', + 'cannot_delete_yourself' => 'We would feel really bad if you deleted yourself, please reconsider.', 'incorrect_user_accepted' => 'The asset you have attempted to accept was not checked out to you.', 'ldap_could_not_connect' => 'Could not connect to the LDAP server. Please check your LDAP server configuration in the LDAP config file.
Error from LDAP Server:', 'ldap_could_not_bind' => 'Could not bind to the LDAP server. Please check your LDAP server configuration in the LDAP config file.
Error from LDAP Server: ', diff --git a/tests/Feature/Api/Users/DeleteUsersTest.php b/tests/Feature/Api/Users/DeleteUsersTest.php new file mode 100644 index 0000000000..e926311dca --- /dev/null +++ b/tests/Feature/Api/Users/DeleteUsersTest.php @@ -0,0 +1,115 @@ +create(); + User::factory()->count(5)->create(['manager_id' => $manager->id]); + $this->assertFalse($manager->isDeletable()); + + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', $manager->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + public function testDisallowUserDeletionViaApiIfStillManagingLocations() + { + $manager = User::factory()->create(); + Location::factory()->count(5)->create(['manager_id' => $manager->id]); + + $this->assertFalse($manager->isDeletable()); + + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', $manager->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + public function testDisallowUserDeletionViaApiIfStillHasLicenses() + { + $manager = User::factory()->create(); + LicenseSeat::factory()->count(5)->create(['assigned_to' => $manager->id]); + + $this->assertFalse($manager->isDeletable()); + + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', $manager->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + public function testDisallowUserDeletionIfNoDeletePermissions() + { + + $this->actingAsForApi(User::factory()->create()) + ->deleteJson(route('api.users.destroy', User::factory()->create())) + ->assertStatus(403) + ->json(); + } + + public function testDisallowUserDeletionIfNotInSameCompanyAndNotSuperadmin() + { + $this->settings->enableMultipleFullCompanySupport(); + [$companyA, $companyB] = Company::factory()->count(2)->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 testUsersCannotDeleteThemselves() + { + $user = User::factory()->deleteUsers()->create(); + $this->actingAsForApi($user) + ->deleteJson(route('api.users.destroy', $user)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + + } + + + + + + + +} diff --git a/tests/Feature/Api/Users/UsersDeleteTest.php b/tests/Feature/Api/Users/UsersDeleteTest.php deleted file mode 100644 index cbdba83278..0000000000 --- a/tests/Feature/Api/Users/UsersDeleteTest.php +++ /dev/null @@ -1,42 +0,0 @@ -create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); - User::factory()->create(['first_name' => 'Lowly', 'last_name' => 'Worker', 'manager_id' => $manager->id]); - $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); - } - - public function testDisallowUserDeletionIfStillManagingLocations() - { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); - Location::factory()->create(['manager_id' => $manager->id]); - $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); - } - - public function testAllowUserDeletionIfNotManagingLocations() - { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); - $this->actingAs(User::factory()->deleteUsers()->create())->assertTrue($manager->isDeletable()); - } - - public function testDisallowUserDeletionIfNoDeletePermissions() - { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); - Location::factory()->create(['manager_id' => $manager->id]); - $this->actingAs(User::factory()->editUsers()->create())->assertFalse($manager->isDeletable()); - } - - -} diff --git a/tests/Feature/Users/DeleteUsersTest.php b/tests/Feature/Users/DeleteUsersTest.php new file mode 100644 index 0000000000..a9ac1ab1ff --- /dev/null +++ b/tests/Feature/Users/DeleteUsersTest.php @@ -0,0 +1,66 @@ +create(); + User::factory()->count(3)->create(['manager_id' => $manager->id]); + + $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); + + $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create()) + ->delete(route('users.destroy', $manager->id)) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + + $this->followRedirects($response)->assertSee('Error'); + } + + public function testDisallowUserDeletionIfStillManagingLocations() + { + $manager = User::factory()->create(); + Location::factory()->count(3)->create(['manager_id' => $manager->id]); + + $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); + + $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create()) + ->delete(route('users.destroy', $manager->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()); + + $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create()) + ->delete(route('users.destroy', $manager->id)) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + + $this->followRedirects($response)->assertSee('Success'); + + } + + public function testDisallowUserDeletionIfNoDeletePermissions() + { + $manager = User::factory()->create(); + Location::factory()->create(['manager_id' => $manager->id]); + $this->actingAs(User::factory()->editUsers()->create())->assertFalse($manager->isDeletable()); + } + + +}