Support binding to form and form file parameters in minimal actions

[martincostello]

PR Title

Support binding to the form and files in the form from parameters in minimal actions.

PR Description

Add basic support for binding to parameters of types IFormCollection, IFormFileCollection, and IFormFile on the HttpRequest in minimal actions.

I've just done this as a draft for now as while the new and existing RequestDelegateFactory tests pass, I'm sure there's better ways to deal with some of the reflection/expression binding stuff I've put in.

TODO

• A public IFromFileMetadata implementation, such as a metadata class/attribute.
• API review for interface and the above missing implementation of it. Add metadata for IFormFile usage with minimal actions #35175
• More test cases probably for more permutations of actions using these parameters.
• Support for binding non-file form parameters ([FromForm("foo")] string value)? Removed due to Support binding to IFormFile in minimal routing APIs #34303 (comment)
• API explorer support for OpenAPI.
• Also support IEnumerable<IFormFile> for parity with MVC? (See 49c653e)

Relates to #34303 and #35100.

/cc @davidfowl @jchannon

[davidfowl]

Needs support in the EndpointMetadataApiDescriptionProvider so it shows up property in API explorer. It also needs the right request content type so it can be swagger shows the file upload UI.

[pranavkm]

Allowing form parameters without antiforgery leaves this open to CSRF out of the box. Has that been thought thru?

[martincostello]

It was mentioned by @davidfowl here #34303 (comment) (I assumed forks was a typo for forms), but I haven't addressed there here other than removing the IFormCollection support that was in the initial commit.

Off the top of my head, if that needs adding in here would that "just" be the case of updating the method that is reading the form to do something similar to what the ValidateAntiforgeryTokenAuthorizationFilter class does?

[davidfowl]

This change looks really good @martincostello!

[davidfowl]

I would add a computed property on the factoryContext HasBody => JsonRequestBodyType is not null || ReadForm and use that here and in the JSON logic

[martincostello]

The problem with doing exactly that is it wouldn't allow you to have more than one IFormFile parameter as trying to bind the second one would find the first and then throw.

[martincostello]

Off the top of my head, if that needs adding in here would that "just" be the case of updating the method that is reading the form to do something similar to what the ValidateAntiforgeryTokenAuthorizationFilter class does?

I had a quick look at this just now with regards to CSRF and discovered for myself that IAntiforgery is defined in Microsoft.AspNetCore.Antiforgery, which then depends on Microsoft.AspNetCore.Http.Extensions, so it's definitely not a case of "just" using that 😄

I'll defer to the experts as to what the implications this feature would have on that and what (if anything) can/should be done.

[martincostello]

Going to mark this ready for review as I can't make any further progress on this without input from the team about CSRF and the new metadata API suggestion. I also have to keep rebasing it 😄

I guess possible outcomes include:

1. Can't implement as-is due to CSRF concerns
2. Gets punted back to 7.0 and CSRF and metadata dealt with when the team has time to come back to it.

[davidfowl]

@martincostello this is something we're willing to take for .NET 7 and with the latest changes, it'll be possible to simplify this change.

[martincostello]

@davidfowl - Ah cool - I stopped rebasing as I saw the team was still putting lots of fit and finish on things so was stepping back waiting for the dust to settle. If/when the majority of the changes you have planned are in for 6.0, let me know and I'll rebase this 👍

[martincostello]

I've applied the feedback from #35175 (comment), there's just a few TODOs regarding what binding sources to use for which types I'm unsure of which is best.

[halter73]

Alternatively I could remove this "default" behaviour for the body by removing the skip for BindingSource.Body and having it dealt with explicitly in CreateApiParameterDescription() instead.

Let's do this for consistency. The main thing that we want to have keep working is if there is no BindingSource.Body or BindingSource.FormFile parameter by "default" and someone added IAcceptsMetadata explicitly with .Accepts<TRequest>(...) or [Consumes(...)] indicating they're manually reading the body, we should continue to create this "fake" ApiParameterDescription from IAcceptsMetadata .

If we know we're consuming the body on behalf of the app, we can just assume that the IAcceptsMetadata is what we added by default. If someone tries to add custom IAcceptsMetadata despite that, it's certainly wrong, so no big loss.

Also nit:

Suggested change

	var alreadyHasAnyBodyParameter = apiDescription.ParameterDescriptions.Any(x =>
	x.Source == BindingSource.Body ||
	x.Source == BindingSource.Form ||
	x.Source == BindingSource.FormFile);
	var alreadyHasAnyBodyParameter = apiDescription.ParameterDescriptions.Any(x =>
	x.Source == BindingSource.Body ||
	x.Source == BindingSource.FormFile);

I don't see the point in looking for BindingSources we'll never create at this point.

[martincostello]

Fixed nit.

Still thinking about the best way to tackle this, as I've tried a few different ways to remove the default/skip body parameter but they all seemed to break something in the tests.

[halter73]

What tests break? I'm okay with changing some of the tests if they're asserting something nonsensical like custom IAcceptsMetadata being used to generate an ApiParameterDescription when we're consuming an application/json request body on behalf of the app. After this change, custom IAcceptsMetadata won't be used to generate an ApiParameterDescription when we're consuming a multipart/form-data on behalf of the app, so at least this is consistent.

[martincostello]

There were some tests checking there was no body parameter at all, where there was now a default. There were others that would fail because there was no implicit accept metadata where it was expected because the unit tests don't run through the real RequestDelegateFactory code because it's all mocked routing.

For the latter I was torn between changing it to do so to make it more realistic vs. the fact the two implementation details were becoming aware of each other.

I'll come back to it with fresh eyes tomorrow and maybe it'll all click how best to refactor it.

[halter73]

Thanks @martincostello!! 🎉

[martincostello]

Thanks @halter73 - if you let me know when the agreed CSRF support lands in main, I'd be happy to do a follow-up PR to take out the guards for the authorized requests added in 45b30d2.

[ghost]

Hi @martincostello. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.