Reduce duplication when configuring cookie options

[JunTaoLuo]

As per discussion in aspnet/Antiforgery#141. We should use the Action<..., CookieOptions> pattern on each components' options to configure the cookie options instead of duplicating them. Need to look through our code base for where we configure cookie options and update them where necessary.

[Tratcher]

Review:

• Security
• Session
• MVC - CookieTempDataProvider
• Localization
• Rewrite

[Tratcher]

@muratg these would be breaking changes that should be considered for RTM

[HaoK]

Alternatively, you could consider using IOptionsSnapshot<CookieOptions> and using named options for each cookie assuming cookie.Name is unique. Then configuring a cookie would be the normal services.Configure<CookieOptions>(cookieName, o => { }) That seems like a reasonable alternative to a mini options pattern for these...

[muratg]

cc @javiercn

[javiercn]

We went with the Action<HttpContext,CookieOptions> approach in the OpenIdConnect/RemoteAuthentication options as needed for aspnet/Security#1262.

The twitter middleware is the last one that requires this on the security repo as far as I know. We shouldn't do this for the cookie policy middleware IMO as it would broaden the scope of the middleware and would require more design, and its something that can be revisited on 2.1

[muratg]

cc @Eilon, @danroth27, @DamianEdwards do we want this in 2.0.0 (and obsolete the existing Options?) Or we can do it in 2.1, but then we can't obsolete them until 3.0.

[muratg]

We'll try to get this in because of the reason I mentioned above.

@natemcmaster please consult with @Tratcher on the ideas how to do this.

[natemcmaster]

I exampled the repos Chris listed above. To be consistent with the pattern established by Antiforgery, I believe these are the changes we would make:

Security

• Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationOptions
  • Obsoleted properties
    • string CookieDomain
    • string CookiePath
    • SameSiteMode CookieSameSite
    • bool CookieHttpOnly
    • CookieSecurePolicy CookieSecure
  • New API
    • Action<HttpContext, CookieOptions> ConfigureCookieOptions

Session

• Microsoft.AspNetCore.Session.SessionOptions
  • Obsoleted properties
    • string CookieDomain
    • string CookiePath
    • bool CookieHttpOnly
    • SameSiteMode SameSiteMode
    • CookieSecurePolicy CookieSecure
  • New API
    • Action<HttpContext, CookieOptions> ConfigureCookieOptions

MVC

• Microsoft.AspNetCore.Mvc.CookieTempDataProviderOptions
  • Obsoleted properties
    • string Domain
    • string Path
  • New API
    • Action<HttpContext, CookieOptions> ConfigureCookieOptions

Localization

I didn't find any API that duplicates CookieOptions. @Tratcher?

Rewrite

• Microsoft.AspNetCore.Rewrite.Internal.UrlActions.ChangeCookieAction
  • Name
  • Value
  • Domain
  • Path
  • Secure
  • HttpOnly

Proposal: leave as is. Normally, a user wouldn't interact with this API. This is "pubternal" anyways.

[JunTaoLuo]

The localization cookies shouldn't need any changes, they have their own format and are used differently from the cookies in other repos.

[HaoK]

Is there any reason we can't just have nested CookieOptions property instead of an Action<CookieOptions>? Then they can just manipulate options.Cookie directly when they are configuring the option, do they really need to use the httpContext often?

[natemcmaster]

The HttpContext is required for Session and Security because CookieOptions does not take support CookieSecurePolicy directly. You have to read context.Request.IsHttps to properly implement CookieSecurePolicy.SameAsRequest.

Side note: I'm not convinced there is enough value in breaking the API. The Action<> approach makes using the API cumbersome. Nested lambdas are less discoverable than top-level properties.

For comparison:

// current
services.AddSession(o =>
{
   o.CookiePath = "/somepath";
   o.CookieSecure = CookieSecurePolicy.SameAsRequest;
});

