-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add dotnet user-jwts tool and runtime support #41520
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
Conversation
8fec032
to
0322b72
Compare
32454fb
to
f8b6b5d
Compare
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.
Build bits look good
🆙 📅 :
|
src/Http/Authentication.Abstractions/src/IAuthenticationConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/Core/src/DefaultAuthenticationConfigurationProvider.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/JwtBearer/src/AuthenticationConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/JwtBearer/src/JwtBearerExtensions.cs
Outdated
Show resolved
Hide resolved
src/Security/Authentication/JwtBearer/src/AuthenticationConfigurationOptions.cs
Outdated
Show resolved
Hide resolved
@@ -166,6 +173,12 @@ private void ConfigureApplication(WebHostBuilderContext context, IApplicationBui | |||
} | |||
} | |||
|
|||
if (Authentication is WebApplicationAuthenticationBuilder webAuthBuilder && webAuthBuilder.IsAuthenticationConfigured is true) | |||
{ | |||
app.UseAuthentication(); |
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.
Do we need to guard this so that the middleware are only added if they're not already added in the app pipeline (like we do with the call to UseRouting()
above)? Otherwise we'll be forcing auth to run earlier than the app wants to, which can cause problems, e.g. CORS.
...ecurity/Authentication/JwtBearer/samples/MinimalJwtBearerSample/appsettings.Development.json
Show resolved
Hide resolved
@@ -166,6 +173,12 @@ private void ConfigureApplication(WebHostBuilderContext context, IApplicationBui | |||
} | |||
} | |||
|
|||
if (Authentication is WebApplicationAuthenticationBuilder webAuthBuilder && webAuthBuilder.IsAuthenticationConfigured is true) |
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.
if (Authentication is WebApplicationAuthenticationBuilder webAuthBuilder && webAuthBuilder.IsAuthenticationConfigured is true) | |
if (_webAuthBuilder.IsAuthenticationConfigured) |
if (Authentication is WebApplicationAuthenticationBuilder webAuthBuilder && webAuthBuilder.IsAuthenticationConfigured is true) | ||
{ | ||
app.UseAuthentication(); | ||
app.UseAuthorization(); |
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.
Can we add something to WebApplicationTests
(or another test class since that's already kinda big) to verify we register services and add middleware when methods are called on the WebApplicationAuthenticationBuilder
? And another test verifying that just accessing the Authentication
getter doesn't do this?
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.
Yep, will add this to verify changes for #41520 (comment)
if (!app.Properties.ContainsKey(AuthenticationMiddlewareSetKey)) | ||
{ | ||
app.Properties[AuthenticationMiddlewareSetKey] = true; | ||
return app.UseMiddleware<AuthenticationMiddleware>(); | ||
} |
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 check shouldn't be there. The user needs to be able to re-run auth if they add this to the pipeline again.
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.
Why would you re-run auth? The AuthN middleware is not route aware and only reacts to the default auth scheme setting.
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.
That's exactly why you'd want to re-run it later in the pipeline to force setting the User based on the default scheme.
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 guess your point is that there's never a situation where running UseAuthentication can change based on where it is in the pipeline. Is that right?
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.
Well a custom IAuthenticationSchemeProvider doesn't have to return the same value every time, so it is valid that rerunning the authentication middleware could result in a different User being set on a second call
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.
The performance of auth already sucks, and this doesn't make it better enough to warrant the breaking change.
They can avoid it by not interacting with WebApplicationBuilder.Authentication and instead calling their methods directly on WebApplicationBuilder.
We can still mark that the middleware was added to avoid adding the one in the web application builder.
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.
Can we add a flag to let them explicitly turn it off? WebApplicationBuider.Authenticaiton.AutoAddMiddleware = false?
That feels a little strange to me. The WebApplicationBuilder.Authentication
pattern is an opt-in to the more simplified approach. It feels weird to have an opt-out to that when the more obvious choice of not using the new Authentication
property at all exists.
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.
How can someone that wants to manually add the middleware prevent the automatic one?
Our check in the WebApplicationBuilder
prevents this from happening. We'll set a flag in UseAuthentication
that is read and avoids re-registering automatically in the WAB.
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.
That feels a little strange to me. The WebApplicationBuilder.Authentication pattern is an opt-in to the more simplified approach. It feels weird to have an opt-out to that when the more obvious choice of not using the new Authentication property at all exists.
It feels strange to me that there'd be scenarios we're consciously aware of where we'd say the workaround is to not use the new property. Can we simply make it so that the WebApplicationBuilder
does not add the middlewares if the app pipeline has already added the authentication middleware? Why is any more than that needed?
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.
Can we simply make it so that the WebApplicationBuilder does not add the middlewares if the app pipeline has already added the authentication middleware? Why is any more than that needed?
It already does that. To clarify, my point was that we didn't need to do anything additional here.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/backport release/7.0-preview5 |
* Add dotnet dev-jwts tool * Add dotnet dev-jwts tool * Address feedback from review * Rename project file * Write auth config to app settings * Address more feedback * 🦭 * Apply suggestions from code review Co-authored-by: Brennan <brecon@microsoft.com> * Address more feedback * Add framework support for authentication changes * Add tests for user-jwts CLI and react to feedback * Move ConsoleTable implementation to avoid conflicts in ProjectTemplates * Update existing auth tests and fix middleware registration * Update AzureAdB2C tests and auth app builder * Fix build and move registration check * Fix up resolution for Certificate test sources * Fix write stream configuration for writing key material * Fix handling missing config section when processing options Co-authored-by: Brennan <brecon@microsoft.com>
* Add dotnet dev-jwts tool * Add dotnet dev-jwts tool * Address feedback from review * Rename project file * Write auth config to app settings * Address more feedback * 🦭 * Apply suggestions from code review Co-authored-by: Brennan <brecon@microsoft.com> * Address more feedback * Add framework support for authentication changes * Add tests for user-jwts CLI and react to feedback * Move ConsoleTable implementation to avoid conflicts in ProjectTemplates * Update existing auth tests and fix middleware registration * Update AzureAdB2C tests and auth app builder * Fix build and move registration check * Fix up resolution for Certificate test sources * Fix write stream configuration for writing key material * Fix handling missing config section when processing options Co-authored-by: Brennan <brecon@microsoft.com> Co-authored-by: Brennan <brecon@microsoft.com>
Part of #39857