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

Sending JWT headers to user_loader function #37

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rythos42
Copy link

Trying to fix #36 .

I moved the get_header out of _decode_jwt_token and into authenticate, so I could use the token a second time. Then called jwt.get_unverified_header, documented here https://pyjwt.readthedocs.io/en/latest/usage.html#reading-headers-without-validation.

I send the headers as a second parameter to user_loader, and to maintain backwards compatibility I catch the TypeError and call the old version of user_loader in the catch block.

No tests because I haven't figured out Python unit testing yet. >.> (I'm primarily a C# developer...) Let me know if you think they need to be added, I'll figure them out soon enough for my project.

@coveralls
Copy link

coveralls commented Dec 20, 2019

Coverage Status

Coverage decreased (-0.9%) to 89.96% when pulling caa8d18 on rythos42:master into 718fb0a on loanzen:master.

@rythos42
Copy link
Author

Added a new parameter to JWTAuthBackend to take a key discovery URL. This URL returns a JSON array of public keys. Each key is iterated over until one is found that matches the kid (key id) of the incoming JWT, and that key is used to verify the incoming JWT.

This scheme is used by Azure authentication.

Because this scheme is still a "JWT auth", I think it belongs in this class. But the class has a hard dependency on being given a string public key in the constructor, which isn't needed for the key discovery workflow. I decided not to remove this parameter because I'm not sure I want to be responsible for asking for breaking changes. :) But done properly, I'd do some refactoring to make it or key_discovery_url required, but not both.

@rythos42
Copy link
Author

It appears the travis-ci builds are failing because Python 2.0 and 3.3 don't exist there?

I'll look into increasing the test coverage for my PR...

@rythos42
Copy link
Author

I wrote a test that I feel covers what I need it to, but I'm struggling with getting the two checks to pass. Could a more experienced developer provide advice? I'm not sure what's wrong with Python 2.7 in the travis-ci build, and I feel that trying to get another 0.9% test coverage in my changes will lead me in a direction where I'm testing that urllib works? Thanks!

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.

JWT headers not accessible
3 participants