Adds permission checks for custom fields and custom fieldsets (#5645) (#5795)

* adds permission checks to custom fields

* adds permission checks to custom fieldsets

* adds separate permissions for custom fieldsets

* check for permissions in views

* Removes custom fieldsets from permissions config

* Proxy the authorization for custom fieldsets down to custom fields.

This allows us to use the existing permissions in use and have more semantically correct authorization checks for custom fieldsets.

* simplifies the authorization check for the custom fields overview

* removes special handling of custom fieldsets in base policy

I just realised that this code duplicates the logic from the custom fieldset policy.
Since we are checking for the authorization of custom fields anyway, we can just use the columnName for the fields.

* cleanup of unused imports

app/Http/Controllers/CustomFieldsController.php

@@ -37,6 +37,7 @@ class CustomFieldsController extends Controller
     */
     public function index()
     {
+        $this->authorize('view', CustomField::class);
 
         $fieldsets = CustomFieldset::with("fields", "models")->get();
         $fields = CustomField::with("fieldset")->get();
@@ -57,6 +58,7 @@ class CustomFieldsController extends Controller
     */
     public function create()
     {
+        $this->authorize('create', CustomField::class);
 
         return view("custom_fields.fields.edit")->with('field', new CustomField());
     }
@@ -72,6 +74,8 @@ class CustomFieldsController extends Controller
     */
     public function store(CustomFieldRequest $request)
     {
+        $this->authorize('create', CustomField::class);
+
         $field = new CustomField([
             "name" => $request->get("name"),
             "element" => $request->get("element"),
@@ -110,6 +114,8 @@ class CustomFieldsController extends Controller
     {
         $field = CustomField::find($field_id);
 
+        $this->authorize('update', $field);
+
         if ($field->fieldset()->detach($fieldset_id)) {
             return redirect()->route('fieldsets.show', ['fieldset' => $fieldset_id])->with("success", trans('admin/custom_fields/message.field.delete.success'));
         }
@@ -128,6 +134,8 @@ class CustomFieldsController extends Controller
     {
         $field = CustomField::find($field_id);
 
+        $this->authorize('delete', $field);
+
         if ($field->fieldset->count()>0) {
             return redirect()->back()->withErrors(['message' => "Field is in-use"]);
         } else {
@@ -149,6 +157,9 @@ class CustomFieldsController extends Controller
     public function edit($id)
     {
         $field = CustomField::find($id);
+
+        $this->authorize('update', $field);
+
         return view("custom_fields.fields.edit")->with('field', $field);
     }
 
@@ -166,6 +177,9 @@ class CustomFieldsController extends Controller
     public function update(CustomFieldRequest $request, $id)
     {
         $field =  CustomField::find($id);
+
+        $this->authorize('update', $field);
+
         $field->name = e($request->get("name"));
         $field->element = e($request->get("element"));
         $field->field_values = e($request->get("field_values"));

app/Http/Controllers/CustomFieldsetsController.php

@@ -38,6 +38,8 @@ class CustomFieldsetsController extends Controller
     {
         $cfset = CustomFieldset::with('fields')->where('id', '=', $id)->orderBy('id', 'ASC')->first();
 
+        $this->authorize('view', $cfset);
+
         if ($cfset) {
             $custom_fields_list = ["" => "Add New Field to Fieldset"] + CustomField::pluck("name", "id")->toArray();
 
@@ -68,6 +70,8 @@ class CustomFieldsetsController extends Controller
     */
     public function create()
     {
+        $this->authorize('create', CustomFieldset::class);
+
         return view("custom_fields.fieldsets.edit");
     }
 
@@ -81,6 +85,8 @@ class CustomFieldsetsController extends Controller
     */
     public function store(Request $request)
     {
+        $this->authorize('create', CustomFieldset::class);
+
         $cfset = new CustomFieldset(
             [
                 "name" => e($request->get("name")),
@@ -141,6 +147,8 @@ class CustomFieldsetsController extends Controller
     {
         $fieldset = CustomFieldset::find($id);
 
+        $this->authorize('delete', $fieldset);
+
         if ($fieldset) {
             $models = AssetModel::where("fieldset_id", "=", $id);
             if ($models->count() == 0) {
@@ -169,6 +177,8 @@ class CustomFieldsetsController extends Controller
 
         $set = CustomFieldset::find($id);
 
+        $this->authorize('update', $set);
+
         foreach ($set->fields as $field) {
             if ($field->id == Input::get('field_id')) {
                 return redirect()->route("fieldsets.show", [$id])->withInput()->withErrors(['field_id' => trans('admin/custom_fields/message.field.already_added')]);

app/Policies/CustomFieldsetPolicy.php

@@ -0,0 +1,20 @@
+<?php
+
+namespace App\Policies;
+
+use App\Policies\SnipePermissionsPolicy;
+
+class CustomFieldsetPolicy extends SnipePermissionsPolicy
+{
+    protected function columnName()
+    {
+        /**
+         * Proxy the authorization for custom fieldsets down to custom fields.
+         * This allows us to use the existing permissions in use and have more 
+         * semantically correct authorization checks for custom fieldsets.
+         *
+         * See: https://github.com/snipe/snipe-it/pull/5795
+         */
+        return 'customfields';
+    }	
+}

app/Providers/AuthServiceProvider.php

@@ -9,6 +9,7 @@ use App\Models\Category;
 use App\Models\Component;
 use App\Models\Consumable;
 use App\Models\CustomField;
+use App\Models\CustomFieldset;
 use App\Models\Department;
 use App\Models\License;
 use App\Models\Location;
@@ -25,6 +26,7 @@ use App\Policies\CategoryPolicy;
 use App\Policies\ComponentPolicy;
 use App\Policies\ConsumablePolicy;
 use App\Policies\CustomFieldPolicy;
+use App\Policies\CustomFieldsetPolicy;
 use App\Policies\DepartmentPolicy;
 use App\Policies\DepreciationPolicy;
 use App\Policies\LicensePolicy;
@@ -56,6 +58,7 @@ class AuthServiceProvider extends ServiceProvider
         Component::class => ComponentPolicy::class,
         Consumable::class => ConsumablePolicy::class,
         CustomField::class => CustomFieldPolicy::class,
+        CustomFieldset::class => CustomFieldsetPolicy::class,
         Department::class => DepartmentPolicy::class,
         Depreciation::class => DepreciationPolicy::class,
         License::class => LicensePolicy::class,
@@ -143,6 +146,7 @@ class AuthServiceProvider extends ServiceProvider
                 || $user->can('view', Company::class)
                 || $user->can('view', Manufacturer::class)
                 || $user->can('view', CustomField::class)
+                || $user->can('view', CustomFieldset::class)                
                 || $user->can('view', Depreciation::class);
         });
     }

config/permissions.php

@@ -417,8 +417,6 @@ return array(
         ),
     ), 
 
-
-
     'Suppliers' => array(
         array(
             'permission' => 'suppliers.view',

resources/views/custom_fields/fieldsets/view.blade.php

@@ -25,7 +25,10 @@
           name="fieldsets" id="sort" class="table table-responsive todo-list">
           <thead>
             <tr>
+              {{-- Hide the sorting handle if we can't update the fieldset --}}
+              @can('update', $custom_fieldset)
               <th class="col-md-1"></th>
+              @endcan
               <th class="col-md-1">{{ trans('admin/custom_fields/general.order') }}</th>
               <th class="col-md-3">{{ trans('admin/custom_fields/general.field_name') }}</th>
               <th class="col-md-2">{{ trans('admin/custom_fields/general.field_format') }}</th>
@@ -37,7 +40,9 @@
           </thead>
           <tbody>
             @foreach($custom_fieldset->fields as $field)
-            <tr class="cansort" data-index="{{ $field->pivot->custom_field_id }}" id="item_{{ $field->pivot->custom_field_id }}">
+            <tr class="{{ Auth::user()->can('update', $custom_fieldset)?'cansort':'' }}" data-index="{{ $field->pivot->custom_field_id }}" id="item_{{ $field->pivot->custom_field_id }}">
+              {{-- Hide the sorting handle if we can't update the fieldset --}}
+              @can('update', $custom_fieldset)
               <td>
                 <!-- drag handle -->
                 <span class="handle">
@@ -45,6 +50,7 @@
                 <i class="fa fa-ellipsis-v"></i>
                 </span>
               </td>
+              @endcan
               <td class="index">{{$field->pivot->order}}</td>
               <td>{{$field->name}}</td>
               <td>{{$field->format}}</td>
@@ -52,7 +58,9 @@
               <td>{{ $field->field_encrypted=='1' ?  trans('general.yes') : trans('general.no') }}</td>
               <td>{{$field->pivot->required ? "REQUIRED" : "OPTIONAL"}}</td>
               <td>
+                @can('update', $custom_fieldset)
                 <a href="{{ route('fields.disassociate', [$field,$custom_fieldset->id]) }}" class="btn btn-sm btn-danger">Remove</a>
+                @endcan
               </td>
             </tr>
             @endforeach
@@ -60,6 +68,7 @@
           <tfoot>
             <tr>
               <td colspan="5" class="text-right">
+                @can('update', $custom_fieldset)
                 {{ Form::open(['route' =>
                 ["fieldsets.associate",$custom_fieldset->id],
                 'class'=>'form-horizontal',
@@ -70,6 +79,7 @@
                 {{ Form::select("field_id",$custom_fields_list,"",["onchange" => "$('#ordering').submit()"]) }}
                 <span class="alert-msg"><?= $errors->first('field_id'); ?></span>
                 {{ Form::close() }}
+                @endcan
               </td>
             </tr>
           </tfoot>
@@ -82,6 +92,8 @@
 @stop
 
 @section('moar_scripts')
+  @can('update', $custom_fieldset)
+
   <script nonce="{{ csrf_token() }}">
   var fixHelperModified = function(e, tr) {
       var $originals = tr.children();
@@ -119,4 +131,5 @@
       stop: updateIndex
   }).disableSelection();
 </script>
+  @endcan
 @stop

resources/views/custom_fields/index.blade.php

@@ -8,6 +8,7 @@
 
 @section('content')
 
+@can('view', \App\Models\CustomFieldset::class)
 <div class="row">
   <div class="col-md-9">
     <div class="box box-default">
@@ -15,7 +16,9 @@
       <div class="box-header with-border">
         <h3 class="box-title">{{ trans('admin/custom_fields/general.fieldsets') }}</h3>
         <div class="box-tools pull-right">
+          @can('create', \App\Models\CustomFieldset::class)
           <a href="{{ route('fieldsets.create') }}" class="btn btn-sm btn-primary" data-toggle="tooltip" title="Create a new fieldset">{{ trans('admin/custom_fields/general.create_fieldset') }}</a>
+          @endcan
         </div>
       </div><!-- /.box-header -->
 
@@ -62,6 +65,7 @@
                 @endforeach
               </td>
               <td>
+                @can('delete', $fieldset)
                 {{ Form::open(['route' => array('fieldsets.destroy', $fieldset->id), 'method' => 'delete']) }}
                   @if($fieldset->models->count() > 0)
                   <button type="submit" class="btn btn-danger btn-sm disabled" disabled><i class="fa fa-trash"></i></button>
@@ -69,6 +73,7 @@
                   <button type="submit" class="btn btn-danger btn-sm"><i class="fa fa-trash"></i></button>
                   @endif
                 {{ Form::close() }}
+                @endcan
               </td>
             </tr>
             @endforeach
@@ -85,14 +90,17 @@
     <p>{{ trans('admin/custom_fields/general.about_fieldsets_text') }} </p>
   </div>
 </div> <!-- .row-->
-
+@endcan
+@can('view', \App\Models\CustomField::class)
 <div class="row">
   <div class="col-md-12">
     <div class="box box-default">
       <div class="box-header with-border">
         <h3 class="box-title">{{ trans('admin/custom_fields/general.custom_fields') }}</h3>
         <div class="box-tools pull-right">
+          @can('create', \App\Models\CustomField::class)
           <a href="{{ route('fields.create') }}" class="btn btn-sm btn-primary" data-toggle="tooltip" title="Create a new custom field">{{ trans('admin/custom_fields/general.create_field') }}</a>
+          @endcan
         </div>
 
       </div><!-- /.box-header -->
@@ -147,17 +155,19 @@
                 @endforeach
               </td>
               <td>
-                {{ Form::open(array('route' => array('fields.destroy', $field->id), 'method' => 'delete')) }}
                 <nobr>
+                  @can('update', $field)
                 <a href="{{ route('fields.edit', $field->id) }}" class="btn btn-warning btn-sm"><i class="fa fa-pencil"></i></a>
-
+                @endcan               
-
+                @can('delete', $field)
+                {{ Form::open(array('route' => array('fields.destroy', $field->id), 'method' => 'delete', 'style' => 'display:inline-block')) }}
                 @if($field->fieldset->count()>0)
                 <button type="submit" class="btn btn-danger btn-sm disabled" disabled><i class="fa fa-trash"></i></button>
                 @else
                 <button type="submit" class="btn btn-danger btn-sm"><i class="fa fa-trash"></i></button>
                 @endif
                 {{ Form::close() }}
+                @endcan
                 </nobr>
               </td>
             </tr>
@@ -169,6 +179,7 @@
     </div><!-- /.box -->
   </div> <!-- /.col-md-9-->
 </div>
+@endcan
 
 @stop
 @section('moar_scripts')

resources/views/layouts/default.blade.php

@@ -534,13 +534,13 @@
                     </a>
 
                     <ul class="treeview-menu">
-                        @can('view', \App\Models\CustomField::class)
+                        @if(Gate::allows('view', App\Models\CustomField::class) || Gate::allows('view', App\Models\CustomFieldset::class))
                             <li {!! (Request::is('fields*') ? ' class="active"' : '') !!}>
                                 <a href="{{ route('fields.index') }}">
                                     {{ trans('admin/custom_fields/general.custom_fields') }}
                                 </a>
                             </li>
-                        @endcan
+                        @endif
 
                         @can('view', \App\Models\Statuslabel::class)
                             <li {!! (Request::is('statuslabels*') ? ' class="active"' : '') !!}>