knox: handle race condition on concurrent logout call

jazzband / django-rest-knox

Authentication Module for django rest auth

MIT License

1.18k stars 213 forks source link

knox: handle race condition on concurrent logout call #186

Open xrmx opened 5 years ago

xrmx commented 5 years ago

With AUTO_REFRESH enabled authentication is racing against token removal of logout. If a token gets removed updating its expiry would led to a DatabaseError raised by the Django ORM.

Fix that by ignoring DatabaseError exception returned by renew_token so that the last request will get a AuthenticationFailed exception instead of a 500 error.

xrmx commented 5 years ago

@johnraz Thanks for the review, it's based on master because develop is missing some 4.1.0 commits. Will add changelog.

johnraz commented 5 years ago

If you base your work on develop and create the PR against develop, missing commits from master should not be an issue. Those are only release related commits I believe.

Writing from my phone so I may be missing a point. Maybe some other contributors could have a look. Cheers.

xrmx commented 5 years ago

@johnraz yeah, but master commits should be merged back to develop or you'll have conflicts, e.g. like in this specific case in the CHANGELOG

johnraz commented 5 years ago

Yes agreed on the merge back to develop but please let’s do that in a separate PR, maybe @belugame can help, won’t have access to my computer soon (travelling right now).

xrmx commented 5 years ago

Updated PR:

rebased on top of develop
renamed tests and added some comments to help understand the scenario
rewritten commit message to make it hopefully more understandable
changed the continue to a break when handling the exception as we found the token

johnraz commented 6 months ago

I added the help wanted on this PR to get it rebased and merged.

@giovannicimolin if you feel like coding be my guest 😄