Skip to content

Commit

Permalink
knox: handle race condition on concurrent logout calls
Browse files Browse the repository at this point in the history
With AUTO_REFRESH enabled in case of multiple logout calls when
one has removed the token while the other is still authenticating
we may get a DatabaseError on TokenAuthentication.renew_token
while updating the expiry of a now removed auth_token.
Fix that by ignoring the token that returned a database error so that
in case of double logout the last request will get an
AuthenticationFailed exception instead of a 500 error.
  • Loading branch information
xrmx committed Jul 19, 2019
1 parent 78e72ba commit 6e8b6dd
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 1 deletion.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## UNRELEASED

- Fix race condition on concurrent logout calls

## 4.1.0

- Expiry format now defaults to whatever is used Django REST framework
Expand Down
7 changes: 6 additions & 1 deletion knox/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ def compare_digest(a, b):
import binascii

from django.contrib.auth import get_user_model
from django.db import DatabaseError
from django.utils import timezone
from django.utils.translation import ugettext_lazy as _
from rest_framework import exceptions
Expand Down Expand Up @@ -73,7 +74,11 @@ def authenticate_credentials(self, token):
raise exceptions.AuthenticationFailed(msg)
if compare_digest(digest, auth_token.digest):
if knox_settings.AUTO_REFRESH and auth_token.expiry:
self.renew_token(auth_token)
try:
self.renew_token(auth_token)
except DatabaseError:
# avoid race condition with concurrent logout calls
continue
return self.validate_user(auth_token)
raise exceptions.AuthenticationFailed(msg)

Expand Down
19 changes: 19 additions & 0 deletions tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
from datetime import datetime, timedelta

from django.contrib.auth import get_user_model
from django.db import DatabaseError
from django.test import override_settings
from django.utils.six.moves import reload_module
from freezegun import freeze_time
from rest_framework.exceptions import AuthenticationFailed
from rest_framework.serializers import DateTimeField
from rest_framework.test import APIRequestFactory, APITestCase as TestCase

Expand All @@ -15,6 +17,13 @@
from knox.settings import CONSTANTS, knox_settings
from knox.signals import token_expired

try:
# Python 3
from unittest import mock
except ImportError:
# Python 2
import mock

try:
# For django >= 2.0
from django.urls import reverse
Expand Down Expand Up @@ -396,3 +405,13 @@ def test_expiry_is_present(self):
response.data['expiry'],
DateTimeField().to_representation(AuthToken.objects.first().expiry)
)

def test_authenticate_credentials_handle_database_error_on_renew_token(self):
instance, token = AuthToken.objects.create(user=self.user)
with override_settings(REST_KNOX=auto_refresh_knox):
reload_module(auth)
token_auth = TokenAuthentication()
with mock.patch.object(token_auth, 'renew_token') as m:
m.side_effect = DatabaseError()
with self.assertRaises(AuthenticationFailed):
token_auth.authenticate_credentials(token.encode('utf-8'))
1 change: 1 addition & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ deps =
django22: Django>=2.2,<2.3
django-nose
markdown<3.0
mock
isort
djangorestframework
freezegun
Expand Down

0 comments on commit 6e8b6dd

Please sign in to comment.