Adds MaxAge Property to CookieOptions and CookieBuilder #904
Conversation
| using Microsoft.AspNetCore.Http.Internal; | ||
| using Microsoft.Extensions.ObjectPool; | ||
| using Microsoft.Net.Http.Headers; |
There was a problem hiding this comment.
No updated ResponseCookies test?
|
@Tratcher Should Max-Age be a header name in HeaderNames.cs? |
|
No, it's not a header name, it's a header property name. |
|
@Tratcher Do you recommend doing a full E2E to verify max-age shows up on requests? |
|
on requests? None of the cookie settings are echoed on the request, on the the name and value. It's worth doing some manual testing though to make sure major browsers and HttpClient honor this setting. |
jkotalik
force-pushed
the
jkotalik/MaxAge
branch
from
40d2a58 to
f57f1d3
Compare
|
ping |
jkotalik
changed the title
|
@Tratcher I verified locally and the max-age header is sent in the response. |
?
|
Did you verify that various browsers and clients honor it? Or at least don't choke on it? HttpClient, IE, Chrome, Firefox... I just want to make sure this new property is useful to somebody. |
| @@ -39,6 +39,16 @@ public void ComputesExpiration() | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void ComputesMaxAge() | |||
| { | |||
| Assert.Null(new CookieBuilder().Build(new DefaultHttpContext()).Expires); | |||
There was a problem hiding this comment.
Why check Expires in a MaxAge test?
jkotalik
force-pushed
the
jkotalik/MaxAge
branch
2 times, most recently
from
bdc5c56 to
1521ee0
Compare
|
ping :) |
|
|
||
| var cookieHeaderValues = headers[HeaderNames.SetCookie]; | ||
| Assert.Equal(1, cookieHeaderValues.Count); | ||
| Assert.Contains("max-age=", cookieHeaderValues[0]); |
There was a problem hiding this comment.
You're setting one hour, you should be able to check that value here.
jkotalik
force-pushed
the
jkotalik/MaxAge
branch
from
1521ee0 to
7d1bbfb
Compare
#688
@Tratcher Not sure if there is more to do here than this.