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

Improve requiring claims #456

Closed

Conversation

dave-shawley
Copy link

Thanks for implementing and maintaining this library! This PR changes the processing of required claims so that arbitrary claims may be required by name. I needed to require that the issuer claim was present without requiring a specific value. There wasn't a easy way to do this, so I figured that I would add one.

I converted the required claim processing to look at any option starting with require_ and require that a claim named by the suffix is present in the payload. Hopefully that explanation makes some sense. If not, the code is pretty simple.

I also took a stab at fixing the testing against Pycryptodome. I can remove the test related commits if necessary.

@coveralls
Copy link

coveralls commented Nov 3, 2019

Pull Request Test Coverage Report for Build 115

  • 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 ?%

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

💛 - Coveralls

I moved the testing requirements into tox.ini for the variant testing.
I added ecdsa, pycryptodome, and tox to the dev extras so that you can
run the tests outside of tox as well.
Note that pycrypto does not work with python 3.8 since they removed
`time.clock` altogether.

pycrypto/pycrypto#283
@dave-shawley
Copy link
Author

@jpadilla
I didn't notice #280 which implements similar functionality in a different manner. Is that approach preferred over the one that I have taken in this PR?

I am also bewildered by the travis build failure ...

@dave-shawley
Copy link
Author

Looks like #280 was merged 👍

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.

2 participants