Skip to content
This repository has been archived by the owner on May 26, 2020. It is now read-only.

Added a JWT verification view #75

Merged
merged 4 commits into from Mar 1, 2015

Conversation

Jwpe
Copy link
Contributor

@Jwpe Jwpe commented Feb 5, 2015

This PR adds a verification view: verify_jwt_token, which checks if a JWT POSTed to it is valid and that the user exists.

Much of the functionality is the same as the existing refresh view and serializer. I abstracted out the majority of the shared serializer logic to maximise DRYness. There are several other ways this could be implemented, however.

I added tests for the new view and moved a test which was more relevant to this functionality from the refresh view tests.

Suggestions for improvements welcome! If the serializer implementation is ok, it could probably use some unit tests.

@Jwpe Jwpe mentioned this pull request Feb 5, 2015
@Jwpe
Copy link
Contributor Author

Jwpe commented Feb 5, 2015

Also, a lot of the logic in the base serializer is the same as in the JSONWebTokenAuthentication class - apart from the errors raised. I wonder if it would be possible/desirable to abstract those out into shared functions.

@jpadilla
Copy link
Owner

jpadilla commented Feb 7, 2015

@Jwpe thanks, checking this out. I think someone tried abstracting that logic from the JSONWebTokenAuthentication a while back. I wouldn't mind seeing it done. There might also be some logic in JSONWebTokenAuthentication that can be shared as well.

@jpadilla
Copy link
Owner

@Jwpe just to be clear, were you gonna tackle the base API view in this PR?

@Jwpe
Copy link
Contributor Author

Jwpe commented Feb 20, 2015

@jpadilla happy to do so, but I can also make a separate PR if you'd rather split it up.

@jpadilla
Copy link
Owner

@Jwpe seems right to me if it were part of this one.

…on serializer. Added a verification view. Added verification tests.
@Jwpe
Copy link
Contributor Author

Jwpe commented Feb 22, 2015

@jpadilla refactored to add the base class. Rebased on the latest master - I can squash the commits if you like but otherwise this should be good to go.

@Jwpe
Copy link
Contributor Author

Jwpe commented Feb 22, 2015

@jpadilla fixed!

@jpadilla
Copy link
Owner

@Jwpe this looks perfect, thanks again for the great work! Only thing missing is adding a bit on it to the docs since we are adding a new feature here.

@Jwpe
Copy link
Contributor Author

Jwpe commented Mar 1, 2015

@jpadilla added a section to the documentation. Let me know what you think!

jpadilla added a commit that referenced this pull request Mar 1, 2015
@jpadilla jpadilla merged commit faa772c into jpadilla:master Mar 1, 2015
@jpadilla
Copy link
Owner

jpadilla commented Mar 1, 2015

@Jwpe looks great, thanks again!

@Jwpe
Copy link
Contributor Author

Jwpe commented Mar 2, 2015

@jpadilla no worries! Thanks for making an awesome library.

@jpadilla jpadilla modified the milestone: 1.3.0 Release Mar 6, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants