From 14d73edb52f05a64f0db58450cbeed8719f8b281 Mon Sep 17 00:00:00 2001 From: snipe Date: Mon, 20 Jul 2026 13:30:36 +0100 Subject: [PATCH] File Uploades: Fixed FD-56588 - tighter controls on model files --- .../Api/UploadedFilesController.php | 12 +- .../Controllers/UploadedFilesController.php | 12 +- app/Policies/AssetModelPolicy.php | 19 ++- app/Policies/SnipePermissionsPolicy.php | 15 ++ database/factories/UserFactory.php | 5 + .../Api/AssetModelFilesPolicyBypassTest.php | 153 ++++++++++++++++++ 6 files changed, 207 insertions(+), 9 deletions(-) create mode 100644 tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php diff --git a/app/Http/Controllers/Api/UploadedFilesController.php b/app/Http/Controllers/Api/UploadedFilesController.php index c4d7fd8afd..5fb1956df9 100644 --- a/app/Http/Controllers/Api/UploadedFilesController.php +++ b/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'))); diff --git a/app/Http/Controllers/UploadedFilesController.php b/app/Http/Controllers/UploadedFilesController.php index eb3a082993..dad39672da 100644 --- a/app/Http/Controllers/UploadedFilesController.php +++ b/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')); diff --git a/app/Policies/AssetModelPolicy.php b/app/Policies/AssetModelPolicy.php index bce4e048fe..5dff76650f 100644 --- a/app/Policies/AssetModelPolicy.php +++ b/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'); + } } diff --git a/app/Policies/SnipePermissionsPolicy.php b/app/Policies/SnipePermissionsPolicy.php index 731fa5da5a..718b46b630 100644 --- a/app/Policies/SnipePermissionsPolicy.php +++ b/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. * diff --git a/database/factories/UserFactory.php b/database/factories/UserFactory.php index b6e5c7d5f5..794a2fff3c 100644 --- a/database/factories/UserFactory.php +++ b/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']); diff --git a/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php b/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php new file mode 100644 index 0000000000..1cd6473304 --- /dev/null +++ b/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php @@ -0,0 +1,153 @@ +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']; + } +}