jaredhendrickson13 / pfsense-api

The missing REST and GraphQL API package for pfSense
https://pfrest.org/
Apache License 2.0
689 stars 106 forks source link

Control parameter 'remove' does not remove last entry in an alias, there always needs to be at least 1 entry #569

Closed jpdsc closed 1 month ago

jpdsc commented 1 month ago

Describe the bug When the alias has only 1 entry and a API call is done to remove the last entry, it doesn't. There needs to be at least one entry left for an entry to be removed.

To Reproduce

  1. Create an alias
  2. Add an IP and name
  3. PATCH /api/v2/firewall/alias with control parameter 'remove' and add the details as added in the entry in the alias
  4. Apply the changes or POST to /api/v2/firewall/apply
  5. The entry is still in the alias
  6. Now try and add a placeholder entry such as 192.168.9.1 and run steps 1-5. The entry added is now succesfully remove and the placeholder stays there.

Expected behavior It should be able to delete all the entry in an alias and keep the alias empty. This is useful for Fail2ban when banned IP's have a bantime.

pfSense Version & Package Version:

Affected Endpoints: /api/v2/firewall/alias

jaredhendrickson13 commented 1 month ago

This is primarily a safety precaution. Like the append control parameter, if the array provided when append or remove is exactly the same as its stored values, nothing happens. This prevents unintentional duplication or deletions when using the controls. It works this because these control parameters take effect well after the object has already been loaded from the config, and does not have context into the client's initial request at that point. In other words, it doesn't know if it got those array values from the client intentionally or from the config as the default. If you're looking to remove all items in an array, a normal PATCH request setting the field's array to [] is the way to do that. Alternatively, you could add an extra item to your array with the remove control enabled to force them to be removed by value.

There are a couple things I can change to make this more apparent:

jpdsc commented 1 month ago

This is primarily a safety precaution. Like the append control parameter, if the array provided when append or remove is exactly the same as its stored values, nothing happens. This prevents unintentional duplication or deletions when using the controls. It works this because these control parameters take effect well after the object has already been loaded from the config, and does not have context into the client's initial request at that point. In other words, it doesn't know if it got those array values from the client intentionally or from the config as the default. If you're looking to remove all items in an array, a normal PATCH request setting the field's array to [] is the way to do that. Alternatively, you could add an extra item to your array with the remove control enabled to force them to be removed by value.

There are a couple things I can change to make this more apparent:

  • There should be a note in the documentation describing this behavior for remove, but it looks like I only added it to the append control parameter documentation. I'll make sure that's added.
  • I can have it throw an error when append or remove are enabled and the array values are exactly the same as the stored values, instead of silently skipping the control.

Thanks for the quick reply! This is all clear now and the reason is very understandable. I'll keep using the placeholder IP so I can always remove any entry made by Fail2ban.