diff --git a/app/Policies/AssetModelPolicy.php b/app/Policies/AssetModelPolicy.php index 5dff76650f..2fa90750fa 100644 --- a/app/Policies/AssetModelPolicy.php +++ b/app/Policies/AssetModelPolicy.php @@ -13,13 +13,14 @@ class AssetModelPolicy extends SnipePermissionsPolicy /** * 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. + * asset visibility: model file attachments (user manuals, spec sheets, + * etc.) apply to every asset of a given model, so anyone who can view + * assets can see them. Managing (upload/delete) still requires the + * dedicated `models.files` grant via manageFiles() below. */ public function files(User $user, $item = null) { - if ($user->hasAccess('assets.files')) { + if ($user->hasAccess('assets.view')) { return true; } diff --git a/app/Presenters/UploadedFilesPresenter.php b/app/Presenters/UploadedFilesPresenter.php index bc690e4083..91115c0f5f 100644 --- a/app/Presenters/UploadedFilesPresenter.php +++ b/app/Presenters/UploadedFilesPresenter.php @@ -8,11 +8,16 @@ namespace App\Presenters; class UploadedFilesPresenter extends Presenter { /** - * Json Column Layout for bootstrap table + * Json Column Layout for bootstrap table. * + * @param array $hide_fields Column field names to omit from the layout. + * Callers pass `['available_actions']` for + * read-only views (e.g. the model files tab + * for users without models.files) so the + * delete button doesn't render at all. * @return string */ - public static function dataTableLayout() + public static function dataTableLayout($hide_fields = []) { $layout = [ @@ -94,7 +99,11 @@ class UploadedFilesPresenter extends Presenter 'title' => trans('general.created_at'), 'visible' => true, 'formatter' => 'dateDisplayFormatter', - ], [ + ], + ]; + + if (! in_array('available_actions', $hide_fields)) { + $layout[] = [ 'field' => 'available_actions', 'scope' => 'col', 'searchable' => false, @@ -105,8 +114,8 @@ class UploadedFilesPresenter extends Presenter 'formatter' => 'deleteUploadFormatter', 'printIgnore' => true, 'class' => 'hidden-print', - ], - ]; + ]; + } return json_encode($layout); } diff --git a/resources/views/blade/table/files.blade.php b/resources/views/blade/table/files.blade.php index 6e75cc184f..93617f8eec 100644 --- a/resources/views/blade/table/files.blade.php +++ b/resources/views/blade/table/files.blade.php @@ -12,7 +12,7 @@ @if(isset($object)) null, 'class' => false, + 'item' => null, ]) - \ No newline at end of file +@can('files', $item) + +@endcan \ No newline at end of file diff --git a/resources/views/hardware/view.blade.php b/resources/views/hardware/view.blade.php index 8e30d56bc6..bc361a3e4d 100755 --- a/resources/views/hardware/view.blade.php +++ b/resources/views/hardware/view.blade.php @@ -69,7 +69,7 @@ /> - + diff --git a/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php b/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php index 1cd6473304..9abba3070e 100644 --- a/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php +++ b/tests/Feature/AssetModels/Api/AssetModelFilesPolicyBypassTest.php @@ -22,16 +22,16 @@ use Tests\TestCase; */ class AssetModelFilesPolicyBypassTest extends TestCase { - public function test_read_cascade_from_assets_files_is_preserved_for_index() + public function test_asset_viewer_can_list_model_files() { // 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(); + // Anyone who can view assets can see the model's file attachments + // (user manuals, spec sheets) because they show up on the asset + // detail page and apply to every asset of that model. + $reader = User::factory()->viewAssets()->create(); $this->actingAsForApi($reader) ->getJson(route('api.files.index', ['object_type' => 'models', 'id' => $model->id])) @@ -39,21 +39,49 @@ class AssetModelFilesPolicyBypassTest extends TestCase ->assertJsonPath('total', 1); } - public function test_read_cascade_from_assets_files_is_preserved_for_show() + public function test_asset_viewer_can_download_model_file() { // Seed one file as superuser. $model = AssetModel::factory()->create(); $fileId = $this->uploadFileAndReturnId(User::factory()->superuser()->create(), $model); - $reader = User::factory()->manageAssetFiles()->create(); + $reader = User::factory()->viewAssets()->create(); $this->actingAsForApi($reader) ->get(route('api.files.show', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId])) ->assertOk(); } + public function test_asset_viewer_cannot_upload_to_model() + { + $model = AssetModel::factory()->create(); + $writer = User::factory()->viewAssets()->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_asset_viewer_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()->viewAssets()->create(); + + $this->actingAsForApi($writer) + ->delete(route('api.files.destroy', ['object_type' => 'models', 'id' => $model->id, 'file_id' => $fileId])) + ->assertForbidden(); + } + public function test_user_with_only_assets_files_cannot_upload_to_model() { + // The originally-reported bypass scenario: assets.files alone used to + // grant model file upload/delete. It must not anymore. $model = AssetModel::factory()->create(); $writer = User::factory()->manageAssetFiles()->create(); @@ -67,7 +95,6 @@ class AssetModelFilesPolicyBypassTest extends TestCase 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); diff --git a/tests/Unit/Presenters/UploadedFilesPresenterTest.php b/tests/Unit/Presenters/UploadedFilesPresenterTest.php new file mode 100644 index 0000000000..bcaded9b21 --- /dev/null +++ b/tests/Unit/Presenters/UploadedFilesPresenterTest.php @@ -0,0 +1,26 @@ +assertContains('available_actions', array_column($layout, 'field')); + } + + public function test_layout_omits_actions_column_when_hidden() + { + $layout = json_decode( + UploadedFilesPresenter::dataTableLayout(['available_actions']), + true + ); + + $this->assertNotContains('available_actions', array_column($layout, 'field')); + } +}