Update the BaseWebAPIToken model for API Tokens v2

Review Request #12341 — Created June 6, 2022 and updated

maubin
Djblets
release-3.x
12419, 12415, 12342
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 the BaseWebAPIToken model:
- Adding expires field
- Adding valid, invalid_date and invalid_reason fields
- Adding token_generator_id field
- Adding last_used field
- Updating max length of token to 255 characters.

We also add help_text for all the BaseWebAPIToken fields for better
clarity.

Ran unit tests in djblets.webapi.tests

Summary
Update the BaseWebAPIToken model for API Tokens v2
Description From Last Updated

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

chipx86chipx86

By default, save() will save every single field in the model. If there are two threads dealing with an API …

chipx86chipx86

Same note about saving.

chipx86chipx86

Blank line before comments.

chipx86chipx86

We haven't been consistent with this, but we should aim to have localized help_text= set for every field. Since that's …

chipx86chipx86

255 should be fine.

chipx86chipx86

'django.utils.timezone' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

continuation line over-indented for hanging indent Column: 13 Error code: E126

reviewbotreviewbot

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

chipx86chipx86

Things can go wrong when comparing dates/times. 1 second seems fine, but what if we hit a leap second while …

chipx86chipx86

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

'datetime.timedelta' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

I feel like this is something we should do per-test-method rather than for everything in the entire suite. I think …

daviddavid
maubin
chipx86
  1. 
      
  2. I think we just want timezone.now(). This is the modern equivalent of our old get_tz_aware_utcnow().

  3. 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',))
    
  4. Same note about saving.

    1. Actually I don't think I need to update the last_used field here because the oauth2_provider.models.AccessToken model doesn't have a last_used or an equivalent field. Will take this out.

  5. djblets/webapi/models.py (Diff revision 1)
     
     
     

    Blank line before comments.

  6. 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=...)
    
  7. djblets/webapi/models.py (Diff revision 1)
     
     

    255 should be fine.

  8. 
      
maubin
Review request changed

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 if last_used field gets updated upon successful authentication
  • Updates the last_used field in WebAPITokenAuthBackend
  • Updates the last_updated field in whenever an API token object is saved and that save is from an update

Description:

~  

As part of our move to API Tokens v2, we need to open API tokens for new token

~   types, expiration, and invalidation. The following changes are made to the
~   BaseWebApiToken model:

  ~

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.

   
~  
  • Adding expires field
~  
  • Adding valid, invalid_date and invalid_reason fields
~  
  • Adding token_generator_id field
~  
  • Adding last_used field
~  
  • Updating max length of token to 255 characters.
  ~

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 the BaseWebAPIToken model:
  + - Adding expires field
  + - Adding valid, invalid_date and invalid_reason fields
  + - Adding token_generator_id field
  + - Adding last_used field
  + - Updating max length of token to 255 characters.

  +
  +

We also add help_text for all the BaseWebAPIToken fields for better

  + clarity.

Commits:

Summary
-
Updating the BaseWebAPIToken model for API Tokens v2
+
Updating the BaseWebAPIToken model for API Tokens v2

Diff:

Revision 2 (+156 -24)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
chipx86
  1. 
      
  2. 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.

  3. 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.

  4. 
      
maubin
david
  1. 
      
  2. 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?

  3. 
      
maubin
david
  1. 
      
  2. djblets/webapi/models.py (Diff revision 5)
     
     
     
     

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

  3. djblets/webapi/tests/test_api_token.py (Diff revision 5)
     
     
     

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

  4. djblets/webapi/tests/test_api_token.py (Diff revision 5)
     
     

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

  5. 
      
maubin
Review request changed

Change Summary:

  • Spy on timezone.now in Token Auth authentication test to see if last_used gets updated
  • Fixed imports ordering issues
  • Uses ModificationTimeStampField for token's last_updated field

Commits:

Summary
-
Updating the BaseWebAPIToken model for API Tokens v2
+
Updating the BaseWebAPIToken model for API Tokens v2

Diff:

Revision 6 (+192 -32)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. 
      
  2. 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
    1. Sure, it makes sense to move it so that its per-test-method.

      For the 2nd problem, that's why I set the time_added and last_updated fields to timezone.now() - timedelta(hours=1) when I create the token. That way when we check to see if last_updated is timezone.now(), it'll fail if it's still timezone.now() - timedelta(hours=1). I think this gives the same effect of what you're looking for?

    2. Ah, you're right. I'm happy after you just move it to be per-method.

  3. 
      
maubin
Review request changed

Change Summary:

Spies on timezone.now on a per-test-method basis rather than for the entire test suite

Commits:

Summary
-
Updating the BaseWebAPIToken model for API Tokens v2
+
Update the BaseWebAPIToken model for API Tokens v2

Diff:

Revision 8 (+194 -32)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
david
  1. Ship It!
  2. 
      
Loading...