Skip to content

Conversation

@carlegbert
Copy link
Contributor

Here's the jwt_optional decorator we discussed on IRC earlier today. I ended up copying most/all of the test cases for failed authorization with @jwt_required and rewriting them for the new decorator. I'm happy to make revisions or change formatting, of course.

@coveralls
Copy link

coveralls commented May 26, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c48975a on carlegbert:jwtopt into 9bfa900 on vimalloc:master.

@vimalloc
Copy link
Owner

This looks good, thanks for contributing! 👍

I was thinking about this more and had one question for you. Would it make more sense to only catch the errors that indicate a complete absence of a JWT in the jwt_optional decorator? I was thinking it may be confusing if someone with an expired token accessed a jwt_optional endpoint, and got the result as if they had no token at all. What are your thoughts on this?

Cheers :)

@carlegbert
Copy link
Contributor Author

That's a good point, initially my thought was that it doesn't make sense to return an error message/page for a view that doesn't necessarily require authorization, but I suppose the client can always just send a request without any authorization header in case of a failure, if it knows that the route has optional authorization. And we definitely do need to tell the client if a token has expired, so it can get a new one. I'll make those changes later today.

@coveralls
Copy link

coveralls commented May 27, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling aca3d28 on carlegbert:jwtopt into 9bfa900 on vimalloc:master.

@vimalloc vimalloc merged commit 886efcc into vimalloc:master May 27, 2017
@carlegbert
Copy link
Contributor Author

carlegbert commented May 27, 2017

ok, the decorator now only catches a NoAuthorizationError, and all other authorization errors will be caught as by @app.errorhandler like normal. Let me know if you have any more thoughts/changes. I'm also happy to add some documentation, I would have already but I wasn't sure where it should go.

@vimalloc
Copy link
Owner

Released as 2.1.0.

If you wanna add some documentation you can look at the docs folder. Generally what I've done is created a demo app in the examples folder, then slurp that in with some additional info in the docs folder. If you have any questions about that or anything dont hesistate to let me know.

Thanks for contributing! 👍

@carlegbert carlegbert deleted the jwtopt branch May 27, 2017 18:03
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.

3 participants