zfcampus / zf-apigility-skeleton

BSD 3-Clause "New" or "Revised" License
444 stars 123 forks source link

InputFilter not used for filtering `POST` data #75

Closed Wilt closed 5 years ago

Wilt commented 10 years ago

Hello,

I have a question or an issue, but I am not sure which of the two it is since I don't know if the behavior is wrong or expected. Let me explain.

When I register an inputFilter for a certain entity and patch an entity with data (a field) that is not registered in the inputFilter I get an ApiProblem response with status 400 and a message "Unrecognized field name".

Now in another case I post data for creating a new entity and I include the same invalid field in the data. When I look at the $data object in the create method in my ResourceListener the $data is apparently not filtered by the inputFilter, because unknown data is still present in the $data object. Normally I would expect the inputFilter to take care of unknown/unwanted by filtering it. Something like this:

$inputFilter->setData($unfilteredData);
if(!$inputFilter->isValid()){
    ...throw exception or return ApiProblem
}
$filteredData = $inputFilter->getValues();

Is the module (as the name would suggest) only for validation of data and that is why the registered inputFilters are not used for filtering POST data at all? Does it mean I still manually have to pull the data trough the filter in my ResourceListener. I also looked at the zfcampus/zf-content-validation src files but could not find anything related to actually filtering the data.

Thanks.

Wilt commented 10 years ago

A little update...

I found out that I can get my inputFilter in my ResourceListener because it is injected in the method injectEventInputFilterIntoResource in RestController: https://github.com/zfcampus/zf-rest/blob/master/src/RestController.php#L775

That means I can get my filtered data as follows: $filteredData = $inputFilter->getValues();

Meaning I should not use the $data parameter from the create method at all.

So now my question changed to the following: Would it not be better if the $data parameter of the create method would automatically be pulled through the configured InputFilter. I think this would be more logic and more safe (people might assume the $data object inside the methods of their listener are already filtered while actually it is not).

weierophinney commented 10 years ago

Regarding the originally stated question, this is actually by design, kind of.

For POST and PUT requests, we filter only the set of data in the defined input filter. You have access to the entire data set if you want, but via the input filter, you'll only be able to retrieve values for defined inputs.

PATCH requests are a bit different. Because PATCH is considered a partial update, we take the incoming fields, and use them to create what's called a "validation group" in the input filter. Due to the nature of the setValidationGroup method on the input filter, you can only specify valid input names; specifying any that are not part of the input raises an exception -- and in Apigility, we catch that and return a validation error. One proposed solution would be to test each key submitted against the input filter, and only create the validation group from the intersection of keys. If you would like to see that, please let me know.

I'll respond to the other question in another comment.

weierophinney commented 10 years ago

Would it not be better if the $data parameter of the create method would automatically be pulled through the configured InputFilter. I think this would be more logic and more safe (people might assume the $data object inside the methods of their listener are already filtered while actually it is not).

This is something we debated heavily during creation of the zf-content-validation module. The problem is one of consistency, and comes down to this: If you do not define any fields, what should $data be?

In order to keep the API consistent between the two use cases (no fields defined and 1 or more fields defined), we opted to have $data be the raw data always.

Wilt commented 10 years ago

Thanks for your explanation. That must have been a nice discussion :P As i understand I could always customize the listener and register my own in the module.config.php under 'ZF\ContentValidation\ContentValidationListener'.

Do you think it would be a good idea to create a configuration option in the zf-content-validation module? Something like.

    'filter_resource_listeners_data_for_all_methods' => true

Or would that kind of configuration become too complicated? I could try to make a pull request... Thanks again!

weierophinney commented 10 years ago

We're definitely considering passing the filtered data by default for 2.0; until then, a flag may be useful, but it would need to be per-service (so as not to break services that already exist). That can likely be done in the zf-content-validation module, if you want to take a crack at it, @Wilt .

improved-broccoli commented 9 years ago

@weierophinney Is this issue still existing ? If yes, I could suggest a PR to add the flag.

zbintiosul commented 9 years ago

Until now exist a global option like: 'filter_resource_listeners_data_for_all_methods' => true, to be by default filtered data, not raw data? To know.

And what is the logic to send raw data if you are validating data with filtered data (for POST)?

Wilt commented 8 years ago

When using pre and post event listeners and resolving the data from the param from the event it turns out the data array also holds the unfiltered data (raw post data).

I think it is a design mistakes inside the Apigility/zf-rest architecture to pass raw unfiltered data to the RestController classes, the ResourceListener and post and pre controller events. I don't think people expect raw post values in this stage of their application process (after dispatch). I believe mistakes are bound to happen. I know that filtered values can be resolved indirectly from the event like this:

    $restController = $event->getTarget();
    $resource = $restController->getResource();
    $inputFilter = $resource->getInputFilter();
    $data = $inputFilter->getValues();

But I would suggest passing filtered data to the controller class methods and the pre and post events in the RestController. It would surely be much more intuitive.

Wilt commented 5 years ago

For those ending up here there are two flags that can be used for setting this up according to the documentation here: https://apigility.org/documentation/modules/zf-content-validation#user-configuration: