Closes #20823: Validate Token expiration date on creation #20862
Conversation
Add model-level validation to prevent creating tokens with a past expiration date. Updates to existing tokens are still allowed, ensuring flexibility for expired token modifications. Includes test cases to verify this behavior. Fixes netbox-community#20823
jeremystretch
requested review from
a team and
jnovinger
and removed request for
a team
| # 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.')}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
2 participants
Fixes: #20823
This PR adds model-level validation to prevent creating API tokens with an expiration date in the past, while still allowing updates to existing tokens (including already-expired ones).
Changes:
Token.clean()to raise aValidationErrorwhen creating a new token (pk is None) whoseexpirestimestamp is already in the past.users/tests/test_models.pycovering:Token.is_expiredproperty forNone, future, and past expiration timesNo database or API schema changes are introduced by this PR.