fix(security): enforce company scope on notes, doc-convert, and member bulk-delete (#668)

v3 port of the v2 authorization fixes.

- Notes IDOR (GHSA-85wc): NotePolicy checks the note's company_id and
  NotesController passes the bound model to authorize() on show/update/destroy.
- Estimate<->Invoice convert IDOR (GHSA-j2vg): EstimatesController::convertToInvoice
  and InvoicesController::convertToEstimate authorize 'view' on the source
  document before creating the target.
- Member bulk-delete (GHSA-wxrv): MembersController scopes ids via
  User::whereCompany() before MemberService::delete.

Adds feature tests for cross-company 403s + same-company happy paths.

app/Http/Controllers/Company/Estimate/EstimatesController.php

@@ -121,6 +121,9 @@ class EstimatesController extends Controller
 
     public function convertToInvoice(Request $request, Estimate $estimate)
     {
+        // Authorize access to the source estimate (tenant isolation) in addition
+        // to the ability to create an invoice.
+        $this->authorize('view', $estimate);
         $this->authorize('create', Invoice::class);
 
         $invoice = $this->estimateService->convertToInvoice($estimate);

app/Http/Controllers/Company/General/NotesController.php

@@ -61,7 +61,7 @@ class NotesController extends Controller
      */
     public function show(Note $note)
     {
-        $this->authorize('view notes');
+        $this->authorize('view notes', $note);
 
         return new NoteResource($note);
     }
@@ -74,7 +74,7 @@ class NotesController extends Controller
      */
     public function update(NotesRequest $request, Note $note)
     {
-        $this->authorize('manage notes');
+        $this->authorize('manage notes', $note);
 
         $note->update($request->getNotesPayload());
 
@@ -97,7 +97,7 @@ class NotesController extends Controller
      */
     public function destroy(Note $note)
     {
-        $this->authorize('manage notes');
+        $this->authorize('manage notes', $note);
 
         $note->delete();
 

app/Http/Controllers/Company/Invoice/InvoicesController.php

@@ -151,6 +151,9 @@ class InvoicesController extends Controller
 
     public function convertToEstimate(Request $request, Invoice $invoice)
     {
+        // Authorize access to the source invoice (tenant isolation) in addition
+        // to the ability to create an estimate.
+        $this->authorize('view', $invoice);
         $this->authorize('create', Estimate::class);
 
         $estimate = $this->invoiceService->convertToEstimate($invoice);

app/Http/Controllers/Company/Members/MembersController.php

@@ -93,7 +93,16 @@ class MembersController extends Controller
         $this->authorize('delete multiple users', User::class);
 
         if ($request->users) {
-            $this->memberService->delete($request->users);
+            // Scope the candidate ids to members of the acting company so a user
+            // from one company cannot delete accounts belonging to another.
+            $ids = User::whereCompany()
+                ->whereIn('id', $request->users)
+                ->pluck('id')
+                ->toArray();
+
+            if ($ids) {
+                $this->memberService->delete($ids);
+            }
         }
 
         return response()->json([

app/Policies/NotePolicy.php

@@ -11,21 +11,23 @@ class NotePolicy
 {
     use HandlesAuthorization;
 
-    public function manageNotes(User $user)
+    public function manageNotes(User $user, ?Note $note = null)
     {
-        if (BouncerFacade::can('manage-all-notes', Note::class)) {
-            return true;
+        if (! BouncerFacade::can('manage-all-notes', $note ?? Note::class)) {
+            return false;
         }
 
-        return false;
+        // When acting on a specific note, enforce tenant isolation: the note
+        // must belong to a company the user is a member of.
+        return $note === null || $user->hasCompany($note->company_id);
     }
 
-    public function viewNotes(User $user)
+    public function viewNotes(User $user, ?Note $note = null)
     {
-        if (BouncerFacade::can('view-all-notes', Note::class)) {
-            return true;
+        if (! BouncerFacade::can('view-all-notes', $note ?? Note::class)) {
+            return false;
         }
 
-        return false;
+        return $note === null || $user->hasCompany($note->company_id);
     }
 }

tests/Feature/Admin/EstimateTest.php

@@ -5,6 +5,7 @@ use App\Http\Requests\DeleteEstimatesRequest;
 use App\Http\Requests\EstimatesRequest;
 use App\Http\Requests\SendEstimatesRequest;
 use App\Mail\SendEstimateMail;
+use App\Models\Company;
 use App\Models\Estimate;
 use App\Models\EstimateItem;
 use App\Models\Tax;
@@ -246,6 +247,16 @@ test('create invoice from estimate', function () {
     $response->assertStatus(200);
 });
 
+test('cannot convert an estimate belonging to another company', function () {
+    $estimate = Estimate::factory()->create([
+        'company_id' => Company::factory()->create()->id,
+        'estimate_date' => now(),
+        'expiry_date' => now()->addMonth(),
+    ]);
+
+    postJson("api/v1/estimates/{$estimate->id}/convert-to-invoice")->assertStatus(403);
+});
+
 test('delete multiple estimates using a form request', function () {
     $this->assertActionUsesFormRequest(
         EstimatesController::class,

tests/Feature/Admin/InvoiceTest.php

@@ -3,6 +3,7 @@
 use App\Http\Controllers\Company\Invoice\InvoicesController;
 use App\Http\Requests\InvoicesRequest;
 use App\Mail\SendInvoiceMail;
+use App\Models\Company;
 use App\Models\Invoice;
 use App\Models\InvoiceItem;
 use App\Models\Tax;
@@ -34,6 +35,12 @@ test('testGetInvoices', function () {
     $response->assertOk();
 });
 
+test('cannot convert an invoice belonging to another company', function () {
+    $invoice = Invoice::factory()->create(['company_id' => Company::factory()->create()->id]);
+
+    postJson("api/v1/invoices/{$invoice->id}/convert-to-estimate")->assertStatus(403);
+});
+
 test('create invoice', function () {
     $invoice = Invoice::factory()
         ->raw([

tests/Feature/Admin/MemberTest.php

@@ -2,10 +2,12 @@
 
 use App\Http\Controllers\Company\Members\MembersController;
 use App\Http\Requests\MemberRequest;
+use App\Models\Company;
 use App\Models\User;
 use Laravel\Sanctum\Sanctum;
 
 use function Pest\Laravel\getJson;
+use function Pest\Laravel\postJson;
 
 beforeEach(function () {
     Artisan::call('db:seed', ['--class' => 'DatabaseSeeder', '--force' => true]);
@@ -48,3 +50,22 @@ test('update member using a form request', function () {
         MemberRequest::class
     );
 });
+
+test('deletes a 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);
+
+    postJson('/api/v1/members/delete', ['users' => [$user->id]])->assertOk();
+
+    $this->assertDatabaseMissing('users', ['id' => $user->id]);
+});
+
+test('cannot bulk delete a member belonging to another company', function () {
+    $user = User::factory()->create();
+    $user->companies()->attach(Company::factory()->create()->id);
+
+    postJson('/api/v1/members/delete', ['users' => [$user->id]])->assertOk();
+
+    $this->assertDatabaseHas('users', ['id' => $user->id]);
+});

tests/Feature/Admin/NotesTest.php

@@ -1,5 +1,6 @@
 <?php
 
+use App\Models\Company;
 use App\Models\Note;
 use App\Models\User;
 use Illuminate\Support\Facades\Artisan;
@@ -65,3 +66,28 @@ test('delete note', function () {
 
     $this->assertModelMissing($note);
 });
+
+test('cannot retrieve a note belonging to another company', function () {
+    $note = Note::factory()->create(['company_id' => Company::factory()->create()->id]);
+
+    getJson("/api/v1/notes/{$note->id}")->assertStatus(403);
+});
+
+test('cannot update a note belonging to another company', function () {
+    $note = Note::factory()->create([
+        'company_id' => Company::factory()->create()->id,
+        'name' => 'original-name',
+    ]);
+
+    putJson("/api/v1/notes/{$note->id}", Note::factory()->raw())->assertStatus(403);
+
+    $this->assertDatabaseHas('notes', ['id' => $note->id, 'name' => 'original-name']);
+});
+
+test('cannot delete a note belonging to another company', function () {
+    $note = Note::factory()->create(['company_id' => Company::factory()->create()->id]);
+
+    deleteJson("/api/v1/notes/{$note->id}")->assertStatus(403);
+
+    $this->assertModelExists($note);
+});