API Tokens #134
No reviewers
Labels
No Label
Priority
Critical
Priority
High
Priority
Low
Priority
Normal
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Type
Breaking
Type
Documentation
Type
Enhancement
Type
Feature
Type
Report
Type
Security
Type
Suggestion
Type
Testing
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: infrastructure/extensions-website#134
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "tokens"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This allows users to create tokens to be used with the API.
The goal is to use those for the API that will allow users to upload new versions of an extension.
The Tokens can be created/managed on the user profile.
Note: There are still no API entries that can use these tokens.
We should think of some minimal ways to audit usage of the keys: at least log the timestamp (and optionally IP address) of the last successful key usage to display it to the user.
It may be convenient for troubleshooting and also potentially gives some info if a token gets leaked: all kinds of API keys periodically leak in various ways, e.g. mistakenly added to logs, committed to version control, etc
Please see also an inline comment regarding storing keys in plain text.
@ -275,1 +276,4 @@
'DEFAULT_SCHEMA_CLASS': 'drf_spectacular.openapi.AutoSchema',
'DEFAULT_AUTHENTICATION_CLASSES': (
'rest_framework.authentication.SessionAuthentication',
'rest_framework.authentication.BasicAuthentication',
why do we need SessionAuthentication and BasicAuthentication?
if we plan to use only our own UserTokenAuthentication, checking the header on each request, we won't need the other mechanisms
@ -0,0 +3,4 @@
class TokensConfig(AppConfig):
default_auto_field = 'django.db.models.BigAutoField'
name = 'tokens'
this is a very generic name, let's make it more specific, e.g.
apitokens
,apikeys
oruserapitokens
@ -0,0 +7,4 @@
class UserToken(models.Model):
id = models.AutoField(primary_key=True)
none of our models specify the
id
field explicitly, but rely ondefault_auto_field = 'django.db.models.BigAutoField'
defined in each of theapps.py
files insteadcan we skip this definition here, and use the BigAutoField inherited from the default?
@ -0,0 +26,4 @@
{% for token in tokens %}
<tr>
<td>{{token.name}}</td>
<td>{{token.token}}</td>
A common advice and a best practice is to avoid displaying a full token after it was initially generated.
This also stems from the approach to avoid storing tokens in plain text, treating them as passwords: this means the server has a full token value only right after the token was generated.
This original value is displayed to the user just once (to be copied somewhere safe), and only a hash and a prefix (for convenience) gets stored in the database.
Regarding usage of UUIDs: the RFC recommends against using uuids for secrets https://datatracker.ietf.org/doc/html/rfc4122.html#section-6
uuid4 uses 16 random bytes:
8b6175c261/Lib/uuid.py (L725)
using python's stdlib guidelines for secrets: https://docs.python.org/3.12/library/secrets.html#how-many-bytes-should-tokens-use
we should use a stronger token, using at least 32 random bytes
since we won't be storing tokens in plain text, we don't have to use a UUID db field, and we can use https://docs.python.org/3.12/library/secrets.html#secrets.token_urlsafe for generating the token value
A good overview of related concerns: https://blog.mergify.com/api-keys-best-practice/
d0b95745bc
to8cfac336a1
All the review topics have been addressed.
This PR lacks tests for invariants we want to introduce:
@ -0,0 +22,4 @@
return None
try:
token_hash = hashlib.sha256(token_key.encode()).hexdigest()
this should be a class method on a model, please see a related comment in the
CreateTokenView
@ -0,0 +29,4 @@
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'})
@ -0,0 +7,4 @@
class UserToken(models.Model):
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)
"begin" is not a noun, let's use a different name, e.g.
token_prefix
@ -0,0 +34,4 @@
<td>
{% if token.date_last_access %}
{{token.date_last_access}}
{% else %}
this if-else can be rewritten using https://docs.djangoproject.com/en/4.2/ref/templates/builtins/#default
@ -0,0 +62,4 @@
def form_valid(self, form):
form.instance.user = self.request.user
token = secrets.token_urlsafe(32)
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
@ -0,0 +63,4 @@
form.instance.user = self.request.user
token = secrets.token_urlsafe(32)
token_hash = hashlib.sha256(token.encode()).hexdigest()
let's hide this implementation in a model class method, then the api will use it as well
@ -0,0 +68,4 @@
form.instance.token_hash = token_hash
form.instance.token_begin = token[:5]
messages.info(self.request, f'{token}')
what is the purpose of wrapping token in an f-string here?
32d9dce524
to3698eb681a
All the review topics have been addressed.
looks good
@ -0,0 +56,4 @@
self.assertContains(response, str(token.token_prefix))
self.assertNotContains(response, str(token_key))
def test_list_page_shows_last_access_time(self):
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.
@Oleg-Komarov should I remove
test_list_page_shows_last_access_time
? or leave it, and then on #138 add the other test?Also, what do you think of:
4e1c5fdae3
(I only added this on #138, but I think it could be merged on this one if you +1)we can remove it if we have some other test that at least renders the list page, but it's ok to keep it as well - no strong opinion here
if this method is needed only for tests and is not required for production code, I would prefer if it was a test utility instead of a part of the model.