Update the BaseWebAPIToken model for API Tokens v2
Review Request #12341 — Created June 6, 2022 and submitted
Information | |
---|---|
maubin | |
Djblets | |
release-3.x | |
12419, 12415, 12342 | |
Reviewers | |
djblets | |
This change is the first in a series of changes for improving our API Tokens
(which we'll refer to as API Tokens v2). Currently, Djblets generates tokens
using SHA1, which has the following disadvantages:
- Tokens aren't able to be validated or identified from other SHA1 tokens.
- Can't use secret scanning to detect leaked tokens
- Aren't as secure as some modern counterparts.In response, we are moving towards a new token format that contain a
vendor/category prefix and can be validated through a checksum. Additionally,
we want to be able to support token expiration and invalidation, which we
currently don't do. Since we are moving away from an old token format, we also
need support for token deprecation.In this change we modify the API tokens model to allow the possibility for
new token types, expiration, and invalidation. The following changes are made
to theBaseWebAPIToken
model:
- Addingexpires
field and anis_expired
method.
- Addingvalid
,invalid_date
andinvalid_reason
fields.
- Addingtoken_generator_id
field.
- Addinglast_used
field.
- Updating max length of token to 255 characters.The token authentication backends were also updated to block authentication
if an expired or invalid token was used. We also added help texts for all
of theBaseWebAPIToken
fields for better clarity.
- Ran unit tests in
djblets.webapi.tests
- Manually tested token authentication when making requests with
curl
andrbt api-get
Summary |
---|
Description | From | Last Updated |
---|---|---|
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 … |
|
|
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 … |
|
|
255 should be fine. |
|
|
'django.utils.timezone' imported but unused Column: 1 Error code: F401 |
![]() |
|
continuation line over-indented for hanging indent Column: 13 Error code: E126 |
![]() |
|
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 … |
|
|
It looks like you've pulled on release-3.x but not rebased, which is why your diff is now undoing these changes. … |
|
|
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? |
|
|
'datetime.timedelta' imported but unused Column: 1 Error code: F401 |
![]() |
|
I feel like this is something we should do per-test-method rather than for everything in the entire suite. I think … |
|
|
This is missing a "Version Added". |
|
|
Might be more clear as "... the token is invalid or expired, and any appropriate HTTP headers to send to … |
|
|
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 … |
|
|
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. |
|
|
We always either override everything in here, or return None. Rather than pre-define it, let's just return { ... } … |
|
|
Same comment about localized string formatting. |
|
|
We should specify which user this pertains to. |
|
|
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 … |
|
|
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 … |
|
|
When logging, we don't want to use %. Instead, pass the value as a positional parameter. |
|
|
Similar comments above apply here. |
|
|
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, … |
|
|
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 … |
|
|
For unit tests, we don't want to use _(...). |
|
|
Same note about import kgb. |
|
|
This should be assertTrue |
|
|
line too long (82 > 79 characters) Column: 80 Error code: E501 |
![]() |
|
continuation line over-indented for visual indent Column: 36 Error code: E127 |
![]() |
|
Dictionaries should (almost) always be in multi-line format, with keys alphabetized and on their own line (not on the { … |
|
|
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 … |
|
|
No need for the .copy() here, since we know it won't be modified in a test. Although, we want to … |
|
|
I'm torn on this. My gut is telling me we don't want to bake this into the API. I think, … |
|
|
We don't want to localize here. Test results should almost always compare against fixed state. Generating dynamic state can mask … |
|
|
'djblets.webapi.errors.LOGIN_FAILED' imported but unused Column: 1 Error code: F401 |
![]() |
|
These should still hard-code the exact string we expect, rather than running it through any Django functions. Otherwise we're just … |
|
-
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 1) I think we just want
timezone.now()
. This is the modern equivalent of our oldget_tz_aware_utcnow()
. -
djblets/webapi/auth/backends/api_tokens.py (Diff revision 1) 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',))
-
-
-
djblets/webapi/models.py (Diff revision 1) 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=...)
-

Change Summary:
- Added
help_text
to token fields and reformatted the fields - Removed updating the
last_used
field for OAuth2 tokens since that field doesnt exist for those tokens - Modified
BaseWebAPIToken
default test to check for the new fields too - Modified
WebAPITokenAuthBackend
's tests to check iflast_used
field gets updated upon successful authentication - Updates the
last_used
field inWebAPITokenAuthBackend
- Updates the
last_updated
field in whenever an API token object is saved and that save is from an update
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+156 -24) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/webapi/auth/backends/oauth2_tokens.py (Diff revision 2) 'django.utils.timezone' imported but unused Column: 1 Error code: F401
-
djblets/webapi/models.py (Diff revision 2) continuation line over-indented for hanging indent Column: 13 Error code: E126

Change Summary:
flake8 fixes
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+154 -24) |
Checks run (2 succeeded)
-
-
djblets/webapi/models.py (Diff revision 3) This is being set after we save. I assume we'll want to set this before the parent save call above.
-
djblets/webapi/tests/test_api_token.py (Diff revision 3) 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.

