nats-io / jwt.net

JWT tokens signed using NKeys for Ed25519 for NATS .NET
Apache License 2.0
2 stars 5 forks source link

Make time based values on JwtClaimsData be temporal instead of longs #11

Closed ckasabula closed 3 weeks ago

ckasabula commented 3 weeks ago

Proposed change

RE: The time based values on JwtClaimsData, i.e. IssuedAt/Expires/NotBefore. Could those be DateTimes? Or TimeSpans (offsets from now). I had to look at comments in the tests to see those values are unix timestamps (seconds). Expires = 1735689600, // 2025-01-01 The older syntax JwtUtils.IssueUserJwt took a nullable TimeSpan. It would be better if native c# temporal types were used.

Use case

Use c# native, temporal types for JwtClaimsData.

Contribution

No response

rickdotnet commented 3 weeks ago

Are you wanting to interact with the model directly? Or, is there a method you're expecting to pass a DateTimeOffset to? The model appears to represent the types properly.

Would adding something like this to the model work? Not sure what rules are in place for the models, but spit-balling until @mtmk has a look. At minimum I think we could update the comment to mention the unix timestamp.

[JsonIgnore]
public DateTimeOffset IssuedAtDateTime
{
    get => DateTimeOffset.FromUnixTimeSeconds(IssuedAt);
    set => IssuedAt = value.ToUnixTimeSeconds();
}

[JsonIgnore]
public DateTimeOffset ExpiresDateTime
{
    get => DateTimeOffset.FromUnixTimeSeconds(Expires);
    set => Expires = value.ToUnixTimeSeconds();
}

[JsonIgnore]
public DateTimeOffset NotBeforeDateTime
{
    get => DateTimeOffset.FromUnixTimeSeconds(NotBefore);
    set => NotBefore = value.ToUnixTimeSeconds();
}
mtmk commented 3 weeks ago

ah ok. didn't think of that, I was thinking we'd implement converters and change the types on the model to DateTimeOffset but his approach might be preferable if people want to be also able set the fields directly since that is the JWT standard at the end of the day.

rickdotnet commented 3 weeks ago

I was thinking we'd implement converters and change the types on the model to DateTimeOffset

Your approach is preferable, imo. It's cleaner and sets a better precedent if conversions become a common theme. I generally have an abstraction layer between third party models. Wouldn't impact me either way.

ckasabula commented 3 weeks ago

We are interacting directly with the model. image

rickdotnet commented 3 weeks ago

Fix should be in soon. Extension methods can be your friend in that situation as well.

// datetimeoffset
uc.SetExpired(expires)

public static void SetExpired(this JwtClaimsData claimsData, DateTimeOffset expires)
    => claimsData.Expires = expires.ToUnixTimeSeconds();