fix(security): enforce company scope on notes, estimate-convert, and user bulk-delete (#661)

- Notes IDOR (GHSA-85wc): NotePolicy::viewNotes/manageNotes now receive the
  Note and require hasCompany($note->company_id); NotesController passes the
  bound model to authorize() on show/update/destroy.
- Estimate->Invoice IDOR (GHSA-j2vg): ConvertEstimateController authorizes
  'view' on the source estimate before creating the invoice.
- User bulk-delete (GHSA-wxrv): UsersController scopes candidate ids via
  User::whereCompany() before deletion so cross-company accounts are protected.

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

app/Http/Controllers/V1/Admin/Estimate/ConvertEstimateController.php

@@ -23,6 +23,10 @@ class ConvertEstimateController extends Controller
      */
     public function __invoke(Request $request, Estimate $estimate, Invoice $invoice)
     {
+        // Authorize access to the source estimate (tenant isolation) in addition
+        // to the ability to create an invoice — otherwise any estimate id from
+        // another company could be converted and disclosed.
+        $this->authorize('view', $estimate);
         $this->authorize('create', Invoice::class);
 
         $estimate->load(['items', 'items.taxes', 'customer', 'taxes']);

app/Http/Controllers/V1/Admin/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/V1/Admin/Users/UsersController.php

@@ -90,7 +90,16 @@ class UsersController extends Controller
         $this->authorize('delete multiple users', User::class);
 
         if ($request->users) {
-            User::deleteUsers($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) {
+                User::deleteUsers($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

@@ -6,6 +6,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;
@@ -247,6 +248,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/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);
+});

tests/Feature/Admin/UserTest.php

@@ -2,6 +2,7 @@
 
 use App\Http\Controllers\V1\Admin\Users\UsersController;
 use App\Http\Requests\UserRequest;
+use App\Models\Company;
 use App\Models\User;
 use Laravel\Sanctum\Sanctum;
 
@@ -86,12 +87,21 @@ test('update user using a form request', function () {
 //     ]);
 // });
 
-// test('delete users', function () {
-//     $user = User::factory()->create();
-//     $data['users'] = [$user->id];
+test('deletes a user 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/users/delete", $data)
-//         ->assertOk();
+    postJson('/api/v1/users/delete', ['users' => [$user->id]])->assertOk();
 
-//     $this->assertModelMissing($user);
-// });
+    $this->assertDatabaseMissing('users', ['id' => $user->id]);
+});
+
+test('cannot bulk delete a user belonging to another company', function () {
+    $user = User::factory()->create();
+    $user->companies()->attach(Company::factory()->create()->id);
+
+    postJson('/api/v1/users/delete', ['users' => [$user->id]])->assertOk();
+
+    $this->assertDatabaseHas('users', ['id' => $user->id]);
+});