Fixed #17620 - DELETE method on custom fields causing method not allowed error

app/Exceptions/Handler.php

@@ -143,8 +143,8 @@ public function render($request, Throwable $e)
                     ->withInput();
             }
 
-        // This gets the MVC model name from the exception and formats in a way that's less fugly
-            $model_name = strtolower(implode(" ", preg_split('/(?=[A-Z])/', last(explode('\\', $e->getModel())))));
+            // This gets the MVC model name from the exception and formats in a way that's less fugly
+            $model_name = trim(strtolower(implode(" ", preg_split('/(?=[A-Z])/', last(explode('\\', $e->getModel()))))));
             $route = str_plural(strtolower(last(explode('\\', $e->getModel())))).'.index';
 
             // Sigh.
@@ -160,9 +160,7 @@ public function render($request, Throwable $e)
                 $route = 'maintenances.index';
             } elseif ($route === 'licenseseats.index') {
                 $route = 'licenses.index';
-            } elseif ($route === 'customfields.index') {
-                $route = 'fields.index';
-            } elseif ($route === 'customfieldsets.index') {
+            } elseif (($route === 'customfieldsets.index') || ($route === 'customfields.index')) {
                 $route = 'fields.index';
             }
 

app/Http/Controllers/CustomFieldsController.php

@@ -144,10 +144,9 @@ public function store(CustomFieldRequest $request) : RedirectResponse
      */
     public function deleteFieldFromFieldset($field_id, $fieldset_id) : RedirectResponse
     {
+        $this->authorize('update', CustomField::class);
         $field = CustomField::find($field_id);
 
-        $this->authorize('update', $field);
-
         // Check that the field exists - this is mostly related to the demo, where we 
         // rewrite the data every x minutes, so it's possible someone might be disassociating 
         // a field from a fieldset just as we're wiping the database
@@ -157,11 +156,12 @@ public function deleteFieldFromFieldset($field_id, $fieldset_id) : RedirectRespo
             return redirect()->route('fieldsets.show', ['fieldset' => $fieldset_id])
                 ->with('success', trans('admin/custom_fields/message.field.delete.success'));
             } else {
-                return redirect()->back()->withErrors(['message' => "Field is in use and cannot be deleted."]);
+                return redirect()->back()->with('error', trans('admin/custom_fields/message.field.delete.error'))
+                    ->withInput();
             }
         }
 
-        return redirect()->back()->withErrors(['message' => "Error deleting field from fieldset"]);
+        return redirect()->back()->with('error', trans('admin/custom_fields/message.field.delete.error'));
 
 
     }
@@ -172,20 +172,16 @@ public function deleteFieldFromFieldset($field_id, $fieldset_id) : RedirectRespo
      * @author [Brady Wetherington] [<uberbrady@gmail.com>]
      * @since [v1.8]
      */
