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

[Feature Request] Simplify the public API #378

Closed
jmprieur opened this issue Jul 30, 2020 · 3 comments
Closed

[Feature Request] Simplify the public API #378

jmprieur opened this issue Jul 30, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request fixed proposal
Milestone

Comments

@jmprieur
Copy link
Collaborator

jmprieur commented Jul 30, 2020

Is your feature request related to a problem? Please describe

Even if Microsoft.Identity.Web simplifies the implementation of Web apps / Web APIs, calling or not downstream APIs, there is still a feeling that the API is still complicated and has some repeats, which are not really necessary

This proposal (which is another breaking change) attempts to address these concerns.

Describe the solution you'd like

2 principles

  • Keep surfacing the .AddAuthentication() as for most complex scenarios, customers will need them (the overrides from the is only provides the simple solution, which does not have the delegates to initialize the options)
    -Avoid repeating names (in the APIs) or parameters, that could be avoided.

For Web apps

Simple override with a configuration section

services.AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApp(Configuration, "AzureAD")
      .CallsWebApi()  // No need for (Configuration, "AzureAD") as it's passed in AddMicrosoftWebApp already
      .AddMemoryTokenCaches();

which is really, with a lot of default values, the same as calling:

services.AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApp(Configuration,
                       configSectionName: "AzureAD",
                       openIdConnectScheme:OpenIdConnectDefaults.AuthenticationScheme,
                       cookieScheme: CookieAuthenticationDefaults.AuthenticationScheme,
                       subscribeToOpenIdConnectMiddlewareDiagnosticsEvents: false)
      .CallsWebApi(initialScopes: null)
      .AddMemoryTokenCaches();

Of course the default values can be overridden for more complex scenarios such as a specific authentication scheme (used when developers want to support several IdPs), or specific cookie scheme.

The subscription to middleware diagnostic events could also happen in configuration instead as a parameters. The ASP.NET Core team also suggested that they could add more traces (issue to be raised on aspnetcore)

The ASP.NET Core team also suggested passing a configuration section instead of the Configuration and the section name. This could be an additional override.

services.AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApp(Configuration.GetSection("AzureAD"))
      .CallsWebApi() 
      .AddMemoryTokenCaches();

discussion

Note the indendations. The CallsWebApi() and AddMemoryTokenCaches() [or other token cache implementations] would be available out of a new specialized builder MicrosoftAuthenticationBuilder and no longer on AuthenticationBuilderwhich. The MicrosoftAuthenticationBuilder would keep values such as the configuration section, the openid and cookie scheme, which means that CallsWebApi() would be drastically simplified as it would not be necessary to pass-in parameters that would already have been passed-in to AddMicrosoftWebApp (or AddMicrosoftWebApi, see below).

Override with delegates

AddMicrosoftWebApp has another override which is useful in more complex scenarios where the developer wants to override the configuration programmatically, or subscribe to OpenIdConnect events.

services.AddAuthentication(OpenIdConnectDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApp(microsoftIdentityOptions => { /* configure MicrosoftIdentityOptions (inherit from OpenIdConnectOptions*/}
                       cookieAuthenticationOptions => { /* configure CookieAuthenticationOptions */}
                      )
      .CallsWebApi(initialScopes,
                   confidentialClientApplicationOptions => { /* ConfidentialClientApplicationOptions  */} ) 
      .AddMemoryTokenCaches();

Note that in CallsWebApi, in that case, developers would not need to re-specify an Action<MicrosoftIdentityOptions> (as it is already passed-in in AddMicrosoftWebApp. Again the public API is simplified a lot.

For Web APIs:

Simple usage

services.AddAuthentication(JwBearerDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApi(Configuration, "AzureAD")
      .CallsWebApi()
      .AddMemoryTokenCaches();

which is really, with a lot of default values, the same as calling:

services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApi(Configuration,
                       configSectionName: "AzureAD",
                       jwtBearerScheme : JwtBearerDefaults.AuthenticationScheme,
                       subscribeToJwtBearerMiddlewareDiagnosticsEvents: false)
      .CallsWebApi(initialScopes: null)
      .AddMemoryTokenCaches();

Advanced usage (with the delegates)

services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
   .AddMicrosoftIndentityPlatformWebApi(Configuration,
                       jwtBearerOptions => { /* configure jwBearerOptions */},
                       microsoftIdentityOptions => { /* configure MicrosoftIdentityOptions */},
                       jwtBearerScheme : JwtBearerDefaults.AuthenticationScheme,
                       subscribeToJwtBearerMiddlewareDiagnosticsEvents: false)
      .CallsWebApi(initialScopes: null
                  confidentialClientApplicationOptions => { /* Configure CondidentialClientApplicationsOptions */ })
      .AddMemoryTokenCaches();

Describe alternatives you've considered

Adding more services?

Today, in the ASP.NET Core templates, we have added two more services to help calling Microsoft.Graph and a downstream API. Now might be the right moment to have these part of Microsoft.Identity.Web directly. DownstreamWebApiService is a generic web api service.

  • services.AddMicrosoftGraph(scopes, baseGraphUri));
  • services.AddDownstreamWebApiService(Configuration);

One question is how this would play with the .CallsWebApi() verb [which is really saying: add msal.net and cache tokens]. Should these be available out of the MicrosoftAuthenticationBuilder ?

Additional context

  • This would be a breaking change in the public API. Ideally we'd want to be ready for .NET 5 RC1
  • This needs to be reviewed with the ASP.NET Core team.
@jmprieur jmprieur added the enhancement New feature or request label Jul 30, 2020
@jmprieur jmprieur changed the title [Feature Request] [DRAFT in progress] Improve the API [Feature Request] [DRAFT in progress] Simplify the public API Jul 30, 2020
@jmprieur jmprieur added this to the [3] Fundamentals milestone Jul 30, 2020
@JunTaoLuo
Copy link

FYI @javiercn, @blowdart

@jennyf19 jennyf19 changed the title [Feature Request] [DRAFT in progress] Simplify the public API [Feature Request] Simplify the public API Aug 7, 2020
@jmprieur
Copy link
Collaborator Author

PR is #425

@jennyf19
Copy link
Collaborator

Included in 0.3.0-preview release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed proposal
Projects
None yet
Development

No branches or pull requests

3 participants