Skip to content
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

Merged
merged 5 commits into from Jun 13, 2023

Conversation

micahmo
Copy link

@micahmo micahmo commented Jun 8, 2023

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • New feature or enhancement
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

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 like ValidateSignature. 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 the TokenValidationParameters), 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:

  • Did you follow the code style guideline as described in CONTRIBUTING.md
  • Did you build the app and test your changes?
  • Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • Did you verify that the change work in Release build configuration
  • Did you verify that all unit tests pass
  • If necessary and if possible, did you verify your changes on:
    • Windows
    • macOS (DevToys 2.0)
    • Linux (DevToys 2.0)

@micahmo
Copy link
Author

micahmo commented Jun 8, 2023

@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.

Copy link
Collaborator

@btiteux btiteux left a 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;
Copy link
Collaborator

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.

Copy link
Author

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.

  1. The same could be said for old code too, so I have not introduced a new problem.
  2. 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.

image

(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.)

@veler
Copy link
Collaborator

veler commented Jun 11, 2023

Would it be possible to add unit tests for the changes in this PR? :)

@micahmo
Copy link
Author

micahmo commented Jun 12, 2023

Would it be possible to add unit tests for the changes in this PR? :)

Of course! d1740e4

@micahmo micahmo requested a review from veler June 12, 2023 03:47
@veler
Copy link
Collaborator

veler commented Jun 12, 2023

Awesome! Thank you so much ! :D
Looks like we have a small merge conflict now 😅 Should be good to go once resolved.

@micahmo
Copy link
Author

micahmo commented Jun 12, 2023

Looks like we have a small merge conflict now 😅 Should be good to go once resolved.

Should be all set now! Thanks!

@veler
Copy link
Collaborator

veler commented Jun 13, 2023

Thank you so much! :D

@veler veler merged commit bda2dce into DevToys-app:main Jun 13, 2023
1 check passed
@micahmo micahmo deleted the feature/validate-without-signature branch June 13, 2023 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants