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

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

Closed
ckasabula opened this issue Aug 22, 2024 · 5 comments · Fixed by #12
Closed

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

ckasabula opened this issue Aug 22, 2024 · 5 comments · Fixed by #12

Comments

@ckasabula
Copy link
Contributor

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
Copy link
Collaborator

rickdotnet commented Aug 23, 2024

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
Copy link
Collaborator

mtmk commented Aug 23, 2024

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
Copy link
Collaborator

rickdotnet commented Aug 23, 2024

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
Copy link
Contributor Author

We are interacting directly with the model.
image

@mtmk mtmk linked a pull request Aug 23, 2024 that will close this issue
@rickdotnet
Copy link
Collaborator

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

@mtmk mtmk closed this as completed in #12 Aug 27, 2024
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 a pull request may close this issue.

3 participants