I think we just want timezone.now(). This is the modern equivalent of our old get_tz_aware_utcnow().

By default, save() will save every single field in the model. If there are two threads dealing with an API token at once, one will overwrite the other's data.
Best to always be explicit:

webapi_token.save(update_fields=('last_used',))

Same note about saving.

Blank line before comments.

We haven't been consistent with this, but we should aim to have localized help_text= set for every field.
Since that's long, the best formatting for fields are:

field = models.FooField(
    param,
    param,
    help_text=...)

255 should be fine.

This is being set after we save. I assume we'll want to set this before the parent save call above.

Things can go wrong when comparing dates/times. 1 second seems fine, but what if we hit a leap second while this runs? Or something causes the process to stall? This sort of stuff unfortunately happens.
A better solution we've found is to hard-code when "now" is, using spies:

self.spy_on(timezone.now, op=kgb.SpyOpReturn(...))


Then we can safely compare the values with strict equal checks.

It looks like you've pulled on release-3.x but not rebased, which is why your diff is now undoing these changes. Can you rebase and update?

This could use djblets.db.fields.ModificationTimestampField in order to avoid having to babysit it in save()

Add a blank line in here (datetime is standard lib, django is third party).

Can you move this to the third-party import section?

I feel like this is something we should do per-test-method rather than for everything in the entire suite.
I think I'd also be happier if we initialized it with some fake timestamp value. Right now in something like test_last_updated, if no time has elapsed between when the token is created and when it's saved, there's no reason to think the timestamp would change. Instead, to be sure everything is working independent of timing, we could do this:

create token
spy on timezone.now with a fake timestamp
save
check timestamp against fake value

Ship It!

This is missing a "Version Added".

This should be "str" now.

Since we're defining a new method, it might be a good opportunity to replace this with a dictionary, for better readability and future expansion.

Any % values must be specified outside of a _(...). Otherwise they'll modify the string that needs to be localized.

We should specify which user this pertains to.

Same comment about localized string formatting.

Same comment about localized string formatting.

We should specify which user this pertains to.

This should contain a Version Added.

We've been moving to a better format. This can be:

result = (
    super(...)
    .valdilate_credentials(...)
)


Though, being Python 3, the super can just be super(), probably.

This can be simplified to:

return self.expires is not None and timezone.now() >= self.expires

Plain imports should be listed before from imports.

Let's switch this to an import kgb, and then access these as attributes from kgb. This is the pattern we're adopting elsewhere.

For unit tests, we don't want to use _(...).

Same note about import kgb.

This should be assertTrue

Might be more clear as "... the token is invalid or expired, and any appropriate HTTP headers to send to the client."

We always either override everything in here, or return None. Rather than pre-define it, let's just return { ... } directly where we need it.
For any conditional values, let's set a variable and then use that as the value in the return { ... }.

For safety, this should ddo LOGIN_FAILED.headers.copy(). That avoids any state leakage (a problem we've had before in the API).

For something like this, formatting works a bit better as:

return_dict['error_message'] = (
    _('...')
    % ...
)


That gives both the message and the formatting arguments breathing room. Changes to one don't impact the other, and it's obvious how to reformat for any new changes.

Rather than format and set a default, let's move this to an else below.

As above, this becomes more clear when the form is:

return_dict['error_message'] = (
    _('This token is invalid. It became invalid %s.'
      'It was made invalid because: %s')
    % (webapi_token.invalid_date,
       webapi_token.invalid_reason)
)


So that being said, I'd like to change the message. Something more like:

'This API token became invalid on %s: %s'


This is just a bit less technical, flows a bit better, and makes the reason more part of the error message itself. An example then becomes:
"This API token became invalid on 2023-06-14 12:13:14.123456: We've reset tokens due to a potential compromise of our internal systems."
Similarly, the above error should be This API token became invalid on %s..
We should probably format the date here as well, rather than using the (above) default representation that has too much precision.
Now we're not at all consistent with this (we mostly do this right in templates via the |date filter), but Django does have some localization code. We can do:

from django.utils.formats import localize

'... %s ...' % localize(webapi_token.invalid_date)


This gives us a string like July 28, 2022, 6:18 a.m. (matching bits of our UI), which users can customize if they really want to via settings.DATETIME_FORMAT (not that we advertise that).

When logging, we don't want to use %. Instead, pass the value as a positional parameter.

Similar comments above apply here.

Dictionaries should (almost) always be in multi-line format, with keys alphabetized and on their own line (not on the { or } lines):

return {
    'error_message': None,
    'headers': None,
}

Same here.

Missing a trailing comma.

Let's actually move these into the assertion, so it's not scattered:

self.assertEqual(
    result,
    (
        False,
        '...',
        {
            # Explicit headers here
        }
    ))


Same below.

No need for the .copy() here, since we know it won't be modified in a test.
Although, we want to be clear on expectations, so we know what regresses from a change. So this should hard-code any headers to check against.
Same below.

I'm torn on this. My gut is telling me we don't want to bake this into the API.
I think, since this is a workaround for wanting to update a timestamp without triggering default behavior, we should work around it there.
We have two approaches we could take for that:


We could use save_base() instead of save(). This can be used just like save() for our purposes. It just bypasses overridden save logic, and some deferred model and field sanity checks that won't apply in our case.


Alternatively, we could do api_token_model.objects.filter(pk=token.pk).update(...) to update the database copy without triggering any functionality (including things like the standard pre_save/post_save signals).


Since we probably aren't interested in making this save entirely invisible — we just want to avoid webapi_token_updated — we should probably go with the first option.

We don't want to localize here. Test results should almost always compare against fixed state. Generating dynamic state can mask bugs.
We can assume a default localization when running tests. If the assumption were to be wrong, then we'd have a test environment/setup issue, which we don't want to mask.

These should still hard-code the exact string we expect, rather than running it through any Django functions. Otherwise we're just testing Django's functionality using Django's functionality. So let's make this whole thing a plain string, no function calls. If the values ever change, it highlights a test environment problem, or a change in Django that we need to be aware of.

Ship It!