Replace the ConfigureCookieOptions action property with the CookieBuilder #148
Conversation
| /// </summary> | ||
| public Action<HttpContext, CookieOptions> ConfigureCookieOptions { get; set; } |
There was a problem hiding this comment.
Does cookie builder have something similar to this?
There was a problem hiding this comment.
No, but we made CookieBuilder.Build(HttpContext context) virtual and AntiforgeryOptions.Cookie settable for edge cases where users need to do something with HttpContext that we don't expect.
There was a problem hiding this comment.
We added this for a reason, so we'll have to figure out an alternative.
There was a problem hiding this comment.
What was the reason? When we were designing CookieBuilder, we figured anything other than the already-available top-level properties is an uncommon usage of this API.
There was a problem hiding this comment.
I know we did this to enable per-request configuration of things like the path, but I can't find an actual customer request for this, so I think we can just forget it until someone asks. Like you said, build is virtual, so we could use our subclass to resurrect this if necessary
| public override string Name | ||
| { | ||
| get => base.Name; | ||
| set => base.Name = value ?? throw new ArgumentNullException(nameof(value)); |
There was a problem hiding this comment.
1.x behavior only checks for null and only throws ArgumentNullException.
| if (_options.CookiePath != null) | ||
| var options = _options.Cookie.Build(httpContext); | ||
|
|
||
| options.Secure = options.Secure || _options.RequireSsl; |
There was a problem hiding this comment.
Why not depend on CookieSecurePolicy?
There was a problem hiding this comment.
This has historically been here along with RequireSsl. I'm not really sure if the reasons are important or not.
There was a problem hiding this comment.
I looked through the code, and didn't find any significant reason for these to be separate options. I've pushed an update that obsoletes RequireSsl and forwards it to Cookie.SecurePolicy.
| /// <para> | ||
| /// <see cref="CookieBuilder.SameSite"/> defaults to <see cref="SameSiteMode.Strict"/>. | ||
| /// <see cref="CookieBuilder.HttpOnly"/> defaults to <c>true</c>. | ||
| /// <see cref="CookieBuilder.SecurePolicy"/> will be forced to <seealso cref="CookieSecurePolicy.Always"/> when <see cref="RequireSsl"/> is <c>true</c>. |
There was a problem hiding this comment.
Map RequireSsl to Cookie.SecurePolicy?
|
|
from me
|
RequireSsl does more than just configure the cookie policy |
| @@ -265,12 +265,11 @@ private void SaveCookieTokenAndHeader(HttpContext httpContext, string cookieToke | |||
|
|
|||
| private void CheckSSLConfig(HttpContext context) | |||
| { | |||
| if (_options.RequireSsl && !context.Request.IsHttps) | |||
| if (_options.Cookie.SecurePolicy == CookieSecurePolicy.Always && !context.Request.IsHttps) | |||
natemcmaster
force-pushed
the
namc/cookie-builder
branch
from
e4489be to
df41fd8
Compare
Addresses aspnet/HttpAbstractions#853.
Uses the new CookieBuilder API added here: aspnet/HttpAbstractions#882
cc @JunTaoLuo @rynowak @javiercn