From c09e93e288afe4bf553102be5d98b6c41e8358b3 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 1 Jun 2024 02:59:04 +0100 Subject: [PATCH 1/6] Updated delete users API tests, moved non-API tests Signed-off-by: snipe --- tests/Feature/Api/Users/DeleteUsersTest.php | 69 +++++++++++++++++++ .../DeleteUsersTest.php} | 4 +- 2 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/Api/Users/DeleteUsersTest.php rename tests/Feature/{Api/Users/UsersDeleteTest.php => Users/DeleteUsersTest.php} (95%) diff --git a/tests/Feature/Api/Users/DeleteUsersTest.php b/tests/Feature/Api/Users/DeleteUsersTest.php new file mode 100644 index 0000000000..830008d0bc --- /dev/null +++ b/tests/Feature/Api/Users/DeleteUsersTest.php @@ -0,0 +1,69 @@ +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(); + } + + + +} diff --git a/tests/Feature/Api/Users/UsersDeleteTest.php b/tests/Feature/Users/DeleteUsersTest.php similarity index 95% rename from tests/Feature/Api/Users/UsersDeleteTest.php rename to tests/Feature/Users/DeleteUsersTest.php index cbdba83278..9903081e20 100644 --- a/tests/Feature/Api/Users/UsersDeleteTest.php +++ b/tests/Feature/Users/DeleteUsersTest.php @@ -1,13 +1,13 @@ Date: Sat, 1 Jun 2024 03:00:45 +0100 Subject: [PATCH 2/6] Fixed typo Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 31 ++++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 5a37c50e1b..c2c18748b0 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); @@ -551,6 +555,15 @@ class UsersController extends Controller return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has ' . $user->managedLocations()->count() . ' locations that they manage.')); } + + + if (($user->managesUsers()) && ($user->managesUsers()->count() > 0)) { + + return response()->json(Helper::formatStandardApiResponse('error', null, 'This user still has ' . $user->managesUsers()->count() . ' users that they manage.')); + } + + + if ($user->delete()) { // Remove the user's avatar if they have one From 52af8afac2332131f2b40ced641b82709ca11ae9 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 1 Jun 2024 03:10:29 +0100 Subject: [PATCH 3/6] Added company scoping test Signed-off-by: snipe --- tests/Feature/Api/Users/DeleteUsersTest.php | 32 +++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/Feature/Api/Users/DeleteUsersTest.php b/tests/Feature/Api/Users/DeleteUsersTest.php index 830008d0bc..3b68cbb4dc 100644 --- a/tests/Feature/Api/Users/DeleteUsersTest.php +++ b/tests/Feature/Api/Users/DeleteUsersTest.php @@ -2,6 +2,8 @@ namespace Tests\Feature\Api\Users; +use App\Models\Asset; +use App\Models\Company; use App\Models\Location; use App\Models\User; use App\Models\LicenseSeat; @@ -64,6 +66,36 @@ class DeleteUsersTest extends TestCase ->json(); } + public function testDisallowUserDeletionIfNotInSameCompanyIfNotSuperadmin() + { + $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(); + + } + + + } From cff382605c59b56ec43f9823c7f6a7484d23f6b7 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 1 Jun 2024 04:01:04 +0100 Subject: [PATCH 4/6] Fixed some missing strings and checks Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 20 +++++++------- .../Controllers/Users/UsersController.php | 26 ++++++++++--------- resources/lang/en-US/admin/users/message.php | 6 +++++ 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index c2c18748b0..4b3b00e7a2 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -539,31 +539,31 @@ 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, 'This user still has ' . $user->managesUsers()->count() . ' users that they manage.')); + 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 diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 9624753b16..3d971d9e6b 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -346,33 +346,35 @@ 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 $user->delete(); return redirect()->route('users.index')->with('success', trans('admin/users/message.success.delete')); 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: ', From ea8d390596971a08db2f39e3bdd27999ceba7246 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 1 Jun 2024 04:01:09 +0100 Subject: [PATCH 5/6] MOAR TESTS Signed-off-by: snipe --- tests/Feature/Api/Users/DeleteUsersTest.php | 16 +++++++++++- tests/Feature/Users/DeleteUsersTest.php | 29 ++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/tests/Feature/Api/Users/DeleteUsersTest.php b/tests/Feature/Api/Users/DeleteUsersTest.php index 3b68cbb4dc..e926311dca 100644 --- a/tests/Feature/Api/Users/DeleteUsersTest.php +++ b/tests/Feature/Api/Users/DeleteUsersTest.php @@ -66,7 +66,7 @@ class DeleteUsersTest extends TestCase ->json(); } - public function testDisallowUserDeletionIfNotInSameCompanyIfNotSuperadmin() + public function testDisallowUserDeletionIfNotInSameCompanyAndNotSuperadmin() { $this->settings->enableMultipleFullCompanySupport(); [$companyA, $companyB] = Company::factory()->count(2)->create(); @@ -94,6 +94,20 @@ class DeleteUsersTest extends TestCase } + 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/Users/DeleteUsersTest.php b/tests/Feature/Users/DeleteUsersTest.php index 9903081e20..3d2da4bf4e 100644 --- a/tests/Feature/Users/DeleteUsersTest.php +++ b/tests/Feature/Users/DeleteUsersTest.php @@ -13,27 +13,44 @@ class DeleteUsersTest extends TestCase public function testDisallowUserDeletionIfStillManagingPeople() { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); - User::factory()->create(['first_name' => 'Lowly', 'last_name' => 'Worker', 'manager_id' => $manager->id]); + $manager = User::factory()->create(); + User::factory()->count(3)->create(['manager_id' => $manager->id]); + $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); + + $this->actingAs(User::factory()->deleteUsers()->create()) + ->delete(route('users.destroy', $manager->id)) + ->assertStatus(302) + ->assertRedirect(route('users.index')); } public function testDisallowUserDeletionIfStillManagingLocations() { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); - Location::factory()->create(['manager_id' => $manager->id]); + $manager = User::factory()->create(); + Location::factory()->count(3)->create(['manager_id' => $manager->id]); + $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); + + $this->actingAs(User::factory()->deleteUsers()->create()) + ->delete(route('users.destroy', $manager->id)) + ->assertStatus(302) + ->assertRedirect(route('users.index')); } public function testAllowUserDeletionIfNotManagingLocations() { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); + $manager = User::factory()->create(); $this->actingAs(User::factory()->deleteUsers()->create())->assertTrue($manager->isDeletable()); + + $this->actingAs(User::factory()->deleteUsers()->create()) + ->delete(route('users.destroy', $manager->id)) + ->assertStatus(302) + ->assertRedirect(route('users.index')); } public function testDisallowUserDeletionIfNoDeletePermissions() { - $manager = User::factory()->create(['first_name' => 'Manager', 'last_name' => 'McManagerson']); + $manager = User::factory()->create(); Location::factory()->create(['manager_id' => $manager->id]); $this->actingAs(User::factory()->editUsers()->create())->assertFalse($manager->isDeletable()); } From 67fb3f10d5607012b30cd050095b1b4c23f090f0 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 1 Jun 2024 04:24:47 +0100 Subject: [PATCH 6/6] Tightened up tests with better checks Signed-off-by: snipe --- app/Http/Controllers/Users/UsersController.php | 8 ++++---- tests/Feature/Users/DeleteUsersTest.php | 13 ++++++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 3d971d9e6b..8df5842929 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -370,10 +370,10 @@ class UsersController extends Controller ->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->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 $user->delete(); diff --git a/tests/Feature/Users/DeleteUsersTest.php b/tests/Feature/Users/DeleteUsersTest.php index 3d2da4bf4e..a9ac1ab1ff 100644 --- a/tests/Feature/Users/DeleteUsersTest.php +++ b/tests/Feature/Users/DeleteUsersTest.php @@ -18,10 +18,12 @@ class DeleteUsersTest extends TestCase $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); - $this->actingAs(User::factory()->deleteUsers()->create()) + $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() @@ -31,10 +33,12 @@ class DeleteUsersTest extends TestCase $this->actingAs(User::factory()->deleteUsers()->create())->assertFalse($manager->isDeletable()); - $this->actingAs(User::factory()->deleteUsers()->create()) + $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() @@ -42,10 +46,13 @@ class DeleteUsersTest extends TestCase $manager = User::factory()->create(); $this->actingAs(User::factory()->deleteUsers()->create())->assertTrue($manager->isDeletable()); - $this->actingAs(User::factory()->deleteUsers()->create()) + $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()