// with action property
services.AddSession(o =>
{
   o.ConfigureCookieOptions = (context, cookie) =>
   {
       cookie.Path = "/somepath";
       cookie.Secure = context.Request.IsHttps;
   };
});

[HaoK]

The current looks simplier to me, what's the benefit of the action?

[Tratcher]

Consistency and future proofing. Right now every component that uses cookies duplicates some subset of CookieOptions. We're not consistent about which options are exposed across the stack. We've also added several CookieOptions recently and had to go expose those new settings everywhere. If we directly expose the Action/CookieOptions then all of the options are consistently available everywhere, and we don't have to update each component when there are additions.

[HaoK]

Can we just add a new SecurePolicy property to CookieOptions, and then add CookieOptions instances to all of the feature options, so it looks like

services.AddSession(o =>
{
   o.Cookie.Path = "/somepath";
   o.Cookie.SecurePolicy = CookieSecurePolicy.SameAsRequest;
});

And just resolve the secure property before we use the options?

[Tratcher]

That doesn't work with concurrent requests unless you clone the CookieOptions each time before modifying the secure setting.

[Tratcher]

That's about the same as the problem we have with TokenValidationParameters in OpenIdConnect, always having to clone them before we can use them.

[HaoK]

That seems okay right? Basically use the options.Cookie as the default cookie options to clone from and its a logical place to hang SecurePolicy.

[javiercn]

@natemcmaster Another option

/ current
services.AddSession(o =>
{
   o.CookiePath = "/somepath";
   o.CookieSecure = CookieSecurePolicy.SameAsRequest;
   });

// with helper method
services
    .AddSession()
    .ConfigureSessionCookie((ctx,o) => { });

I don't think the lambda is terrible and it can be made pretty with a helper method. And the action is better for all the reasons @Tratcher mentions above.

[HaoK]

Exposing a sub CookieOptions directly also solves future proofing and consistency, without any of the complexity that the Action brings... So that's not a differentiation between the two choices.

I think its also slightly weird to make it look like IOptions/Configure when its not actually using Options

[Tratcher]

It doesn't need to be the cookie options type directly, that's a per cookie instance. What if there were a cookie settings object that had things like secure policy rather than the secure bool.

[natemcmaster]

Don't we normally use fluent api for builders? I'm can't think of places where we use properties on builders

[Tratcher]

I wasn't suggesting a builder, just a POCO settings object.

[HaoK]

We definitely have some that expose properties:

https://github.com/aspnet/Configuration/blob/dev/src/Microsoft.Extensions.Configuration.Abstractions/IConfigurationBuilder.cs

https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationSchemeBuilder.cs

I think its more of a look/feel choice, if we want it to feel like configuring options, I'd go with properties. The fluent seems nicer for chaining via services.AddXyzThatReturnsBuilder.UseXyz().

@Tratcher In this case, the builder is a POCO settings object with a Build method, its just clear what its doing from the name this way.

class SessionOptions {
   public CookieOptionsBuilder Cookie { get; }
}

class CookieOptionsBuilder {
   public string Path {get; set;}; 
   public CookieSecurePolicy { get;set;}
   public CookieOptions Build(HttpContext context) => new CookieOptions { Path = Path, Secure = context.Request.IsSecure };
}

[natemcmaster]

I like this. +1

[Tratcher]

Getting there. Build needs to account for SecurePolicy. CookieOptionsBuilder would have most/all of the CookieOptions expose, correct?

Include: Domain, Path, SameSite, HttpOnly.
Exclude: Secure, Expires
Add: CookieSecurePolicy, Name, Expiration (nullable TimeSpan, relative version of Expires).

Note Build does not need to be a method on the type itself, all of the information is public. We're going to want a version of Build that takes an ISytemClock for calculating expiration.

[HaoK]

Yeah it should have all of the properties that are on cookie options. Ideally you have a public Build that does all the common stuff so you can test it in httpabstractions. Each repo could then further tweak after building as stuff like ISystemClock only lives in security right?

