From 65e8765bddb2a1b19ec9d39073274c50671cc16d Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 17:45:42 +0100 Subject: [PATCH 01/22] Added tests Signed-off-by: snipe --- tests/Feature/Users/Api/DeleteUserTest.php | 22 ++++++++++++++++++++ tests/Feature/Users/Ui/DeleteUserTest.php | 24 ++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index f3f8e80f3467..b127543c52fa 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -11,6 +11,28 @@ class DeleteUserTest extends TestCase { + public function testUserCanDeleteAnotherUserViaApi() + { + + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', User::factory()->create())) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); + } + + + public function testErrorReturnedViaApiIfUserDoesNotExist() + { + $this->actingAsForApi(User::factory()->deleteUsers()->create(['company_id'=> null])) + ->deleteJson(route('api.users.destroy', 'invalid-id')) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + public function testDisallowUserDeletionViaApiIfStillManagingPeople() { diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 61ee2337958f..5679bb3ac12e 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -14,6 +14,30 @@ 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('general.error')); + } + + public function testPermissionsToDeleteUser() { From 06737f45ad64c2a0349465f927bcd49535eb2c07 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 18:36:02 +0100 Subject: [PATCH 02/22] Added test Signed-off-by: snipe --- tests/Feature/Users/Ui/DeleteUserTest.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 5679bb3ac12e..1fde5d955854 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -34,7 +34,18 @@ public function testErrorReturnedIfUserDoesNotExist() ->delete(route('users.destroy', ['user' => '40596803548609346'])) ->assertStatus(302) ->assertRedirect(route('users.index')); - //$this->followRedirects($response)->assertSee(trans('general.error')); + $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')); } From 61f76dedc6971a3faf1337d3c4eda5c089bbf631 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 18:36:08 +0100 Subject: [PATCH 03/22] Added test Signed-off-by: snipe --- tests/Feature/Users/Api/DeleteUserTest.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index b127543c52fa..a0d88318b166 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -25,7 +25,7 @@ public function testUserCanDeleteAnotherUserViaApi() public function testErrorReturnedViaApiIfUserDoesNotExist() { - $this->actingAsForApi(User::factory()->deleteUsers()->create(['company_id'=> null])) + $this->actingAsForApi(User::factory()->deleteUsers()->create()) ->deleteJson(route('api.users.destroy', 'invalid-id')) ->assertOk() ->assertStatus(200) @@ -33,6 +33,17 @@ public function testErrorReturnedViaApiIfUserDoesNotExist() ->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() { From d27bde6047e53f820aa6ee8eefe38c25a909502b Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 18:36:24 +0100 Subject: [PATCH 04/22] Added deleted factory for tests Signed-off-by: snipe --- database/factories/UserFactory.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/database/factories/UserFactory.php b/database/factories/UserFactory.php index cb1ccd89b5b3..5c885666dff8 100644 --- a/database/factories/UserFactory.php +++ b/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 () { From e19922abd026b4c1f72ad447ad0237e51fea07d1 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 18:49:35 +0100 Subject: [PATCH 05/22] Cleaned up UI controller Signed-off-by: snipe --- .../Controllers/Users/UsersController.php | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 1100b5f3f8e9..0be1d697f1bd 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/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; @@ -336,16 +338,22 @@ 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')); - } + 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', compact('id'))); + } + return redirect()->route('users.index')->with('error', trans('admin/users/message.user_not_found')); } From dd9e9c7a6d74aecb11e3265a4aeabb5346868db9 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 18:49:49 +0100 Subject: [PATCH 06/22] Fixed return types Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 26 +++++++------- app/Http/Requests/DeleteUserRequest.php | 36 ++++++++++++-------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 19cf1c3bdfd1..c41afe0a3cc0 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/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] [] * @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] [] * @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] [] * @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,7 +516,7 @@ public function update(SaveUserRequest $request, $id) * @author [A. Gianotto] [] * @since [v4.0] * @param int $id - * @return \Illuminate\Http\Response + * @return \Illuminate\Http\JsonResponse */ public function destroy(DeleteUserRequest $request, $id) { @@ -543,7 +545,7 @@ public function destroy(DeleteUserRequest $request, $id) } - return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id')))); + return response()->json(Helper::formatStandardApiResponse('error', null, 'Whoops.'.trans('admin/users/message.user_not_found'))); } @@ -553,7 +555,7 @@ public function destroy(DeleteUserRequest $request, $id) * @author [A. Gianotto] [] * @since [v3.0] * @param $userId - * @return string JSON + * @return array | \Illuminate\Http\JsonResponse */ public function assets(Request $request, $id) { @@ -626,7 +628,7 @@ public function emailAssetList(Request $request, $id) * @author [A. Gianotto] [] * @since [v3.0] * @param $userId - * @return string JSON + * @return array | \Illuminate\Http\JsonResponse */ public function consumables(Request $request, $id) { @@ -644,7 +646,7 @@ public function consumables(Request $request, $id) * @author [A. Gianotto] [] * @since [v4.6.14] * @param $userId - * @return string JSON + * @return array */ public function accessories($id) { @@ -663,7 +665,7 @@ public function accessories($id) * @author [N. Mathar] [] * @since [v5.0] * @param $userId - * @return string JSON + * @return array | \Illuminate\Http\JsonResponse */ public function licenses($id) { @@ -726,7 +728,7 @@ public function postTwoFactorReset(Request $request) * @author [Juan Font] [] * @since [v4.4.2] * @param \Illuminate\Http\Request $request - * @return \Illuminate\Http\Response + * @return array */ public function getCurrentUserInfo(Request $request) { @@ -739,7 +741,7 @@ public function getCurrentUserInfo(Request $request) * @author [E. Taylor] [] * @param int $userId * @since [v6.0.0] - * @return JsonResponse + * @return \Illuminate\Http\JsonResponse */ public function restore($userId = null) { diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php index 8136bd68e2d2..d70c7e0f5612 100644 --- a/app/Http/Requests/DeleteUserRequest.php +++ b/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,13 @@ class DeleteUserRequest extends FormRequest */ public function authorize(): bool { - return true; + return Gate::allows('delete', new User); + //return true; } - /** - * Get the validation rules that apply to the request. - * - * @return array|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 +36,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> + */ + public function rules(): array + { return [ - 'id' => ['exists:users,id'], - 'user' => Rule::notIn([Auth::user()->id]), + 'user' => ['in:null|not_in: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' => 'Boop. '.trans('admin/users/message.user_not_found'), + // Cannot delete yourself 'user.not_in' => trans('admin/users/message.error.cannot_delete_yourself'), @@ -84,6 +90,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); } From 0e2526f627bacfe5ff05ac124edc2eed42f22289 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:32:49 +0100 Subject: [PATCH 07/22] Removed comment Signed-off-by: snipe --- app/Http/Requests/DeleteUserRequest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php index d70c7e0f5612..6ac70526759b 100644 --- a/app/Http/Requests/DeleteUserRequest.php +++ b/app/Http/Requests/DeleteUserRequest.php @@ -20,7 +20,6 @@ class DeleteUserRequest extends FormRequest public function authorize(): bool { return Gate::allows('delete', new User); - //return true; } public function prepareForValidation(): void From 1d26ccac4ea9846d97ab303805351eb12353a84a Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:33:06 +0100 Subject: [PATCH 08/22] Check for the additional auth for that user Signed-off-by: snipe --- app/Http/Controllers/Users/UsersController.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 0be1d697f1bd..0b16dd0a958e 100755 --- a/app/Http/Controllers/Users/UsersController.php +++ b/app/Http/Controllers/Users/UsersController.php @@ -335,13 +335,13 @@ public function update(SaveUserRequest $request, $id = null) */ public function destroy(DeleteUserRequest $request, $id = null) { - $this->authorize('delete', User::class); if ($user = User::find($id)) { - if ($user->delete()) { + $this->authorize('delete', $user); + if ($user->delete()) { if (Storage::disk('public')->exists('avatars/' . $user->avatar)) { try { Storage::disk('public')->delete('avatars/' . $user->avatar); @@ -351,7 +351,6 @@ public function destroy(DeleteUserRequest $request, $id = null) } 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')); From 9e59bd5687b19b80e4e9258a6deca93fae3c59bf Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:33:36 +0100 Subject: [PATCH 09/22] Cleaned up controller code a bit Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index c41afe0a3cc0..ec8943705767 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -521,13 +521,11 @@ public function update(SaveUserRequest $request, $id) 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 @@ -541,6 +539,7 @@ 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'))); } From 5ec8e2da66071f4e838bcb39e322dbb7d11809a7 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:33:44 +0100 Subject: [PATCH 10/22] Breaking tests :( Signed-off-by: snipe --- tests/Feature/Users/Api/DeleteUserTest.php | 48 +++++++++++----------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index a0d88318b166..b2c7ec750380 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -11,27 +11,16 @@ class DeleteUserTest extends TestCase { - public function testUserCanDeleteAnotherUserViaApi() - { - - $this->actingAsForApi(User::factory()->deleteUsers()->create()) - ->deleteJson(route('api.users.destroy', User::factory()->create())) - ->assertOk() - ->assertStatus(200) - ->assertStatusMessageIs('success') - ->json(); - } - - public function testErrorReturnedViaApiIfUserDoesNotExist() - { - $this->actingAsForApi(User::factory()->deleteUsers()->create()) - ->deleteJson(route('api.users.destroy', 'invalid-id')) - ->assertOk() - ->assertStatus(200) - ->assertStatusMessageIs('error') - ->json(); - } +// 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() { @@ -89,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(); @@ -107,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) From bbbfa91db95611abf3ee173ba6e4a99690f29866 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:40:26 +0100 Subject: [PATCH 11/22] Fixed wonky rule Signed-off-by: snipe --- app/Http/Requests/DeleteUserRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php index 6ac70526759b..747540337fe3 100644 --- a/app/Http/Requests/DeleteUserRequest.php +++ b/app/Http/Requests/DeleteUserRequest.php @@ -48,7 +48,7 @@ public function prepareForValidation(): void public function rules(): array { return [ - 'user' => ['in:null|not_in:auth()->user()->id'], + 'user' => Rule::notIn([auth()->user()->id]), 'managed_users' => Rule::in([0]), 'managed_locations' => Rule::in([0]), 'assigned_assets' => Rule::in([0]), From e60dbc883c6c44e77e864bb9285e7bbd17963807 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:42:34 +0100 Subject: [PATCH 12/22] Removed test code Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 2 +- app/Http/Requests/DeleteUserRequest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index ec8943705767..ac13931bc963 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -544,7 +544,7 @@ public function destroy(DeleteUserRequest $request, $id) } - return response()->json(Helper::formatStandardApiResponse('error', null, 'Whoops.'.trans('admin/users/message.user_not_found'))); + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found'))); } diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php index 747540337fe3..569b1049b1b5 100644 --- a/app/Http/Requests/DeleteUserRequest.php +++ b/app/Http/Requests/DeleteUserRequest.php @@ -68,7 +68,7 @@ public function messages(): array $messages = array_merge([ - 'user.exists' => 'Boop. '.trans('admin/users/message.user_not_found'), + 'user.exists' => trans('admin/users/message.user_not_found'), // Cannot delete yourself 'user.not_in' => trans('admin/users/message.error.cannot_delete_yourself'), From 3825f5fb61da2cc94e15e934a37534890ac042bf Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:47:59 +0100 Subject: [PATCH 13/22] Added more permissions tests to user delete UI Signed-off-by: snipe --- tests/Feature/Users/Ui/DeleteUserTest.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 1fde5d955854..645a7150900c 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -49,7 +49,7 @@ public function testErrorReturnedIfUserIsAlreadyDeleted() } - public function testPermissionsToDeleteUser() + public function testFmcsPermissionsToDeleteUser() { $this->settings->enableMultipleFullCompanySupport(); @@ -60,14 +60,22 @@ public function testPermissionsToDeleteUser() $userFromA = User::factory()->for($companyA)->create(); $userFromB = User::factory()->for($companyB)->create(); - $this->followingRedirects()->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) + $response = $this->followingRedirects()->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) ->delete(route('users.destroy', ['user' => $userFromB->id])) ->assertStatus(403); + $this->followRedirects($response)->assertSee('sad-panda.png'); - $this->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) + $response = $this->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) ->delete(route('users.destroy', ['user' => $userFromA->id])) ->assertStatus(302) ->assertRedirect(route('users.index')); + $this->followRedirects($response)->assertSee('sad-panda.png'); + + $response = $this->actingAs($superuser) + ->delete(route('users.destroy', ['user' => $userFromA->id])) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + $this->followRedirects($response)->assertSee('Success'); } From 9f5b264e04ff42144f91afa02c91a0c7a2279bfb Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:51:56 +0100 Subject: [PATCH 14/22] Normalize the tests between UI and API Signed-off-by: snipe --- tests/Feature/Users/Api/DeleteUserTest.php | 24 ++++++++++------------ tests/Feature/Users/Ui/DeleteUserTest.php | 8 ++++---- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index b2c7ec750380..835a410dada9 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -96,31 +96,29 @@ public function testSuccessPermissionsForDeletingUserViaApi() ->json(); } - - - + public function testPermissionsForDeletingIfNotInSameCompanyAndNotSuperadmin() { $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()); + [$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($userInCompanyA) - ->deleteJson(route('api.users.destroy', $userInCompanyB)) + $this->actingAsForApi($userFromA) + ->deleteJson(route('api.users.destroy', ['user' => $userFromB->id])) ->assertStatus(403) ->json(); - $this->actingAsForApi($userInCompanyB) - ->deleteJson(route('api.users.destroy', $userInCompanyA)) + $this->actingAsForApi($userFromB) + ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id])) ->assertStatus(403) ->json(); - $this->actingAsForApi($superUser) - ->deleteJson(route('api.users.destroy', $userInCompanyA)) + $this->actingAsForApi($superuser) + ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id])) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('success') diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 645a7150900c..2d4a7bdc515f 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -57,15 +57,15 @@ public function testFmcsPermissionsToDeleteUser() [$companyA, $companyB] = Company::factory()->count(2)->create(); $superuser = User::factory()->superuser()->create(); - $userFromA = User::factory()->for($companyA)->create(); - $userFromB = User::factory()->for($companyB)->create(); + $userFromA = User::factory()->deleteUsers()->for($companyA)->create(); + $userFromB = User::factory()->deleteUsers()->for($companyB)->create(); - $response = $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'); - $response = $this->actingAs(User::factory()->deleteUsers()->for($companyA)->create()) + $response = $this->actingAs($userFromB) ->delete(route('users.destroy', ['user' => $userFromA->id])) ->assertStatus(302) ->assertRedirect(route('users.index')); From 196db9718e37b0cab7926b1cb52ffe49975eaece Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 19:55:58 +0100 Subject: [PATCH 15/22] Use class name instead Signed-off-by: snipe --- app/Http/Requests/DeleteUserRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php index 569b1049b1b5..282919d7ae4a 100644 --- a/app/Http/Requests/DeleteUserRequest.php +++ b/app/Http/Requests/DeleteUserRequest.php @@ -19,7 +19,7 @@ class DeleteUserRequest extends FormRequest */ public function authorize(): bool { - return Gate::allows('delete', new User); + return Gate::allows('delete', User::class); } public function prepareForValidation(): void From c2e649e2bf578e8e3568c80a03b80c6bfa64228c Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:07:37 +0100 Subject: [PATCH 16/22] Fixed small permissions issue Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index ac13931bc963..b01f3df672f5 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -634,7 +634,7 @@ 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); } From 1ef0c1adac5f16b441a65475f22f1a219014cf5f Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:07:46 +0100 Subject: [PATCH 17/22] Fixed tests! And added more!! Signed-off-by: snipe --- tests/Feature/Users/Api/DeleteUserTest.php | 17 +++++++++++++++-- tests/Feature/Users/Ui/DeleteUserTest.php | 10 ++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index 835a410dada9..2bd274d771aa 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -109,14 +109,24 @@ public function testPermissionsForDeletingIfNotInSameCompanyAndNotSuperadmin() $this->actingAsForApi($userFromA) ->deleteJson(route('api.users.destroy', ['user' => $userFromB->id])) - ->assertStatus(403) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') ->json(); + $userFromB->refresh(); + $this->assertNull($userFromB->deleted_at); + $this->actingAsForApi($userFromB) ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id])) - ->assertStatus(403) + ->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() @@ -124,6 +134,9 @@ public function testPermissionsForDeletingIfNotInSameCompanyAndNotSuperadmin() ->assertStatusMessageIs('success') ->json(); + $userFromA->refresh(); + $this->assertNotNull($userFromA->deleted_at); + } public function testUsersCannotDeleteThemselves() diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 2d4a7bdc515f..da4c5a37ee66 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -65,18 +65,28 @@ public function testFmcsPermissionsToDeleteUser() ->assertStatus(403); $this->followRedirects($response)->assertSee('sad-panda.png'); + $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); + } From 74c78d75779365525f0a74202d3af3720d21819b Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:09:04 +0100 Subject: [PATCH 18/22] Added back in commented out test Signed-off-by: snipe --- tests/Feature/Users/Api/DeleteUserTest.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index 2bd274d771aa..e8da98fe3141 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -12,15 +12,15 @@ 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 testErrorReturnedViaApiIfUserDoesNotExist() + { + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', 'invalid-id')) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } public function testErrorReturnedViaApiIfUserIsAlreadyDeleted() { From 060b17df011a0effff70a94307baf948a5f96908 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:34:49 +0100 Subject: [PATCH 19/22] Pulled duplicated test Signed-off-by: snipe --- tests/Feature/Users/Api/ViewUserTest.php | 38 ------------------------ 1 file changed, 38 deletions(-) diff --git a/tests/Feature/Users/Api/ViewUserTest.php b/tests/Feature/Users/Api/ViewUserTest.php index 859333698999..07346dbbb881 100644 --- a/tests/Feature/Users/Api/ViewUserTest.php +++ b/tests/Feature/Users/Api/ViewUserTest.php @@ -20,42 +20,4 @@ public function testCanReturnUser() ->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(); - - } - } From 7ec44e46ce7aaaee1c140a67ed3468ac84986e2f Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:35:28 +0100 Subject: [PATCH 20/22] Added ability check for restoring users at all Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index b01f3df672f5..bd90ab856a7e 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -742,10 +742,12 @@ public function getCurrentUserInfo(Request $request) * @since [v6.0.0] * @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 == '') { @@ -764,8 +766,6 @@ public function restore($userId = null) 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); From 3a05d72124f0964c59d990ba3ec9161e10cbc7e7 Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:35:40 +0100 Subject: [PATCH 21/22] Based restore API test Signed-off-by: snipe --- tests/Feature/Users/Api/RestoreUserTest.php | 109 ++++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 tests/Feature/Users/Api/RestoreUserTest.php diff --git a/tests/Feature/Users/Api/RestoreUserTest.php b/tests/Feature/Users/Api/RestoreUserTest.php new file mode 100644 index 000000000000..e73509a1ac8e --- /dev/null +++ b/tests/Feature/Users/Api/RestoreUserTest.php @@ -0,0 +1,109 @@ +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(); + \Log::warning($deleted_user); + + $admin = User::factory()->deleteUsers()->create(); + \Log::warning($admin->permissions); + + $this->actingAsForApi($admin) + ->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); + + } + + + + +} From c1be94c4ad30363f41ec8aa6b469523d5daecb4a Mon Sep 17 00:00:00 2001 From: snipe Date: Sat, 22 Jun 2024 20:37:49 +0100 Subject: [PATCH 22/22] Added another base test Signed-off-by: snipe --- tests/Feature/Users/Api/RestoreUserTest.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/Feature/Users/Api/RestoreUserTest.php b/tests/Feature/Users/Api/RestoreUserTest.php index e73509a1ac8e..0ffac8f07e32 100644 --- a/tests/Feature/Users/Api/RestoreUserTest.php +++ b/tests/Feature/Users/Api/RestoreUserTest.php @@ -45,12 +45,8 @@ public function testDeniedPermissionsForRestoringUserViaApi() public function testSuccessPermissionsForRestoringUserViaApi() { $deleted_user = User::factory()->deletedUser()->create(); - \Log::warning($deleted_user); - $admin = User::factory()->deleteUsers()->create(); - \Log::warning($admin->permissions); - - $this->actingAsForApi($admin) + $this->actingAsForApi(User::factory()->admin()->create()) ->postJson(route('api.users.restore', ['user' => $deleted_user])) ->assertOk() ->assertStatus(200)