From 55c98cc27a1bd7a8b5c81a5172e904fcfd7b5b8f Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 27 Jun 2024 14:05:28 +0100 Subject: [PATCH 1/2] Check that the user exists before trying to fill the request Signed-off-by: snipe --- app/Http/Controllers/Api/UsersController.php | 114 ++++++++++--------- 1 file changed, 60 insertions(+), 54 deletions(-) diff --git a/app/Http/Controllers/Api/UsersController.php b/app/Http/Controllers/Api/UsersController.php index bd90ab856a..3374f1907e 100644 --- a/app/Http/Controllers/Api/UsersController.php +++ b/app/Http/Controllers/Api/UsersController.php @@ -437,77 +437,83 @@ class UsersController extends Controller { $this->authorize('update', User::class); - $user = User::find($id); - $this->authorize('update', $user); - - /** - * This is a janky hack to prevent people from changing admin demo user data on the public demo. - * The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder. - * Thanks, jerks. You are why we can't have nice things. - snipe - * - */ + if ($user = User::find($id)) { - if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) { - return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.')); - } + $this->authorize('update', $user); + + /** + * This is a janky hack to prevent people from changing admin demo user data on the public demo. + * The $ids 1 and 2 are special since they are seeded as superadmins in the demo seeder. + * Thanks, jerks. You are why we can't have nice things. - snipe + * + */ - $user->fill($request->all()); - - if ($user->id == $request->input('manager_id')) { - return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager')); - } - - if ($request->filled('password')) { - $user->password = bcrypt($request->input('password')); - } - - // We need to use has() instead of filled() - // here because we need to overwrite permissions - // if someone needs to null them out - if ($request->has('permissions')) { - $permissions_array = $request->input('permissions'); - - // Strip out the individual superuser permission if the API user isn't a superadmin - if (! Auth::user()->isSuperUser()) { - unset($permissions_array['superuser']); + if ((($id == 1) || ($id == 2)) && (config('app.lock_passwords'))) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'Permission denied. You cannot update user information via API on the demo.')); } - $user->permissions = $permissions_array; - } + $user->fill($request->all()); - // Update the location of any assets checked out to this user - Asset::where('assigned_type', User::class) - ->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]); + if ($user->id == $request->input('manager_id')) { + return response()->json(Helper::formatStandardApiResponse('error', null, 'You cannot be your own manager')); + } - - app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'image', 'avatars', 'avatar'); - - if ($user->save()) { + if ($request->filled('password')) { + $user->password = bcrypt($request->input('password')); + } - // Check if the request has groups passed and has a value, AND that the user us a superuser - if (($request->has('groups')) && (Auth::user()->isSuperUser())) { + // We need to use has() instead of filled() + // here because we need to overwrite permissions + // if someone needs to null them out + if ($request->has('permissions')) { + $permissions_array = $request->input('permissions'); - $validator = Validator::make($request->only('groups'), [ - 'groups.*' => 'integer|exists:permission_groups,id', - ]); - - if ($validator->fails()) { - return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors())); + // Strip out the individual superuser permission if the API user isn't a superadmin + if (!Auth::user()->isSuperUser()) { + unset($permissions_array['superuser']); } - // Sync the groups since the user is a superuser and the groups pass validation - $user->groups()->sync($request->input('groups')); - - + $user->permissions = $permissions_array; } - return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update'))); + + // Update the location of any assets checked out to this user + Asset::where('assigned_type', User::class) + ->where('assigned_to', $user->id)->update(['location_id' => $request->input('location_id', null)]); + + + app('App\Http\Requests\ImageUploadRequest')->handleImages($user, 600, 'image', 'avatars', 'avatar'); + + if ($user->save()) { + + // Check if the request has groups passed and has a value, AND that the user us a superuser + if (($request->has('groups')) && (Auth::user()->isSuperUser())) { + + $validator = Validator::make($request->only('groups'), [ + 'groups.*' => 'integer|exists:permission_groups,id', + ]); + + if ($validator->fails()) { + return response()->json(Helper::formatStandardApiResponse('error', null, $validator->errors())); + } + + // Sync the groups since the user is a superuser and the groups pass validation + $user->groups()->sync($request->input('groups')); + + + } + + return response()->json(Helper::formatStandardApiResponse('success', (new UsersTransformer)->transformUser($user), trans('admin/users/message.success.update'))); + } + + return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); } - return response()->json(Helper::formatStandardApiResponse('error', null, $user->getErrors())); + return response()->json(Helper::formatStandardApiResponse('error', null, trans('admin/users/message.user_not_found', compact('id')))); + } /** From 85ce47f5bbfb54713e9d203e91d732b97cac1aab Mon Sep 17 00:00:00 2001 From: snipe Date: Thu, 27 Jun 2024 14:21:27 +0100 Subject: [PATCH 2/2] Updated tests Signed-off-by: snipe --- tests/Feature/Users/Api/UpdateUserTest.php | 45 +++++++++++++++++----- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/tests/Feature/Users/Api/UpdateUserTest.php b/tests/Feature/Users/Api/UpdateUserTest.php index 020bb9a07b..1c66bbdda9 100644 --- a/tests/Feature/Users/Api/UpdateUserTest.php +++ b/tests/Feature/Users/Api/UpdateUserTest.php @@ -153,47 +153,74 @@ class UpdateUserTest extends TestCase // Admin for Company A should allow updating user from Company A $this->actingAsForApi($adminA) ->patchJson(route('api.users.update', $scoped_user_in_companyA)) - ->assertStatus(200); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); // Admin for Company A should get denied updating user from Company B $this->actingAsForApi($adminA) ->patchJson(route('api.users.update', $scoped_user_in_companyB)) - ->assertStatus(403); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); // Admin for Company A should get denied updating user without a company $this->actingAsForApi($adminA) ->patchJson(route('api.users.update', $scoped_user_in_no_company)) - ->assertStatus(403); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); // Admin for Company B should allow updating user from Company B $this->actingAsForApi($adminB) ->patchJson(route('api.users.update', $scoped_user_in_companyB)) - ->assertStatus(200); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); // Admin for Company B should get denied updating user from Company A $this->actingAsForApi($adminB) ->patchJson(route('api.users.update', $scoped_user_in_companyA)) - ->assertStatus(403); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); // Admin for Company B should get denied updating user without a company $this->actingAsForApi($adminB) ->patchJson(route('api.users.update', $scoped_user_in_no_company)) - ->assertStatus(403); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); // Admin without a company should allow updating user without a company $this->actingAsForApi($adminNoCompany) ->patchJson(route('api.users.update', $scoped_user_in_no_company)) - ->assertStatus(200); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success') + ->json(); // Admin without a company should get denied updating user from Company A $this->actingAsForApi($adminNoCompany) ->patchJson(route('api.users.update', $scoped_user_in_companyA)) - ->assertStatus(403); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); // Admin without a company should get denied updating user from Company B $this->actingAsForApi($adminNoCompany) ->patchJson(route('api.users.update', $scoped_user_in_companyB)) - ->assertStatus(403); + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error') + ->json(); } public function testUserGroupsAreOnlyUpdatedIfAuthenticatedUserIsSuperUser()