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

Option to turn off or ignore certain token parameters during token validation #331

Closed
nathanpovo opened this issue May 13, 2021 · 7 comments
Assignees

Comments

@nathanpovo
Copy link

I wish to parse and validate some JWTs that have expired (to generate new tokens with a refresh token). However, when parsing a token using the following code:

string json = JwtBuilder.Create()
    .WithAlgorithm(new HMACSHA256Algorithm())
    .WithSecret(secret)
    .MustVerifySignature()
    .Decode<string>(token);

an exception is thrown if the token has expired.

To workaround the issue, I have to change the code to this:

IJsonSerializer serializer = new JsonNetSerializer();
UtcDateTimeProvider provider = new UtcDateTimeProvider();
IJwtValidator validator = new JwtValidator(serializer, provider);
IBase64UrlEncoder urlEncoder = new JwtBase64UrlEncoder();
IJwtAlgorithm algorithm = new HMACSHA256Algorithm();
JwtDecoder decoder = new JwtDecoder(serializer, validator, urlEncoder, algorithm);

JwtParts jwtParts = new JwtParts(token);

try
{
    decoder.Validate(jwtParts, secret);
}
catch (TokenExpiredException)
{
    // do nothing
}

string json = decoder.Decode(token);

which is quite uglier than the original fluent style code. The other option was to create a new validation class that inherits from IJwtValidator and remove the expiration validation, but this would create a lot of duplicate code.

Ideally, we would be able to pass options to the method .MustVerifySignature(), something like the following:

string json = JwtBuilder.Create()
    .WithAlgorithm(new HMACSHA256Algorithm())
    .WithSecret(secret)
    .MustVerifySignature(options =>
        options.TokenValidationParameters = new TokenValidationParameters
        {
            ValidateIssuerSigningKey = true,
            ValidateIssuer = false,
            ValidateAudience = false,
            ValidateLifetime = true,
            ClockSkew = TimeSpan.Zero
        }
    )
    .Decode<string>(token);

This would allow us to control the execution of certain parts of the validation class to suit the individual needs of the token validation.

Let me know what you think, and I'd be happy to create a PR to address this issue.

@abatishchev
Copy link
Member

Hi! Thanks for proposing a change and the willingness to make it. Appreciate that!

Let me understand the intend better. So you want to validate the signature but want to turn off the other aspects of the validation?

Right now you can only turn it on or off completely:

DoNotVerifySignature();
// or
WithVerifySignature(false);

@nathanpovo
Copy link
Author

Yes, I would like to turn off certain parts of the validation. In my case, I only need to turn off the expiration validation but, to make it as accessible as possible for any other users of the library, I think it would be ideal to be able to turn off each aspect of the validation.

@abatishchev
Copy link
Member

abatishchev commented May 13, 2021

Btw you don't necessarily need to switch from fluent to "classic" syntax to achieve the same:

var builder = JwtBuilder.Create()
                        .WithAlgorithm(new HMACSHA256Algorithm())
                        .WithSecret(secret)
                        .MustVerifySignature();

string json = null;
try
{
    json = builder.Decode<string>(token);
}
catch (TokenExpiredException)
{
    // do nothing
}

Just saying :)

If you'd like to submit a PR to enhance the library - it will be very much appreciated! I'll get it reviewed and published asap.
Please just don't forget to update the readme and bump the package version inside JWT.csproj. Thanks!

@nathanpovo
Copy link
Author

nathanpovo commented May 17, 2021

I have gone over the code a bit and, with the way the library is structured, I can see three ways to implement this feature:

  1. Modify the JwtValidator class and the IJwtValidator interface to something like:
public void Validate(string decodedPayload, IAsymmetricAlgorithm alg, byte[] bytesToSign,
    byte[] decodedSignature, ValidationParameters validationParameters)
{
    var ex = GetValidationException(alg, decodedPayload, bytesToSign, decodedSignature);

    if (validationParameters.ValidateSignature || validationParameters.ValidateIssuedTime)
    {
        if (ex is SignatureVerificationException)
        {
            throw ex;
        }
    }

    if (validationParameters.ValidateLifetime && ex is TokenExpiredException)
    {
        throw ex;
    }

    if (ex is not null)
    {
        if (ex is not SignatureVerificationException || ex is not TokenExpiredException)
        {
            throw ex;
        }
    }
}

This would require changes in the IJwtValidator, and IJwtDecoder interfaces. With the amount of overloads these interfaces have, implementing all the new methods could get a bit tedious for any consumers of the interfaces.

  1. Modify the Validate method in the JwtValidator class to call the method _jwtValidator.TryValidate instead of the method _jwtValidator.Validate and implement the logic above to check if an exception should be thrown. This would require changes to the IJwtDecoder interface to pass in the ValidationParameters class.
  2. Modify the WithVerifySignature method in the JwtBuilder class to include the logic above to check if any exception should be thrown after calling _decoder.Decode. This would not require any changes to any interface. However, this would require a try-catch block and all exceptions would still get thrown but would just be caught (might impact performance in certain situations).

Not sure which implementation you would prefer.

@abatishchev
Copy link
Member

Hey @nathanpovo, thanks for flushing out the options and sorry for a delayed response!
In general, it's not a good practice to adjust the interface in order to adjust its implementation. Agree?
In this particular case instead of passing the parameters to the method, we can pass them to the ctor. The call to which is controlled by library itself so won't be a breaking change. The existing ctor will pass the parameters marching the existing behavior, the newly added ctor will accept the parameters from the user, so will the builder.
Makes sense?

@nathanpovo
Copy link
Author

I agree that the interfaces should not be modified, that's why I wanted to see what you thought before working on it.

Modifying the constructor of the JwtValidator class was enough to add the requested behavior (and required much less work than implementing a bunch of interface methods).

I have opened a pull request to close this issue, let me know what you think.

@abatishchev
Copy link
Member

Resolved by #376.

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

Successfully merging a pull request may close this issue.

2 participants