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

Do not fail in JWT decode() if at_hash claim is missing #76

Merged
merged 2 commits into from
Nov 26, 2018

Conversation

leplatrem
Copy link
Contributor

Fixes #75

jose/jwt.py Outdated
@@ -418,12 +418,11 @@ def _validate_at_hash(claims, access_token, algorithm):
"""
if 'at_hash' not in claims and not access_token:
return
elif access_token and 'at_hash' not in claims:
Copy link
Owner

Choose a reason for hiding this comment

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

We can refactor this logic for simplification:

if 'at_hash' not in claims:
    return

if not access_token:
    msg = 'No access_token provided to compare against at_hash claim.'
    raise JWTClaimsError(msg)

@@ -418,12 +418,11 @@ def _validate_at_hash(claims, access_token, algorithm):
"""
Copy link
Owner

Choose a reason for hiding this comment

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

Nit: Can you update the docstring for this method to pull the explanation from the RFC, similar to the other validation methods?

    """
    Validates that the 'at_hash' is valid.
    
    Its value is the base64url encoding of the left-most half of the hash
    of the octets of the ASCII representation of the access_token value,
    where the hash algorithm used is the hash algorithm used in the alg
    Header Parameter of the ID Token's JOSE Header. For instance, if the
    alg is RS256, hash the access_token value with SHA-256, then take the
    left-most 128 bits and base64url encode them. The at_hash value is a
    case sensitive string.  Use of this claim is OPTIONAL.

    Args:
        claims (dict): The claims dictionary to validate.
        access_token (str): The access token returned by the OpenID Provider.
        algorithm (str): The algorithm used to sign the JWT, as specified by
            the token headers.
    """

@mpdavis
Copy link
Owner

mpdavis commented Dec 19, 2017

It looks like there are builds failing on Python 2.6 and 3.3.

Python 2.6 is failing because pytest dropped support. It looks like we will need to pin pytest in tox.ini for 2.6 builds (or possibly just all builds if easier).

I am still looking into the 3.3 failure. I can take a look later if you don't want to worry about it.

These are mostly notes for myself:

pytest removing support: https://docs.pytest.org/en/latest/changelog.html#pytest-3-3-0-2017-11-23

tox conditional settings: http://tox.readthedocs.io/en/latest/config.html#factors-and-factor-conditional-settings

@leplatrem
Copy link
Contributor Author

I also saw this:

$ tox

Matching undeclared envs is deprecated. Be sure all the envs that Tox should run are declared in the tox config.

@mpdavis mpdavis force-pushed the master branch 3 times, most recently from f993fa8 to 402c815 Compare January 16, 2018 04:45
@leplatrem leplatrem force-pushed the 75-at_hash-optional branch from be6b22d to a2cfd30 Compare February 2, 2018 09:11
@codecov-io
Copy link

Codecov Report

Merging #76 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   93.69%   93.67%   -0.03%     
==========================================
  Files          12       12              
  Lines         841      838       -3     
==========================================
- Hits          788      785       -3     
  Misses         53       53
Impacted Files Coverage Δ
jose/jwt.py 94.07% <100%> (-0.12%) ⬇️

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 28cc671...a2cfd30. Read the comment docs.

@leplatrem
Copy link
Contributor Author

@mpdavis This should be ready to land :)

@zejn
Copy link
Collaborator

zejn commented May 4, 2018

The Python 2.6 has been unsupported for a long time now and 3.3 is also unsupported since late 2017.

App engine has Python 2.7, so this looks good to me.

@Integralist
Copy link

What came of this? I've recently discovered this issue myself and would be keen to see a fix land. Thanks all for their work on this 👍

@techdragon
Copy link

Is this still problematic? Or can this just be merged, I've noticed this is the root cause of some issues in my deployed applications, I'd rather not work around such a simple, and already written, fix.

@rcjsuen
Copy link

rcjsuen commented Nov 21, 2018

I hope this can eventually land at some point. In the meantime, thank you @leplatrem for finding the issue and suggesting a fix!

@mpdavis
Copy link
Owner

mpdavis commented Nov 26, 2018

lgtm

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.

7 participants