Add dotnet user-jwts tool and runtime support#41520
Add dotnet user-jwts tool and runtime support#41520captainsafia merged 18 commits intodotnet:mainfrom
Conversation
8fec032 to
0322b72
Compare
32454fb to
f8b6b5d
Compare
|
🆙 📅 :
|
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
|
|
||
| if (Authentication is WebApplicationAuthenticationBuilder webAuthBuilder && webAuthBuilder.IsAuthenticationConfigured is true) | ||
| { | ||
| app.UseAuthentication(); |
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| if (Authentication is WebApplicationAuthenticationBuilder webAuthBuilder && webAuthBuilder.IsAuthenticationConfigured is true) |
There was a problem hiding this comment.
| 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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