-
Notifications
You must be signed in to change notification settings - Fork 208
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
Proposal for the parameters of AddProtectedApi ... methods #69
Conversation
I propose to get rid of hybrid methods, that is provide only: - configuration and section name - or delegates to set the option (several delegates)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the proposed changes make sense.
src/Microsoft.Identity.Web/WebApiServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
} | ||
/// <summary> | ||
/// Protects the Web API with Microsoft identity platform (formerly Azure AD v2.0) | ||
/// This supposes that the configuration files have a section named configSectionName (typically "AzureAD") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is no longer applicable.
|
||
builder.Services.AddHttpContextAccessor(); | ||
builder.Services.AddSingleton<IJwtBearerMiddlewareDiagnostics, JwtBearerMiddlewareDiagnostics>(); | ||
|
||
// Change the authentication configuration to accommodate the Microsoft identity platform endpoint (v2.0). | ||
builder.AddJwtBearer(jwtBearerScheme, options => | ||
{ | ||
var microsoftIdentityOptions = configuration.GetSection(configSectionName).Get<MicrosoftIdentityOptions>(); | ||
// TODO: | ||
// Suspect. Why not get the IOption<MicrosoftIdentityOptions>? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
bool subscribeToJwtBearerMiddlewareDiagnosticsEvents = false) | ||
{ | ||
builder.Services.Configure(jwtBearerScheme, jwtOptionsMapper); | ||
builder.Services.Configure<MicrosoftIdentityOptions>(microsoftIdentityOptionsMapper); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the generic type can be removed, since it can be inferred or we can add one in the line above for consistency and if we want to be explicit or not. (Same applied to similar methods below).
src/Microsoft.Identity.Web/WebApiServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/WebApiServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
Looks good. I'll take a look at the related web app work. thanks. In reply to: 383584388 [](ancestors = 383584388) |
…nd delegates) Addressing pmartak's PR comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
I propose to get rid of hybrid methods, that is provide only:
@jennyf19 : let's discuss this when/if you have time.