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

Add decode timezone support #516

Closed
wants to merge 3 commits into from

Conversation

guyzyl
Copy link

@guyzyl guyzyl commented Sep 5, 2020

Added timezone support to options dictionary.
The change is needed since the expiry of the tokens are validated against the machines local timezone, this new option enables explicitly choosing and setting the timezone used for decoding and validations.
I made to sure to document the new option + wrote a test for it.
This solves issue #487 .

@jpadilla
Copy link
Owner

jpadilla commented Sep 6, 2020

@auvipy we need to think through this one. what does the spec say? what are other libraries doing? not sure this is the way forward.

@auvipy
Copy link
Collaborator

auvipy commented Sep 6, 2020

yup, that's why not merging but waiting for your thoughts.

@guyzyl
Copy link
Author

guyzyl commented Oct 2, 2020

@auvipy would it be possible to merge it now?

Copy link
Owner

@jpadilla jpadilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guyzyl I think this shouldn't be a thing that can be specified, it should always be UTC according to the spec: https://tools.ietf.org/html/rfc7519

NumericDate
A JSON numeric value representing the number of seconds from
1970-01-01T00:00:00Z UTC until the specified UTC date/time,
ignoring leap seconds. This is equivalent to the IEEE Std 1003.1,
2013 Edition [POSIX.1] definition "Seconds Since the Epoch", in
which each day is accounted for by exactly 86400 seconds, other
than that non-integer values can be represented. See RFC 3339
[RFC3339] for details regarding date/times in general and UTC in
particular.

Also, can this be done without introducing a dependency on pytz?

Copy link
Author

@guyzyl guyzyl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@guyzyl
Copy link
Author

guyzyl commented Oct 10, 2020

If it shouldn't be specified then it's possible to close this PR + ticket.
Regarding pytz, it's possible to drop it as a dependency, will do that if we decide we want to merge the feature.

@guyzyl
Copy link
Author

guyzyl commented Oct 17, 2020

@jpadilla Should I close the PR or fix it?

@guyzyl
Copy link
Author

guyzyl commented Sep 5, 2021

@jpadilla Should I fix it (as per you comment), or close the PR?

@jpadilla
Copy link
Owner

jpadilla commented Sep 5, 2021

@guyzyl sorry for the delay. i don't think this is a change/feature we should introduce.

@guyzyl
Copy link
Author

guyzyl commented Sep 5, 2021

@jpadilla no worries, makes total sense.

@guyzyl guyzyl closed this Sep 5, 2021
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 this pull request may close these issues.

None yet

4 participants