Updating HttpMethodMetadata based on ICorsMetadata #43077
Conversation
ghost
added
the
| @@ -19,52 +19,6 @@ public class CorsApplicationModelProviderTest | |||
| { | |||
| private readonly IOptions<MvcOptions> OptionsWithoutEndpointRouting = Options.Create(new MvcOptions { EnableEndpointRouting = false }); | |||
|
|
|||
| [Fact] | |||
| public void OnProvidersExecuting_SetsEndpointMetadata_IfCorsAttributeIsPresentOnController() | |||
There was a problem hiding this comment.
I assume we have some end-to-end tests covering this scenario. Do you know where they are?
There was a problem hiding this comment.
| { | ||
| // Since we found a CORS metadata we will update it | ||
| // to make sure the acceptCorsPreflight is set to true. | ||
| httpMethodMetadata.AcceptCorsPreflight = true; |
There was a problem hiding this comment.
Metadata is supposed to be immutable. Rather than modifying the HTTP method metadata, have you considered checking for ICorsMetadata inside the HTTP method policy? If it's present, then go ahead with accepting preflight.
There was a problem hiding this comment.
Unrelated to this PR but while we are on subject of immutable metadata. IAuthorizeData has public setters for its properties. Wondering if it should be deprecated and removed though it would be a breaking change.
There was a problem hiding this comment.
Metadata doesn't have to be immutable, and it's not worth breaking changes. It's a pattern to try to encourage.
There was a problem hiding this comment.
Metadata is supposed to be immutable. Rather than modifying the HTTP method metadata, have you considered checking for ICorsMetadata inside the HTTP method policy? If it's present, then go ahead with accepting preflight.
@JamesNK that was my first idea but after talking with @halter73 the suggestion to mutate the metadata sounded reasonable
There was a problem hiding this comment.
@halter73 I can easily make the the changes in Http method policy without need a public api, do you have any concerns?
There was a problem hiding this comment.
The only behavior that I don't like about not mutating the metadata, change only the policy, is that the AcceptsCorsPreFlight will never be true, so, there is no reason for it to exist.
There was a problem hiding this comment.
@JamesNK Interesting idea. I also don't love mutating the metadata. We had a conversation earlier about how we could avoid this by replacing the metadata instead (#43077 (comment)), but that has other issues.
Having the CORS middleware HttpMethodMatcherPolicy handle this itself is another approach that I like. My problem with it is that:
The only behavior that I don't like about not mutating the metadata, change only the policy, is that the AcceptsCorsPreFlight will never be true, so, there is no reason for it to exist.
If we go this route, we need to obsolete AcceptCorsPreflight and tell people to add the appropriate attribute the the endpoints metadata instead. I don't have much of a preference. As you say, we do have other mutable metadata, so we're not breaking entirely new ground here.
I've already approved this PR and I still think it's okay as is, but if people want to obsolete AcceptCorsPreflight instead, I'm okay with that too. That will need to be a new API proposal, but we could probably get that approved over email this afternoon if we think it's worth it.
There was a problem hiding this comment.
I don't have enough time before code complete to think and test this through to offer a strong opinion 😢
There was a problem hiding this comment.
Ok, if not objections I am planning to merge the PR as is. It is broken since 3.x, so I feel like it is a must to have change.
Co-authored-by: Stephen Halter <halter73@gmail.com>
|
Can this change be merged into .net 6 for a subsequent release? I'm interested in the fix for issue "Cors not working in Minimal API endpoints" #42994. I'm not keen to upgrade to .net 7 yet. Thanks |
|
Hi @prkemp. 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. |
|
There is a workaround for .net 6, add |
|
Great, thanks for this. |
|
Hi @prkemp. 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. |
Fixes #42994