-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Allow token validation without signature #848
Allow token validation without signature #848
Conversation
@veler Please tell me if I'm creating too much burden with my recent PRs! I know you're pushing towards 2.0 and a lot of this will have to be rewritten anyway. |
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.
Hello, @micahmo thank you for this improvement, no worries for the pull requests.
The changes are good for me.
{ | ||
SigningCredentials? signingCredentials = GetValidationCredentials(tokenParameters); | ||
validationParameters.ValidateIssuerSigningKey = decodeParameters.ValidateIssuerSigningKey; | ||
validationParameters.IssuerSigningKey = signingCredentials.Key; |
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 signingCredentials
is null, this will crash.
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 believe this is ok for two reasons.
- The same could be said for old code too, so I have not introduced a new problem.
- It won't "crash"; instead, the exception will get caught and propagated to the UI. In this case, it will tell the user that the key is null, so they must enter a key if they want to validate the token. Again, that is the same as the existing behavior.
(Technically the error is from GetValidationCredentials
, which should either throw or return a non-nullable. So I guess we could remove the nullable here to alleviate your concern. But again I was mostly refactoring the existing code.)
Would it be possible to add unit tests for the changes in this PR? :) |
Of course! d1740e4 |
Awesome! Thank you so much ! :D |
Should be all set now! Thanks! |
Thank you so much! :D |
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
It is impossible to "validate" various parts of a JWT without providing the signature / signing key.
What is the new behavior?
This PR separates signing key validation as a separate step from overall token validation. As a result, I can choose to validate only the expiration, for example, without providing a key, which is much more convenient.
DevToys-JWT-Validate.mp4
Other information
If we were starting from scratch, I think it would make sense for the overall validation operation to be called something like
ValidateToken
, and then it could have an option likeValidateSignature
. However, the latter term is already used for the whole operation (and refers to a persistent setting), so I didn't want to change that. Instead I introduded a new option,ValidateIssuerSigningKey
(which does align with theTokenValidationParameters
), and as a result,ValidateSignature
now has a slightly different meaning. I did update the UI strings to reflect this. Hopefully it's ok!Quality check
Before creating this PR: