Added token validation parameters

[nathanpovo]

Adds the option to pass validation parameters to the constructor of JwtValidator to turn off certain parts of the token validation.

The validation parameters can also be passed to the WithVerifySignature method to be used with the fluent API.

Fixes #331

[abatishchev]

Thanks for adding tests! Everyone like to have them and nobody likes to write them :) In long-run that's the single most beneficial change among any. Please add as much tests as possible, see if you can add any additional tests.

[abatishchev]

@nathanpovo sorry for coming late! I've made few changes? Can you please take a look?

Also I'm thinking TimeMargin must be a property on ValidationParameters. Because it's a validation parameter too.

Since this is your PR, I don't want to make too many changes myself. If you agree, can you please do that? The actor accepting timeMargin may just update to the value. Or we can remove it altogether, effectively making a breaking change and bump version to 9.0.0.

[nathanpovo]

Could these default values be added back? Creating a new ValidationParameters object with a single property value set to false (such as in the tests) without these default values would require a consumer to either define the value for each property:

var valParams = new ValidationParameters
{
    ValidateIssuerSigningKey = true,
    ValidateLifetime = true,
    ValidateIssuedTime = false
};

or use the default property ValidationParameters.Default and then manually set the value of the property you want to change:

var valParams = ValidationParameters.Default;
valParams.ValidateIssuedTime = false;

which seems a bit clunky.

[abatishchev]

Do we want to introduce Fluent API for ValidationParameters to? It can be simply a set of extensions methods:

public ValidationParameters NotValidateIssuedTime(this ValidationParameters  valParams) =>
    valParams.ValidateIssuedTime(false);

public ValidationParameters ValidateIssuedTime(this ValidationParameters  valParams, bool value)
{
valParams.ValidateIssuedTime = value;
return valParams;
}

[nathanpovo]

I think that adding a fluent api for the ValidationParameters is not needed since it's just a class with a bunch of properties; using the fluent api would end up needing more code and would increase maintenance if any additional parameters were added.

However, I do think it should be clear what the default property values are.

[abatishchev]

Hey @binki, I'm sorry once again! I'm getting overloaded at work, coming back to your PR as soon as get a spare moment.

I was thinking more about the default values and setting them all to true is unusual and counterintuitive. Should we have flip all the properties so they all would have false by default so just new would be identical to Default? Same story for margin which is 0 by default already.

Also maybe we should add the ctor accepting margin back? And make it pass the value to the ctor of parameters

[binki]

@abatishchev I’m sure you meant to mention @nathanpovo (though GitHub thinks you shouldn’t mention him because he is the one who reported this issue). However, I still have opinions. Having initialization on properties is not strange for classes. Changing the parameter names to allow default values to make sense (e.g., having DoNotValidateIssuerSigningKey implicitly initialized to false instead of ValidateIssuerSigningKey initialized to true) is only needed for structs.

I think it would be most bad to make it easy to accidentally turn validation off entirely. So I’d prefer the properties to be initialized to what are considered good defaults.

If you want people to use Default instead, then you should make the constructor private so that no one can accidentally instantiate the class directly. This way, they would be forced to choose None or Default instead of being able to accidentally use None by calling the constructor.

[nathanpovo]

@abatishchev, no worries! The feature isn't urgently needed, I'm just adding it to the library to help out and to make it a bit easier to use.

@binki, thanks for the valuable input. I agree that it's important to not make it easy to turn off the validation by accident; that's why I had set the property values to true so that the validation would get done by default. Setting the constructor to private seems like the easiest thing to do; that way we could leave the defaults of the properties to whatever the default of the type is and also prevent a user from creating validation parameters that will not validate. We could then use an extension method, as suggested by @binki, to easily change the values from the default.

[binki]

Thanks, I am glad to see discussion is happening!

I think I must clarify/condition my suggestion to use an extension method just to make sure we are understanding each other ;-). I do not think it would be appropriate to add the exact extension method (With<T>) I suggested to this library. That is more of a personal coding style/choice. I define that in my own projects because sometimes I want to be able to stuff related things into an expression. For example, I might use it to initialize XmlWriterSettings. It belongs in a utility class or package for use by those who subscribe to this coding style.

What would be more appropriate would probably be a “fluent” API as discussed earlier. This could be as simple as calling a callback (like with my With<T>, but just targeted to the ValidationParameters type instead of generic).

[abatishchev]

@binki sorry for involving you into the discussion outside your will :) and thanks for valuable input!
Totally agree that the most important aspect should be preventing accidentally turning off the validation, that would be disastrous.

[abatishchev]

@nathanpovo generic With<T> is too generic in my opinion. it's better to add few ValidationParameters-specific methods. This way they'll be scoped, discoverable, and readable. what you think?
Sorry for going back and forth on this!

[nathanpovo]

Yeah, a generic method wouldn't make sense in this case; I added an extension method just to modify ValidationParameters objects.

[nathanpovo]

@abatishchev, the library is yours; you know what best to change in it 😄

About TimeMargin being a validation parameter, I think it makes sense to include it in ValidationParameters. And removing the TimeMargin would make the constructor less confusing; all the validation parameters would be in the same place.

If you want to go ahead with the breaking change of throwing ArgumentNullException when creating a JwtValidator with null values then I think the we can remove the JwtValidator constructors with the TimeMargin. If not, the constructors could be marked as obsolete for now and then removed at a later date.

[binki]

I think you need to use a different name than With<T>(). Something like SetValidationParameters might work. I wonder if @abatishchev wants to more fully embrace the Fluent API style, adding a method per setting such as:

• EnableValidateIssuerSigningKey/DisableValidateIssuerSigningKey
• EnableValidateLifetime/DisableValidateLifetime
• etc.…

Could you specify a bit more clearly/fully what is acceptable, @abatishchev ? Thanks!

[nathanpovo]

I used With to make it synonymous with the with statements for records in C# 9.0 (which, sadly, cannot be used here). Maybe it could be changed to something like WithParameters, but the With method seems clear enough

var validationParameters = ValidationParameters.Default
    .With(parameters =>
    {
        parameters.ValidateLifetime = false;
        parameters.TimeMargin = 1;
    });

With regards to fully embracing the fluent api style, I think it would add additional maintanence without a lot of gain since the class is just a parameter class; any fluent methods would not have enough to do, they would just toggle the values of boolean parameters.

[abatishchev]

@nathanpovo my sincere apologizes that this PR took forever me to come back (high demanding job and burn out among other things).
In order to more easily work on the changes and get them shipped, I'm closing this PR off your branch and opening #376 off mine.

[abatishchev]

Released to nuget.org as JWT 9.0.0-beta2