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

API Review for System.Web adapters (Part 3): Session/Auth #42092

Closed
twsouthwick opened this issue Jun 8, 2022 · 3 comments
Closed

API Review for System.Web adapters (Part 3): Session/Auth #42092

twsouthwick opened this issue Jun 8, 2022 · 3 comments
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Milestone

Comments

@twsouthwick
Copy link
Member

twsouthwick commented Jun 8, 2022

Background and Motivation

We've been developing some adapters to help with migrating from ASP.NET Framework that adapter the System.Web.HttpContext to the Microsoft.AspNetCore.Http.HttpContext. We've released Preview 1 and would like to review the API shape.

This issue focuses on the APIs for session and authentication.

NOTE: Session is currently a separate package Microsoft.AspNetCore.SystemWebAdapters.SessionState because it relies on System.Text.Json to serialize/deserialize session state. However, this is limited to a single service, that some people may want to override. The rest of the infrastructure may make sense in the base Microsoft.AspNetCore.SystemWebAdapters package.

Related API Designs:

Proposed API

//
// Microsoft.AspNetCore.SystemWebAdapters
//

#if NET6_0_OR_GREATER
namespace Microsoft.AspNetCore.Builder
#else
namespace System.Web
#endif
{
  public static class RemoteAppExtensions
  {
      public static ISystemWebAdapterBuilder AddRemoteApp(this ISystemWebAdapterBuilder builder, Action<RemoteAppOptions> configure);
  }

  public static class RemoteAppAuthenticationExtensions
  {
      public static ISystemWebAdapterBuilder AddRemoteAppAuthentication(this ISystemWebAdapterBuilder systemWebAdapterBuilder, Action<RemoteAppAuthenticationOptions>? configureOptions = null);

#if NET6_0_OR_GREATER
      public static ISystemWebAdapterBuilder AddRemoteAppAuthentication(this ISystemWebAdapterBuilder systemWebAdapterBuilder, bool isDefaultScheme, Action<RemoteAppAuthenticationOptions>? configureOptions = null);

      public static AuthenticationBuilder AddRemoteAppAuthentication(this AuthenticationBuilder authenticationBuilder);

      public static AuthenticationBuilder AddRemoteAppAuthentication(this AuthenticationBuilder authenticationBuilder, string scheme);

      public static AuthenticationBuilder AddRemoteAppAuthentication(this AuthenticationBuilder authenticationBuilder, string scheme, Action<RemoteAppAuthenticationOptions>? configureOptions = null);

      public static AuthenticationBuilder AddRemoteAppAuthentication(this AuthenticationBuilder authenticationBuilder, Action<RemoteAppAuthenticationOptions>? configureOptions = null);
#endif
  }
}

namespace Microsoft.AspNetCore.SystemWebAdapters
{
  public class RemoteAppOptions
  {
      public string ApiKeyHeader { get; set; };

      public string ApiKey { get; set; }

  #if NET6_0_OR_GREATER
      public Uri RemoteAppUrl { get; set; }
  #endif
  }
}

namespace Microsoft.AspNetCore.SystemWebAdapters.Authentication
{
  public class RemoteAppAuthenticationOptions
#if NET6_0_OR_GREATER
    : AuthenticationSchemeOptions
#endif
{
#if NET6_0_OR_GREATER
    public static readonly IEnumerable<string> DefaultRequestHeadersToForward;

    public static readonly IEnumerable<string> DefaultResponseHeadersToForward;

    public ICollection<string> RequestHeadersToForward { get; }

    public ICollection<string> ResponseHeadersToForward { get; }

    public TimeSpan NetworkTimeout { get; set; }
#endif

    public string AuthenticationEndpointPath { get; set; }
}

#if NET6_0_OR_GREATER
  public interface IRemoteAppAuthenticationResultProcessor
  {
      Task ProcessAsync(RemoteAppAuthenticationResult result, HttpContext context);
  }

  public static class RemoteAppAuthenticationDefaults
  {
      public const string AuthenticationScheme = "Remote";
  }

  public class RemoteAppAuthenticationResult
  {
      public RemoteAppAuthenticationResult(ClaimsPrincipal? user, int statusCode, HttpRequestMessage? authRequest);

      public ClaimsPrincipal? User { get; }

      public int StatusCode { get; }

      public HttpRequestMessage? AuthenticationRequest { get; }

      public IHeaderDictionary ResponseHeaders { get; }
  }
#endif
}

// 
// Microsoft.AspNetCore.SystemWebAdapters.SessionState
//

#if NET6_0_OR_GREATER
namespace Microsoft.AspNetCore.Builder
#else
namespace System.Web
#endif
{
  public static class RemoteAppSessionStateExtensions
  {
      public static ISystemWebAdapterBuilder AddRemoteAppSession(this ISystemWebAdapterBuilder builder, Action<RemoteAppSessionStateOptions>? configureRemote = null);
  }

  public static class SessionSerializerExtensions
  {
    public static ISystemWebAdapterBuilder AddSessionSerializer(this ISystemWebAdapterBuilder builder, Action<SessionSerializerOptions>? configure = null);
  }

  public static class JsonSessionSerializerExtensions
  {
      public static ISystemWebAdapterBuilder AddJsonSessionKeySerializer(this ISystemWebAdapterBuilder builder, Action<JsonSessionSerializerOptions>? configure = null);
  }

#if NET6_0_OR_GREATER
  public static class ISystemWebAdapterBuilderSessionExtensions
  {
    public static ISystemWebAdapterBuilder WrapAspNetCoreSession(this ISystemWebAdapterBuilder builder, Action<Builder.SessionOptions>? options = null);
  }
#endif
}

namespace Microsoft.AspNetCore.SystemWebAdapters
{
  public interface ISessionKeySerializer
  {
      bool TrySerialize(string key, object value, out byte[] bytes);

      bool TryDeserialize(string key, byte[] bytes, out object? obj);
  }
}

namespace Microsoft.AspNetCore.SystemWebAdapters.SessionState.Serialization
{
  public class JsonSessionSerializerOptions
  {
      public IDictionary<string, Type> KnownKeys { get; }

      public void RegisterKey<T>(string key);
  }

  public interface ISessionSerializer
  {
      Task<ISessionState?> DeserializeAsync(Stream stream, CancellationToken token);

      Task SerializeAsync(ISessionState state, Stream stream, CancellationToken token);
  }

  public class SessionSerializerOptions
  {
      public bool ThrowOnUnknownSessionKey { get; set; }
  }
}

namespace Microsoft.AspNetCore.SystemWebAdapters.SessionState.RemoteSession;

public class RemoteAppSessionStateOptions
{
    public string SessionEndpointPath { get; set; }

    public string CookieName { get; set; }

#if !NETFRAMEWORK
    public TimeSpan NetworkTimeout { get; set; }
#endif
}

Usage Examples

Remote app session and authentication

ASP.NET Core

builder.Services.AddSystemWebAdapters()
    .AddRemoteApp(options =>
    {
        options.RemoteAppUrl = new(builder.Configuration["ReverseProxy:Clusters:fallbackCluster:Destinations:fallbackApp:Address"]);
        options.ApiKey = ClassLibrary.RemoteServiceUtils.ApiKey;
    })
    .AddRemoteAppSession()
    .AddJsonSessionSerializer(options => ClassLibrary.RemoteServiceUtils.RegisterSessionKeys(options.KnownKeys))
    .AddRemoteAppAuthentication(true);

ASP.NET Framework

public class MvcApplication : System.Web.HttpApplication
{
    protected void Application_Start()
    {
        ...

        Application.AddSystemWebAdapters()
            .AddProxySupport(options => options.UseForwardedHeaders = true)
            .AddRemoteApp(options =>
            {
                options.ApiKey = ClassLibrary.RemoteServiceUtils.ApiKey;
            })
            .AddRemoteAppSession()
            .AddJsonSessionSerializer(options => ClassLibrary.RemoteServiceUtils.RegisterSessionKeys(options.KnownKeys))
            .AddRemoteAppAuthentication();
    }
}

Wrapping Microsoft.AspNetCore.ISession

builder.Services.AddSystemWebAdapters()
    .AddJsonSessionSerializer(options => ClassLibrary.SessionUtils.RegisterSessionKeys(options.KnownKeys))
    .WrapAspNetCoreSession();
@twsouthwick twsouthwick added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 8, 2022
@twsouthwick twsouthwick changed the title API Review for System.Web adapters: Session API Review for System.Web adapters (Part 3): Session Jun 8, 2022
@Tratcher Tratcher added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 8, 2022
@ghost
Copy link

ghost commented Jun 8, 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.

@BrennanConroy BrennanConroy added this to the 7.0-preview6 milestone Jun 8, 2022
@brunolins16 brunolins16 removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 9, 2022
@twsouthwick twsouthwick changed the title API Review for System.Web adapters (Part 3): Session API Review for System.Web adapters (Part 3): Session/Auth Jul 15, 2022
@halter73
Copy link
Member

API Review Notes:

  • How should we layer packages?
    • Either one big package with all the dependencies including System.Text.Json
    • Or a metapackage with subpackages that allow you to replace things like S.T.J with MessagePack or something
  • Can we make things like .AddRemoteAppSession() part of the .AddRemoteApp(options?
    • Makes sense
  • Can we avoid using the same name for types that are at all structurally different and will not ever have shared code calling it from top?
    • RemoteAppAuthenticationOptions, RemoteAppOptions and AddRemoteAppAuthentication is an examples of that. There are more.
    • Maybe use SystemWeb and AspNetCore prefix?
    • Or Client and Server?
      • This seems to be the consensus.
      • For example, AddRemoteAppAuthentication to AddRemoteAppAuthenticationClient and AddRemoteAppSessionClient and AddRemoteAppSessionServer.
  • We messed up earlier! ALL IServiceCollection extension methods should be in the Microsoft.Extensions.DependencyInjection.
  • DefaultRequestHeadersToForward and DefaultResponseHeadersToForward probably don't need to be public since they're the initial values for the options. If they are made public, they should be properties instead of field.
  • NetworkTimeout could probably be made an HttpClient? It's more flexible, and it might help with testing.
  • AuthenticationEndpointPath should be a PathString on ASP.NET Core.

@twsouthwick
Copy link
Member Author

With some of the changes we've done, it makes sense to continue the API review on the single issue #42091

@halter73 halter73 removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Sep 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 19, 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
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

No branches or pull requests

8 participants