Skip to content

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.

Sign up for GitHub

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

Merged
merged 18 commits into from
May 31, 2022
Merged

Add dotnet user-jwts tool and runtime support #41520

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
bf23154
Add dotnet dev-jwts tool
captainsafia May 3, 2022
6961df5
Add dotnet dev-jwts tool
captainsafia May 3, 2022
212cf04
Address feedback from review
captainsafia May 6, 2022
3ae682e
Rename project file
captainsafia May 6, 2022
5f5c040
Write auth config to app settings
captainsafia May 24, 2022
2675c50
Address more feedback
captainsafia May 24, 2022
c37a2a6
:seal:
captainsafia May 24, 2022
212b42f
Apply suggestions from code review
captainsafia May 24, 2022
ef45270
Address more feedback
captainsafia May 25, 2022
751c1d7
Add framework support for authentication changes
captainsafia May 25, 2022
6562366
Add tests for user-jwts CLI and react to feedback
captainsafia May 27, 2022
672fb64
Move ConsoleTable implementation to avoid conflicts in ProjectTemplates
captainsafia May 27, 2022
bd19796
Update existing auth tests and fix middleware registration
captainsafia May 27, 2022
1f3a990
Update AzureAdB2C tests and auth app builder
captainsafia May 31, 2022
23e9b0c
Fix build and move registration check
captainsafia May 31, 2022
f0aa386
Fix up resolution for Certificate test sources
captainsafia May 31, 2022
98b504f
Fix write stream configuration for writing key material
captainsafia May 31, 2022
50a3cda
Fix handling missing config section when processing options
captainsafia May 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...ureAD/Authentication.AzureADB2C.UI/test/AzureAdB2CAuthenticationBuilderExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.AspNetCore.Authentication.OpenIdConnect;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Abstractions;
Expand Down Expand Up @@ -262,6 +263,7 @@ public void AddAzureADB2CBearer_AddsAllAuthenticationHandlers()
// Arrange
var services = new ServiceCollection();
services.AddSingleton<ILoggerFactory>(new NullLoggerFactory());
services.AddSingleton<IConfiguration>(new ConfigurationManager());

// Act
services.AddAuthentication()
Expand All @@ -279,6 +281,7 @@ public void AddAzureADB2CBearer_ConfiguresAllOptions()
// Arrange
var services = new ServiceCollection();
services.AddSingleton<ILoggerFactory>(new NullLoggerFactory());
services.AddSingleton<IConfiguration>(new ConfigurationManager());

// Act
services.AddAuthentication()
Expand Down Expand Up @@ -315,6 +318,7 @@ public void AddAzureADB2CBearer_CanOverrideJwtBearerOptionsConfiguration()
// Arrange
var services = new ServiceCollection();
services.AddSingleton<ILoggerFactory>(new NullLoggerFactory());
services.AddSingleton<IConfiguration>(new ConfigurationManager());

// Act
services.AddAuthentication()
Expand Down Expand Up @@ -348,6 +352,7 @@ public void AddAzureADB2CBearer_RegisteringJwtBearerHasNoImpactOnAzureAAExtensio
// Arrange
var services = new ServiceCollection();
services.AddSingleton<ILoggerFactory>(new NullLoggerFactory());
services.AddSingleton<IConfiguration>(new ConfigurationManager());

// Act
services.AddAuthentication()
Expand Down
4 changes: 4 additions & 0 deletions src/DefaultBuilder/src/Microsoft.AspNetCore.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,8 @@
<Reference Include="Microsoft.Extensions.Logging.EventSource" />
</ItemGroup>

<ItemGroup>
<InternalsVisibleTo Include="Microsoft.AspNetCore.Authentication.Test" />
</ItemGroup>

</Project>
7 changes: 2 additions & 5 deletions src/DefaultBuilder/src/WebApplicationBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,8 @@ private void ConfigureApplication(WebHostBuilderContext context, IApplicationBui

if (_webAuthBuilder.IsAuthenticationConfigured)
{
if (!_builtApplication.Properties.ContainsKey(AuthenticationMiddlewareSetKey))
{
app.UseAuthentication();
app.UseAuthorization();
}
_builtApplication.UseAuthentication();
_builtApplication.UseAuthorization();
}

// Wire the source pipeline to run in the destination pipeline
Expand Down
12 changes: 9 additions & 3 deletions src/Security/Authentication/Core/src/AuthAppBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace Microsoft.AspNetCore.Builder;
/// </summary>
public static class AuthAppBuilderExtensions
{
private const string AuthenticationMiddlewareSetKey = "__AuthenticationMiddlewareSet";
internal const string AuthenticationMiddlewareSetKey = "__AuthenticationMiddlewareSet";

/// <summary>
/// Adds the <see cref="AuthenticationMiddleware"/> to the specified <see cref="IApplicationBuilder"/>, which enables authentication capabilities.
Expand All @@ -24,7 +24,13 @@ public static IApplicationBuilder UseAuthentication(this IApplicationBuilder app
throw new ArgumentNullException(nameof(app));
}

app.Properties[AuthenticationMiddlewareSetKey] = true;
return app.UseMiddleware<AuthenticationMiddleware>();
// Don't add more than one instance of the middleware
if (!app.Properties.ContainsKey(AuthenticationMiddlewareSetKey))
{
app.Properties[AuthenticationMiddlewareSetKey] = true;
return app.UseMiddleware<AuthenticationMiddleware>();
}
Copy link
Member

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.

Copy link
Member

@Tratcher Tratcher May 31, 2022 •
edited
Loading

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.

Copy link
Member

@davidfowl davidfowl May 31, 2022 •
edited
Loading

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.

Copy link
Member

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?

Copy link
Member

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

Copy link
Member

@davidfowl davidfowl May 31, 2022 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.


return app;
}
}
19 changes: 19 additions & 0 deletions src/Security/Authentication/test/AuthenticationMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,25 @@ public async Task IAuthenticateResultFeature_SettingResultSetsUser()
Assert.Same(context.User, newTicket.Principal);
}

[Fact]
public async Task WebApplicationBuilder_RegistersAuthenticationMiddlewares()
{
var builder = WebApplication.CreateBuilder();
builder.Authentication.AddJwtBearer();
await using var app = builder.Build();

var webAppAuthBuilder = Assert.IsType<WebApplicationAuthenticationBuilder>(builder.Authentication);
Assert.True(webAppAuthBuilder.IsAuthenticationConfigured);

// Authentication middleware isn't registered until application
// is built on startup
Assert.False(app.Properties.ContainsKey("__AuthenticationMiddlewareSet"));

await app.StartAsync();

Assert.True(app.Properties.ContainsKey("__AuthenticationMiddlewareSet"));
}

private HttpContext GetHttpContext(
Action<IServiceCollection> registerServices = null,
IAuthenticationService authenticationService = null)
Expand Down
1 change: 1 addition & 0 deletions src/Security/Authentication/test/Microsoft.AspNetCore.Authentication.Test.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
</ItemGroup>

<ItemGroup>
<Reference Include="Microsoft.AspNetCore" />
<Reference Include="Microsoft.AspNetCore.Authentication.Certificate" />
<Reference Include="Microsoft.AspNetCore.Authentication.Cookies" />
<Reference Include="Microsoft.AspNetCore.Authentication.Facebook" />
Expand Down