Skip to content

Conversation

sourcevelocity-andy
Copy link
Contributor

The constructor for ValidationParameters was private, which produces compile errors for the examples on the main page and prevents them from being used (https://github.com/jwt-dotnet/jwt#turning-off-parts-of-token-validation, https://github.com/jwt-dotnet/jwt#or-using-the-fluent-builder-api-4). Making the constructor public makes them valid. It's possible to use the "With" method and the "Default" or "None" properties, but it is cumbersome.

…compile errors for the examples on the main page and prevents them from being used (https://github.com/jwt-dotnet/jwt#turning-off-parts-of-token-validation, https://github.com/jwt-dotnet/jwt#or-using-the-fluent-builder-api-4). Making the constructor public makes them valid. It's possible to use the "With" method and the "Default" or "None" properties, but it is cumbersome.
@abatishchev
Copy link
Member

Hi! Tanks for not just reporting the issue but offering to fix it right away. I appreciate it.

Would it be better though to update the sample rather than the code?

The ctor was intentionally made private so the fluent style code like this would be used instead:

ValidationParameters.None
                    .With(p => p.ValidateSignature = true)
                    .With!p => p.TimeMargin = 100);

Whether it's cumbersome, I think it's a matter of taste. I'm not defending it, as it was a contribution by another user.

But what I admit that it's pretty inefficient, creates a bunch of allocations for very little benefits.

Thinking more about it, I now recollect that the main reason to make it this was that new ValidatingParameters() would disable all validation, i.e. would make the defaults very insecure.

Can you please make few more changes:

  1. Change the default values of all 3 boolean properties to true
  2. Add a couple of simple tests, if there aren't some already
  3. Bump the version inside JWT.csproj, the third segment should be fine, I guess
  4. Update the changelog

@sourcevelocity-andy
Copy link
Contributor Author

Will do! That makes sense to me! Thanks for the information as well, that makes more sense now. I appreciate your consideration.

…, also added tests to ensure that they are being set and updated correctly
@sourcevelocity-andy
Copy link
Contributor Author

Done and done :). Thank you for allowing me to contribute to your project. I made the requested changes. I'm happy to make any other changes if you see anything that you'd like to be different.

@abatishchev
Copy link
Member

Awesome! I took the liberty to make a couple of cosmetic changes. Approved, will merge once the PR validation finishes. Thanks for the contribution! Please do more :)

@abatishchev abatishchev changed the title Allow ValidationParameters to be instantiated so that example code will compile Made ctor of ValidationParameters public Sep 27, 2023
@abatishchev abatishchev merged commit bc9d9d2 into jwt-dotnet:main Sep 27, 2023
@abatishchev
Copy link
Member

abatishchev commented Sep 27, 2023

Published to NuGet as 10.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants