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 JWTPayload(dict) for extended verification #322

Closed
wants to merge 1 commit into from

Conversation

sirosen
Copy link
Contributor

@sirosen sirosen commented Jan 10, 2018

The JWTPayload class allows PyJWT.decode() to expose header, signature, signing_input, and compute_hash_digest() (based on header) without changing the pyjwt API in a breaking way.
Merely making this info accessible to the client (without specify an additional verification callback scheme) is simpler for everyone.

The intent is to make the JWT payload change as little as possible while still making it easy to add more verification after the fact.
More exposition on the options, and why I chose this, visible in #314 and the docstrings.

Add a simple test for JWTPayload.compute_hash_digest()

Closes #314, #295

I'm doing this primarily to satisfy a real need in my application to resolve #295 in a way that isn't a hack.

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling b8d257a on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling b8d257a on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling 2e8c10e on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling 2e8c10e on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling 2e8c10e on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling 87370b4 on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 10, 2018

Coverage Status

Coverage decreased (-0.4%) to 99.57% when pulling 87370b4 on sirosen:jwt-payload-object into 0c80a71 on jpadilla:master.

@coveralls
Copy link

coveralls commented Jan 10, 2018

Pull Request Test Coverage Report for Build 201

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 200: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

@sirosen
Copy link
Contributor Author

sirosen commented Mar 23, 2018

@mark-adams, I'm wondering if I could impose upon you to review this?

If you'd much prefer the alternative path, in which callbacks are passed to decode(), that's fine and I'll write that instead. IMO it will be more work to maintain, but pyjwt isn't mine to maintain, so I don't mean to impose my opinion on the project.

Apologies if this seems pushy/impatient -- I just want some solution so that I can clean out some items from my backlog. It's not a Big Deal ™️ .

Copy link
Collaborator

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

would you mind rebasing the PR?

The JWTPayload class allows PyJWT.decode() to expose header, signature,
signing_input, and compute_hash_digest() (based on header) without
changing the pyjwt API in a breaking way.
Merely making this info accessible to the client without specifying an
additional verification callback scheme is simpler for everyone.

Include doc on why JWTPayload is a good idea in a module docstring,
since it's a little unusual to subclass `dict`. The intent is to make
the JWT payload change as little as possible while still making it easy
to add more verification after the fact.

Add a simple test for `JWTPayload.compute_hash_digest()` and a test
for compute_hash_digest with cryptography (which is compared against a
manual hashlib usage).

Closes jpadilla#314, jpadilla#295
@sirosen
Copy link
Contributor Author

sirosen commented May 14, 2020

Just rebased. All tests passed on my local tox except for py38-contrib_crypto, which seems to be failing on master on Travis too.

@sirosen
Copy link
Contributor Author

sirosen commented May 14, 2020

I just started poking around here for the first time in a long time because the build system is clearly in a bad state. Looks like the whole GitHub Actions setup is stale:

pip install -r requirements.txt
(there is no such requirements file anymore)

However, that led me to notice that pyjwt is dropping py2.7 support. Is the next release going to be v2.0? I don't recall much about this change, but I know for sure that I only built it this way (subclassing dict, which is pretty weird) to remain backwards compatible.

I'd have to take more time to reread, but I think I'd remove the dict inheritance if I were planning on a change for 2.0. It's much nicer, as an interface, if it's a special-purpose object with whatever attributes are wanted. You can implement __getitem__ and __contains__ on it using the inner dict if you want, but I don't even know if that's necessary.

@auvipy auvipy self-assigned this May 14, 2020
@auvipy
Copy link
Collaborator

auvipy commented May 14, 2020

OK will look into it soon

@sirosen
Copy link
Contributor Author

sirosen commented Jan 20, 2021

Realistically, this is not going to happen. 2.0 would have been the appropriate time to convert the return value to a custom mapping type which could support additional methods or attributes.

It's fine that this isn't being taken as an approach, but it would have been nice to get some response on the issues I filed.

@sirosen sirosen closed this Jan 20, 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
3 participants