From 8afba32169d032abd7e6a4d00c01368badac9278 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Dec 2024 14:23:23 +0000 Subject: [PATCH 1/4] Validate group data Signed-off-by: snipe --- app/Http/Requests/SaveUserRequest.php | 6 +- tests/Feature/Users/Api/StoreUserTest.php | 88 +++++++++++++++++++++++ 2 files changed, 92 insertions(+), 2 deletions(-) create mode 100644 tests/Feature/Users/Api/StoreUserTest.php diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index 5a47362cfc..b2d9389b2c 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -7,6 +7,7 @@ use Illuminate\Contracts\Validation\Validator; use Illuminate\Foundation\Http\FormRequest; use Illuminate\Http\Exceptions\HttpResponseException; use App\Rules\UserCannotSwitchCompaniesIfItemsAssigned; +use Illuminate\Support\Facades\Gate; class SaveUserRequest extends FormRequest { @@ -17,7 +18,7 @@ class SaveUserRequest extends FormRequest */ public function authorize() { - return true; + return Gate::allows('users.create'); } public function response(array $errors) @@ -35,7 +36,8 @@ class SaveUserRequest extends FormRequest $rules = [ 'department_id' => 'nullable|exists:departments,id', 'manager_id' => 'nullable|exists:users,id', - 'company_id' => ['nullable','exists:companies,id'] + 'company_id' => ['nullable','exists:companies,id'], + 'groups' => ['nullable','exists:permission_groups,id'] ]; switch ($this->method()) { diff --git a/tests/Feature/Users/Api/StoreUserTest.php b/tests/Feature/Users/Api/StoreUserTest.php new file mode 100644 index 0000000000..98e99011a3 --- /dev/null +++ b/tests/Feature/Users/Api/StoreUserTest.php @@ -0,0 +1,88 @@ +actingAsForApi(User::factory()->create()) + ->postJson(route('api.users.store')) + ->assertForbidden() + ->json(); + } + + public function testCanSaveUserViaPost() + { + $admin = User::factory()->superuser()->create(); + $manager = User::factory()->create(); + $company = Company::factory()->create(); + $department = Department::factory()->create(); + $location = Location::factory()->create(); + $group = Group::factory()->create(); + + + $response = $this->actingAsForApi($admin) + ->postJson(route('api.users.store'), [ + 'first_name' => 'Mabel', + 'last_name' => 'Mora', + 'username' => 'mabel', + 'password' => 'super-secret', + 'password_confirmation' => 'super-secret', + 'email' => 'mabel@example.com', + 'permissions' => '{"a.new.permission":"1"}', + 'activated' => true, + 'phone' => '619-555-5555', + 'jobtitle' => 'Host', + 'manager_id' => $manager->id, + 'employee_num' => '1111', + 'notes' => 'Pretty good artist', + 'company_id' => $company->id, + 'department_id' => $department->id, + 'location_id' => $location->id, + 'remote' => true, + 'groups' => $group->id, + 'vip' => true, + 'start_date' => '2021-08-01', + 'end_date' => '2025-12-31', + ]) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('success'); + + $user = User::find($response['payload']['id']); + $this->assertEquals($admin->id, $user->created_by, 'Created by was not saved'); + } + + public function testDoesNotAcceptBogusGroupData() + { + $admin = User::factory()->superuser()->create(); + $manager = User::factory()->create(); + $company = Company::factory()->create(); + $department = Department::factory()->create(); + $location = Location::factory()->create(); + + $response = $this->actingAsForApi($admin) + ->postJson(route('api.users.store'), [ + 'first_name' => 'Mabel', + 'username' => 'mabel', + 'password' => 'super-secret', + 'password_confirmation' => 'super-secret', + 'groups' => ['blah'], + ]) + ->assertOk() + ->assertStatus(200) + ->assertStatusMessageIs('error'); + + } + + +} From dbd33e1a34187fb63f8d6dc8782d0be0e908ad23 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Dec 2024 14:34:16 +0000 Subject: [PATCH 2/4] Check for specific error message Signed-off-by: snipe --- tests/Feature/Users/Api/StoreUserTest.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/Feature/Users/Api/StoreUserTest.php b/tests/Feature/Users/Api/StoreUserTest.php index 98e99011a3..860f16916a 100644 --- a/tests/Feature/Users/Api/StoreUserTest.php +++ b/tests/Feature/Users/Api/StoreUserTest.php @@ -80,7 +80,15 @@ class StoreUserTest extends TestCase { ]) ->assertOk() ->assertStatus(200) - ->assertStatusMessageIs('error'); + ->assertStatusMessageIs('error') + ->assertJson( + [ + 'messages' => [ + 'groups' => + [0 => trans('The selected groups is invalid.')] + ] + ]) + ->json(); } From a0bbafeb30629ffc79a5661b4a23436ab08b3104 Mon Sep 17 00:00:00 2001 From: snipe Date: Tue, 10 Dec 2024 14:35:22 +0000 Subject: [PATCH 3/4] Removed unused code Signed-off-by: snipe --- tests/Feature/Users/Api/StoreUserTest.php | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/tests/Feature/Users/Api/StoreUserTest.php b/tests/Feature/Users/Api/StoreUserTest.php index 860f16916a..0338f50847 100644 --- a/tests/Feature/Users/Api/StoreUserTest.php +++ b/tests/Feature/Users/Api/StoreUserTest.php @@ -7,7 +7,6 @@ use App\Models\Department; use App\Models\Group; use App\Models\Location; use App\Models\User; -use Illuminate\Support\Facades\Hash; use Tests\TestCase; class StoreUserTest extends TestCase { @@ -65,12 +64,8 @@ class StoreUserTest extends TestCase { public function testDoesNotAcceptBogusGroupData() { $admin = User::factory()->superuser()->create(); - $manager = User::factory()->create(); - $company = Company::factory()->create(); - $department = Department::factory()->create(); - $location = Location::factory()->create(); - $response = $this->actingAsForApi($admin) + $this->actingAsForApi($admin) ->postJson(route('api.users.store'), [ 'first_name' => 'Mabel', 'username' => 'mabel', From f98f978502a591cc7a9a2bf1d3c398d61b8ad598 Mon Sep 17 00:00:00 2001 From: snipe Date: Wed, 11 Dec 2024 16:53:15 +0000 Subject: [PATCH 4/4] Fixed typos Signed-off-by: snipe --- app/Http/Requests/SaveUserRequest.php | 2 +- app/Policies/SnipePermissionsPolicy.php | 20 ++++++++------------ 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/app/Http/Requests/SaveUserRequest.php b/app/Http/Requests/SaveUserRequest.php index b2d9389b2c..b8ddac7d79 100644 --- a/app/Http/Requests/SaveUserRequest.php +++ b/app/Http/Requests/SaveUserRequest.php @@ -18,7 +18,7 @@ class SaveUserRequest extends FormRequest */ public function authorize() { - return Gate::allows('users.create'); + return (Gate::allows('users.create') || Gate::allows('users.edit')); } public function response(array $errors) diff --git a/app/Policies/SnipePermissionsPolicy.php b/app/Policies/SnipePermissionsPolicy.php index 96c94cd776..12cd7a1343 100644 --- a/app/Policies/SnipePermissionsPolicy.php +++ b/app/Policies/SnipePermissionsPolicy.php @@ -23,7 +23,7 @@ use Illuminate\Auth\Access\HandlesAuthorization; abstract class SnipePermissionsPolicy { /** - * This should return the key of the model in the users json permission string. + * This should return the key of the model in the user's JSON permission string. * * @return bool */ @@ -37,11 +37,7 @@ abstract class SnipePermissionsPolicy { /** * If an admin, they can do all item related tasks, but ARE constrained by FMCSA company access. - * That scoping happens on the model level (except for the Users model) via the Companyable trait. - * - * This does lead to some inconsistencies in the responses, since attempting to edit assets, - * accessories, etc (anything other than users) will result in a Forbidden error, whereas the users - * area will redirect with "That user doesn't exist" since the scoping is handled directly on those queries. + * That scoping happens on the model level via the Companyable trait. * * The *superuser* global permission gets handled in the AuthServiceProvider before() method. * @@ -53,7 +49,7 @@ abstract class SnipePermissionsPolicy } /** - * If we got here by $this→authorize('something', $actualModel) then we can continue on Il but if we got here + * If we got here by $this→authorize('something', $actualModel) then we can continue on, but if we got here * via $this→authorize('something', Model::class) then calling Company:: isCurrentUserHasAccess($item) gets weird. * Bail out here by returning "nothing" and allow the relevant method lower in this class to be called and handle authorization. */ @@ -85,7 +81,7 @@ abstract class SnipePermissionsPolicy } /** - * Determine whether the user can view the accessory. + * Determine whether the user can view the item. * * @param \App\Models\User $user * @return mixed @@ -112,7 +108,7 @@ abstract class SnipePermissionsPolicy } /** - * Determine whether the user can update the accessory. + * Determine whether the user can update the item. * * @param \App\Models\User $user * @return mixed @@ -124,7 +120,7 @@ abstract class SnipePermissionsPolicy /** - * Determine whether the user can update the accessory. + * Determine whether the user can checkout the item. * * @param \App\Models\User $user * @return mixed @@ -135,7 +131,7 @@ abstract class SnipePermissionsPolicy } /** - * Determine whether the user can delete the accessory. + * Determine whether the user can delete the item. * * @param \App\Models\User $user * @return mixed @@ -151,7 +147,7 @@ abstract class SnipePermissionsPolicy } /** - * Determine whether the user can manage the accessory. + * Determine whether the user can manage the item. * * @param \App\Models\User $user * @return mixed