Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add general property bag to cookies to support potential new cookie "standards" #39968

Closed
blowdart opened this issue Feb 3, 2022 · 7 comments · Fixed by #42119
Closed

Add general property bag to cookies to support potential new cookie "standards" #39968

blowdart opened this issue Feb 3, 2022 · 7 comments · Fixed by #42119
Assignees
Labels
affected-medium This issue impacts approximately half of our customers api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating.
Milestone

Comments

@blowdart
Copy link
Contributor

blowdart commented Feb 3, 2022

Problem

Google in their quest to remove third party cookies are proposing moving to yet another cookie standard.

It's both better than, and worse than the same site changes we had to go through before to keep oauth's lights on for IdPs that couldn't support more modern flows.

The CHIPS proposal adds new attributes to cookies, and as we've traditionally had strong properties on cookies any new cookie property needs a lot of work to support, and we end up having a slow reaction.

Potential solution

I propose a general property bag of names and values (with values allowing for NULL) for outbound cookie properties, which would allow customers to be more flexible in what gets added as a cookie property and not have to wait for us to push new code to test out a standard which may, or may not get ratified.

Of course the property bag would have to nicely sync with the existing "strong" properties.

@blowdart blowdart added Needs: Design This issue requires design work before implementating. enhancement This issue represents an ask for new feature or an enhancement to an existing one area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-runtime affected-medium This issue impacts approximately half of our customers labels Feb 3, 2022
@Tratcher
Copy link
Member

Tratcher commented Feb 3, 2022

Design point: both key= and key can be valid formats depending on the parameter. We might not know how to properly format entries from a key-value collection. A List<string> might be better, or even a flat string for people to append extra stuff to.

@Tratcher Tratcher added this to the .NET 7 Planning milestone Feb 4, 2022
@ghost
Copy link

ghost commented Feb 4, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@blowdart
Copy link
Contributor Author

@adityamandaleeka weve been asked by a couple of internal teams to make this available, rather than it just being me planning.

Please assign to a milestone coming soon so it doesn't get lost and adjust priority upwards as you see fit.

@Tratcher
Copy link
Member

Tratcher commented Jun 8, 2022

A contributor added SetCookieHeaderValue.Extensions back in 5.0 that should work for this. This covers the lowest layer that parses and serializes cookies.
https://docs.microsoft.com/en-us/dotnet/api/microsoft.net.http.headers.setcookieheadervalue.extensions
#22181

The next layer up that we need to add this to is CookieOptions and then CookieBuilder:
https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.cookieoptions
https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.cookiebuilder

These two APIs are used most places we work with cookies, like HttpResponse.Cookies.Append and CookieAuthenticationOptions, so we shouldn't have to do much additional work to expose it on individual components.

@Tratcher
Copy link
Member

Tratcher commented Jun 9, 2022

Proposed API

namespace Microsoft.AspNetCore.Http;

public class CookieOptions
{
+    public CookieOptions(CookieOptions options); // Copy constructor, avoids manual copying when needing to change something.
+    public IList<string> Extensions { get; };
+    public SetCookieHeaderValue CreateCookie(string name, string value); // Factory, avoids manual copying when creating the header
}

public class CookieBuilder
{
+    public IList<string> Extensions { get; };
}

Usage Examples

httpContext.Response.Cookies.Append(testCookie, "value", new CookieOptions()
{
     Extensions = { "simple", "key=value" }
});

var sharedOptions = new CookieOptions() { Secure = true };
httpContext.Response.Cookies.Append(testCookie, "value", new CookieOptions(sharedOptions)
{
     Extensions = { "simple", "key=value" }
});
services.AddAuthentication().AddCookie(o =>
{
     o.Cookie.Extensions.Add("extension");
});

@Tratcher Tratcher added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 9, 2022
@ghost
Copy link

ghost commented Jun 9, 2022

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@adityamandaleeka adityamandaleeka removed their assignment Jun 20, 2022
@BrennanConroy
Copy link
Member

API review notes:

  • Should CreateCookie be renamed to CreateCookieHeader?
    • The name is a bit ambiguous, is it creating a new cookie header or a new cookie option?
    • Return type does make it more obvious that it's the header but we think it's better to not use the return type to disambiguate
namespace Microsoft.AspNetCore.Http;

public class CookieOptions
{
+    public CookieOptions(CookieOptions options); // Copy constructor, avoids manual copying when needing to change something.
+    public IList<string> Extensions { get; };
+    public SetCookieHeaderValue CreateCookieHeader(string name, string value); // Factory, avoids manual copying when creating the header
}

public class CookieBuilder
{
+    public IList<string> Extensions { get; };
}

API approved!

@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 21, 2022
@danroth27 danroth27 changed the title Add general property bag to cookies to support potential new cookie "standards: Add general property bag to cookies to support potential new cookie "standards" Jul 18, 2022
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 17, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-medium This issue impacts approximately half of our customers api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one Needs: Design This issue requires design work before implementating.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants