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,