API Tokens #134

Merged
Dalai Felinto merged 6 commits from tokens into main 2024-05-27 12:53:31 +02:00

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.





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. --- <img src="https://projects.blender.org/infrastructure/extensions-website/attachments/ea968b76-557e-48ef-ba7f-c3a13a269b1b" width=800> <br /> <img src="https://projects.blender.org/infrastructure/extensions-website/attachments/917558c7-8a85-47f6-810d-39d7c8a10f6d" width=500> <br /> <img src="https://projects.blender.org/infrastructure/extensions-website/attachments/e08ee0bd-4544-429f-b1e3-30101e2e94b5" width=800> <br /> <img src="https://projects.blender.org/infrastructure/extensions-website/attachments/8bc96abc-4ac2-461d-98e4-13c74dc1d23b" width=500>
Oleg-Komarov reviewed 2024-05-21 11:28:45 +02:00
Oleg-Komarov left a comment
Owner

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.

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',
Owner

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

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
dfelinto marked this conversation as resolved
tokens/apps.py Outdated
@ -0,0 +3,4 @@
class TokensConfig(AppConfig):
default_auto_field = 'django.db.models.BigAutoField'
name = 'tokens'
Owner

this is a very generic name, let's make it more specific, e.g. apitokens, apikeys or userapitokens

this is a very generic name, let's make it more specific, e.g. `apitokens`, `apikeys` or `userapitokens`
dfelinto marked this conversation as resolved
tokens/models.py Outdated
@ -0,0 +7,4 @@
class UserToken(models.Model):
id = models.AutoField(primary_key=True)
Owner

none of our models specify the id field explicitly, but rely on default_auto_field = 'django.db.models.BigAutoField' defined in each of the apps.py files instead

can we skip this definition here, and use the BigAutoField inherited from the default?

none of our models specify the `id` field explicitly, but rely on `default_auto_field = 'django.db.models.BigAutoField'` defined in each of the `apps.py` files instead can we skip this definition here, and use the BigAutoField inherited from the default?
dfelinto marked this conversation as resolved
@ -0,0 +26,4 @@
{% for token in tokens %}
<tr>
<td>{{token.name}}</td>
<td>{{token.token}}</td>
Owner

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.

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.
dfelinto marked this conversation as resolved
Owner

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

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: https://github.com/python/cpython/blob/8b6175c2619c8fefa74a1ba33b8d37b6d13716e3/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
Owner

A good overview of related concerns: https://blog.mergify.com/api-keys-best-practice/

A good overview of related concerns: https://blog.mergify.com/api-keys-best-practice/
Dalai Felinto force-pushed tokens from d0b95745bc to 8cfac336a1 2024-05-21 23:29:59 +02:00 Compare
Author
Owner

All the review topics have been addressed.

All the review topics have been addressed.
Oleg-Komarov reviewed 2024-05-23 13:05:28 +02:00
Oleg-Komarov left a comment
Owner

This PR lacks tests for invariants we want to introduce:

  • a full token value is displayed to a user only once
  • the list page doesn't display a full token value (unless a token was just created)
  • the list page shows last access time
This PR lacks tests for invariants we want to introduce: - a full token value is displayed to a user only once - the list page doesn't display a full token value (unless a token was just created) - the list page shows last access time
@ -0,0 +22,4 @@
return None
try:
token_hash = hashlib.sha256(token_key.encode()).hexdigest()
Owner

this should be a class method on a model, please see a related comment in the CreateTokenView

this should be a class method on a model, please see a related comment in the `CreateTokenView`
dfelinto marked this conversation as resolved
@ -0,0 +29,4 @@
token.ip_address_last_access = clean_ip_address(request)
token.date_last_access = datetime.datetime.now()
Owner

+ token.save(update_fields={'ip_address_last_access','date_last_access'})

`+ token.save(update_fields={'ip_address_last_access','date_last_access'})`
dfelinto marked this conversation as resolved
@ -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)
Owner

"begin" is not a noun, let's use a different name, e.g. token_prefix

"begin" is not a noun, let's use a different name, e.g. `token_prefix`
dfelinto marked this conversation as resolved
@ -0,0 +34,4 @@
<td>
{% if token.date_last_access %}
{{token.date_last_access}}
{% else %}
Owner
this if-else can be rewritten using https://docs.djangoproject.com/en/4.2/ref/templates/builtins/#default
dfelinto marked this conversation as resolved
@ -0,0 +62,4 @@
def form_valid(self, form):
form.instance.user = self.request.user
token = secrets.token_urlsafe(32)
Owner

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

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
dfelinto marked this conversation as resolved
@ -0,0 +63,4 @@
form.instance.user = self.request.user
token = secrets.token_urlsafe(32)
token_hash = hashlib.sha256(token.encode()).hexdigest()
Owner

let's hide this implementation in a model class method, then the api will use it as well

let's hide this implementation in a model class method, then the api will use it as well
dfelinto marked this conversation as resolved
@ -0,0 +68,4 @@
form.instance.token_hash = token_hash
form.instance.token_begin = token[:5]
messages.info(self.request, f'{token}')
Owner

what is the purpose of wrapping token in an f-string here?

what is the purpose of wrapping token in an f-string here?
dfelinto marked this conversation as resolved
Dalai Felinto force-pushed tokens from 32d9dce524 to 3698eb681a 2024-05-25 13:21:28 +02:00 Compare
Author
Owner

All the review topics have been addressed.

All the review topics have been addressed.
Oleg-Komarov approved these changes 2024-05-27 09:14:26 +02:00
Oleg-Komarov left a comment
Owner

looks good

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):
Owner

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.

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.
Author
Owner

@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)

@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: https://projects.blender.org/infrastructure/extensions-website/commit/4e1c5fdae34fc4d430bb02f96e8bd2c432567442 (I only added this on #138, but I think it could be merged on this one if you +1)
Owner

should I remove test_list_page_shows_last_access_time? or leave it, and then on #138 add the other test?

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

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)

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.

> should I remove `test_list_page_shows_last_access_time`? or leave it, and then on #138 add the other test? 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 > Also, what do you think of: https://projects.blender.org/infrastructure/extensions-website/commit/4e1c5fdae34fc4d430bb02f96e8bd2c432567442 (I only added this on #138, but I think it could be merged on this one if you +1) 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.
Dalai Felinto merged commit 4ef93583a2 into main 2024-05-27 12:53:31 +02:00
Dalai Felinto referenced this issue from a commit 2024-05-27 12:53:31 +02:00
Dalai Felinto deleted branch tokens 2024-05-27 12:53:31 +02:00
Márton Lente referenced this issue from a commit 2024-05-27 16:57:58 +02:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: infrastructure/extensions-website#134
No description provided.