diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index 19cf1c3bdfd1..bd90ab856a7e 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,18 +516,16 @@ 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) { $this->authorize('delete', User::class); - $user = User::with('assets', 'assets.model', 'consumables', 'accessories', 'licenses', 'userloc')->withTrashed()->find($id); - $this->authorize('delete', $user); + if ($user = User::withTrashed()->find($id)) { + $this->authorize('delete', $user); - if ($user) { - if ($user->delete()) { // Remove the user's avatar if they have one @@ -539,11 +539,12 @@ 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'))); } - return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id')))); + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found'))); } @@ -553,7 +554,7 @@ 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,14 +627,14 @@ 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) { $this->authorize('view', User::class); $this->authorize('view', Consumable::class); $user = User::findOrFail($id); - $this->authorize('update', $user); + $this->authorize('view', $user); $consumables = $user->consumables; return (new ConsumablesTransformer)->transformConsumables($consumables, $consumables->count(), $request); } @@ -644,7 +645,7 @@ 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 +664,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 +727,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,12 +740,14 @@ 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) + public function restore($userId) { + $this->authorize('delete', User::class); if ($user = User::withTrashed()->find($userId)) { + $this->authorize('delete', $user); if ($user->deleted_at == '') { @@ -763,8 +766,6 @@ 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); diff --git a/app/Http/Controllers/Users/UsersController.php b/app/Http/Controllers/Users/UsersController.php index 1100b5f3f8e9..0b16dd0a958e 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; @@ -333,19 +335,24 @@ public function update(SaveUserRequest $request, $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()->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')); - } + $this->authorize('delete', $user); - return redirect()->route('users.index') - ->with('error', trans('admin/users/message.user_not_found', compact('id'))); + 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')); } diff --git a/app/Http/Requests/DeleteUserRequest.php b/app/Http/Requests/DeleteUserRequest.php index 8136bd68e2d2..282919d7ae4a 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,12 @@ class DeleteUserRequest extends FormRequest */ public function authorize(): bool { - return true; + return Gate::allows('delete', User::class); } - /** - * 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 +35,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' => Rule::notIn([auth()->user()->id]), 'managed_users' => Rule::in([0]), 'managed_locations' => Rule::in([0]), 'assigned_assets' => Rule::in([0]), 'assigned_licenses' => Rule::in([0]), 'assigned_accessories' => Rule::in([0]), + 'deleted_at' => Rule::in([null]), ]; } public function messages(): array { - $user_to_delete = User::find(request()->route('user')); - $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' => trans('admin/users/message.user_not_found'), + // Cannot delete yourself 'user.not_in' => trans('admin/users/message.error.cannot_delete_yourself'), @@ -84,6 +89,8 @@ 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); } 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 () { diff --git a/tests/Feature/Users/Api/DeleteUserTest.php b/tests/Feature/Users/Api/DeleteUserTest.php index f3f8e80f3467..e8da98fe3141 100644 --- a/tests/Feature/Users/Api/DeleteUserTest.php +++ b/tests/Feature/Users/Api/DeleteUserTest.php @@ -12,6 +12,28 @@ class DeleteUserTest extends TestCase { + public function testErrorReturnedViaApiIfUserDoesNotExist() + { + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', 'invalid-id')) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + public function testErrorReturnedViaApiIfUserIsAlreadyDeleted() + { + $user = User::factory()->deletedUser()->create(); + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->deleteJson(route('api.users.destroy', $user->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + public function testDisallowUserDeletionViaApiIfStillManagingPeople() { $manager = User::factory()->create(); @@ -56,41 +78,65 @@ 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(); - $superUser = $companyA->users()->save(User::factory()->superuser()->make()); - $userInCompanyA = $companyA->users()->save(User::factory()->deleteUsers()->make()); - $userInCompanyB = $companyB->users()->save(User::factory()->deleteUsers()->make()); + $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)) - ->assertStatus(403) + $this->actingAsForApi($userFromA) + ->deleteJson(route('api.users.destroy', ['user' => $userFromB->id])) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') ->json(); - $this->actingAsForApi($userInCompanyB) - ->deleteJson(route('api.users.destroy', $userInCompanyA)) - ->assertStatus(403) + $userFromB->refresh(); + $this->assertNull($userFromB->deleted_at); + + $this->actingAsForApi($userFromB) + ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id])) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') ->json(); - $this->actingAsForApi($superUser) - ->deleteJson(route('api.users.destroy', $userInCompanyA)) + $userFromA->refresh(); + $this->assertNull($userFromA->deleted_at); + + $this->actingAsForApi($superuser) + ->deleteJson(route('api.users.destroy', ['user' => $userFromA->id])) ->assertOk() ->assertStatus(200) ->assertStatusMessageIs('success') ->json(); + $userFromA->refresh(); + $this->assertNotNull($userFromA->deleted_at); + } public function testUsersCannotDeleteThemselves() diff --git a/tests/Feature/Users/Api/RestoreUserTest.php b/tests/Feature/Users/Api/RestoreUserTest.php new file mode 100644 index 000000000000..0ffac8f07e32 --- /dev/null +++ b/tests/Feature/Users/Api/RestoreUserTest.php @@ -0,0 +1,105 @@ +actingAsForApi(User::factory()->deleteUsers()->create()) + ->postJson(route('api.users.restore', 'invalid-id')) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + public function testErrorReturnedViaApiIfUserIsNotDeleted() + { + $user = User::factory()->create(); + $this->actingAsForApi(User::factory()->deleteUsers()->create()) + ->postJson(route('api.users.restore', $user->id)) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + } + + + public function testDeniedPermissionsForRestoringUserViaApi() + { + $this->actingAsForApi(User::factory()->create()) + ->postJson(route('api.users.restore', User::factory()->deletedUser()->create())) + ->assertStatus(403) + ->json(); + } + + public function testSuccessPermissionsForRestoringUserViaApi() + { + $deleted_user = User::factory()->deletedUser()->create(); + + $this->actingAsForApi(User::factory()->admin()->create()) + ->postJson(route('api.users.restore', ['user' => $deleted_user])) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); + + $deleted_user->refresh(); + $this->assertNull($deleted_user->deleted_at); + } + + public function testPermissionsForRestoringIfNotInSameCompanyAndNotSuperadmin() + { + $this->settings->enableMultipleFullCompanySupport(); + + [$companyA, $companyB] = Company::factory()->count(2)->create(); + + $superuser = User::factory()->superuser()->create(); + $userFromA = User::factory()->deletedUser()->deleteUsers()->for($companyA)->create(); + $userFromB = User::factory()->deletedUser()->deleteUsers()->for($companyB)->create(); + + $this->actingAsForApi($userFromA) + ->postJson(route('api.users.restore', ['user' => $userFromB->id])) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + + $userFromB->refresh(); + $this->assertNotNull($userFromB->deleted_at); + + $this->actingAsForApi($userFromB) + ->postJson(route('api.users.restore', ['user' => $userFromA->id])) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); + + $userFromA->refresh(); + $this->assertNotNull($userFromA->deleted_at); + + $this->actingAsForApi($superuser) + ->postJson(route('api.users.restore', ['user' => $userFromA->id])) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); + + $userFromA->refresh(); + $this->assertNull($userFromA->deleted_at); + + } + + + + +} 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(); - - } - } diff --git a/tests/Feature/Users/Ui/DeleteUserTest.php b/tests/Feature/Users/Ui/DeleteUserTest.php index 61ee2337958f..da4c5a37ee66 100644 --- a/tests/Feature/Users/Ui/DeleteUserTest.php +++ b/tests/Feature/Users/Ui/DeleteUserTest.php @@ -14,7 +14,42 @@ class DeleteUserTest extends TestCase { - public function testPermissionsToDeleteUser() + public function testUserCanDeleteAnotherUser() + { + $user = User::factory()->deleteUsers()->viewUsers()->create(); + $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create())->assertTrue($user->isDeletable()); + + $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create()) + ->delete(route('users.destroy', ['user' => $user->id])) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + + $this->followRedirects($response)->assertSee(trans('general.notification_success')); + } + + + public function testErrorReturnedIfUserDoesNotExist() + { + $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create()) + ->delete(route('users.destroy', ['user' => '40596803548609346'])) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + $this->followRedirects($response)->assertSee(trans('alert-danger')); + } + + public function testErrorReturnedIfUserIsAlreadyDeleted() + { + $user = User::factory()->deletedUser()->viewUsers()->create(); + $response = $this->actingAs(User::factory()->deleteUsers()->viewUsers()->create()) + ->delete(route('users.destroy', $user->id)) + ->assertStatus(302) + ->assertRedirect(route('users.index')); + + $this->followRedirects($response)->assertSee(trans('general.error')); + } + + + public function testFmcsPermissionsToDeleteUser() { $this->settings->enableMultipleFullCompanySupport(); @@ -22,17 +57,35 @@ public function testPermissionsToDeleteUser() [$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(); - $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'); + + $userFromB->refresh(); + $this->assertNull($userFromB->deleted_at); - $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')); + $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); }