Add HttpContext ot ITicketStore methods (CookieAuthenticationHandler)

[vanbukin]

Background and Motivation

Current ITicketStore interface didn’t have any methods that provides access to HttpContext

Proposed API

namespace Microsoft.AspNetCore.Authentication.Cookies;

public interface ITicketStore
{
    Task<string> StoreAsync(AuthenticationTicket ticket);
    Task<string> StoreAsync(AuthenticationTicket ticket, CancellationToken cancellationToken) => StoreAsync(ticket);
+    Task<string> StoreAsync(HttpContext httpContext, AuthenticationTicket ticket, CancellationToken cancellationToken) => StoreAsync(ticket, cancellationToken);
    Task RenewAsync(string key, AuthenticationTicket ticket);
    Task RenewAsync(string key, AuthenticationTicket ticket, CancellationToken cancellationToken) => RenewAsync(key, ticket);
+    Task RenewAsync(HttpContext httpContext, string key, AuthenticationTicket ticket, CancellationToken cancellationToken) => RenewAsync(key, ticket, cancellationToken);
    Task<AuthenticationTicket?> RetrieveAsync(string key);
    Task<AuthenticationTicket?> RetrieveAsync(string key, CancellationToken cancellationToken) => RetrieveAsync(key);
+    Task<AuthenticationTicket?> RetrieveAsync(HttpContext httpContext, string key, CancellationToken cancellationToken) => RetrieveAsync(key, cancellationToken);
    Task RemoveAsync(string key);
    Task RemoveAsync(string key, CancellationToken cancellationToken) => RemoveAsync(key);
+    Task RemoveAsync(HttpContext httpContext, string key, CancellationToken cancellationToken) => RemoveAsync(key, cancellationToken);
}

Usage Examples

It will improves developer experience in scenarios when storage is implemented by the scoped service (EF DbContext for example, and comment in another ITicketStore issue).

Alternative Designs

If I try to use any scoped service in my ITicketStore implementation, then I have two options:

• Inject the IServiceScopeFactory, create scope that separated from a current request's scope and then resolve and use any scoped service there
• Inject IHttpContextAccessor, that holds HttpContext in AsyncLocal and then resolve scoped service from there

Risks

There is no risks. ITicketStore already have methods, that accepts cancellation tokens, and default interface implementation is just calls an overload without cancellation tokens. Methods that accepts HttpContext can be safely added in a same way. That will help to avoid any breaking changes.
The only place when the ITicketStore is called is CookieAuthenticationHandler, and all callers has access to HttpContext and can pass it to ITicketStore.

[Tratcher]

As you outlined, there are already several different ways to do this. Why are those not sufficient? It's not clear why you need the HttpContext just to get services when you have other ways to get them.

[vanbukin]

@Tratcher Because I lose request scope when inject factory, or I have a deal with IHttpContextAccessor and underlying AsyncLocal (lower performance).
HttpContext is an extensibility point that provides access to request scope and helps to different application parts with different lifetimes to interact with each other.

[vanbukin]

I can make a PR if this change is acceptable.

[ghost]

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.

[halter73]

Another option if we decide to do something like this might be adding an HttpContext property to AuthenticationTicket.

[vanbukin]

@halter73 AuthenticationTicket, from my point of view, is a complex DTO that contains schema-independent authentication information, and ITicketStore is a service that runs inside a CookieHandler. In other words, it is a storage abstraction that operates inside the request context.

[halter73]

API Review Notes:

• Can we do the AuthenticationTicket thing?
  • No. The ticket is stored in the cache, and we don't want to root the HttpContext.
• Should the new parameter come last?
  • Last before the CancellationToken to keep the parameter orders as consistent as possible.

Approved!

namespace Microsoft.AspNetCore.Authentication.Cookies;

public interface ITicketStore
{
    Task<string> StoreAsync(AuthenticationTicket ticket);
    Task<string> StoreAsync(AuthenticationTicket ticket, CancellationToken cancellationToken) => StoreAsync(ticket);
+    Task<string> StoreAsync(AuthenticationTicket ticket, HttpContext httpContext, CancellationToken cancellationToken) => StoreAsync(ticket, cancellationToken);
    Task RenewAsync(string key, AuthenticationTicket ticket);
    Task RenewAsync(string key, AuthenticationTicket ticket, CancellationToken cancellationToken) => RenewAsync(key, ticket);
+    Task RenewAsync(string key, AuthenticationTicket ticket, HttpContext httpContext, CancellationToken cancellationToken) => RenewAsync(key, ticket, cancellationToken);
    Task<AuthenticationTicket?> RetrieveAsync(string key);
    Task<AuthenticationTicket?> RetrieveAsync(string key, CancellationToken cancellationToken) => RetrieveAsync(key);
+    Task<AuthenticationTicket?> RetrieveAsync(string key, HttpContext httpContext, CancellationToken cancellationToken) => RetrieveAsync(key, cancellationToken);
    Task RemoveAsync(string key);
    Task RemoveAsync(string key, CancellationToken cancellationToken) => RemoveAsync(key);
+    Task RemoveAsync(string key, HttpContext httpContext, CancellationToken cancellationToken) => RemoveAsync(key, cancellationToken);
}

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[halter73]

I can make a PR if this change is acceptable.

Please feel free to submit a PR if you want to implement the API as approved above. Thanks for the suggestion! I put it in the backlog because I don't think we're going to spend the time to do this ourselves, but if you submit a PR, we can likely get the change in for .NET 7.