API Tokens #134
@ -1,5 +1,4 @@
|
||||
import datetime
|
||||
import hashlib
|
||||
|
||||
from rest_framework.authentication import BaseAuthentication
|
||||
from rest_framework.exceptions import AuthenticationFailed
|
||||
@ -22,12 +21,13 @@ class UserTokenAuthentication(BaseAuthentication):
|
||||
return None
|
||||
|
||||
try:
|
||||
token_hash = hashlib.sha256(token_key.encode()).hexdigest()
|
||||
token_hash = UserToken.generate_hash(token_key=token_key)
|
||||
token = UserToken.objects.get(token_hash=token_hash)
|
||||
dfelinto marked this conversation as resolved
Outdated
|
||||
except UserToken.DoesNotExist:
|
||||
raise AuthenticationFailed('Invalid token')
|
||||
|
||||
token.ip_address_last_access = clean_ip_address(request)
|
||||
token.date_last_access = datetime.datetime.now()
|
||||
token.save(update_fields={'ip_address_last_access', 'date_last_access'})
|
||||
|
||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
`+ token.save(update_fields={'ip_address_last_access','date_last_access'})`
|
||||
return (token.user, token)
|
||||
|
@ -1,4 +1,4 @@
|
||||
# Generated by Django 4.2.11 on 2024-05-21 21:26
|
||||
# Generated by Django 4.2.11 on 2024-05-25 09:43
|
||||
|
||||
from django.conf import settings
|
||||
from django.db import migrations, models
|
||||
@ -19,7 +19,7 @@ class Migration(migrations.Migration):
|
||||
fields=[
|
||||
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
|
||||
('name', models.CharField(max_length=255)),
|
||||
('token_begin', models.CharField(editable=False, max_length=5)),
|
||||
('token_prefix', models.CharField(editable=False, max_length=5)),
|
||||
('token_hash', models.CharField(editable=False, max_length=64, unique=True)),
|
||||
('date_created', models.DateTimeField(auto_now_add=True)),
|
||||
('date_last_access', models.DateTimeField(blank=True, editable=False, null=True)),
|
||||
|
@ -1,3 +1,6 @@
|
||||
import hashlib
|
||||
import secrets
|
||||
|
||||
from django.db import models
|
||||
from django.urls import reverse
|
||||
|
||||
@ -7,7 +10,7 @@ from users.models import User
|
||||
class UserToken(models.Model):
|
||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
"begin" is not a noun, let's use a different name, e.g. "begin" is not a noun, let's use a different name, e.g. `token_prefix`
|
||||
user = models.ForeignKey(User, on_delete=models.CASCADE, related_name='tokens')
|
||||
name = models.CharField(max_length=255)
|
||||
token_begin = models.CharField(max_length=5, editable=False)
|
||||
token_prefix = models.CharField(max_length=5, editable=False)
|
||||
token_hash = models.CharField(max_length=64, editable=False, unique=True)
|
||||
date_created = models.DateTimeField(auto_now_add=True)
|
||||
date_last_access = models.DateTimeField(null=True, blank=True, editable=False)
|
||||
@ -17,4 +20,17 @@ class UserToken(models.Model):
|
||||
return reverse('apitokens:delete', kwargs={'pk': self.pk})
|
||||
|
||||
def __str__(self):
|
||||
return f"{self.user.username} - {self.token_begin} - {self.name}"
|
||||
return f"{self.user.username} - {self.token_prefix} - {self.name}"
|
||||
|
||||
@staticmethod
|
||||
def generate_hash(token_key: str) -> str:
|
||||
return hashlib.sha256(token_key.encode()).hexdigest()
|
||||
|
||||
@staticmethod
|
||||
def generate_token_key() -> str:
|
||||
return secrets.token_urlsafe(32)
|
||||
|
||||
@classmethod
|
||||
def generate_token_prefix(cls, token_key: str) -> str:
|
||||
token_prefix_length = cls._meta.get_field('token_prefix').max_length
|
||||
return token_key[:token_prefix_length]
|
||||
|
@ -29,14 +29,10 @@
|
||||
{% for token in tokens %}
|
||||
<tr>
|
||||
<td>{{token.name}}</td>
|
||||
<td>{{token.token_begin}}...</td>
|
||||
<td>{{token.token_prefix}}...</td>
|
||||
<td>{{token.date_created}}</td>
|
||||
<td>
|
||||
{% if token.date_last_access %}
|
||||
{{token.date_last_access}}
|
||||
{% else %}
|
||||
-
|
||||
{% endif %}
|
||||
{{ token.date_last_access|default_if_none:"-" }}
|
||||
</td>
|
||||
<td><a href="{{ token.get_delete_url }} " class="btn btn-danger i-trash"></a></td>
|
||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
this if-else can be rewritten using https://docs.djangoproject.com/en/4.2/ref/templates/builtins/#default this if-else can be rewritten using https://docs.djangoproject.com/en/4.2/ref/templates/builtins/#default
|
||||
</tr>
|
||||
|
0
apitokens/tests/__init__.py
Normal file
79
apitokens/tests/test_user_token.py
Normal file
@ -0,0 +1,79 @@
|
||||
from datetime import datetime
|
||||
from django.utils import timezone
|
||||
|
||||
from django.test import TestCase
|
||||
from django.urls import reverse
|
||||
|
||||
from apitokens.models import UserToken
|
||||
from common.tests.factories.users import UserFactory
|
||||
|
||||
|
||||
class UserTokenTest(TestCase):
|
||||
def setUp(self) -> None:
|
||||
self.user = UserFactory()
|
||||
self.client.force_login(self.user)
|
||||
self.assertEqual(UserToken.objects.count(), 0)
|
||||
return super().setUp()
|
||||
|
||||
def test_token_displayed_only_once(self):
|
||||
response = self.client.post(
|
||||
reverse('apitokens:create'),
|
||||
{
|
||||
'name': 'Test Token',
|
||||
},
|
||||
)
|
||||
|
||||
self.assertEqual(response.status_code, 302)
|
||||
self.assertRedirects(response, reverse('apitokens:list'))
|
||||
self.assertEqual(UserToken.objects.count(), 1)
|
||||
token = UserToken.objects.first()
|
||||
|
||||
# Check if the success message with the token value is displayed
|
||||
messages = list(response.wsgi_request._messages)
|
||||
self.assertEqual(len(messages), 2)
|
||||
|
||||
token_key = messages[0].message
|
||||
self.assertIn(token.token_prefix, token_key)
|
||||
self.assertIn('Your new token has been generated', messages[1].message)
|
||||
|
||||
token_hash = UserToken.generate_hash(token_key)
|
||||
self.assertEqual(token, UserToken.objects.get(token_hash=token_hash))
|
||||
|
||||
# Verify the token value is shown only on the creation page
|
||||
response = self.client.get(reverse('apitokens:list'))
|
||||
self.assertNotContains(response, token_key)
|
||||
|
||||
def test_list_page_does_not_display_full_token_value(self):
|
||||
token_key = UserToken.generate_token_key()
|
||||
|
||||
token_prefix = UserToken.generate_token_prefix(token_key)
|
||||
token_hash = UserToken.generate_hash(token_key)
|
||||
token = UserToken.objects.create(
|
||||
user=self.user, name='Test Token', token_prefix=token_prefix, token_hash=token_hash
|
||||
)
|
||||
|
||||
response = self.client.get(reverse('apitokens:list'))
|
||||
self.assertContains(response, str(token.token_prefix))
|
||||
self.assertNotContains(response, str(token_key))
|
||||
|
||||
def test_list_page_shows_last_access_time(self):
|
||||
Oleg-Komarov
commented
I expected to have a different test: a one that triggers the actual code path where date_last_access is supposed to be updated. It might be easy to add it in the other PR, where we have a new endpoint that exercises that code path organically. I expected to have a different test: a one that triggers the actual code path where date_last_access is supposed to be updated.
It would catch the bug with the missing `save` that was fixed during review.
It might be easy to add it in the other PR, where we have a new endpoint that exercises that code path organically.
|
||||
token = UserToken.objects.create(user=self.user, name='Test Token')
|
||||
|
||||
# Create a timezone-aware datetime object.
|
||||
date_last_access_str = '1994-01-02 10:10:36'
|
||||
date_last_access_naive = datetime.strptime(date_last_access_str, '%Y-%m-%d %H:%M:%S')
|
||||
date_last_access_aware = timezone.make_aware(
|
||||
date_last_access_naive, timezone.get_default_timezone()
|
||||
)
|
||||
token.date_last_access = date_last_access_aware
|
||||
|
||||
# Format the datetime to match the expected response format.
|
||||
formatted_date = (
|
||||
date_last_access_aware.strftime('%b. %-d, %Y, %-I:%M %p')
|
||||
.replace('AM', 'a.m.')
|
||||
.replace('PM', 'p.m.')
|
||||
)
|
||||
|
||||
token.save()
|
||||
response = self.client.get(reverse('apitokens:list'))
|
||||
self.assertContains(response, formatted_date)
|
@ -1,6 +1,4 @@
|
||||
import hashlib
|
||||
import logging
|
||||
import secrets
|
||||
|
||||
from django import forms
|
||||
from django.contrib.auth.mixins import LoginRequiredMixin
|
||||
@ -62,13 +60,11 @@ class CreateTokenView(LoginRequiredMixin, SuccessMessageMixin, CreateView):
|
||||
def form_valid(self, form):
|
||||
form.instance.user = self.request.user
|
||||
|
||||
token = secrets.token_urlsafe(32)
|
||||
token_hash = hashlib.sha256(token.encode()).hexdigest()
|
||||
token_key = UserToken.generate_token_key()
|
||||
form.instance.token_hash = UserToken.generate_hash(token_key)
|
||||
form.instance.token_prefix = UserToken.generate_token_prefix(token_key)
|
||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
this should be defined in the model code (e.g. as a class method), then it will be possible to write tests that generate tokens this should be defined in the model code (e.g. as a class method), then it will be possible to write tests that generate tokens
|
||||
|
||||
dfelinto marked this conversation as resolved
Outdated
Oleg-Komarov
commented
let's hide this implementation in a model class method, then the api will use it as well let's hide this implementation in a model class method, then the api will use it as well
|
||||
form.instance.token_hash = token_hash
|
||||
form.instance.token_begin = token[:5]
|
||||
|
||||
messages.info(self.request, f'{token}')
|
||||
messages.info(self.request, token_key)
|
||||
return super().form_valid(form)
|
||||
|
||||
|
||||
|
this should be a class method on a model, please see a related comment in the
CreateTokenView