Clarify behavior of CookieAuthenticationOptions.Cookie.Expiration

[natemcmaster]

aspnet/Security#1285 added CookieAuthenticationOptions.Cookie.Expiration, a nullable TimeSpan, but we still have CookieAuthenticationOptions.ExpireTimeSpan. Cookie.Expiration is ignored. We should clarify what Cookie.Expiration is meant to do and how it interacts with ExpireTimeSpan

[Tratcher]

Where? In doc comments? Samples? Or make it throw?

[natemcmaster]

For others new to this thread, let me add a little context. The reason for the ExpireTimeSpan API is that it represents the expiration of the ticket, not the cookie that contains the ticket.

Example: we haven't figure out how to handle something like this.

Cookie.Expiration = TimeSpan.FromDays(9999);
ExpiresTimeSpan = TimeSpan.FromMinutes(30);

This would create an auth ticket valid for the next 30 minutes, but the cookie will be stored by the browser 9999 days. That means in a month, the browser will still send the cookie, but auth won't accept the ticket contained in the cookie. Until 2.0.0, this wasn't a problem because users couldn't configure cookie expiration manually. We always inferred cookie expiration from ticket expiration.

[natemcmaster]

@Tratcher I think it would be useful to assert that Cookie.Expiration >= ExpireTimeSpan. If not, throw. There is nothing wrong with Cookie.Expiration >= ExpireTimeSpan, it's just means you have a cookie that will never work and doesn't get cleaned up right away. But on the other hand, if someone configures Cookie.Expiration but not ExpireTimeSpan, they will get an error.

[Tratcher]

Cookie.Expiration is currently ignored. It never makes it to the browser. https://github.com/aspnet/Security/blob/a53bf093a7d86b35e019c80515c92d7626982325/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L181

Auth cookies have a separate flag for persisting cookies (via setting their expiration) and it's intentionally only available per request because it requires user consent.
https://github.com/aspnet/Security/blob/a53bf093a7d86b35e019c80515c92d7626982325/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L222-L225

Setting Cookie.Expiration is an error in the current design, we just don't validate it.

[HaoK]

So in 2.1, we should just update CookieAuthenticationHandler to throw if expiration is ever set before using cookies?

[Tratcher]

Yes, that's an option. I don't know if @Eilon will take kindly to throwing new exceptions though.

[natemcmaster]

The way I see it, we have 3 options:

1. Break. Throw a new exception or change the API surface. Remove Cookie.Expiration or throw when it's used.
2. Try to reason about the interaction ExpireTimeSpan and Cookie.Expiration. One implies the other? Take the shorter of the two? Idk. Some heuristic that seems reasonable. We couldn't agree on one in the initial implementation so we left it out.
3. Do nothing. Users will inevitably end up with cookies that don't expire because they used the wrong one.

[HaoK]

I like option 1, throwing an exception (pointing them at the right setting) instead of just ignoring it seems way better. It would be a really easy breaking change for people to fix, and its not like setting it actually did what they wanted anyways...

[scottsauber]

Random user here but my vote in order of preference is:

1. Throw an exception as early as possible like on Startup with the pointer to the right setting. Preferably not have it throw on issuing of a cookie which would be less discoverable post-upgrade.
2. Mark as Obsolete with docs to new setting.
3. Flat out removal. Don't love this bc no pointer to correct behavior then.

Sidenote - It'd be cool if MS or the community provided upgrade Codemods in the way of Roslyn analyzers to autofix your code on upgrades. The React team at Facebook does this for instance..

[Eilon]

I of course don't like the idea of throwing from the setter because of the breaking change aspect.

But, worse than that, property setters that throw on condition of another property's value are a Very Bad Idea™️ because you can only set the properties in a particular order, and that's a serious usability pattern.

So, that leaves throwing at "runtime" (either in some sort of property validation phase, or during a request), but that's still a breaking change, so I'm still not a fan.

I'm ok with marking something as obsolete if we think that's an appropriate choice, then we can delete in 3.0. Is that reasonable?

[Tratcher]

On a related note, that property is coming from a shared library (RequestPathBaseCookieBuilder), CookieAuth doesn't actually own it. We'd have to derive and override it to add an exception. I don't think an obsolete attribute would even surface to the user, we're not obsoleting it on the base type. And it can't be removed from the base type as it's valid for other uses.

Back to the drawing board.

https://github.com/aspnet/Security/blob/a53bf093a7d86b35e019c80515c92d7626982325/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs#L65
https://github.com/aspnet/Security/blob/a53bf093a7d86b35e019c80515c92d7626982325/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationOptions.cs#L16

[scottsauber]

There is a virtual Validate method off of the base class AuthenticationSchemeOptions that's not being overridden on CookieAuthenticationOptions

Not sure how many people would actually call that... but... it's there.

https://github.com/aspnet/Security/blob/b3e4bf382c9676e2d912636b7ee0255cad244e11/src/Microsoft.AspNetCore.Authentication/AuthenticationSchemeOptions.cs#L16

Edit: Still a breaking change though I guess.

[Eilon]

Indeed, more fundamentally, the question is: How bad is this really?

We can very easily document this in the /// comments for Intellisense and the docs site. I'm guessing a lot of people never set this stuff at all and just use the defaults (which are hopefully somewhat sensible). And hopefully those who set it will see the docs - at least if they see some odd behavior and need to get more info...

[HaoK]

You could also argue that as far as breaking changes go, its not too bad, since this is actually better behavior. Setting the property does nothing, and is basically always developer error Anyone who sets this really needed to set cookie.ExpiresTimeSpan instead, so the exception would let them know their cookie actually isn't expiring when they expect, instead of silently being ignored...