Add CookieBuilder to CookieAuthenticationOptions and obsolete the duplicated properties #1285
Conversation
|
@natemcmaster, |
|
|
||
| if (Options.CookieSecure == CookieSecurePolicy.SameAsRequest) | ||
| if (Options.Cookie.Path == null && OriginalPathBase.HasValue) |
There was a problem hiding this comment.
Same issue here as being discussed https://github.com/aspnet/Security/pull/1284/files/6b218ab9f3a1439a5c038f49332ef16cbaedc712#r124940460
There was a problem hiding this comment.
Is this doing the same thing? I'd still expect, if a cookie does have a path but I say SameAsRequest it'd match the Secure flag to the request protocol.
There was a problem hiding this comment.
The diff github is showing is a little misleading. The behavior your asking about moved into the CookieBuilder:
|
@blowdart for a usability check |
| Path = Options.CookiePath ?? (OriginalPathBase.HasValue ? OriginalPathBase.ToString() : "/"), | ||
| }; | ||
| var cookieOptions = Options.Cookie.Build(Context); | ||
| // ignore the 'Expires' value as this will be computed elsewhere |
There was a problem hiding this comment.
Didn't we add Expiration support to Build for Security, or is it being used elsewhere?
There was a problem hiding this comment.
The behavior here is funky, and FWIW one of the reasons I was suggesting we leave Expiration off CookeBuilder. This handler takes into consideration CookieSignInContext.Properties.ExpiresUtc and CookieSignInContext.Properties.IsPersistent to calculate CookieOptions.Expires. So, the value of .Expires set by the builder needs to be unset to avoid changing behavior.
It would be good to find ways to clean this up without breaking existing behavior.
There was a problem hiding this comment.
This is fine, I just wanted to make sure we are using the expires overload somewhere, and it looks like we are with the nonce/correlation cookies so its cool. Otherwise I'd have suggested removing the unused overload.
natemcmaster
force-pushed
the
namc/cookie-options
branch
from
4b0ee5d to
c36c94d
Compare
|
Updated. Rebased on #1284 so I could re-use RequestPathBaseCookieBuilder |
|
🔔 updated to remove CookieAuthenticationOptions.CookieSameSite completely, which was never shipped as an RTM api |
natemcmaster
force-pushed
the
namc/replace-cookie-builder
branch
from
a565a24 to
968237d
Compare
…licated properties
natemcmaster
force-pushed
the
namc/cookie-options
branch
from
375f3c8 to
a7bf561
Compare
| @@ -283,14 +270,14 @@ private CookieOptions BuildCookieOptions() | |||
|
|
|||
| if (!signInContext.Properties.ExpiresUtc.HasValue) | |||
| { | |||
| signInContext.Properties.ExpiresUtc = issuedUtc.Add(Options.ExpireTimeSpan); | |||
| signInContext.Properties.ExpiresUtc = issuedUtc.Add(Options.Cookie.Expiration ?? default(TimeSpan)); | |||
There was a problem hiding this comment.
This fallback to default(TimeSpan) doesn't make sense, you end up issuing an already expired cookie.
- Revert the obsoleting of CookieAuthenticationOptions.ExpireTimeSpan in #1285 - Add test to ensure Cookie.Expiration is ignored
Addresses aspnet/HttpAbstractions#853.
Uses the new CookieBuilder API added here: aspnet/HttpAbstractions#882