Skip to content

Add option to provide current time when decoding JWT - #267

Open
ZipFile wants to merge 1 commit into
mpdavis:masterfrom
ZipFile:current-time-provisioning
Open

Add option to provide current time when decoding JWT#267
ZipFile wants to merge 1 commit into
mpdavis:masterfrom
ZipFile:current-time-provisioning

Conversation

@ZipFile

Copy link
Copy Markdown
  • Makes possible to fake a time without using freezegun:freeze_time or unittest.mock:patch. Less monkey patching == better code.
  • Makes possible to reuse current time fetched elsewhere (e.g. from request).

@codecov

codecovBot commented Jul 4, 2021

Copy link
Copy Markdown

Codecov Report

Merging #267 (7a66496) into master (be8e914) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 7a66496 differs from pull request most recent head 9667b78. Consider uploading reports for the commit 9667b78 to get more accurate results
Impacted file tree graph

@@ Coverage Diff @@## master #267 +/- ##
=======================================
Coverage 92.94% 92.94% =======================================
Files 15 15 Lines 1418 1418 =======================================
Hits 1318 1318 Misses 100 100 
Impacted FilesCoverage Δ
jose/jwt.py98.01% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be8e914...9667b78. Read the comment docs.

@blagblag left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dependency injection to ease testing? 👍
Dependency injection to improve usability? 👍
Code churn for no discernible reason? 👎

If you have a good reason for prepending the now parameter like you did, I'll definitely consider it, but I haven't seen a good reason for it yet. Please point me to it if I missed it. 😅

Comment threadjose/jwt.py Outdated
Comment threadjose/jwt.py Outdated


def _validate_claims(claims, audience=None, issuer=None, subject=None, algorithm=None, access_token=None, options=None):
def _validate_claims(now, claims, audience=None, issuer=None, subject=None, algorithm=None, access_token=None, options=None):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why change the function signature? Wouldn't it be easier to append now=None to the end of the parameter list and keep the function signature the same? And then use

now=nowordatetime.utcnow()

to default to datetime.utcnow(), just in case anybody is using these private methods.

I realize that they're private methods, and people are signing up to keep their code up-to-date when they use these private methods, but there's no point in code churn without a good reason.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It was not part of the public API (this particular function does not even have docstring to serve as a documentation), so there was no hesitation to change the signature. Making now (required) positional argument better states the intention to me.

Either way, updated.

@ZipFile
ZipFileforce-pushed the current-time-provisioning branch from 7a66496 to 9667b78CompareJuly 5, 2021 06:19
@ZipFile
ZipFile requested a review from blagJuly 6, 2021 06:37
@jojolebarjos

Copy link
Copy Markdown

Hi folks, this feature would be very welcome, any chance this gets accepted?

@ZipFile
ZipFileforce-pushed the current-time-provisioning branch from 9667b78 to 8fd0ed9CompareMay 31, 2025 09:52
@ZipFile

Copy link
Copy Markdown
Author

Rebased with latest master, just in case.

Sign up for freeto 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.

3 participants

@ZipFile@jojolebarjos@blag