ashokdelphia commented 4 years ago

Add some more tests for error cases with 'bad' tokens. (c2c2c8b)
Handle more token errors. (a33765d)

There are a number of subclasses of InvalidTokenError that bubble up as unhandled errors in the old code.

I've kept the explicit handling of DecodeError and Expired Signature, which are also children of InvalidTokenError, in order to preserve their distinctive responses.

(There's a similar ladder in rest_framework_jwt.utils::check_payload, but I didn't dig very far into whether that should also be handling all InvalidTokenErrors.)

I ran into this after migrating algorithms, and after dropping support for the old algorithm I get a 500 error from the unhandled InvalidAlgorithmError when using an 'old' token. Digging in, I saw there were a handful of other cases that would also bubble up as unhandled errors.

I didn't try and handle PyJWTError or InvalidKeyError (which are higher up the tree of exceptions than InvalidTokenError) as those seem more like things that are misconfigured, whereas I think everything under InvalidTokenError could reasonably be some oddball on the internet sending a funny-looking token.

codecov-commenter commented 4 years ago

Codecov Report

Merging #72 into master will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #72      +/-   ##
==========================================
+ Coverage   98.12%   98.14%   +0.01%     
==========================================
  Files          19       19              
  Lines         481      484       +3     
  Branches       45       46       +1     
==========================================
+ Hits          472      475       +3     
  Misses          7        7              
  Partials        2        2

Flag	Coverage Δ
#codecov	`98.14% <100.00%> (+0.01%)`	:arrow_up:
#dj111	`97.67% <100.00%> (+0.01%)`	:arrow_up:
#dj20	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#dj21	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#dj22	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#dj30	`97.72% <100.00%> (+0.01%)`	:arrow_up:
#drf310	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#drf311	`97.72% <100.00%> (+0.01%)`	:arrow_up:
#drf37	`97.25% <100.00%> (-0.41%)`	:arrow_down:
#drf38	`97.67% <100.00%> (+0.01%)`	:arrow_up:
#drf39	`97.67% <100.00%> (+0.01%)`	:arrow_up:
#py27	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#py35	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#py36	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#py37	`97.25% <100.00%> (+0.01%)`	:arrow_up:
#py38	`97.10% <100.00%> (+0.01%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/rest_framework_jwt/authentication.py	`97.11% <100.00%> (+0.08%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 12c08db...a33765d. Read the comment docs.

ashokdelphia commented 4 years ago

I'd really appreciate it if we could review / merge / release this.

In one project, I'm getting a number of 500 errors every day, because people with expired tokens that use an old algorithm are trying them. I don't think there's a good way to handle that outside of the library, but this change would mean they get a straightforward 401 instead.

fitodic commented 4 years ago

@ashokdelphia Thank you for the contribution and I apologize for the delay. It's released in 1.17.0.

Styria-Digital / django-rest-framework-jwt

Handle more invalid token errors for authenticated calls #72

Codecov Report