Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure old tokens continue to work by removing token_key #362

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ __pycache__/
# Distribution / packaging
.Python
env/
.venv/
build/
develop-eggs/
dist/
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 5.1.0
- Removed `token_key` which was no longer necessary after removal of salt in 4.2.0
- Fix old tokens not being accepted in 5.0.0, 5.0.1 and 5.0.2

## 5.0.2
- Implement AUTO_REFRESH_MAX_TTL to limit total token lifetime when AUTO_REFRESH = True

Expand Down
38 changes: 19 additions & 19 deletions knox/auth.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import binascii
import logging
from hmac import compare_digest

from django.utils import timezone
from django.utils.translation import gettext_lazy as _
Expand All @@ -11,7 +10,7 @@

from knox.crypto import hash_token
from knox.models import get_token_model
from knox.settings import CONSTANTS, knox_settings
from knox.settings import knox_settings
from knox.signals import token_expired

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -52,27 +51,28 @@ def authenticate(self, request):

def authenticate_credentials(self, token):
'''
Due to the random nature of hashing a value, this must inspect
each auth_token individually to find the correct one.

Tokens that have expired will be deleted and skipped
'''
msg = _('Invalid token.')
token = token.decode("utf-8")
for auth_token in get_token_model().objects.filter(
token_key=token[:CONSTANTS.TOKEN_KEY_LENGTH]):
if self._cleanup_token(auth_token):
continue

try:
digest = hash_token(token)
except (TypeError, binascii.Error):
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)
return self.validate_user(auth_token)
raise exceptions.AuthenticationFailed(msg)

try:
digest = hash_token(token)
except (TypeError, binascii.Error):
raise exceptions.AuthenticationFailed(msg)

try:
auth_token = get_token_model().objects.get(digest=digest)
except get_token_model().DoesNotExist:
raise exceptions.AuthenticationFailed(msg)

if self._cleanup_token(auth_token):
raise exceptions.AuthenticationFailed(msg)

if knox_settings.AUTO_REFRESH and auth_token.expiry:
self.renew_token(auth_token)

return self.validate_user(auth_token)

def renew_token(self, auth_token) -> None:
current_expiry = auth_token.expiry
Expand Down
17 changes: 17 additions & 0 deletions knox/migrations/0010_remove_authtoken_token_key.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.0.7 on 2024-08-03 12:58

from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
("knox", "0009_extend_authtoken_field"),
]

operations = [
migrations.RemoveField(
model_name="authtoken",
name="token_key",
),
]
11 changes: 2 additions & 9 deletions knox/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
from knox import crypto
from knox.settings import CONSTANTS, knox_settings

sha = knox_settings.SECURE_HASH_ALGORITHM

User = settings.AUTH_USER_MODEL


Expand All @@ -25,8 +23,8 @@ def create(
if expiry is not None:
expiry = timezone.now() + expiry
instance = super().create(
token_key=token[:CONSTANTS.TOKEN_KEY_LENGTH], digest=digest,
user=user, expiry=expiry, **kwargs)
digest=digest, user=user, expiry=expiry, **kwargs
)
return instance, token


Expand All @@ -36,11 +34,6 @@ class AbstractAuthToken(models.Model):

digest = models.CharField(
max_length=CONSTANTS.DIGEST_LENGTH, primary_key=True)
token_key = models.CharField(
max_length=CONSTANTS.MAXIMUM_TOKEN_PREFIX_LENGTH +
CONSTANTS.TOKEN_KEY_LENGTH,
db_index=True
)
user = models.ForeignKey(User, null=False, blank=False,
related_name='auth_token_set', on_delete=models.CASCADE)
created = models.DateTimeField(auto_now_add=True)
Expand Down
1 change: 0 additions & 1 deletion knox/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ class CONSTANTS:
'''
Constants cannot be changed at runtime
'''
TOKEN_KEY_LENGTH = 15
DIGEST_LENGTH = 128
MAXIMUM_TOKEN_PREFIX_LENGTH = 10

Expand Down
36 changes: 31 additions & 5 deletions tests/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def test_login_creates_keys(self):
for _ in range(5):
self.client.post(url, {}, format='json')
self.assertEqual(AuthToken.objects.count(), 5)
self.assertTrue(all(e.token_key for e in AuthToken.objects.all()))
self.assertTrue(all(e.digest for e in AuthToken.objects.all()))

def test_login_returns_serialized_token(self):
self.assertEqual(AuthToken.objects.count(), 0)
Expand Down Expand Up @@ -196,16 +196,16 @@ def test_expired_tokens_deleted(self):
self.client.post(url, {}, format='json')
self.assertEqual(AuthToken.objects.count(), 0)

def test_update_token_key(self):
def test_update_digest(self):
self.assertEqual(AuthToken.objects.count(), 0)
instance, token = AuthToken.objects.create(self.user)
rf = APIRequestFactory()
request = rf.get('/')
request.META = {'HTTP_AUTHORIZATION': f'Token {token}'}
(self.user, auth_token) = TokenAuthentication().authenticate(request)
self.assertEqual(
token[:CONSTANTS.TOKEN_KEY_LENGTH],
auth_token.token_key,
crypto.hash_token(token),
auth_token.digest,
)

def test_authorization_header_empty(self):
Expand Down Expand Up @@ -237,7 +237,7 @@ def test_authorization_header_spaces_in_token_string(self):
)

def test_invalid_token_length_returns_401_code(self):
invalid_token = "1" * (CONSTANTS.TOKEN_KEY_LENGTH - 1)
invalid_token = "1" * (knox_settings.AUTH_TOKEN_CHARACTER_LENGTH - 1)
self.client.credentials(HTTP_AUTHORIZATION=('Token %s' % invalid_token))
response = self.client.post(root_url, {}, format='json')
self.assertEqual(response.status_code, 401)
Expand Down Expand Up @@ -531,3 +531,29 @@ def test_tokens_created_before_prefix_still_work(self):
response = self.client.get(root_url, {}, format='json')
self.assertEqual(response.status_code, 200)
reload(views)

def test_old_tokens_still_work(self):
self.assertEqual(AuthToken.objects.count(), 0)

old_token = "02d233c901e7bd38df1dbc486b7e22c5c81b089c40cbb31d35d7b032615f5778"
# Hash generated using crypto.hash_token on 4.2.0 with
# SECURE_HASH_ALGORITHM = 'cryptography.hazmat.primitives.hashes.SHA512'
old_hash = (
"c7f9f2904decf77e0fa0341bc3eb96daa1437649825f4bfdd38cdad64d69c4be55938d71f17"
"34131c656f9bbbfc5d991bef295accd268921b23d9cdd0d9d60d0"
)

AuthToken(
digest=old_hash,
user=self.user,
).save()

rf = APIRequestFactory()
request = rf.get('/')
request.META = {'HTTP_AUTHORIZATION': f'Token {old_token}'}
user, auth_token = TokenAuthentication().authenticate(request)
self.assertEqual(self.user, user)
self.assertEqual(
old_hash,
auth_token.digest,
)
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ envlist =
[testenv]
commands =
python manage.py migrate
coverage run manage.py test
coverage run manage.py test {posargs}
coverage report
setenv =
DJANGO_SETTINGS_MODULE = knox_project.settings
Expand Down
Loading