Add dotnet user-jwts tool and runtime support (#41520) #41956
Conversation
* 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>
captainsafia
requested review from
Tratcher,
BrennanConroy,
halter73,
a team,
dougbu,
wtgodbe and
Pilchie
as code owners
captainsafia
added
Servicing-consider
|
Hi @captainsafia. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge. To learn more about how to prepare a servicing PR click here. |
| ValidIssuers = new[] { issuer }, | ||
| ValidateAudience = audiences.Length > 0, | ||
| ValidAudiences = audiences, |
There was a problem hiding this comment.
How come one issuer but multiple audiences when the properties are both arrays?
There was a problem hiding this comment.
Agreed. We can address that in next preview.
There was a problem hiding this comment.
@martincostello we should capture all these suggestions in s new issue. We're up against it for preview.5 so this has to go in as-is.
| internal sealed class CreateCommand | ||
| { | ||
| private static readonly string[] _dateTimeFormats = new[] { | ||
| "yyyy-MM-dd", "yyyy-MM-dd HH:mm", "yyyy/MM/dd", "yyyy/MM/dd HH:mm" }; |
There was a problem hiding this comment.
Would accepting the "O" format be useful so people could paste in strings copied from something that outputs them in ISO-8601 format?
There was a problem hiding this comment.
Possibly? Can you log a separate issue to capture that suggestion please?
|
|
||
| var issuerOption = cmd.Option( | ||
| "--issuer", | ||
| "The issuer of the JWT. Defaults to the dotnet-user-jwts", |
There was a problem hiding this comment.
Typo or cut-off sentence?
| "The issuer of the JWT. Defaults to the dotnet-user-jwts", | |
| "The issuer of the JWT. Defaults to 'dotnet-user-jwts'.", |
|
|
||
| var schemeNameOption = cmd.Option( | ||
| "--scheme", | ||
| "The scheme name to use for the generated token. Defaults to 'Bearer'", |
There was a problem hiding this comment.
| "The scheme name to use for the generated token. Defaults to 'Bearer'", | |
| "The scheme name to use for the generated token. Defaults to 'Bearer'.", |
|
|
||
| var audienceOption = cmd.Option( | ||
| "--audience", | ||
| "The audiences to create the JWT for. Defaults to the URLs configured in the project's launchSettings.json", |
There was a problem hiding this comment.
| "The audiences to create the JWT for. Defaults to the URLs configured in the project's launchSettings.json", | |
| "The audiences to create the JWT for. Defaults to the URLs configured in the project's launchSettings.json.", |
|
|
||
| var notBeforeOption = cmd.Option( | ||
| "--not-before", | ||
| @"The UTC date & time the JWT should not be valid before in the format 'yyyy-MM-dd [[HH:mm[[:ss]]]]'. Defaults to the date & time the JWT is created", |
There was a problem hiding this comment.
| @"The UTC date & time the JWT should not be valid before in the format 'yyyy-MM-dd [[HH:mm[[:ss]]]]'. Defaults to the date & time the JWT is created", | |
| @"The UTC date & time the JWT should not be valid before in the format 'yyyy-MM-dd [[HH:mm[[:ss]]]]'. Defaults to the date & time the JWT is created.", |
There was a problem hiding this comment.
This one still seems to have the overly complex datetime format description still. We can clean these things up in preview.6
| { | ||
| if (!force) | ||
| { | ||
| reporter.Output("Are you sure you want to reset the JWT signing key? This will invalidate all JWTs previously issued for this project.\n [Y]es / [N]o"); |
There was a problem hiding this comment.
| reporter.Output("Are you sure you want to reset the JWT signing key? This will invalidate all JWTs previously issued for this project.\n [Y]es / [N]o"); | |
| reporter.Output($"Are you sure you want to reset the JWT signing key? This will invalidate all JWTs previously issued for this project.{Environment.NewLine} [Y]es / [N]o"); |
| } | ||
| var jwtStore = new JwtStore(userSecretsId); | ||
|
|
||
| if (!jwtStore.Jwts.ContainsKey(id)) |
There was a problem hiding this comment.
Use TryGetValue()? Then you don't need to use the indexer on line 53.
| userSecretsId = null; | ||
| if (project == null) | ||
| { | ||
| reporter.Error($"No project found at `-p|--project` path or current directory."); |
There was a problem hiding this comment.
Would this message be a bit misleading if more than one *.proj file was found for some reason?
| var value = applicationUrl.GetString(); | ||
| if (value is { } applicationUrls) | ||
| { | ||
| return applicationUrls.Split(";"); |
There was a problem hiding this comment.
| return applicationUrls.Split(";"); | |
| return applicationUrls.Split(';'); |
| { | ||
| public static void Register(ProjectCommandLineApplication app) | ||
| { | ||
| app.Command("delete", cmd => |
There was a problem hiding this comment.
We use 'remove' in other places in CLI tools, should we be consistent?
There was a problem hiding this comment.
Yep fair. We can make that change in preview.6
| userSecretsId = GetUserSecretsId(project); | ||
| if (userSecretsId == null) | ||
| { | ||
| reporter.Error($"Project does not contain a user secrets ID."); |
There was a problem hiding this comment.
Recommend adding instructions on how to resolve?
|
|
||
| if (existingUserSecretsId == null) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
I've always disliked this about user-secrets -- I've told the tool my intent, if you can't find the secrets key, just create one for me, don't make me extra hop to 'init' it. Let's do that here...if not found, create a new secrets id, add it to the project, output that created one as info ('No secrets file found, created with id {}')
There was a problem hiding this comment.
Rather than gate the check-in on these suggestions, I think we should log them in a separate issue that can be addressed in preview.6
|
Approved via email. |
captainsafia
added
Servicing-approved
|
@dotnet/aspnet-build Can I get help merging? |
Description
This PR introduces a
dotnet user-jwtsCLI tool for generating JSON Web Tokens for use in local development and runtime changes in ASP.NET Core to support reading options for configuring authentication from config.Fixes #39857
Customer Impact
The changes introduced in this changeset make it easier for user to configure JWT bearer-based authentication in their applications for local development by:
Regression?
Risk
To minimize the risk associated with this change, we've:
WebApplicationBuilderVerification
Packaging changes reviewed?