-    public function destroy($field_id) : RedirectResponse
+    public function destroy(CustomField $field) : RedirectResponse
     {
-        if ($field = CustomField::find($field_id)) {
-            $this->authorize('delete', $field);
+        $this->authorize('delete', CustomField::class);
 
-            if (($field->fieldset) && ($field->fieldset->count() > 0)) {
-                return redirect()->back()->withErrors(['message' => 'Field is in-use']);
-            }
-            $field->delete();
-            return redirect()->route("fields.index")
-                ->with("success", trans('admin/custom_fields/message.field.delete.success'));
+        if (($field->fieldset) && ($field->fieldset->count() > 0)) {
+            return redirect()->back()->with('error', trans('admin/custom_fields/message.field.delete.in_use'));
         }
-
-        return redirect()->back()->withErrors(['message' => 'Field does not exist']);
+        $field->delete();
+        return redirect()->route("fields.index")
+            ->with("success", trans('admin/custom_fields/message.field.delete.success'));
     }
 
 
@@ -198,7 +194,7 @@ public function destroy($field_id) : RedirectResponse
      */
     public function edit(Request $request, CustomField $field) : View | RedirectResponse
     {
-        $this->authorize('update', $field);
+        $this->authorize('update', CustomField::class);
         $fieldsets = CustomFieldset::get();
         $customFormat = '';
         if ((stripos($field->format, 'regex') === 0) && ($field->format !== CustomField::PREDEFINED_FORMATS['MAC'])) {
@@ -228,7 +224,7 @@ public function edit(Request $request, CustomField $field) : View | RedirectResp
      */
     public function update(CustomFieldRequest $request, CustomField $field) : RedirectResponse
     {
-        $this->authorize('update', $field);
+        $this->authorize('update', CustomField::class);
         $show_in_email = $request->get("show_in_email", 0);
         $display_in_user_view = $request->get("display_in_user_view", 0);
 
@@ -265,7 +261,6 @@ public function update(CustomFieldRequest $request, CustomField $field) : Redire
 
         if ($field->save()) {
 
-
             // Sync fields with fieldsets
             $fieldset_array = $request->input('associate_fieldsets');
             if ($request->has('associate_fieldsets') && (is_array($fieldset_array))) {

app/Policies/SnipePermissionsPolicy.php

@@ -85,7 +85,7 @@ public function index(User $user)
     }
 
     /**
-     * Determine whether the user can view the accessory.
+     * Determine whether the user can view the model.
      *
      * @param  \App\Models\User  $user
      * @return mixed
@@ -101,7 +101,7 @@ public function files(User $user, $item = null)
     }
 
     /**
-     * Determine whether the user can create accessories.
+     * Determine whether the user can create model.
      *
      * @param  \App\Models\User  $user
      * @return mixed
@@ -112,7 +112,7 @@ public function create(User $user)
     }
 
     /**
-     * Determine whether the user can update the accessory.
+     * Determine whether the user can update the model.
      *
      * @param  \App\Models\User  $user
      * @return mixed
@@ -124,7 +124,7 @@ public function update(User $user, $item = null)
 
 
     /**
-     * Determine whether the user can update the accessory.
+     * Determine whether the user can update the model.
      *
      * @param  \App\Models\User  $user
      * @return mixed
@@ -135,7 +135,7 @@ public function checkout(User $user, $item = null)
     }
 
     /**
-     * Determine whether the user can delete the accessory.
+     * Determine whether the user can delete the model.
      *
      * @param  \App\Models\User  $user
      * @return mixed
@@ -151,7 +151,7 @@ public function delete(User $user, $item = null)
     }
 
     /**
-     * Determine whether the user can manage the accessory.
+     * Determine whether the user can manage the model.
      *
      * @param  \App\Models\User  $user
      * @return mixed

database/factories/UserFactory.php

@@ -351,6 +351,17 @@ public function canImport()
         return $this->appendPermission(['import' => '1']);
     }
 
+    public function createCustomFields()
+    {
+        return $this->appendPermission(['customfields.create' => '1']);
+    }
+
+    public function viewCustomFields()
+    {
+        return $this->appendPermission(['customfields.view' => '1']);
+    }
+
+
     public function deleteCustomFields()
     {
         return $this->appendPermission(['customfields.delete' => '1']);

resources/views/custom_fields/index.blade.php

@@ -84,15 +84,12 @@ class="table table-striped snipe-table"
                 @endcan
 
                 @can('delete', $fieldset)
-                <form method="POST" action="{{ route('fieldsets.destroy', $fieldset->id) }}" accept-charset="UTF-8" style="display:inline-block">
-                  {{ method_field('DELETE') }}
-                  @csrf
+
                   @if($fieldset->models->count() > 0)
                   <button type="submit" class="btn btn-danger btn-sm disabled" data-tooltip="true" title="{{ trans('general.cannot_be_deleted') }}" disabled><i class="fas fa-trash"></i></button>
                   @else
-                  <button type="submit" class="btn btn-danger btn-sm delete-asset" data-tooltip="true" title="{{ trans('general.delete') }}" data-toggle="modal" data-title="{{ trans('general.delete') }}" data-content="{{ trans('general.sure_to_delete_var', ['item' => $fieldset->name]) }}" data-icon="fa fa-trash" data-target="#dataConfirmModal" onClick="return false;"><i class="fas fa-trash"></i></button>
+                  <a type="submit" href="{{ route('fieldsets.destroy', $fieldset) }}" class="btn btn-danger btn-sm delete-asset" data-tooltip="true" title="{{ trans('general.delete') }}" data-toggle="modal" data-title="{{ trans('general.delete') }}" data-content="{{ trans('general.sure_to_delete_var', ['item' => $fieldset->name]) }}" data-icon="fa fa-trash" data-target="#dataConfirmModal" onClick="return false;"><i class="fas fa-trash"></i></a>
                   @endif
-                </form>
                 @endcan
                   </nobr>
               </td>
@@ -237,9 +234,6 @@ class="sr-only">{{ trans('admin/custom_fields/general.unique') }}</span></i></th
               </td>
               <td>
                 <nobr>
-                  <form method="POST" action="{{ route('fields.destroy', $field->id) }}" accept-charset="UTF-8" style="display:inline-block">
-                    {{ method_field('DELETE') }}
-                    @csrf
                   @can('update', $field)
                     <a href="{{ route('fields.edit', $field->id) }}" class="btn btn-warning btn-sm" data-tooltip="true" title="{{ trans('general.update') }}">
                       <i class="fas fa-pencil-alt" aria-hidden="true"></i>
@@ -249,19 +243,19 @@ class="sr-only">{{ trans('admin/custom_fields/general.unique') }}</span></i></th
 
                 @can('delete', $field)
 
-                  @if($field->fieldset->count()>0)
+                  @if ($field->fieldset->count() > 0)
                     <button type="submit" class="btn btn-danger btn-sm disabled" data-tooltip="true" title="{{ trans('general.cannot_be_deleted') }}" disabled>
                       <i class="fas fa-trash" aria-hidden="true"></i>
-                      <span class="sr-only">{{ trans('button.delete') }}</span></button>
+                      <span class="sr-only">{{ trans('button.delete') }}</span>
+                    </button>
                   @else
-                    <button type="submit" class="btn btn-danger btn-sm delete-asset" data-tooltip="true" title="{{ trans('general.delete') }}" data-toggle="modal" data-title="{{ trans('general.delete') }}" data-content="{{ trans('general.sure_to_delete_var', ['item' => $field->name]) }}" data-target="#dataConfirmModal" data-icon="fa fa-trash" onClick="return false;">
+                    <a href="{{ route('fields.destroy', $field) }}" class="btn btn-danger btn-sm delete-asset" data-tooltip="true" title="{{ trans('general.delete') }}" data-toggle="modal" data-title="{{ trans('general.delete') }}" data-content="{{ trans('general.sure_to_delete_var', ['item' => $field->name]) }}" data-target="#dataConfirmModal" data-icon="fa fa-trash" onClick="return false;">
                       <i class="fas fa-trash" aria-hidden="true"></i>
                       <span class="sr-only">{{ trans('button.delete') }}</span>
-                    </button>
+                    </a>
                   @endif
 
                 @endcan
-                  </form>
                 </nobr>
               </td>
             </tr>

tests/Feature/CustomFields/Ui/DeleteCustomFieldsTest.php

@@ -0,0 +1,69 @@
+<?php
+
+namespace Tests\Feature\CustomFields\Ui;
+
+use App\Models\CustomField;
+use App\Models\CustomFieldset;
+use App\Models\User;
+use Tests\TestCase;
+
+class DeleteCustomFieldsTest extends TestCase
+{
+    public function testPermissionNeededToDeleteField()
+    {
+        $this->actingAs(User::factory()->create())
+            ->delete(route('fields.destroy', CustomField::factory()->create()))
+            ->assertForbidden();
+    }
+
+
+    public function testCanDeleteCustomField()
+    {
+        $field = CustomField::factory()->create();
+        $this->assertDatabaseHas('custom_fields', ['id' => $field->id]);
+
+        $this->actingAs(User::factory()->deleteCustomFields()->create())
+            ->delete(route('fields.destroy', $field))
+            ->assertRedirectToRoute('fields.index')
+            ->assertStatus(302)
+            ->assertSessionHas('success');
+
+        $this->assertDatabaseMissing('custom_fields', ['id' => $field->id]);
+    }
+
+    public function testCannotDeleteCustomFieldThatDoesNotExist()
+    {
+
+        $response = $this->actingAs(User::factory()->viewCustomFields()->deleteCustomFields()->create())
+            ->delete(route('fields.destroy', '49857589'))
+            ->assertRedirect(route('fields.index'))
+            ->assertSessionHas('error');
+
+        $temp = $this->followRedirects($response);
+        $temp->assertSee(trans('general.error'))->assertSee(trans('general.generic_model_not_found', ['model' => 'custom field']));
+
+    }
+
+    public function testCannotDeleteFieldThatIsAssociatedWithFieldsets()
+    {
+        $field = CustomField::factory()->create();
+        $fieldset = CustomFieldset::factory()->create();
+
+        $this->actingAs(User::factory()->superuser()->create())
+            ->post(route('fieldsets.associate', $fieldset), [
+                'field_id' => $field->id,
+            ]);
+
+        $response = $this->actingAs(User::factory()->viewCustomFields()->deleteCustomFields()->create())
+            ->from(route('fields.index'))
+            ->delete(route('fields.destroy', $field))
+            ->assertStatus(302)
+            ->assertRedirect(route('fields.index'))
+            ->assertSessionHas('error');
+
+        $this->followRedirects($response)->assertSee(trans('general.error'))->assertSee(trans('admin/custom_fields/message.field.delete.in_use'));
+
+        // Ensure the field is still in the database
+        $this->assertDatabaseHas('custom_fields', ['id' => $field->id]);
+    }
+}