…

On Jun 29, 2017, at 10:52 AM, Chris Ross ***@***.***> wrote: Getting there. Build needs to account for SecurePolicy. CookieOptionsBuilder would have most/all of the CookieOptions expose, correct? Include: Domain, Path, SameSite, HttpOnly. Exclude: Secure, Expires Add: CookieSecurePolicy, Name, Expiration (nullable TimeSpan, relative version of Expires). Note Build does not need to be a method on the type itself, all of the information is public. We're going to want a version of Build that takes an ISytemClock for calculating expiration. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[natemcmaster]

How about we leave off Expires/Expiration for now until? We don't need it in Session or Antiforgery. We can add it later. Optionally, Security can extend the builder to support ISystemClock.

Proposal:

namespace Microsoft.AspNetCore.Http
{
    public class CookieOptionsBuilder
    {
        public string Path { get; set; }

        public string Domain { get; set; }

        public bool HttpOnly { get; set; }

        public SameSiteMode SameSite { get; set; }

        public CookieSecurePolicy SecurePolicy { get; set; }

        public CookieOptions Build(HttpContext context)
        {
        }
    }
}

[natemcmaster]

This is what it looks like in code:

https://github.com/aspnet/HttpAbstractions/compare/namc/cookie-builder
https://github.com/aspnet/Session/compare/namc/cookie-builder
https://github.com/aspnet/Security/compare/namc/cookie-builder

[HaoK]

Is CookieName universal right now? If so maybe we should move that into CookieBuilder/CookieOptions?

[natemcmaster]

It's not. OpenIdConnectOptions doesn't let users fully customize the NonceCookie name.

[javiercn]

IMO this is a bit overkill.
The CookieBuilder doesn't use the HttpContext for nothing and doesn't allow a user to configure the cookie based on it. It adds an additional layer of abstraction over Action<HttpContext,CookieOptions> and limits the set of scenarios available (you can't configure the cookie per request).

In my option, having Action<HttpContext,CookieOptions> is more than enough for the following reasons:

1. We don't have to expose the set of default that we use when we set cookies in a configurable way.
2. Users configuring the cookie options is not a common scenario.
   • No component used in our templates requires you to configure the cookie on Startup for it to work.
   • No component we own forces you to explicitly configure the cookie for it to work.
3. Action<HttpContext,CookieOptions> allows you to override the defaults on each component in case you need to in a way that allows configuration based on the context of the request.

[HaoK]

I don't think consumers would agree, personally I'd much rather set options.Cookie.Path = "/path", than wire up some gross looking lambda options.ConfigureCookieOptions(ignored,o => o.Path = "/path") 🤢

The public surface area to people is normal options configuration, they don't call build, we call build, so they don't even need to understand what HttpContext is used for cuz they don't see it.

[Tratcher]

Meeting notes:

AddCookie(builder.Name, value, builder.Build(context))

CookieAuthOptions
{
  public CookieBuilder Cookie {get; set;}
}

    public class CookieBuilder
    {
        public virtual string Name { get; set; }        

        public virtual string Path { get; set; }

        public virtual string Domain { get; set; }

        public virtual bool HttpOnly { get; set; }

        public virtual SameSiteMode SameSite { get; set; }

        public virtual TimeSpan? Expiration { get; set; }

        public virtual CookieSecurePolicy SecurePolicy { get; set; }

        public CookieOptions virtual Build(HttpContext context) => Build(context, DateTimeOffset.Now)
        {
        }

        public CookieOptions virtual Build(HttpContext context, DateTimeOffset now)
        {
// Resolve SecurePolicy
// Build CookieOptions
        }
    }

[natemcmaster]

This was completed in the following PRs:

#882
aspnet/Security#1284
aspnet/Security#1285
aspnet/Session#173
aspnet/Mvc#6472
aspnet/Antiforgery#148