Change Summary:
- Uses
spy_on()
to settimezone.now
- Added unit test for
save()
updating thelast_updated
field
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+323 -1172) |
Checks run (2 succeeded)
-
-
README.rst (Diff revision 4) 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?

Change Summary:
Rebased with release-3.x
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+180 -28) |
Checks run (2 succeeded)
-
-
djblets/webapi/models.py (Diff revision 5) This could use
djblets.db.fields.ModificationTimestampField
in order to avoid having to babysit it insave()
-
djblets/webapi/tests/test_api_token.py (Diff revision 5) Add a blank line in here (
datetime
is standard lib,django
is third party). -
djblets/webapi/tests/test_api_token.py (Diff revision 5) Can you move this to the third-party import section?

Change Summary:
- Spy on
timezone.now
in Token Auth authentication test to see iflast_used
gets updated - Fixed imports ordering issues
- Uses
ModificationTimeStampField
for token'slast_updated
field
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+192 -32) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 6) 'datetime.timedelta' imported but unused Column: 1 Error code: F401

Change Summary:
got rid of unused import
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+190 -32) |
Checks run (2 succeeded)
-
-
djblets/webapi/tests/test_api_token.py (Diff revision 7) 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

Change Summary:
Spies on
timezone.now
on a per-test-method basis rather than for the entire test suite
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+194 -32) |
Checks run (2 succeeded)

Change Summary:
- Added a method for checking if a token is expired.
- The token authentication process now checks for invalid or expired tokens.
- The
last_used
field of the token was being updated twice.
One area would update it even if authentication failed (from the user not being active).
Changed it so thatlast_used
is only updated on successful authentications.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commits: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+740 -48) |
Checks run (2 succeeded)
-
-
-
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) 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.
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) Any
%
values must be specified outside of a_(...)
. Otherwise they'll modify the string that needs to be localized. -
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) We should specify which user this pertains to.
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) Same comment about localized string formatting.
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) Same comment about localized string formatting.
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) We should specify which user this pertains to.
-
-
djblets/webapi/auth/backends/api_tokens.py (Diff revision 9) We've been moving to a better format. This can be:
result = ( super(...) .valdilate_credentials(...) )
Though, being Python 3, the
super
can just besuper()
, probably. -
djblets/webapi/models.py (Diff revision 9) This can be simplified to:
return self.expires is not None and timezone.now() >= self.expires
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9) Plain
import
s should be listed beforefrom
imports. -
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9) Let's switch this to an
import kgb
, and then access these as attributes fromkgb
. This is the pattern we're adopting elsewhere. -
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 9) For unit tests, we don't want to use
_(...)
. -
-

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+830 -72) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10) line too long (82 > 79 characters) Column: 80 Error code: E501
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 10) continuation line over-indented for visual indent Column: 36 Error code: E127

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+834 -72) |
Checks run (2 succeeded)
-
-
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) Might be more clear as "... the token is invalid or expired, and any appropriate HTTP headers to send to the client."
-
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) We always either override everything in here, or return
None
. Rather than pre-define it, let's justreturn { ... }
directly where we need it.For any conditional values, let's set a variable and then use that as the value in the
return { ... }
. -
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) For safety, this should ddo
LOGIN_FAILED.headers.copy()
. That avoids any state leakage (a problem we've had before in the API). -
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) 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.
-
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) Rather than format and set a default, let's move this to an
else
below. -
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) 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 viasettings.DATETIME_FORMAT
(not that we advertise that). -
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) When logging, we don't want to use
%
. Instead, pass the value as a positional parameter. -
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 9 - 11) Similar comments above apply here.

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+840 -72) |
Checks run (2 succeeded)
-
-
djblets/webapi/auth/backends/api_tokens.py (Diff revisions 11 - 12) 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, }
-
-
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revisions 11 - 12) Let's actually move these into the assertion, so it's not scattered:
self.assertEqual( result, ( False, '...', { # Explicit headers here } ))
Same below.
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revisions 11 - 12) 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.

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+880 -72) |
Checks run (1 failed, 1 succeeded)
flake8
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revision 13) 'djblets.webapi.errors.LOGIN_FAILED' imported but unused Column: 1 Error code: F401

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 14 (+878 -74) |
Checks run (2 succeeded)

Change Summary:
Does not emit the
webapi_token_updated
signal when a token is used for authentication (this would happen because thelast_used
field was being updated whenever a token was used)
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 15 (+976 -80) |
Checks run (2 succeeded)
-
-
djblets/webapi/models.py (Diff revisions 12 - 15) 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 ofsave()
. This can be used just likesave()
for our purposes. It just bypasses overriddensave
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 standardpre_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. -
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revisions 12 - 15) 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.

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 16 (+880 -74) |
Checks run (2 succeeded)
-
-
djblets/webapi/tests/test_api_auth_backend.py (Diff revisions 15 - 16) 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.

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 17 (+866 -74) |