Use SystemWebCookieManager by default

[Tratcher]

This mitigates https://github.com/aspnet/AspNetKatana/wiki/System.Web-response-cookie-integration-issues for most users by defaulting to the SystemWeb cookies collection when available.

@kevinchalet thoughts?

[kevinchalet]

Wooooot, I was really tempted to open a ticket to suggest doing that when I posted dotnet/aspnetcore#45278 (comment) 2 weeks ago 😅

The idea is great, but the layering - having Microsoft.Owin depend on System.Web - is sadly far from ideal.

I'd suggest opting for a different approach, designed after what already exists for IDataProtector/IDataProtectionProvider:

• The host would inject one or two CookieManagerDelegate properties into IAppBuilder.Properties (one for the regular cookie manager, one for the chunking manager).
• The security middleware (cookies, OAuth 2.0, etc.) would resolve these properties and use the corresponding managers if their CookieManager property was left to null.

It's a bit more code, but it solves the layering problem very nicely.

[kevinchalet]

Oh, and since you're making my wildest dreams come true, what about making ThrowForPartialCookies false by default? 🤣

[Tratcher]

having Microsoft.Owin depend on System.Web - is sadly far from ideal.

Yeah, that's what put me off this approach for so long. Ultimately though, this is full framework so that dependency doesn't really mean much, it's all there on disk regardless. Worst case an extra assembly gets loaded at runtime for self-host apps. I'm less concerned about that since Katana is disproportionately used on System.Web.

Oh, and since you're making my wildest dreams come true, what about making ThrowForPartialCookies false by default? 🤣

Sure :-)

[kevinchalet]

Worst case an extra assembly gets loaded at runtime for self-host apps. I'm less concerned about that since Katana is disproportionately used on System.Web.

It's likely not unreasonable, but System.Web is still a heavy dependency (2/3MB IIRC?) that self-hosters would want to avoid, so if there's a way to avoid that, it's worth giving it a try.

The IDataProtector approach isn't too complicated and doesn't have this con. Would you like me to give it a try? 😄

[Tratcher]

The IDataProtector approach isn't too complicated and doesn't have this con. Would you like me to give it a try? 😄

Ok, have fun

[Tratcher]

It's likely not unreasonable, but System.Web is still a heavy dependency (2/3MB IIRC?) that self-hosters would want to avoid, so if there's a way to avoid that, it's worth giving it a try.

Ha, C:\Windows\Microsoft.NET\Framework\v4.0.30319\System.Web.dll is 5mb! And that doesn't include transitive dependencies.

[Tratcher]

Would this be a factory that returns a ICookieManager? Maybe with an option for chunking?

[kevinchalet]

Not gonna lie, setting up that repo was quite a challenge 😅

• Not sure why, but VS complains about missing tools (running restore.cmd didn't help, sadly):

NU1603: Katana.Sandbox.WebServer dépend de Microsoft.TemplateEngine.Tasks (>= 7.0.100-preview.2.22075.3), mais Microsoft.TemplateEngine.Tasks 7.0.100-preview.2.22075.3 est introuvable. Une meilleure correspondance approximative de Microsoft.TemplateEngine.Tasks 7.0.100-preview.2.22153.5 a été résolue.
NU1101: Package RichCodeNav.EnvVarDump introuvable. Aucun package associé à cet ID n'existe dans la ou les sources suivantes : dotnet-eng, dotnet-public, dotnet-tools

• It's not clear why, but startvs.cmd kept telling me .NET Core was not installed (and I have tons of SDK versions installed 😅)

• All the projects enforce strong naming signing with a non-public key, so you can't run any project by default. Enabling strong naming only on CI would probably make the UX better 😃

• The sandbox projects no longer have hardcoded client ids/secrets for the external providers (I assume you have them as env vars on your machine 😄), which requires commenting out quite some code to get the sandbox running.

Anyway, I gave it a try and here's the result: kevinchalet@6c14fc2

It's very crude ATM but if it's an approach you like, it can be polished. Two things to note:

• The CookieOptions.CreateFromDictionary()/ToDictionary() helpers are public because they need to be used by SystemWeb. If it's not desirable (and it's likely not), it could be solved by moving these helpers to SystemWeb or using [InternalsVisibleTo].

• There's a small behavior change in the OAuth 1.0/2.0/OIDC middleware, where the cookie manager is no longer set in the options constructor but from the middleware (like what the cookies middleware does). It's unlikely folks expect the *Options.CookieManager property to be non-null before calling app.Use*Authentication() but if it's a breaking change you want to avoid, a special "default" marker instance could be used to detect whether the cookie manager was replaced by the user.

[Tratcher]

Yeah, switching to the new build infrastructure left some rough edges 😁. I'm tempted to remove those sandbox projects.

[kevinchalet]

I'm tempted to remove those sandbox projects.

Or convert them to SDK-style projects using CZEMacLeod's MSBuild.SDK.SystemWeb? 😄

The result is incredible: https://github.com/openiddict/openiddict-core/blob/dev/sandbox/OpenIddict.Sandbox.AspNet.Client/OpenIddict.Sandbox.AspNet.Client.csproj (that project even builds on macOS/Linux thanks to the .NET Framework reference assemblies package, mind blowing... )

[kevinchalet]

Copying these comments here so they don't disappear once the branch will be removed.

I made the same experiment but without the loosely-typed tuples: kevinchalet@500ca47

Unsurprisingly, it's much less "OWINy" - it had its own charm - but it simplifies things a lot 🤣

[Tratcher]

Very nice. Want to send a PR?

[kevinchalet]

Very nice. Want to send a PR?

Yeah, I'll do that.

Note: one of the two concerns vanished when removing the loosely-typed OWINisms, but the other one remains. Should we do something to avoid a behavior change?

• There's a small behavior change in the OAuth 1.0/2.0/OIDC middleware, where the cookie manager is no longer set in the options constructor but from the middleware (like what the cookies middleware does). It's unlikely folks expect the *Options.CookieManager property to be non-null before calling app.Use*Authentication() but if it's a breaking change you want to avoid, a special "default" marker instance could be used to detect whether the cookie manager was replaced by the user.

[Tratcher]

I'm not concerned about anyone reading that property. Did you have any examples of that in the aspnet-contrib libs?

[kevinchalet]

Opened: #486.

Did you have any examples of that in the aspnet-contrib libs?

Nope. The only - unusual? - pattern I can think of would be this one:

var options = new OpenIdConnectAuthenticationOptions();
options.CookieManager = new MyCookieManagerWrapperThatDoesSameSiteStuff(options.CookieManager);

app.UseOpenIdConnectAuthentication(options);

[Tratcher]

Oh, and since you're making my wildest dreams come true, what about making ThrowForPartialCookies false by default? 🤣

This is the only part of this PR left. Want to include that in your other PR, or send a separate one for it?

[kevinchalet]

This is the only part of this PR left. Want to include that in your other PR, or send a separate one for it?

Done: #488

Many generations of poor people DOSed by ThrowForPartialCookies will thank you for accepting this change 😄