From d3202b8b2a769018ea5b077ab8cb5bfe1919bce6 Mon Sep 17 00:00:00 2001 From: Darko Gjorgjijoski Date: Sun, 14 Jun 2026 23:42:11 +0200 Subject: [PATCH] fix(members): scope member view & update to the acting company Member view/update bound the target user by global id and authorized only that the requester owns their active company, not that the target belonged to it. Bind the route model under the members param and require shared company membership in UserPolicy so an owner of one company can no longer read or modify users of another. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Company/Members/MembersController.php | 14 ++++---- app/Http/Requests/MemberRequest.php | 2 +- app/Policies/UserPolicy.php | 32 ++++++++++--------- tests/Feature/Admin/MemberTest.php | 26 ++++++++++++++- 4 files changed, 50 insertions(+), 24 deletions(-) diff --git a/app/Http/Controllers/Company/Members/MembersController.php b/app/Http/Controllers/Company/Members/MembersController.php index 451e70a7..81d503e1 100644 --- a/app/Http/Controllers/Company/Members/MembersController.php +++ b/app/Http/Controllers/Company/Members/MembersController.php @@ -61,11 +61,11 @@ class MembersController extends Controller * * @return JsonResponse */ - public function show(User $user) + public function show(User $member) { - $this->authorize('view', $user); + $this->authorize('view', $member); - return new UserResource($user); + return new UserResource($member); } /** @@ -73,13 +73,13 @@ class MembersController extends Controller * * @return JsonResponse */ - public function update(MemberRequest $request, User $user) + public function update(MemberRequest $request, User $member) { - $this->authorize('update', $user); + $this->authorize('update', $member); - $this->memberService->update($user, $request); + $this->memberService->update($member, $request); - return new UserResource($user); + return new UserResource($member); } /** diff --git a/app/Http/Requests/MemberRequest.php b/app/Http/Requests/MemberRequest.php index 320e803e..dff566dc 100644 --- a/app/Http/Requests/MemberRequest.php +++ b/app/Http/Requests/MemberRequest.php @@ -52,7 +52,7 @@ class MemberRequest extends FormRequest $rules['email'] = [ 'required', new IdnEmail, - Rule::unique('users')->ignore($this->user), + Rule::unique('users')->ignore($this->member), ]; $rules['password'] = [ 'nullable', diff --git a/app/Policies/UserPolicy.php b/app/Policies/UserPolicy.php index ad1305b0..1c64c46b 100644 --- a/app/Policies/UserPolicy.php +++ b/app/Policies/UserPolicy.php @@ -30,11 +30,7 @@ class UserPolicy */ public function view(User $user, User $model): bool { - if ($user->isOwner()) { - return true; - } - - return false; + return $user->isOwner() && $this->sharesActiveCompany($model); } /** @@ -58,11 +54,7 @@ class UserPolicy */ public function update(User $user, User $model): bool { - if ($user->isOwner()) { - return true; - } - - return false; + return $user->isOwner() && $this->sharesActiveCompany($model); } /** @@ -72,11 +64,7 @@ class UserPolicy */ public function delete(User $user, User $model): bool { - if ($user->isOwner()) { - return true; - } - - return false; + return $user->isOwner() && $this->sharesActiveCompany($model); } /** @@ -134,4 +122,18 @@ class UserPolicy return false; } + + /** + * A company owner may only act on members of the company set in the + * `company` request header. Without this, view/update/delete resolve the + * target user by global id, which would let an owner of one company read + * or overwrite users belonging to another company (cross-tenant IDOR). + */ + private function sharesActiveCompany(User $model): bool + { + $companyId = request()->header('company'); + + return $companyId + && $model->companies()->wherePivot('company_id', $companyId)->exists(); + } } diff --git a/tests/Feature/Admin/MemberTest.php b/tests/Feature/Admin/MemberTest.php index 277d9acc..68c92c96 100644 --- a/tests/Feature/Admin/MemberTest.php +++ b/tests/Feature/Admin/MemberTest.php @@ -8,6 +8,7 @@ use Laravel\Sanctum\Sanctum; use function Pest\Laravel\getJson; use function Pest\Laravel\postJson; +use function Pest\Laravel\putJson; beforeEach(function () { Artisan::call('db:seed', ['--class' => 'DatabaseSeeder', '--force' => true]); @@ -37,12 +38,35 @@ test('store member using a form request', function () { ); }); -test('get member', function () { +test('get member belonging to the current company', function () { + $companyId = User::where('role', 'super admin')->first()->companies()->first()->id; $user = User::factory()->create(); + $user->companies()->attach($companyId); getJson("/api/v1/members/{$user->id}")->assertOk(); }); +test('cannot view a member belonging to another company', function () { + $user = User::factory()->create(); + $user->companies()->attach(Company::factory()->create()->id); + + getJson("/api/v1/members/{$user->id}")->assertForbidden(); +}); + +test('cannot update a member belonging to another company', function () { + $companyId = User::where('role', 'super admin')->first()->companies()->first()->id; + $user = User::factory()->create(); + $user->companies()->attach(Company::factory()->create()->id); + + putJson("/api/v1/members/{$user->id}", [ + 'name' => 'Hacked', + 'email' => 'pwned@attacker.test', + 'companies' => [['id' => $companyId, 'role' => 'super admin']], + ])->assertForbidden(); + + $this->assertDatabaseMissing('users', ['email' => 'pwned@attacker.test']); +}); + test('update member using a form request', function () { $this->assertActionUsesFormRequest( MembersController::class,