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 IAuthenticationConfigurationProvider.GetAuthenticationConfiguration #41989

Closed
captainsafia opened this issue Jun 1, 2022 · 10 comments
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@captainsafia
Copy link
Member

captainsafia commented Jun 1, 2022

To support reading default schemes from configuration, we need to add an API to IAuthenticationConfigurationProvider​ that allows us to extract the root Authentication​ property from configuration.

The PR also adds a set of shared constants for use in the user-jwts CLI and the runtime with regarding to accessing these configuration keys.

Risks
Low, we've discussed adding this is a follow-up item from preview5.

Pull Request
#41987

Proposed API

namespace Microsoft.AspNetCore.Authentication;

public interface IAuthenticationConfigurationProvider
{
+    public IConfiguration Authentication { get; }
}
namespace Microsoft.AspNetCore.Authentication;

public static class AuthenticationConfigurationProviderExtensions
{
+    public static IConfiguration GetSchemeConfiguration(this IAuthenticationConfigurationProvider provider, string authenticationScheme);
}

Sample Usage
An end-user can implement a custom IAuthenticationConfigurationProvider to point to where the top-level configuration key in their application is.

public class MyAuthenticationConfigurationProvider : IAuthenticationConfigurationProvider
{
    private IConfiguration _configuration;
 
    public DefaultAuthenticationConfigurationProvider(IConfiguration configuration)
    {
        _configuration = configurationRoot;
    }
 
    public IConfiguration Authentication => _configuration.GetSection("MyCustomAuthName");
}

Sample Config

{
  "MyCustomAuthName": {
    "DefaultScheme": "ClaimedDetails",
    "Schemes": {
      "Bearer": {
        "Audiences": [
          "https://localhost:7259",
          "http://localhost:5259"
        ],
        "ClaimsIssuer": "dotnet-user-jwts"
      },
      "ClaimedDetails": {
        "Audiences": [
          "https://localhost:7259",
          "http://localhost:5259"
        ],
        "ClaimsIssuer": "dotnet-user-jwts"
      }
    }
  }
}

In our ConfigureOptions implementation, we use the GetSchemeConfiguration extension method to access individual schemes

internal sealed class JwtBearerConfigureOptions : IConfigureNamedOptions<JwtBearerOptions>
{
    public void Configure(string? name, JwtBearerOptions options)
    {
        var configSection = _authenticationConfigurationProvider.GetSchemeConfiguration(name);
    }
}
@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 1, 2022
@ghost
Copy link

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

@davidfowl
Copy link
Member

Why does this need the schemes as a first class property?

@captainsafia
Copy link
Member Author

Why does this need the schemes as a first class property?

It makes sense to me to have every key we support under Authentication as a first-class property. I suppose on alternative is to store an Authentication:Schemes key but it made sense to keep the properties separate and leave it up to the implementor to combine as needed.

@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview6 milestone Jun 2, 2022
@davidfowl
Copy link
Member

It makes sense to me to have every key we support under Authentication as a first-class property. I suppose on alternative is to store an Authentication:Schemes key but it made sense to keep the properties separate and leave it up to the implementor to combine as needed.

Do we need a strongly typed object model on authentication configuration for every top level configuration property? I think we should keep this type simple. It's a way to get access to the top level auth config section, and the config section for a scheme. Otherwise, it's no longer an interface but instead a poco like AuthenticationOptions.

GetAuthenticationConfiguration should be a property.

@halter73
Copy link
Member

halter73 commented Jun 6, 2022

API Review Notes:

  • Do we like properties instead of methods?
    • Yes.
  • Should we suffix the property names with "Configure"?
    • No. It's in the interface name already. This only shows up if you resolve it from DI, so you should know what the type does.
namespace Microsoft.AspNetCore.Authentication;

public interface IAuthenticationConfigurationProvider
{
-    public IConfiguration GetAuthenticationConfiguration();
+    public IConfiguration Authentication { get; }
+    public IConfiguration AuthenticationSchemes { get; }
}

@halter73 halter73 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 6, 2022
@captainsafia captainsafia added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels 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.

@halter73
Copy link
Member

halter73 commented Jun 9, 2022

Here's the proposed diff from the originally approved API if I'm reading this correctly:

namespace Microsoft.AspNetCore.Authentication;

public interface IAuthenticationConfigurationProvider
{
    public IConfiguration Authentication { get; }
-    public IConfiguration AuthenticationSchemes { get; }
}

+ public static class AuthenticationConfigurationProviderExtensions
+ {
+    public static IConfiguration GetSchemeConfiguration(this IAuthenticationConfigurationProvider provider, string authenticationScheme);
+ }

Is there a benefit of calling _authenticationConfigurationProvider.GetSchemeConfiguration(name); over _authenticationConfigurationProvider.GetSection($"Scheme:{name}")? Is it just trying to get rid of the magic string? Seems reasonable

It does seem a little weird to call _authenticationConfigurationProvider.Authentication for the parent config section and call _authenticationConfigurationProvider.GetSchemeConfiguration(name) for the subsection because one calls out Configuration in the name and the other doesn't. Should _authenticationConfigurationProvider.Authentication be _authenticationConfigurationProvider.AuthenticationConfiguration instead?

@captainsafia
Copy link
Member Author

Is there a benefit of calling _authenticationConfigurationProvider.GetSchemeConfiguration(name); over _authenticationConfigurationProvider.GetSection($"Scheme:{name}")? Is it just trying to get rid of the magic string? Seems reasonable

Yep, and to provide an abstraction over getting the scheme from the provider, which we will be using in all the different options from config implementations (e.g. JwtBearerConfigureOptions, CookieConfigureOptions)

Should _authenticationConfigurationProvider.Authentication be _authenticationConfigurationProvider.AuthenticationConfiguration instead?

Sure. That works.

@halter73
Copy link
Member

Approved API (relative to what's currently checked in):

namespace Microsoft.AspNetCore.Authentication;

public interface IAuthenticationConfigurationProvider
{
-    IConfiguration GetAuthenticationSchemeConfiguration(string authenticationScheme);
+    IConfiguration AuthenticationConfiguration { get; }
}

+ public static class AuthenticationConfigurationProviderExtensions
+ {
+    public static IConfiguration GetSchemeConfiguration(this IAuthenticationConfigurationProvider provider, string authenticationScheme);
+ }

@halter73 halter73 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 10, 2022
@captainsafia
Copy link
Member Author

Closed via #41987.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

No branches or pull requests

4 participants