-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Fix up user-jwts interactions #42125
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
64d2c70
to
060ba2e
Compare
Is this just a copy/pasta error (all on the same line):
|
Can we have it so that the values for |
We can do this in a later change, but it would be nice if we left-padded the values so they all left-aligned when printing the details, e.g.:
|
Ish. Found this bug and fixed it after running through the CLI scenario.
Doable!
Sure. |
Co-authored-by: Damian Edwards <damian@damianedwards.com>
Co-authored-by: Damian Edwards <damian@damianedwards.com>
Co-authored-by: Damian Edwards <damian@damianedwards.com>
@BrennanConroy Can I get a review on this? |
@@ -147,10 +156,14 @@ public static void Register(ProjectCommandLineApplication app) | |||
reporter.Error(Resources.FormatCreateCommand_InvalidPeriod_Error("--valid-for")); | |||
} | |||
expiresOn = notBefore.Add(validForValue); | |||
optionsString += $"{Resources.JwtPrint_ExpiresOn}: {expiresOn:O}{Environment.NewLine}"; |
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.
expiresOnOption and validForOption conflict, is there a warning/error for this? Should we only print one of them?
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.
Good call! I think we probably want to throw an error and treat the input arguments as invalid in this case.
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, I think that's what I did in the original prototype, you have to specify either/or.
reporter.Output($"{Resources.JwtPrint_TokenPayload}: {fullToken.Payload.SerializeToJson()}"); | ||
} | ||
|
||
var tokenValueFieldName = showAll ? Resources.JwtPrint_CompactToken : Resources.JwtPrint_Token; |
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.
Is this backward?
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.
It's intentional. When the user provides the showAll
flag, we'll print the full token with the header and payload, when that happens we're printing "Compact" token to make it clear that the value is the encoded token. Same doesn't need to be done if we never print the full token.
Co-authored-by: Brennan <brecon@microsoft.com>
@@ -42,11 +42,11 @@ private static int Execute(IReporter reporter, string projectPath, bool showToke | |||
if (jwtStore.Jwts is { Count: > 0 } jwts) | |||
{ | |||
var table = new ConsoleTable(reporter); | |||
table.AddColumns("Id", "Scheme Name", "Audience", "Issued", "Expires"); | |||
table.AddColumns(Resources.JwtPrint_Id, Resources.JwtPrint_Scheme, Resources.JwtPrint_Audiences, Resources.JwtPrint_IssuedOn, Resources.JwtPrint_ExpiresOn); |
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.
Idk if you saw my question, should audience and name both be here? I think the issue mentioned including both.
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.
Yeah, so the issue refers to showing the name in the print
command. For list
, I figured it would make sense to limit it to the properties that impact the JWT's behavior at runtime. Also, I wanted to be a little cautious about having too many columns in the table since our ConsoleTable
implementation is pretty rudimentary.
Closes #42113
Closes #41973