Skip to content
Open
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
9 changes: 9 additions & 0 deletions netbox/users/models/tokens.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

from django.conf import settings
from django.contrib.postgres.fields import ArrayField
from django.core.exceptions import ValidationError
from django.core.validators import MinLengthValidator
from django.db import models
from django.urls import reverse
Expand Down Expand Up @@ -86,6 +87,14 @@ def get_absolute_url(self):
def partial(self):
return f'**********************************{self.key[-6:]}' if self.key else ''

def clean(self):
super().clean()

# Prevent creating a token with a past expiration date
# while allowing updates to existing tokens.
if self.pk is None and self.is_expired:
raise ValidationError({'expires': _('Expiration time must be in the future.')})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should consider giving the user some feedback here on what the current date, time, and timezone the server expects are. NetBox's settings.py defaults to UTC if an installation's config does not set a custom TIME_ZONE setting, which actually tripped me up when I was testing this locally as I didn't have it set and so was confused why this wasn't working for something just 15 mins ahead of the current time (I'm UTC-6:00 at the moment).

I'm not arguing that we should include it by default in the UI, but perhaps just when this validation error is raised. You can look at netbox.tables.columns.DateTimeColumn to see how we've accessed the TIME_ZONE setting and displayed it in table columns, although you may need to format it a little differently here. I would also review Django's docs on internationalization to find the best way to include a dynamic element like that in a translatable string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed review and for pointing out the timezone behavior! That context about the default UTC setting and DateTimeColumn is really helpful.

I’ll look into surfacing the server’s current date/time and timezone when this validation error is raised and make sure it’s done in a way that plays nicely with Django’s i18n and translations.


def save(self, *args, **kwargs):
if not self.key:
self.key = self.generate_key()
Expand Down
68 changes: 67 additions & 1 deletion netbox/users/tests/test_models.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,72 @@
from datetime import timedelta

from django.core.exceptions import ValidationError
from django.test import TestCase
from django.utils import timezone

from users.models import User, Token
from utilities.testing import create_test_user


from users.models import User
class TokenTest(TestCase):
"""
Test class for testing the functionality of the Token model.
"""

@classmethod
def setUpTestData(cls):
"""
Set up test data for the Token model.
"""
cls.user = create_test_user('User 1')

def test_is_expired(self):
"""
Test the is_expired property.
"""
# Token with no expiration
token = Token(user=self.user, expires=None)
self.assertFalse(token.is_expired)

# Token with future expiration
token.expires = timezone.now() + timedelta(days=1)
self.assertFalse(token.is_expired)

# Token with past expiration
token.expires = timezone.now() - timedelta(days=1)
self.assertTrue(token.is_expired)

def test_cannot_create_token_with_past_expiration(self):
"""
Test that creating a token with an expiration date in the past raises a ValidationError.
"""
past_date = timezone.now() - timedelta(days=1)
token = Token(user=self.user, expires=past_date)

with self.assertRaises(ValidationError) as cm:
token.clean()
self.assertIn('expires', cm.exception.error_dict)

def test_can_update_existing_expired_token(self):
"""
Test that updating an already expired token does NOT raise a ValidationError.
"""
# Create a valid token first with an expiration date in the past
# bypasses the clean() method
token = Token.objects.create(user=self.user)
token.expires = timezone.now() - timedelta(days=1)
token.save()

# Try to update the description
token.description = 'New Description'
try:
token.clean()
token.save()
except ValidationError:
self.fail('Updating an expired token should not raise ValidationError')

token.refresh_from_db()
self.assertEqual(token.description, 'New Description')


class UserConfigTest(TestCase):
Expand Down
Loading