Add new CookiePolicy middleware #452
Conversation
|
|
||
| public Task Invoke(HttpContext context) | ||
| { | ||
| // REVIEW: Do we need to check if there is a Cookie feature already present like SendFile?? |
There was a problem hiding this comment.
This piece needs review, is it safe to blindly set the feature?
There was a problem hiding this comment.
Consider wrapping the existing feature (if any) rather than just wrapping context.Response.Cookies. If none, default to a new ResponseCookiesFeature.
|
Naive question: why submitting this thing here and not in aspnet/HttpAbstractions or aspnet/BasicMiddleware? |
|
@PinpointTownes HttpAbstractions can't have any real middleware due to repo layering. Hosting depends on HttpAbstractions, so HttpAbstractions can't have any web app samples or TestHost tests. BasicMiddleware was considered, it was a toss up between it and Security. I think Security only won because this feature may help make your site more secure. |
|
|
||
| if (Policy.OnAppendCookie != null) | ||
| { | ||
| var context = new AppendCookieContext(Context, options: null, name: key, value: value); |
There was a problem hiding this comment.
What if they want to add options in the callback?
There was a problem hiding this comment.
I think you'd always call the other overload if the callback is set.
|
Updated PR, wrapping Cookies feature now and fixed other feedback |
| public CookiesWrapper(HttpContext context, CookiePolicyOptions options) | ||
| { | ||
| Context = context; | ||
| Feature = context.Features.Get<IResponseCookiesFeature>() ?? new ResponseCookiesFeature(context.Features); |
There was a problem hiding this comment.
It would be clearer that this was nesting if the Get was in Invoke and passed through.
|
|
|
Updated with the test refactoring, I'm thinking this will be how the security tests end up looking too, as its similar already |
|
Nice, |
Fixes aspnet/HttpAbstractions#42
cc @Tratcher @divega @lodejard @davidfowl @blowdart
Note: one quirk of the new instance based options Use methods .
Configure<CookiePolicyOptions>in ConfigureServices will ALWAYS be ignored silently.