File Uploades: Fixed FD-56588 - tighter controls on model files

app/Http/Controllers/Api/UploadedFilesController.php

@@ -89,9 +89,12 @@ class UploadedFilesController extends Controller
     public function store(UploadFileRequest $request, $object_type, $id): JsonResponse
     {
 
-        // Check the permissions to make sure the user can view the object
+        // Check the permissions to make sure the user is allowed to upload
+        // to the object. `manageFiles` is stricter than `files` (used by
+        // index/show below) so a read-only cascade like the one on
+        // AssetModelPolicy does not accidentally grant write access.
         $object = self::$map_object_type[$object_type]::withTrashed()->find($id);
-        $this->authorize('files', $object);
+        $this->authorize('manageFiles', $object);
 
         if (! $object) {
             return response()->json(Helper::formatStandardApiResponse('error', null, trans('general.file_upload_status.invalid_object')));
@@ -192,9 +195,10 @@ class UploadedFilesController extends Controller
     public function destroy($object_type, $id, $file_id): JsonResponse
     {
 
-        // Check the permissions to make sure the user can view the object
+        // See store(): `manageFiles` is the strict write ability so a
+        // read-only cascade in files() does not authorize deletion.
         $object = self::$map_object_type[$object_type]::withTrashed()->find($id);
-        $this->authorize('files', $object);
+        $this->authorize('manageFiles', $object);
 
         if (! $object) {
             return response()->json(Helper::formatStandardApiResponse('error', null, trans('general.file_upload_status.invalid_object')));

app/Http/Controllers/UploadedFilesController.php

@@ -35,9 +35,12 @@ class UploadedFilesController extends Controller
     public function store(UploadFileRequest $request, $object_type, $id): RedirectResponse
     {
 
-        // Check the permissions to make sure the user can view the object
+        // Check the permissions to make sure the user is allowed to upload
+        // to the object. `manageFiles` is stricter than `files` (used by
+        // show/download) so a read-only cascade like the one on
+        // AssetModelPolicy does not accidentally grant write access.
         $object = self::$map_object_type[$object_type]::withTrashed()->find($id);
-        $this->authorize('files', $object);
+        $this->authorize('manageFiles', $object);
 
         if (! $object) {
             return redirect()->back()->withFragment('files')->with('error', trans('general.file_upload_status.invalid_object'));
@@ -129,9 +132,10 @@ class UploadedFilesController extends Controller
     public function destroy($object_type, $id, $file_id): RedirectResponse
     {
 
-        // Check the permissions to make sure the user can view the object
+        // See store(): `manageFiles` is the strict write ability so a
+        // read-only cascade in files() does not authorize deletion.
         $object = self::$map_object_type[$object_type]::withTrashed()->find($id);
-        $this->authorize('files', $object);
+        $this->authorize('manageFiles', $object);
 
         if (! $object) {
             return redirect()->back()->withFragment('files')->with('error', trans('general.file_upload_status.invalid_object'));

app/Policies/AssetModelPolicy.php

@@ -11,13 +11,30 @@ class AssetModelPolicy extends SnipePermissionsPolicy
         return 'models';
     }
 
+    /**
+     * READ ability for model files (index / show / download). Cascades from
+     * assets.files because a model's file attachments (user manuals, spec
+     * sheets, etc.) show up on the asset detail page and are legitimately
+     * useful to anyone who can see the asset itself.
+     */
     public function files(User $user, $item = null)
     {
-        // Set this to true so that users who can see the asset can also see the associated model files
         if ($user->hasAccess('assets.files')) {
             return true;
         }
 
         return $user->hasAccess($this->columnName().'.files');
     }
+
+    /**
+     * WRITE ability for model files (upload / delete). Strict: requires the
+     * dedicated `models.files` grant. The read cascade above must NOT
+     * short-circuit here or an admin who withheld `models.files` while
+     * granting `assets.files` to routine technicians would still see them
+     * mutating the shared model file catalog.
+     */
+    public function manageFiles(User $user, $item = null)
+    {
+        return $user->hasAccess($this->columnName().'.files');
+    }
 }

app/Policies/SnipePermissionsPolicy.php

@@ -107,6 +107,21 @@ abstract class SnipePermissionsPolicy
         return $user->hasAccess($this->columnName().'.files');
     }
 
+    /**
+     * Determine whether the user can upload or delete files on the resource.
+     * Callers should use this for write actions (POST/DELETE on the files
+     * endpoint) and `files()` for read actions (index/show). Defaults to the
+     * same check as `files()` so most resources need no override. Subclasses
+     * override this when the read ability is deliberately broader than the
+     * write ability, e.g. AssetModelPolicy where asset viewers can see model
+     * files inline but must not be able to upload or delete them without the
+     * dedicated `models.files` grant.
+     */
+    public function manageFiles(User $user, $item = null)
+    {
+        return $this->files($user, $item);
+    }
+
     /**
      * Determine whether the user can create model.
      *

database/factories/UserFactory.php

@@ -551,6 +551,11 @@ class UserFactory extends Factory
         return $this->appendPermission(['assets.audit' => '1']);
     }
 
+    public function manageAssetFiles()
+    {
+        return $this->appendPermission(['assets.files' => '1']);
+    }
+
     public function manageModelFiles()
     {
         return $this->appendPermission(['models.files' => '1']);

tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php

@@ -0,0 +1,153 @@
+<?php
+
+namespace Tests\Feature\AssetModels\Api;
+
+use App\Models\AssetModel;
+use App\Models\User;
+use Illuminate\Http\UploadedFile;
+use Tests\TestCase;
+
+/**
+ * Regression tests for a broken-access-control bug where a user with only
+ * `assets.files` (and not `models.files`) could upload and delete files on
+ * any asset model instance-wide. The `AssetModelPolicy::files()` method
+ * short-circuited on `assets.files`, and the controller used the same
+ * `files` ability for both read and write actions, so the intended write
+ * gate `models.files` was bypassable.
+ *
+ * Fix: added `manageFiles()` as the write ability. AssetModelPolicy overrides
+ * it to require `models.files` strictly. The read cascade on `files()` is
+ * preserved so anyone who can view an asset can still see its model's file
+ * attachments inline on the asset detail page.
+ */
+class AssetModelFilesPolicyBypassTest extends TestCase
+{
+    public function test_read_cascade_from_assets_files_is_preserved_for_index()
+    {
+        // Seed one file on the model as a superuser so there is something to see.
+        $model = AssetModel::factory()->create();
+        $this->uploadFileAs(User::factory()->superuser()->create(), $model);
+
+        // A user with only assets.files (no models.files) can still see model files
+        // because the model's files show up on the asset detail page and blocking
+        // that read would be a UX regression.
+        $reader = User::factory()->manageAssetFiles()->create();
+
+        $this->actingAsForApi($reader)
+            ->getJson(route('api.files.index', ['object_type' => 'models', 'id' => $model->id]))
+            ->assertOk()
+            ->assertJsonPath('total', 1);
+    }
+
+    public function test_read_cascade_from_assets_files_is_preserved_for_show()
+    {
+        // Seed one file as superuser.
+        $model = AssetModel::factory()->create();
+        $fileId = $this->uploadFileAndReturnId(User::factory()->superuser()->create(), $model);
+
+        $reader = User::factory()->manageAssetFiles()->create();
+
+        $this->actingAsForApi($reader)
+            ->get(route('api.files.show', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId]))
+            ->assertOk();
+    }
+
+    public function test_user_with_only_assets_files_cannot_upload_to_model()
+    {
+        $model = AssetModel::factory()->create();
+        $writer = User::factory()->manageAssetFiles()->create();
+
+        $this->actingAsForApi($writer)
+            ->post(
+                route('api.files.store', ['object_type' => 'models', 'id' => $model->id]),
+                ['file' => [UploadedFile::fake()->create('test.jpg', 100)]]
+            )
+            ->assertForbidden();
+    }
+
+    public function test_user_with_only_assets_files_cannot_delete_model_file()
+    {
+        // Seed a file as superuser so there is something for the low-priv user to try to delete.
+        $model = AssetModel::factory()->create();
+        $fileId = $this->uploadFileAndReturnId(User::factory()->superuser()->create(), $model);
+
+        $writer = User::factory()->manageAssetFiles()->create();
+
+        $this->actingAsForApi($writer)
+            ->delete(route('api.files.destroy', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId]))
+            ->assertForbidden();
+    }
+
+    public function test_user_with_models_files_can_upload_to_model()
+    {
+        $model = AssetModel::factory()->create();
+        $writer = User::factory()->manageModelFiles()->create();
+
+        $this->actingAsForApi($writer)
+            ->post(
+                route('api.files.store', ['object_type' => 'models', 'id' => $model->id]),
+                ['file' => [UploadedFile::fake()->create('test.jpg', 100)]]
+            )
+            ->assertOk();
+    }
+
+    public function test_user_with_models_files_can_delete_model_file()
+    {
+        $model = AssetModel::factory()->create();
+        $fileId = $this->uploadFileAndReturnId(User::factory()->superuser()->create(), $model);
+
+        $writer = User::factory()->manageModelFiles()->create();
+
+        $this->actingAsForApi($writer)
+            ->delete(route('api.files.destroy', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId]))
+            ->assertOk();
+    }
+
+    public function test_user_with_no_file_permissions_is_forbidden_on_all_actions()
+    {
+        $model = AssetModel::factory()->create();
+        $fileId = $this->uploadFileAndReturnId(User::factory()->superuser()->create(), $model);
+
+        $noPerms = User::factory()->create();
+
+        $this->actingAsForApi($noPerms)
+            ->getJson(route('api.files.index', ['object_type' => 'models', 'id' => $model->id]))
+            ->assertForbidden();
+
+        $this->actingAsForApi($noPerms)
+            ->get(route('api.files.show', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId]))
+            ->assertForbidden();
+
+        $this->actingAsForApi($noPerms)
+            ->post(
+                route('api.files.store', ['object_type' => 'models', 'id' => $model->id]),
+                ['file' => [UploadedFile::fake()->create('test.jpg', 100)]]
+            )
+            ->assertForbidden();
+
+        $this->actingAsForApi($noPerms)
+            ->delete(route('api.files.destroy', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId]))
+            ->assertForbidden();
+    }
+
+    private function uploadFileAs(User $user, AssetModel $model): void
+    {
+        $this->actingAsForApi($user)
+            ->post(
+                route('api.files.store', ['object_type' => 'models', 'id' => $model->id]),
+                ['file' => [UploadedFile::fake()->create('test.jpg', 100)]]
+            )
+            ->assertOk();
+    }
+
+    private function uploadFileAndReturnId(User $user, AssetModel $model): int
+    {
+        $this->uploadFileAs($user, $model);
+
+        return $this->actingAsForApi($user)
+            ->getJson(route('api.files.index', ['object_type' => 'models', 'id' => $model->id]))
+            ->assertOk()
+            ->decodeResponseJson()
+            ->json()['rows'][0]['id'];
+    }
+}