Add base exception for invalid tokens

[wbolster]

Fixes #59.

[jpadilla]

@wbolster hmmm debating this one, haven't come across the need. @mark-adams since you seem to be using this library recently, any thoughts?

Update: I really don't mind merging this in, but just want to make sure before we add another exception class here.

[mark-adams]

I could definitely see the use in being able to capture any type of JWT-related exception by using the base class and since it doesn't change the API in any significant way, I don't see any problem with it.

I don't have a specific use-case for it, but I don't think it clutters things very much either. I can picture someone wishing to handle all of the JWT-related exceptions in the same way while wanting to handle any other exceptions in a different manner. For the record though, I haven't run into a need for this either, but I don't have anything against it.

I do have a problem with the naming however. PEP 8 says that exception names should usually end in Error. In this case, since we are creating a generic base case, I'd probably go with JWTError as the base class name.

[wbolster]

A bit more about the rationale: since JWT is used to convey authorizations, a general way to handle "hey, this jwt should be rejected" is really useful. Expired tokens, corrupted tokens, wrong signatures, wrong audience, wrong publisher, wrong whatever; they all mean the JWT should be rejected. A base exception that covers all of these solves this. This general "rejection" exception would be used by code that checks whether a client is authorised to make a certain HTTP request for instance, since that code doesn't care about the exact reason.

Re. API 'cluttering'; my opinion is that is actually avoiding any clutter. Right now application code has to catch multiple exceptions, and if it forgets to handle one, the application would crash if those exceptions happen later on. Especially for security APIs, a fool-proof and simple way for applications to handle "hey this is not allowed" is very important.

Re. the naming: I agree that exception names should usually have Error as a suffix. The reason I chose this name is to be consistent with the other (already existing) exceptions such as InvalidIssuer, so it's really not my call. :) Btw, I don't like the jwt.JWTError name; it looks and sounds ugly. Something like jwt.InvalidTokenError conveys the meaning much better I think.

[jpadilla]

@wbolster sounds good. It's a bit late to fix the naming on the older exceptions, so let's leave those as they are for now and correctly name the new one. I like InvalidTokenError.

[wbolster]

Okay, renamed to InvalidTokenError; also rebased onto master, so should be ready to merge.

[mark-adams]

@wbolster I understand your rationale completely and agree with it all (which is why I can't find any problems with it). I agree with your thoughts on the naming as well. I looked at a couple of other libraries and it looks like using a base exception for functional purposes (InvalidTokenError) is much more common than organizationally with a name like JWTError. My choice of name was purely based on gut; but my gut now tells me yours is better. :-)

@jpadilla What do you think about changing the real names of the existing exceptions to have Error appended as well so they line up with PEP 8, but create aliases for API compatibility / transition purposes? This could be something as simple as ExpiredSignature = ExpiredSignatureError

[wbolster]

Seems like our comments crossed mid-air, @mark-adams :)

Let me know if you want me to rename the other exceptions as well (with aliases for compatibility, but is this pyjwt version already stable API? :))

[jpadilla]

I've been wanting to hit 1.0 already for a while now, since I'm pretty sure we already have a pretty solid and stable API, but I've been holding off with the latest "influx" of contributions.

Could we set DeprecationWarning to the old exceptions and keeping them "aliased" to the new ones? That way when we flip the switch and release v1.0.0 we can just remove the old exceptions without that much "remorse".

Makes sense?

[wbolster]

I can't think of a way to use DeprecationWarning, since it's impossible (at least in a sane way) to know when these old class names are used. Ideas?

[wbolster]

Btw, I think the README should be updated to reflect that the InvalidTokenError is the one applications likely care about.

[mark-adams]

I'd suggest making a decorator (something like this) to wrap those errors and have the decorator call warnings.warn("This exception has been deprecated", DeprecationWarning) before the actual function is invoked. That seems like a pretty sane idea. :-)

Also, I'm not sure about the statement that "InvalidTokenError is the one applications likely care about" versus the more specialized errors. I can't speak for others but in the application where I'm using pyjwt, I'm actually taking different actions depending on why the token is failing. (Incrementing login failed counters, logging messages, sending notifications, etc.) In my case, I have differing catch blocks for each type of error.

In the case of the README, I think the simplest base case for JWT failure is testing to see if the signature is good or bad and that is what the documentation is showing. I do think it wouldn't be a bad idea for us to add docstrings to the errors to explain what each one of them is for but that probably can be something separate from this.

[jpadilla]

Regarding the DeprecationWarning, I think either a decorator as @mark-adams pointed out or just doing it in their __init__?

Agree with @mark-adams and the README. Also adding docstrings to those exceptions is also a good idea, but that can be managed out of this PR as well.

[wbolster]

The problem with the DeprecationWarning is not pyjwt itself, since it would not use deprecated API internally. The decorator approach you pointed at would only work for code creating instances of the exception, which in the pyjwt case would only be pyjwt itself. That means there's no point using it. (Or maybe I'm not getting it...)

The problem is that application code that does sth like this:

try:
    jwt.decode(...)
except jwt.InvalidIssuer:
    handle_error()

...only mentions the jwt.InvalidIssuer symbol, but doesn't call any method on it, so there's no way a DeprecationWarning can be sneaked in.

[wbolster]

Re. the "this is likely the error you want" remarks: I think your use case is already a quite specialized one. The most basic use case for JWT would just be API authorizations, and then anything that isn't a valid token should just be blindly rejected. Anything extra is specialization, I'd say.

[jpadilla]

@wbolster yeah, now I get that. Let's just rename them and keep aliases for compatibility. We'll call it out in the release notes.

[mark-adams]

@wbolster, That's a really good point about the decorators.

This looks good to merge as far as I'm concerned once the last bit with the aliases is in

[wbolster]

Okay, I'll update the PR.

[wbolster]

Okay, updated the PR. Please review.

[mark-adams]

Looks good to me. Thanks @wbolster!